GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-118761: Add test_lazy_import for more modules by danielhollas · Pull Request #133057 · python/cpython

Conversation

@danielhollas

@danielhollas danielhollas commented Apr 27, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I went through my past PRs where I sped up the import time of threading and pathlib modules (#114509 and #123520) and added regression tests to ensure that the respective module imports stay lazy, using @JelleZijlstra's new ensure_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.

  • Issue: Improve import time of various stdlib modules #118761

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@danielhollas
pythongh-118761: test_lazy_import for threading and pathlib modules
3f0cdae
@danielhollas
danielhollas requested a review from barneygale as a code owner April 27, 2025 20:56
@bedevere-app bedevere-app Bot added awaiting review tests Tests in the Lib/test dir labels Apr 27, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Apr 27, 2025
Improve import time of various stdlib modules #118761
Closed
@picnixz picnixz added the skip news label Apr 27, 2025
@danielhollas

danielhollas commented Apr 27, 2025

Copy link
Copy Markdown
Contributor Author

@Wulian233 I am not sure what you mean. Please see #132614 and #118761 (comment) for the motivation.

JelleZijlstra reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra reviewed Apr 27, 2025
View reviewed changes
Comment thread Lib/test/test_pathlib/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Apr 27, 2025

Copy link
Copy Markdown
Member

Thanks! Feel free to make the same change for more modules in this PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@danielhollas
Add test_lazy_import for enum, functools, and email.utils
97e8d47
@danielhollas
danielhollas requested review from a team, ethanfurman and rhettinger as code owners April 28, 2025 00:12
@danielhollas

danielhollas commented Apr 28, 2025

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra thanks for taking a look! I've added the tests for enum, functools and email.utils modules. This should cover the modules handled in the original issue #109653. I'll stop for now.

Unless somebody else beats me to it, I can go through the rest of the modules handled in #118761, maybe next weekend.

JelleZijlstra reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra reviewed Apr 28, 2025
View reviewed changes
Comment thread Lib/test/test_enum.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@danielhollas danielhollas changed the title gh-118761: test_lazy_import for threading and pathlib modules gh-118761: Add test_lazy_import for more modules Apr 28, 2025
JelleZijlstra
JelleZijlstra reviewed Apr 28, 2025
View reviewed changes
Comment thread Lib/test/test_pathlib/test_pathlib.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@rhettinger
rhettinger removed their request for review April 29, 2025 16:07
danielhollas added 3 commits May 4, 2025 20:36
@danielhollas
Review
875e0bd
@danielhollas
Add test_lazy_import for pickle, mimetypes, base64, csv, pprint and s…
34e39ab
…ocket modules
@danielhollas
Merge branch 'main' into test-lazy-import
d224483
JelleZijlstra
JelleZijlstra reviewed May 5, 2025
View reviewed changes
Comment thread Lib/test/test_base64.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_mimetypes.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@danielhollas
Add test_lazy_import for ast, cmd, gettext, locale, optparse, pstats,…
eabf651
… string and zipfile modules
@danielhollas
danielhollas requested a review from Eclips4 as a code owner May 5, 2025 17:30
@danielhollas

danielhollas commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

Unless somebody else beats me to it, I can go through the rest of the modules handled in #118761, maybe next weekend.

I went through the rest of the modules from #118761 and added the respective tests.
I only skipped importlib.metadata and tomllib since it is not clear to me if the tests should live in cpython repo or in the respective upstream repositories. Leaving that discussion for a follow-up PR.

This should be good to go. @JelleZijlstra I pushed one more commit which I forgot to push with the others, apologies.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra reviewed May 5, 2025
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_ast/test_ast.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_gettext.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented May 5, 2025

Copy link
Copy Markdown
Member

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.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danielhollas and others added 2 commits May 5, 2025 22:14
@danielhollas @JelleZijlstra
Apply suggestions from code review
4628f14
Co-authored-by: Jelle Zijlstra <[email protected]>
@danielhollas
Merge branch 'main' into test-lazy-import
aa19ba5
@danielhollas

danielhollas commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

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.)

Done, thanks for a thorough review!

I'm getting these by searching for "import" in the source file and collecting the imports inside functions

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@danielhollas
Merge branch 'main' into test-lazy-import
efedb94
@JelleZijlstra

JelleZijlstra commented May 5, 2025

Copy link
Copy Markdown
Member

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".

danielhollas reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra
JelleZijlstra approved these changes May 5, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 5, 2025
@JelleZijlstra
JelleZijlstra enabled auto-merge (squash) May 5, 2025 22:37
@JelleZijlstra
JelleZijlstra merged commit cae660d into python:main May 5, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label May 5, 2025
@danielhollas
danielhollas deleted the test-lazy-import branch May 5, 2025 23:00
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@danielhollas @JelleZijlstra @Pranjal095
pythongh-118761: Add test_lazy_import for more modules (python#133057)
2388b77
Co-authored-by: Jelle Zijlstra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@barneygale barneygale Awaiting requested review from barneygale barneygale is a code owner
@ethanfurman ethanfurman Awaiting requested review from ethanfurman ethanfurman is a code owner
@Eclips4 Eclips4 Awaiting requested review from Eclips4 Eclips4 is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@danielhollas @JelleZijlstra @picnixz
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.