Skip to content

[CLN] resolve circular Period dependency, prepare setup.py #21854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions pandas/_libs/__init__.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
# -*- coding: utf-8 -*-
# flake8: noqa

from .tslibs import iNaT, NaT, Timestamp, Timedelta, OutOfBoundsDatetime

# TODO
# period is directly dependent on tslib and imports python
# modules, so exposing Period as an alias is currently not possible
# from period import Period
from .tslibs import (
iNaT, NaT, Timestamp, Timedelta, OutOfBoundsDatetime, Period)
1 change: 1 addition & 0 deletions pandas/_libs/tslibs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
from .conversion import normalize_date, localize_pydatetime, tz_convert_single
from .nattype import NaT, iNaT
from .np_datetime import OutOfBoundsDatetime
from .period import Period, IncompatibleFrequency
from .timestamps import Timestamp
from .timedeltas import delta_to_nanoseconds, ints_to_pytimedelta, Timedelta
10 changes: 8 additions & 2 deletions pandas/_libs/tslibs/resolution.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ from timestamps import Timestamp

from pandas._libs.properties import cache_readonly

from pandas.core.algorithms import unique # TODO: Avoid this non-cython import

# ----------------------------------------------------------------------
# Constants

Expand Down Expand Up @@ -574,6 +572,10 @@ cdef class _FrequencyInferer(object):
if len(self.ydiffs) > 1:
return None

# lazy import to prevent circularity
# TODO: Avoid non-cython dependency
from pandas.core.algorithms import unique

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure that would be ok

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, will do this in follow up

if len(unique(self.fields['M'])) > 1:
return None

Expand Down Expand Up @@ -618,6 +620,10 @@ cdef class _FrequencyInferer(object):
# if not lib.ismember(wdiffs, set([4, 5, -47, -49, -48])).all():
# return None

# lazy import to prevent circularity
# TODO: Avoid non-cython dependency
from pandas.core.algorithms import unique

weekdays = unique(self.index.weekday)
if len(weekdays) > 1:
return None
Expand Down
2 changes: 2 additions & 0 deletions pandas/tests/tslibs/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def test_namespace():
api = ['NaT',
'iNaT',
'OutOfBoundsDatetime',
'Period',
'IncompatibleFrequency',
'Timedelta',
'Timestamp',
'delta_to_nanoseconds',
Expand Down
69 changes: 32 additions & 37 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@ def is_platform_windows():
return sys.platform == 'win32' or sys.platform == 'cygwin'


def is_platform_linux():
return sys.platform == 'linux2'


def is_platform_mac():
return sys.platform == 'darwin'


min_cython_ver = '0.28.2'
try:
import Cython
Expand All @@ -53,24 +45,26 @@ def is_platform_mac():
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are no objections, I'd like to move this dictionary up above the min_cython_version = '0.28.2 line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure



# The import of Extension must be after the import of Cython, otherwise
# we do not get the appropriately patched class.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems odd?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is so weird, but ok

# TODO: reference as to why?
from distutils.extension import Extension # noqa:E402
from distutils.command.build import build # noqa:E402
from distutils.command.build_ext import build_ext as _build_ext # noqa:E402

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think that is ok to remove

# Pre 0.25
# TODO: Can we remove this branch since 0.28 is now required?
from Cython.Distutils import build_ext as _build_ext
cython = True
except ImportError:
from distutils.command.build_ext import build_ext as _build_ext
cython = False


if cython:
else:
try:
try:
from Cython import Tempita as tempita
Expand Down Expand Up @@ -103,27 +97,30 @@ def is_platform_mac():


class build_ext(_build_ext):
def build_extensions(self):
@classmethod
def render_templates(cls, pxifiles):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to be able to call this method separately to get cythonize working.

for pxifile in pxifiles:
# build pxifiles first, template extension must be .pxi.in
assert pxifile.endswith('.pxi.in')
outfile = pxifile[:-3]

# if builing from c files, don't need to
# generate template output
if cython:
for pxifile in _pxifiles:
# build pxifiles first, template extension must be .pxi.in
assert pxifile.endswith('.pxi.in')
outfile = pxifile[:-3]

if (os.path.exists(outfile) and
os.stat(pxifile).st_mtime < os.stat(outfile).st_mtime):
# if .pxi.in is not updated, no need to output .pxi
continue
if (os.path.exists(outfile) and
os.stat(pxifile).st_mtime < os.stat(outfile).st_mtime):
# if .pxi.in is not updated, no need to output .pxi
continue

with open(pxifile, "r") as f:
tmpl = f.read()
pyxcontent = tempita.sub(tmpl)
with open(pxifile, "r") as f:
tmpl = f.read()
pyxcontent = tempita.sub(tmpl)

with open(outfile, "w") as f:
f.write(pyxcontent)
with open(outfile, "w") as f:
f.write(pyxcontent)

def build_extensions(self):
# if building from c files, don't need to
# generate template output
if cython:
self.render_templates(_pxifiles)

numpy_incl = pkg_resources.resource_filename('numpy', 'core/include')

Expand Down Expand Up @@ -360,7 +357,6 @@ def run(self):
class CheckingBuildExt(build_ext):
"""
Subclass build_ext to get clearer report if Cython is necessary.

"""

def check_cython_extensions(self, extensions):
Expand All @@ -379,9 +375,11 @@ def build_extensions(self):


class CythonCommand(build_ext):
"""Custom distutils command subclassed from Cython.Distutils.build_ext
"""
Custom distutils command subclassed from Cython.Distutils.build_ext
to compile pyx->c, and stop there. All this does is override the
C-compile method build_extension() with a no-op."""
C-compile method build_extension() with a no-op.
"""
def build_extension(self, ext):
pass

Expand Down Expand Up @@ -445,7 +443,6 @@ def srcpath(name=None, suffix='.pyx', subdir='src'):
lib_depends.append('pandas/_libs/src/util.pxd')
else:
lib_depends = []
plib_depends = []

common_include = ['pandas/_libs/src/klib', 'pandas/_libs/src']

Expand All @@ -471,8 +468,6 @@ def pxd(name):

tseries_depends = np_datetime_headers + ['pandas/_libs/tslibs/np_datetime.pxd']

# some linux distros require it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently not. It has apparently sat there unused for a while.

libraries = ['m'] if not is_platform_windows() else []

ext_data = {
'_libs.algos': {
Expand Down