Skip to content
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

Significant increase in import time for several sub-packages #9555

Open
astrofrog opened this issue Nov 7, 2019 · 6 comments
Open

Significant increase in import time for several sub-packages #9555

astrofrog opened this issue Nov 7, 2019 · 6 comments

Comments

@astrofrog
Copy link
Member

It looks like there has been a significant (~40%) increase in import time for several sub-packages recently:

https://www.astropy.org/astropy-benchmarks/#imports.timeraw_import_astropy_coordinates

https://www.astropy.org/astropy-benchmarks/#imports.timeraw_import_astropy_nddata

asv seems to suggest that #9365 is the cause (cc @mhvk @eteq ).

Interestingly astropy.time also saw such an increase:

https://www.astropy.org/astropy-benchmarks/#imports.timeraw_import_astropy_time

but that was then fixed in #9491.

@pllim
Copy link
Member

pllim commented Nov 7, 2019

For coordinates, one should also review whether that same PR sped up other stuff or not. If it sped up some calculations significantly, import time increase is a small price to pay.

Unfortunately, no benchmark is available for nddata computations.

@mhvk
Copy link
Contributor

mhvk commented Nov 7, 2019

@astrofrog - not completely sure, but the problem with the auto-update of the leap seconds is that the moment you use any time (like setting the J2000 epoch in the coordinate frames), it will check whether the leap second table is up to date, which means importing utils.iers and therefore table and thus all of io. I didn't like this much but saw no easy alternative. Options:
0. Not worry; generally people will use tables/io at some point anyway.

  1. Only check for leap seconds when one gets a time beyond the last leap second built into erfa. I thought about this a bit, but concluded it was quite invasive within Time (obviously, I may be wrong).
  2. Read in the leap seconds with np.loadtxt or so. But then, we have this nice table interface and I think it is logical to expect leap seconds to be presented as a Table (and it to be part of utils.iers).
  3. Avoid importing all of io in table (right now, done to ensure the lists of readers and docstrings are complete).

Personally, I prefer (3), but this may not be trivial either; @taldcroft and @MSeifert04 did quite a nice job to make the reader registration and docstring autogeneration work, but perhaps it can somehow be done more lazily.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Nov 7, 2019

I haven't had time to look but what in nddata requires time? Is it the from astropy.coordinates import SkyCoord in utils? Or the units import?

@mhvk
Copy link
Contributor

mhvk commented Nov 7, 2019

coordinates does need Time (for the epoch/equinox definitions), units does not.

@astrofrog
Copy link
Member Author

Regarding:

Avoid importing all of io in table (right now, done to ensure the lists of readers and docstrings are complete).

it just occurred to me that one way around this would be to actually move all the connect.py files (which is where we defines readers/writers) to the astropy.table module and make sure that the imports to various io modules then happens inside the readers/writers as needed. I'm not sure why I didn't think of that before, but that could speed things up a lot?

@mhvk
Copy link
Contributor

mhvk commented Nov 7, 2019

That sounds very reasonable, though probably good to check the timing (IIRC that is quite easy with python 3.7), just to be sure it is in fact io that is the part that adds most time, not table itself (a quick check with python3 -X importtime -c 'import astropy.nddata' does suggest this is the case).

For nddata, it may also not be crazy to import coordinates locally (though, then again, if one almost always ends up using it anyway, one might as well get it upfront).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants