Repository: pandas-dev/pandas
Pull request: BLD: list proper deps for tslib
closes #18089


Problem statement:
BLD: tslib dependencies not fully specified
```
    '_libs.tslib': {'pyxfile': '_libs/tslib',
                    'pxdfiles': ['_libs/src/util'],
                    'depends': tseries_depends,
                    'sources': np_datetime_sources},
```

is not complete, in fact this directly depends on 

```
(pandas) bash-3.2$ ls pandas/_libs/tslibs/*.pxd
pandas/_libs/tslibs/conversion.pxd      pandas/_libs/tslibs/nattype.pxd         pandas/_libs/tslibs/timedeltas.pxd
pandas/_libs/tslibs/frequencies.pxd     pandas/_libs/tslibs/np_datetime.pxd     pandas/_libs/tslibs/timezones.pxd
(pandas) bash-3.2$ ls pandas/_libs/tslib*.pxd
pandas/_libs/tslib.pxd
```

not really sure of the exact rule, but I think anything you cimport from a .pxd must be listed as a dep.
Now rebuilding from clean just works, but rebuilding from a change in one part of the structure will only *sometimes* work.

Hints from issue discussion:
@jbrockmendel need to fix this before any other changes.
>  but rebuilding from a change in one part of the structure will only sometimes work.

Do you have an example for how this fails?  My understanding is that cython checks the timestamps on other cython files for determining if dependencies have changed.
try making a change on a .pxd (e.g moving something), them try to make, it will fail because it can't load the object.
nothing to do with time stamps, rather the build process doesn't know about deps. that it is depending, IOW tslib depends on tslibs/converstion.pxd, but this is not indicated anywhere (to rebuild tslib).
> try making a change on a .pxd (e.g moving something), them try to make, it will fail because it can't load the object

I noticed this yesterday when I tried to build `pandas` from source in an existing environment.  Some Cython files didn't get compiled again as needed (because dependencies were not specified), causing a statement like `import pandas` to fail.
> nothing to do with time stamps, rather the build process doesn't know about deps

OK I'll try to grok this.

Cython clearly knows _something_ about up-to-dateness:

```
$python setup.py build_ext --inplace
running build_ext
skipping 'pandas/io/sas/sas.c' Cython extension (up-to-date)
skipping 'pandas/_libs/properties.c' Cython extension (up-to-date)
skipping 'pandas/_libs/groupby.c' Cython extension (up-to-date)
[...]
```
@jbrockmendel : That's actually us :smile: .  We compare timestamps on all files and dependencies to determine whether they need updating.
> try making a change on a .pxd (e.g moving something), them try to make, it will fail because it can't load the object.

OK, I just took an existing branch, went to conversion.pxd and made a whitespace edit, then ran setup.py build_ext:


```
$python setup.py build_ext --inplace
[...]
skipping 'pandas/_libs/tslibs/frequencies.c' Cython extension (up-to-date)
skipping 'pandas/_libs/hashtable.c' Cython extension (up-to-date)
cythoning pandas/_libs/tslibs/conversion.pyx to pandas/_libs/tslibs/conversion.c
building 'pandas._libs.tslibs.conversion' extension
clang -fno-strict-aliasing -fno-common -dynamic -g -O2 -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes -Ipandas/_libs/src/klib -Ipandas/_libs/src -I/usr/local/lib/python2.7/site-packages/numpy/core/include -I/usr/local/include -I/usr/local/opt/openssl/include -I/usr/local/opt/sqlite/include -I/usr/local/Cellar/python/2.7.14/Frameworks/Python.framework/Versions/2.7/include/python2.7 -c pandas/_libs/tslibs/conversion.c -o build/temp.macosx-10.12-x86_64-2.7/pandas/_libs/tslibs/conversion.o -Wno-unused-function
[...]
skipping 'pandas/_libs/interval.c' Cython extension (up-to-date)
[...]
$python
>>> import pandas as pd
>>>
```

So far so good.  So I guess you're referring to something less benign than a whitespace change?  If I comment out `convert_to_tsobject` in conversion.pxd then I'll get an error at import-time.  Ideally this would raise at build-time, which I guess is what you're getting at here.

The cython folks are at least [aware](https://github.com/cython/cython/issues/1428
) of this; lack of discussion suggests it isn't a high priority.

There is a cython option that would allow us to do without pxd files entirely in some cases, but I'm a bit wary of it: https://github.com/cython/cython/issues/1866

Pretty sure I've seen this on the cython mailing list, will try to track that down. 
> That's actually us 😄 . We compare timestamps on all files to determine whether they need updating.

Hah.  Good to know.
> If I comment out convert_to_tsobject in conversion.pxd then I'll get an error at import-time. Ideally this would raise at build-time, which I guess is what you're getting at here.

Yes, that's exactly what happened to me yesterday.  I wouldn't worry too much about anything Cython-issue-related, as that's beyond our control from a `pandas` perspective.  The best thing would be to try to correct the dependencies on our end so that we don't get import errors.

Commit message: BLD: list proper deps for tslib

closes #18089

