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

PERF: delay frontends initialization until yt.load is actually called #4539

Merged
merged 3 commits into from
Jul 3, 2023

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Jun 26, 2023

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.

@neutrinoceros neutrinoceros added performance enhancement Making something better labels Jun 26, 2023
@neutrinoceros neutrinoceros force-pushed the delay_frontends_load branch 5 times, most recently from ed268cd to f904af2 Compare June 26, 2023 12:41
matthewturk
matthewturk previously approved these changes Jun 26, 2023
Comment on lines 49 to 52
if value == "_all":
for _ in __all__:
__getattr__(_)
return
Copy link
Member Author

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.

chrishavlin
chrishavlin previously approved these changes Jul 3, 2023
Copy link
Contributor

@chrishavlin chrishavlin left a 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.

yt/frontends/__init__.py Show resolved Hide resolved
yt/frontends/__init__.py Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

@chrishavlin chrishavlin Jul 3, 2023

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.

Copy link
Member Author

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)

@neutrinoceros
Copy link
Member Author

@matthewturk this is ready !

@matthewturk matthewturk merged commit 9bb7bfe into yt-project:main Jul 3, 2023
@neutrinoceros neutrinoceros deleted the delay_frontends_load branch July 3, 2023 20:11
@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants