explicitly set 'include' to numpy_incls #18112

jbrockmendel · 2017-11-04T19:49:03Z

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

pep8speaks · 2017-11-04T19:49:05Z

Hello @jbrockmendel! Thanks for updating the PR.

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

Comment last updated on November 13, 2017 at 14:19 Hours UTC

jreback · 2017-11-04T19:50:50Z

setup.py

@@ -460,6 +460,13 @@ def pxd(name):
    return os.path.abspath(pjoin('pandas', name + '.pxd'))


+if _have_setuptools:


this whole setuptools optional biz is a mess. we should just require it (and we do in our recipes, so this is all old code). note nothing to do with this PR.

jreback · 2017-11-04T19:51:08Z

this is ok..

jreback · 2017-11-04T22:35:28Z

setup.py

@@ -570,14 +580,16 @@ def pxd(name):
    '_libs.tslibs.timezones': {
        'pyxfile': '_libs/tslibs/timezones'},
    '_libs.testing': {
-        'pyxfile': '_libs/testing'},
+        'pyxfile': '_libs/testing',
+        'include': []},


what is the difference betwee not passing include and an empty list? (from a compilation perspective)?

I think this can be removed IINM.

Without the 'include' key, it gets set to a default of common_include. This has two unwanted side effects. First: the search path for numpy now includes src/numpy.pxd which we should move away from. Second: making it explicit avoids a problem a year from now when we see that common_include is added to _libs.testing but no one remembers that it isn't actually needed.

…_deps2

codecov · 2017-11-05T01:29:33Z

Codecov Report

Merging #18112 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            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%)`	⬆️
#single	`40.32% <ø> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <0%> (-0.1%)`	⬇️
pandas/core/indexes/timedeltas.py	`91.19% <0%> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`95.51% <0%> (ø)`	⬆️
pandas/plotting/_converter.py	`65.2% <0%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3268f4e...37ad0f2. Read the comment docs.

codecov · 2017-11-05T01:29:35Z

Codecov Report

Merging #18112 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            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%)`	⬆️
#single	`39.42% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️
pandas/plotting/_converter.py	`65.25% <0%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f4c960...0274152. Read the comment docs.

jreback · 2017-11-05T10:56:16Z

rebase

jbrockmendel · 2017-11-05T16:17:05Z

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.

jbrockmendel · 2017-11-05T16:18:29Z

    '_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.

jbrockmendel · 2017-11-05T16:20:25Z

    '_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

jbrockmendel · 2017-11-05T16:22:23Z

    '_libs.index': {
        'pyxfile': '_libs/index',
        'pxdfiles': ['_libs/src/util', '_libs/hashtable'],
        'depends': _pxi_dep['index'],
        'sources': np_datetime_sources},

Nope on the sources

…_deps2 Fix incorrect entries in setup

jreback · 2017-11-05T16:23:34Z

np_datetime is now part of tseries_depends

jreback · 2017-11-05T16:24:48Z

setup.py

@@ -576,31 +585,29 @@ def pxd(name):
                     '_libs/tslibs/conversion']},
    '_libs.tslibs.parsing': {
        'pyxfile': '_libs/tslibs/parsing',
-        'pxdfiles': ['_libs/src/util',


pls don’t change this; if u look in the source these depend on these

Are we looking at different source files? tslibs.parsing doesn't cimport from util or khash.

you are right here

jreback · 2017-11-05T16:24:59Z

setup.py

    '_libs.tslibs.strptime': {
        'pyxfile': '_libs/tslibs/strptime',
        'pxdfiles': ['_libs/src/util',
-                     '_libs/tslibs/nattype'],
-        'depends': tseries_depends,


tseries_depends already incldes np_datetime, so no need to add this explicty.

i think this one needs to change back

From the diff it isn't clear which file you're referring to. Best guess is tslibs.strptime?

for tslibs.strptime, still needs to have tseries_depends, np? (e.g. np_datetime is there)

jreback · 2017-11-05T16:25:15Z

setup.py

    '_libs.tslibs.timedeltas': {
        'pyxfile': '_libs/tslibs/timedeltas',
-        'pxdfiles': ['_libs/src/util'],
-        'depends': np_datetime_headers,


jbrockmendel · 2017-11-05T16:30:11Z

It's almost as if dumping a bunch of unnecessary stuff into setup.py made it easier for people in new and exciting ways.

jreback

you need to rebase. some of your comments are on old sources.

jreback · 2017-11-06T13:27:24Z

setup.py

    '_libs.tslibs.strptime': {
        'pyxfile': '_libs/tslibs/strptime',
        'pxdfiles': ['_libs/src/util',
-                     '_libs/tslibs/nattype'],
-        'depends': tseries_depends,


tseries_depends already incldes np_datetime, so no need to add this explicty.

jreback · 2017-11-06T13:27:30Z

setup.py

@@ -576,31 +585,29 @@ def pxd(name):
                     '_libs/tslibs/conversion']},
    '_libs.tslibs.parsing': {
        'pyxfile': '_libs/tslibs/parsing',
-        'pxdfiles': ['_libs/src/util',


you are right here

…_deps2

jreback · 2017-11-07T13:39:02Z

this looks ok, but why is this necessary? what is wrong with the current?

jbrockmendel · 2017-11-07T15:06:41Z

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.

jreback · 2017-11-07T15:19:24Z

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?

jbrockmendel · 2017-11-07T15:40:23Z

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().

…_deps2

jreback · 2017-11-07T16:02:31Z

setup.py

    '_libs.tslibs.strptime': {
        'pyxfile': '_libs/tslibs/strptime',
        'pxdfiles': ['_libs/src/util',
-                     '_libs/tslibs/nattype'],
-        'depends': tseries_depends,


i think this one needs to change back

…_deps2

jreback · 2017-11-10T19:53:24Z

setup.py

@@ -578,14 +587,12 @@ def pxd(name):
                     '_libs/tslibs/frequencies']},
    '_libs.tslibs.parsing': {
        'pyxfile': '_libs/tslibs/parsing',
-        'pxdfiles': ['_libs/src/util',


side note, should parsing depend on nattype? (then for example nat strings are in 1 place).

Well nat_strings is defined but not c-defined in nattype, so we could import it into parsing without needing to specify the dependency.

I'd be fine with that, or with having it defined one extra place. parsing has no intra-pandas dependencies at the moment (neither import nor cimport) which makes it really low-stress.

jreback · 2017-11-10T19:54:44Z

setup.py

    '_libs.tslibs.strptime': {
        'pyxfile': '_libs/tslibs/strptime',
        'pxdfiles': ['_libs/src/util',
-                     '_libs/tslibs/nattype'],
-        'depends': tseries_depends,


for tslibs.strptime, still needs to have tseries_depends, np? (e.g. np_datetime is there)

jbrockmendel · 2017-11-10T22:58:29Z

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?

jreback · 2017-11-11T13:14:46Z

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.

jreback · 2017-11-11T13:15:14Z

I still also dont' understand why this is actually needed. what is broken if this is not included (the status quo)?

jbrockmendel · 2017-11-11T19:27:52Z

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?

jbrockmendel · 2017-11-11T19:28:08Z

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?

jbrockmendel · 2017-11-11T19:29:23Z

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?)

jbrockmendel · 2017-11-11T19:30:20Z

unless you can prove unambiguously that they are not needed.

https://groups.google.com/forum/#!topic/cython-users/YdGpGqi6Qw4

jbrockmendel · 2017-11-11T19:32:26Z

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.

jbrockmendel · 2017-11-11T19:35:33Z

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?

…_deps2

jbrockmendel · 2017-11-12T16:46:41Z

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.

jbrockmendel · 2017-11-12T18:57:37Z

Clipboard.

jreback · 2017-11-12T19:38:13Z

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?

jbrockmendel · 2017-11-13T00:05:46Z

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.

…_deps2

jbrockmendel · 2017-11-13T19:43:04Z

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.

jbrockmendel · 2017-11-15T16:18:22Z

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.

jreback · 2017-11-16T00:26:51Z

ok thanks. We do need to refactor setup.py to use cythonize that will reduce the code a lot, while preserving the dependencies (w/o us having to do anything).

jreback · 2017-11-19T01:54:02Z

so I am pretty sure this broke our wheel building for 2.7, see here: https://travis-ci.org/MacPython/pandas-wheels/jobs/304148523

this is not easy to try a fix, and we have to commit it and see (though you could try forking that repo if you want).

This reverts commit 54f2a5e.

jbrockmendel · 2017-11-19T01:58:09Z

Huh.

ImportError: No module named _build_utils.apple_accelerate

This reverts commit 54f2a5e.

explicitly set include to numpy_incls

b81e094

jreback reviewed Nov 4, 2017

View reviewed changes

jreback added the Build Library building on various platforms label Nov 4, 2017

jreback added this to the 0.22.0 milestone Nov 4, 2017

type fixup

e944e7c

jreback reviewed Nov 4, 2017

View reviewed changes

jbrockmendel added 2 commits November 4, 2017 18:29

fixup whitespace

37ad0f2

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

d00a814

…_deps2

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

710aa94

…_deps2 Fix incorrect entries in setup

jreback requested changes Nov 5, 2017

View reviewed changes

jreback requested changes Nov 6, 2017

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

4b1332a

…_deps2

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

7252bdd

…_deps2

jreback requested changes Nov 7, 2017

View reviewed changes

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

2467cb4

…_deps2

jreback requested changes Nov 10, 2017

View reviewed changes

jbrockmendel added 2 commits November 12, 2017 08:41

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

719a457

…_deps2

revert controversial changes

c7ffb02

jreback removed this from the 0.22.0 milestone Nov 12, 2017

Merge branch 'master' of https://github.com/pandas-dev/pandas into np…

0274152

…_deps2

jreback added this to the 0.22.0 milestone Nov 16, 2017

jreback approved these changes Nov 16, 2017

View reviewed changes

jreback merged commit 54f2a5e into pandas-dev:master Nov 16, 2017

jreback added a commit to jreback/pandas that referenced this pull request Nov 19, 2017

Revert "explicitly set 'include' to numpy_incls (pandas-dev#18112)"

fc15377

This reverts commit 54f2a5e.

jreback mentioned this pull request Nov 19, 2017

Revert "explicitly set 'include' to numpy_incls (#18112)" #18365

Merged

jreback added a commit that referenced this pull request Nov 19, 2017

Revert "explicitly set 'include' to numpy_incls (#18112)" (#18365)

f1b1158

This reverts commit 54f2a5e.

jbrockmendel deleted the np_deps2 branch December 8, 2017 19:38

		@@ -460,6 +460,13 @@ def pxd(name):
		return os.path.abspath(pjoin('pandas', name + '.pxd'))


		if _have_setuptools:

explicitly set 'include' to numpy_incls #18112

explicitly set 'include' to numpy_incls #18112

Conversation

jbrockmendel commented Nov 4, 2017

pep8speaks commented Nov 4, 2017 • edited Loading

Comment last updated on November 13, 2017 at 14:19 Hours UTC

Choose a reason for hiding this comment

jreback commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2017

Codecov Report

codecov bot commented Nov 5, 2017 • edited Loading

Codecov Report

jreback commented Nov 5, 2017

jbrockmendel commented Nov 5, 2017

jbrockmendel commented Nov 5, 2017

jbrockmendel commented Nov 5, 2017

jbrockmendel commented Nov 5, 2017

jreback commented Nov 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 5, 2017

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 7, 2017

jbrockmendel commented Nov 7, 2017

jreback commented Nov 7, 2017

jbrockmendel commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 10, 2017

jreback commented Nov 11, 2017

jreback commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017

jbrockmendel commented Nov 11, 2017 • edited Loading

jbrockmendel commented Nov 11, 2017

jbrockmendel commented Nov 12, 2017

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jbrockmendel commented Nov 13, 2017

jbrockmendel commented Nov 13, 2017

jbrockmendel commented Nov 15, 2017

jreback commented Nov 16, 2017

jreback commented Nov 19, 2017

jbrockmendel commented Nov 19, 2017

pep8speaks commented Nov 4, 2017 •

edited

Loading

codecov bot commented Nov 5, 2017 •

edited

Loading

jbrockmendel commented Nov 11, 2017 •

edited

Loading