-
Notifications
You must be signed in to change notification settings - Fork 280
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 frontends initialization until yt.load is actually called #4539
PERF: delay frontends initialization until yt.load is actually called #4539
Conversation
ed268cd
to
f904af2
Compare
f904af2
to
e565cd6
Compare
yt/frontends/__init__.py
Outdated
if value == "_all": | ||
for _ in __all__: | ||
__getattr__(_) | ||
return |
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.
note: the reason this special case is needed is that nesting from yt.frontends import *
is syntactically invalid.
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 good -- a couple very small comments.
@@ -90,6 +90,8 @@ def load( | |||
yt.utilities.exceptions.YTAmbiguousDataType | |||
If the data format matches more than one class of similar specialization levels. | |||
""" | |||
from yt.frontends import _all # type: ignore [attr-defined] # noqa |
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.
Oh and one extra comment/question -- by nesting this import in the load
and load_simulation
calls, does that mean one could import yt
and then directly instantiate a frontend without ever importing all the other frontends? That would actually be a pretty nice consequence beyond simply delaying the imports... might be worth a mention in the docs even. Could make a difference for batch processing scripts.
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.
indeed, I think with this patch you'd be able to short-circuit all frontends being loaded, but I think at this point it should be seen as an implementation detail rather than a feature (in particular because circumventing yt.load
would require different code if your frontends isn't builtin, but lives in an extension)
5ecad14
Co-authored-by: Chris Havlin <[email protected]>
@matthewturk this is ready ! |
PR Summary
Still pursuing my quest of making yt's CLI decently fast. This saves about 50 to 100ms overhead on startup time on my (relatively quick) laptop.