explicitly set 'include' to numpy_incls
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Build
description: Library building on various platforms

Comments:
Hello @jbrockmendel! Thanks for updating the PR.



Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on November 13, 2017 at 14:19 Hours UTC
this is ok..
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=h1) Report
> Merging [#18112](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/3268f4e976a42065db3dcbc869f0c73adc0c7522?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18112/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #18112      +/-   ##
==========================================
+ Coverage   91.25%   91.26%   +<.01%     
==========================================
  Files         163      163              
  Lines       50123    50124       +1     
==========================================
+ Hits        45741    45745       +4     
+ Misses       4382     4379       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.07% <ø> (+0.02%)` | :arrow_up: |
| #single | `40.32% <ø> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.75% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.19% <0%> (ø)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.51% <0%> (ø)` | :arrow_up: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.2% <0%> (+1.81%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=footer). Last update [3268f4e...37ad0f2](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=h1) Report
> Merging [#18112](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/7f4c960c7d0c100e5df9f0a3bc68401fc4eace4d?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18112/graphs/tree.svg?width=650&src=pr&token=eZ4WkYLtcO&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #18112      +/-   ##
==========================================
+ Coverage   91.38%   91.38%   +<.01%     
==========================================
  Files         164      164              
  Lines       49884    49884              
==========================================
+ Hits        45584    45587       +3     
+ Misses       4300     4297       -3
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.19% <ø> (+0.02%)` | :arrow_up: |
| #single | `39.42% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.8% <0%> (-0.1%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.25% <0%> (+1.81%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=footer). Last update [7f4c960...0274152](https://codecov.io/gh/pandas-dev/pandas/pull/18112?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

rebase 
How did this happen in master:
```
    '_libs.tslibs.parsing': {
        'pyxfile': '_libs/tslibs/parsing',
        'pxdfiles': ['_libs/src/util',
                     '_libs/src/khash']},
```
?

tslibs.parsing doesnt use either of those.
```
    '_libs.tslibs.timedeltas': {
        'pyxfile': '_libs/tslibs/timedeltas',
        'pxdfiles': ['_libs/src/util'],
        'depends': np_datetime_headers,
        'sources': np_datetime_sources},
```
nope on the depends and sources.
```
    '_libs.tslibs.strptime': {
        'pyxfile': '_libs/tslibs/strptime',
        'pxdfiles': ['_libs/src/util',
                     '_libs/tslibs/nattype'],
        'depends': tseries_depends,
        'sources': np_datetime_sources},
```
nope on depends and sources, missing np_datetime
```
    '_libs.index': {
        'pyxfile': '_libs/index',
        'pxdfiles': ['_libs/src/util', '_libs/hashtable'],
        'depends': _pxi_dep['index'],
        'sources': np_datetime_sources},
```

Nope on the sources
np_datetime is now part of tseries_depends
It's almost as if dumping a bunch of unnecessary stuff into setup.py made it easier for people in new and exciting ways.
this looks ok, but why is this necessary? what is wrong with the current?
> this looks ok, but why is this necessary? what is wrong with the current?

Dependencies are over-specified.  For extensions that just have include=numpy_incls, it is explicitly saying "this is not tightly coupled"

Also I really don't like having both numpy_incl and src/numpy.pxd on the search path.
> Dependencies are over-specified. For extensions that just have include=numpy_incls, it is explicitly saying "this is not tightly coupled"

can you show how you determine this?


> can you show how you determine this?

I look through the module for cimports.  If the only non-cython/cpython cimports it uses are numpy, then it has no pandas c-deps and the build only needs `np.get_include()`.
> for tslibs.strptime, still needs to have tseries_depends, np? (e.g. np_datetime is there)

I thought we landed on not specifying recursive dependencies.  No?
> I thought we landed on not specifying recursive dependencies. No?

you mean transitive I suppose.

well this is a c-include and is needed for compilation. compile it in isolation and see if it works when you don't have these. anywhere we are implicity or explicity including np_datetime which is almost everywhere in tslibs these need to be specified, unless you can prove unambiguously that they are not needed.
I still also dont' understand why this is actually needed. what is broken if this is not included (the status quo)?
> you mean transitive I suppose.

Sure.  To be extra-clear: if A cimports from B and B cimports or "cdef extern"s from C, then A transitively/recursively depends on C, but does not directly depend on C.  Same page?
> anywhere we are implicity or explicity including np_datetime which is almost everywhere in tslibs these need to be specified, 

My understanding was that the policy is to specify only direct dependencies.  Otherwise, say, why  aren't hashtable's dependencies specified in all of the other extensions that cimport from hashtable?
> well this is a c-include and is needed for compilation. compile it in isolation and see if it works when you don't have these. 

Yah this gets back to the original thing of me not understanding why you wanted to add a bunch of junk to these specifications in the first place.  As long as direct dependencies are listed it compiles fine.  (wouldn't the CI here scoff otherwise?)
> unless you can prove unambiguously that they are not needed.

https://groups.google.com/forum/#!topic/cython-users/YdGpGqi6Qw4
> I still also dont' understand why this is actually needed. what is broken if this is not included (the status quo)?

The problem is that _too much_ is specified, and it is specified in _too many_ places, which is a recipe for confusion.

Explicitly setting the include key to either `[]` or `numpy_incls` where possible pins that down to the minimal viable set of dependencies.
The `numpy_incls` part of the PR is largely orthogonal to the discussion about transitive dependencies.  How about I revert the changes that affect transitive dependencies and we can address that elsewhere/elsewhen?
Just pushed a commit reverting the subset of changes on which there has been controversy.

Now this just sets the `include` key to either `[]` or `numpy_incls` where applicable, ignores specification (or possible over-specification) of transitive dependencies.
Clipboard.
> As long as direct dependencies are listed it compiles fine. (wouldn't the CI here scoff otherwise?)

well the CI won't complain, but building from a previous state, DOES complain. I don't want to have to constantly ``make clean``; the current building is actually nice, if a .pxd changes or a dep changes, then it only recompiles what is necessary.

So back to this PR. I don't see any effect (pos or neg), so what exactly is symptom that this is trying to fix?

> So back to this PR. I don't see any effect (pos or neg), so what exactly is symptom that this is trying to fix?

Specifying dependencies (or "includes") which the relevant extensions are not actually dependent on is an anti-pattern.  This PR fixes that for the subset of pyx files that have zero intra-pandas dependencies.
On green let’s either merge or kill, avoid clogging the CI queue.  Especially in light of reopening the depends issue, I think this is worthwhile.
Doing some googling on what affects compile/build times, one thing that came up frequently was unnecessary includes.  No idea how applicable it is here, but its plausible that trimming unnecessary includes and sources could cut build time.