-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-118761: Add test_lazy_import for more modules #133057
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
Conversation
@Wulian233 I am not sure what you mean. Please see #132614 and #118761 (comment) for the motivation. |
Thanks! Feel free to make the same change for more modules in this PR. |
@JelleZijlstra thanks for taking a look! I've added the tests for Unless somebody else beats me to it, I can go through the rest of the modules handled in #118761, maybe next weekend. |
… string and zipfile modules
I went through the rest of the modules from #118761 and added the respective tests. This should be good to go. @JelleZijlstra I pushed one more commit which I forgot to push with the others, apologies. |
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.
Found a few more
Also a merge conflict. If you don't get to it I can spend a bit more time later today fixing it up, hopefully we can still get it in by the beta. (And it's not the end of the world if we don't.) |
Co-authored-by: Jelle Zijlstra <[email protected]>
Done, thanks for a thorough review!
Ah, good thinking. I've only included those modules that were specifically made lazy in the various PRs. This got me thinking though that the current approach is somewhat suboptimal -- it would be more robust to have an allowlist of modules. Right now, a previously unused module can still be added and the tests might still pass. But this PR is definitely an improvement, LMK if you want me to open a follow-up issue for discussion. |
An allowlist could make sense too, but might be harder to maintain. Feel free to open an issue discussing it. I guess my primary motivation was "I did all this work to make it so typing doesn't import annotationlib, let's make it so we don't regress by accident". |
I went through my past PRs where I sped up the import time of
threading
andpathlib
modules (#114509 and #123520) and added regression tests to ensure that the respective module imports stay lazy, using @JelleZijlstra's newensure_lazy_import
test fixture in #132614.Happy to do this for more modules if it is better to do it in one PR instead of many.
I wasn't quite sure where to put these tests so happy to take guidance on that.