-
Notifications
You must be signed in to change notification settings - Fork 279
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
PERF: delay rarely used imports (netcdf4, importlib, multiprocessing, tarfile, tomllib, tomli_w) #4517
Conversation
83aeeae
to
0c611fe
Compare
0c611fe
to
10e162d
Compare
blocked by #4515 |
b08b582
to
9ba463c
Compare
Do we have a breakdown of import costs? Maybe it could be a good way to look at splitting stuff up? |
I profile startup with tuna ( This command opens an interactive flame graph showing everything that happens on python -X importtime -c "import yt" 2> import.log && python -m tuna import.log
python -X importtime -c "import numpy; import matplotlib; import sympy; import unyt; import yt" 2> import.log && python -m tuna import.log I highly recommend running this profile with Python 3.11, and latest versions of sympy, unyt, and cmyt, all of which contain optimisations that make yt's startup slightly faster. Also note that when switching branch/making changes, the measured startup time include byte code compilation, so results should not be considered realistic on the very first run. Splitting up modules is unlikely to bring direct gains. Unyt is by far the biggest offender, but I think I've made every possible "easy" optimisations between v2.8 and v2.9.5 already. |
c496693
to
c974677
Compare
b40784a
to
b41eec5
Compare
|
07bad14
to
fa6b927
Compare
currently blocked by #4540 |
2c52296
to
0424228
Compare
So I've looked at this a few times and I'm feeling on the fence: when it's just one or two uses within a module, nesting those imports inside function calls is OK by me, particularly if it is an import of an external package. Where I'm uncertain is for our internal imports that are used throughout a module: e.g., I think the readability of If there are parts of yt that both take a long time to load and are not used in the majority of sessions, then I think it'd be worth looking at a more formal lazy import framework using something like So I think I'd add an approval here but would appreciate a bit more discussion before this goes in cause it feels like a shift in coding style -- it seems hard to maintain these gains going forward without recommending nested imports in new code, because it certainly doesn't seem tenable for folks to simply remember which imports should be nested and which don't need it. |
Thank you for having a look. I'm okay to drop parts of this PR, as the gains are absolutely not worth any level of controversy :) #4539 is a much more efficient, and completely different approach, towards the same goal, so if that one is okay I'll be happy to re-evaluate the present patch. |
Well I don't think this strays into controversial :) It's more a practical question of whether the gains you get here will be easily maintained down the line. I think the external imports are easy enough, but not so sure about internal yt imports. The very specific yt imports (like your changes to |
Yeah that's the biggest problem with this technique, it's very easy to ruin hours of effort with just one |
0424228
to
b35fc15
Compare
@chrishavlin so I did my homework: here's a box plot of import times for the main branch against this branch I ran |
Thanks for breaking it down by commit! Ya, I think with just the first commit I'd hit approve right away :) Does look like the |
…ib.resources, multiprocessing, tarfile, tomllib, tomli_w, csv, matplotlib.font_manager)
b35fc15
to
a75975d
Compare
revamped ! I kept the first commit, and followed your suggestions on what to keep fro the 2nd and 3rd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! LGTM now!
PR Summary
import netCDF4
is responsible for ~6% startup overhead, let's delay it when we can afford it.import importlib.metadata
is about ~3% (we still need to import it to determine matplotlib's version at runtime, which we'll be able to avoid when support for mpl<3.5 is dropped)multiprocessing, tarfile, tomllib, tomli_w are (collectively) about ~1%
nothing life-changing here but every % counts (yt's CLI still takes a couple seconds to do anything).