Patch to review:
diff --git a/setup.py b/setup.py
index 3464169e8d8d1..4128051df3e21 100755
--- a/setup.py
+++ b/setup.py
@@ -474,7 +474,8 @@ def pxd(name):
                        'pandas/_libs/src/datetime/np_datetime_strings.h']
 np_datetime_sources = ['pandas/_libs/src/datetime/np_datetime.c',
                        'pandas/_libs/src/datetime/np_datetime_strings.c']
-tseries_depends = np_datetime_headers + ['pandas/_libs/src/datetime.pxd']
+tseries_depends = np_datetime_headers + ['pandas/_libs/src/datetime.pxd',
+                                         'pandas/_libs/tslibs/np_datetime.pxd']
 
 # some linux distros require it
 libraries = ['m'] if not is_platform_windows() else []
@@ -535,15 +536,24 @@ def pxd(name):
         'depends': _pxi_dep['sparse']},
     '_libs.tslib': {
         'pyxfile': '_libs/tslib',
-        'pxdfiles': ['_libs/src/util'],
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/conversion',
+                     '_libs/tslibs/timedeltas',
+                     '_libs/tslibs/timezones',
+                     '_libs/tslibs/nattype'],
         'depends': tseries_depends,
         'sources': np_datetime_sources},
     '_libs.tslibs.conversion': {
         'pyxfile': '_libs/tslibs/conversion',
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/timezones',
+                     '_libs/tslibs/np_datetime',
+                     '_libs/tslibs/timedeltas'],
         'depends': tseries_depends,
         'sources': np_datetime_sources},
     '_libs.tslibs.fields': {
         'pyxfile': '_libs/tslibs/fields',
+        'pxdfiles': ['_libs/src/util'],
         'depends': tseries_depends,
         'sources': np_datetime_sources},
     '_libs.tslibs.frequencies': {
@@ -557,18 +567,26 @@ def pxd(name):
         'depends': np_datetime_headers,
         'sources': np_datetime_sources},
     '_libs.tslibs.offsets': {
-        'pyxfile': '_libs/tslibs/offsets'},
+        'pyxfile': '_libs/tslibs/offsets',
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/conversion']},
     '_libs.tslibs.parsing': {
         'pyxfile': '_libs/tslibs/parsing',
         'pxdfiles': ['_libs/src/util']},
     '_libs.tslibs.strptime': {
         'pyxfile': '_libs/tslibs/strptime',
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/nattype'],
         'depends': tseries_depends,
         'sources': np_datetime_sources},
     '_libs.tslibs.timedeltas': {
-        'pyxfile': '_libs/tslibs/timedeltas'},
+        'pyxfile': '_libs/tslibs/timedeltas',
+        'pxdfiles': ['_libs/src/util'],
+        'depends': np_datetime_headers,
+        'sources': np_datetime_sources},
     '_libs.tslibs.timezones': {
-        'pyxfile': '_libs/tslibs/timezones'},
+        'pyxfile': '_libs/tslibs/timezones',
+        'pxdfiles': ['_libs/src/util']},
     '_libs.testing': {
         'pyxfile': '_libs/testing'},
     '_libs.window': {


Code review comments:
diff_hunk: @@ -535,15 +536,24 @@ def pxd(name):
         'depends': _pxi_dep['sparse']},
     '_libs.tslib': {
         'pyxfile': '_libs/tslib',
-        'pxdfiles': ['_libs/src/util'],
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/conversion',
+                     '_libs/tslibs/timedeltas',
+                     '_libs/tslibs/timezones',
+                     '_libs/tslibs/nattype'],
         'depends': tseries_depends,
         'sources': np_datetime_sources},
     '_libs.tslibs.conversion': {
         'pyxfile': '_libs/tslibs/conversion',
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/timezones',
+                     '_libs/tslibs/np_datetime',
path: setup.py
text: @user1:
`np_datetime` is doubled up here and in tseries_depends.

@author:
yep just fixed that

diff_hunk: @@ -535,15 +536,24 @@ def pxd(name):
         'depends': _pxi_dep['sparse']},
     '_libs.tslib': {
         'pyxfile': '_libs/tslib',
-        'pxdfiles': ['_libs/src/util'],
+        'pxdfiles': ['_libs/src/util',
+                     '_libs/tslibs/conversion',
+                     '_libs/tslibs/timedeltas',
+                     '_libs/tslibs/timezones',
+                     '_libs/tslibs/nattype'],
         'depends': tseries_depends,
path: setup.py
text: @user1:
Does it matter that `np_datetime` is included in 'depends' via `tseries_depends` but not in 'pxdfiles'?

Putting e.g. conversion.pxd into tslib's 'pxdfiles' entry adds it to the `sources` kwarg of Extension.  Why is that the desired outcome?  (and uh, what does that do?  the distutils [docs](https://docs.python.org/3/distutils/apiref.html#distutils.core.Extension) don't actually say anything useful)

@author:
the disutils docs are pretty bad.

I went with the premise that if we are cimporting then we care if the api changes.  to test is easy, change something in nattype.pxd, you get recompiles in lots of things, but not everything (e.g. fields/frequencies and such). change timedeltas.pxd and you get less.

this has the desired behavior of only dependency based compilation.

I think the sources/depends are actually much more on the c-level and are separate from this. (but I don't think this is really an issue as we basically include _libs/src in the compilation each time so that the c-headers are there).