-
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
ENH: reduce import time by avoiding top-level expensive imports #4025
Conversation
my optimisations for unyt are public: and I already released optimisations for cmyt |
a86b70c
to
8cd0557
Compare
I mostly like this, but it's probably worth changing the name to say we're avoiding top-level matplotlib imports, right? |
oh yes, absolutely, somehow this survived my commit squash abuse |
8cd0557
to
2a7f46d
Compare
2a7f46d
to
0770b04
Compare
Actually it's even more general than that because |
I think we may want to explore a more elegant way of doing the imports, but everything I have come up with involves globals and whatnot, which ... yeah, not great. |
There are people thinking about this very seriously at the ecosystem level https://scientific-python.org/specs/spec-0001/ Though it's not clear if/when we'll be able to leverage anything that comes out of it. |
58f2c78
to
0770b04
Compare
I think that we should move forward, and move back to top level if the ecosystem wide change works out for us. Thank you for doing this. |
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.
Looks pretty good to me.
I guess you could merge it now if you want, since failures are unrelated (#4023). |
2a01be4
to
ff3e610
Compare
@yt-fido test this please |
ff3e610
to
957af7e
Compare
messed up somewhere, let's switch back to draft for now |
…b just to check its version)
167b243
to
ebff1c6
Compare
add4d93
to
359a682
Compare
There's also an ongoing PEP to support lazy imports support directly in importlib, currently targeted at Python 3.12 |
import matplotlib._png as _png | ||
except ImportError: | ||
from PIL import Image | ||
from PIL import Image |
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.
Just so we're on the same page -- if I'm reading it right, PIL
should already be a transitive dependency, right?
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.
almost: it is a transitive dependency if MPL 3.3 or newer is installed. In the future, as we progressively fade out support to older versions, it will always be a transitive dependency, yes.
PR Summary
Reduce yt's import time by 35-60%
This estimation was performed using other optimizations for core dependencies (cmyt and unyt).
To be clear, I'm comparing yt's import time with and without the present patch, but I'm including cmyt and unyt optimizations in both cases.
Here's the command I've used to measure and target main offenders: