[CLN] resolve circular Period dependency, prepare setup.py #21854
Conversation
| # lazy import to prevent circularity | ||
| # TODO: Avoid non-cython dependency | ||
| from pandas.core.algorithms import unique | ||
|
|
There was a problem hiding this comment.
I'm now leaning towards actually moving _FrequencyInferer back out of cython (it came from tseries.frequencies previously). The only place it is used is in frequencies.infer_freq, which itself is only used from DatetimelikeArrayMixin. i.e. it could reasonably be considered out-of-scope for tslibs, which is largely scalar-centric.
Moving it out would allow us to get rid of the unsightly khash dependency, which is the last non-src pandas dependency for tslibs.
There was a problem hiding this comment.
great, will do this in follow up
| @@ -103,27 +97,30 @@ def is_platform_mac(): | |||
|
|
|||
|
|
|||
| class build_ext(_build_ext): | |||
| def build_extensions(self): | |||
| @classmethod | |||
| def render_templates(cls, pxifiles): | |||
There was a problem hiding this comment.
I needed to be able to call this method separately to get cythonize working.
| @@ -53,24 +45,26 @@ def is_platform_mac(): | |||
| } | |||
There was a problem hiding this comment.
If there are no objections, I'd like to move this dictionary up above the min_cython_version = '0.28.2 line
Codecov Report
@@ Coverage Diff @@
## master #21854 +/- ##
==========================================
+ Coverage 91.91% 91.91% +<.01%
==========================================
Files 164 164
Lines 49987 49988 +1
==========================================
+ Hits 45947 45948 +1
Misses 4040 4040
Continue to review full report at Codecov.
|
| # lazy import to prevent circularity | ||
| # TODO: Avoid non-cython dependency | ||
| from pandas.core.algorithms import unique | ||
|
|
| @@ -53,24 +45,26 @@ def is_platform_mac(): | |||
| } | |||
setup.py
Outdated
| @@ -53,24 +45,26 @@ def is_platform_mac(): | |||
| } | |||
|
|
|||
|
|
|||
| # The import of Extension must be after the import of Cython, otherwise | |||
| # we do not get the appropriately patched class. | |||
There was a problem hiding this comment.
Found the appropriate reference in the cython docs:
Note that when using setuptools, you should import it before Cython as setuptools may replace the Extension class in distutils. Otherwise, both might disagree about the class to use here.
setup.py
Outdated
|
|
||
| try: | ||
| if not _CYTHON_INSTALLED: | ||
| raise ImportError('No supported version of Cython installed.') | ||
| try: | ||
| from Cython.Distutils.old_build_ext import old_build_ext as _build_ext # noqa:F811,E501 | ||
| from Cython.Distutils.old_build_ext import old_build_ext as _build_ext | ||
| except ImportError: |
There was a problem hiding this comment.
yes I think that is ok to remove
| @@ -471,8 +468,6 @@ def pxd(name): | |||
|
|
|||
| tseries_depends = np_datetime_headers + ['pandas/_libs/tslibs/np_datetime.pxd'] | |||
|
|
|||
| # some linux distros require it | |||
There was a problem hiding this comment.
Apparently not. It has apparently sat there unused for a while.
|
thanks! happy to take these cleaning PRs! |
For a long time there has been a comment in
_libs.__init__saying it would be nice to importPerioddirectly but that is not possible due to circular imports. This resolves that issue.Also does some cleanup in setup.py, motivated by the goals of a) using
cythonizeand b) implementing test coverage for cython files. I've gotten those working locally, but they involve big diffs, so this gets some of the easy stuff out of the way.