Skip to content

CLN: circular/runtime imports in tslibs #34563

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 5 commits into from
Jun 4, 2020

Conversation

jbrockmendel
Copy link
Member

ATM tzconversion depends on timedeltas (for delta_to_nanoseconds), which throws a wrench in the erstwhile dependency hierarchy. By making the one usage of delta_to_nanoseconds a runtime import, we make it possible to remove a bunch of other runtime imports.

@@ -123,6 +122,7 @@ timedelta-like}
elif nonexistent == 'shift_backward':
shift_backward = True
elif PyDelta_Check(nonexistent):
from .timedeltas import delta_to_nanoseconds
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to move delta_to_nanoseconds even higher eg to base.pyx ?

Copy link
Member Author

Choose a reason for hiding this comment

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

ive considered this and gotten hung up on attribute lookup behavior which isnt doing optimizations that i think it should. I'd like to punt on this for now until I get that sorted out.

@jreback jreback added the Clean label Jun 4, 2020
@jreback jreback added this to the 1.1 milestone Jun 4, 2020
@@ -49,7 +47,9 @@ from pandas._libs.tslibs.timezones cimport utc_pytz as UTC
from pandas._libs.tslibs.tzconversion cimport tz_convert_single

from .timedeltas cimport delta_to_nanoseconds

from .timedeltas import Timedelta
Copy link
Contributor

Choose a reason for hiding this comment

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

are we consisten about relative imports? I really don't like mixing 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.

we're not, but the above line is relative so i figured this should be too since its the same module.

while we generally do absolute, i think we should move to relative for tslibs since self-containment is a big deal here

@jreback jreback merged commit 740f94f into pandas-dev:master Jun 4, 2020
@jreback
Copy link
Contributor

jreback commented Jun 4, 2020

ok fair, but we should document this somewhere i think.

@jbrockmendel jbrockmendel deleted the ref-delta_to_nanoseconds branch June 4, 2020 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants