Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-102567: Add -X importtime=2 for logging an importtime message for already-loaded modules#118655

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

noahbkim
Copy link
Contributor

@noahbkimnoahbkim commentedMay 6, 2024
edited
Loading

As mentioned in the issue:

While-X importtime is incredibly useful for analyzing module import times, by design, it doesn't log anything if an imported module has already been loaded.-X importtime=2 would provide additional output for every module that's already been loaded:

The updated example (-Ximporttime=2):

>>> import uuidimport time: cached    | cached     | builtinsimport time: cached    | cached     | linecacheimport time: cached    | cached     |   _ioimport time: cached    | cached     |   osimport time: cached    | cached     |   sysimport time: cached    | cached     |   enumimport time:       594 |        594 |   _uuidimport time:      2428 |       3022 | uuid

With-Ximporttime:

>>> import uuidimport time:       351 |        351 |   _uuidimport time:       843 |       1194 | uuid

Discussion:https://discuss.python.org/t/x-importtrace-to-supplement-x-importtime-for-loaded-modules/23882/5
Prior email chain:https://mail.python.org/archives/list/python-ideas@python.org/thread/GEISYQ5BXWGKT33RWF77EOSOMMMFUBUS/

davidwitten reacted with rocket emoji
@Eclips4
Copy link
Member

Eclips4 commentedMay 6, 2024
edited
Loading

Hello!
This PR needs to add:

  1. ANEWS entry
  2. Awhatsnew entry
  3. A new paragraph in theDoc/cmdline for the-X option.
  4. New test case in theLib/test/test_cmd_line

@noahbkim
Copy link
ContributorAuthor

Hello! This PR needs to add:

  1. ANEWS entry
  2. Awhatsnew entry
  3. A new paragraph in theDoc/cmdline for the-X option.
  4. New test case in theLib/test/test_cmd_line

Thanks for the quick feedback. I've committed a first pass of 1-3. As for tests: there appear to be noimporttime tests (likely due to the fact that any simple implementation would be super noisy as other parts of the interpreter change). Would it be sufficient to do some rudimentary check i.e.import foo then ensure a correspondingfoo row appears in the output?

Thanks again!

@Eclips4
Copy link
Member

Hello! This PR needs to add:

  1. ANEWS entry
  2. Awhatsnew entry
  3. A new paragraph in theDoc/cmdline for the-X option.
  4. New test case in theLib/test/test_cmd_line

Thanks for the quick feedback. I've committed a first pass of 1-3. As for tests: there appear to be noimporttime tests (likely due to the fact that any simple implementation would be super noisy as other parts of the interpreter change). Would it be sufficient to do some rudimentary check i.e.import foo then ensure a correspondingfoo row appears in the output?

Thanks again!

Yes, it would be sufficient :)

@Eclips4
Copy link
Member

Also, there's a merge conflict. Can you resolve it? (I can do it myself, but it seems like you have turned off this option)

@noahbkim
Copy link
ContributorAuthor

Also, there's a merge conflict. Can you resolve it? (I can do it myself, but it seems like you have turned off this option)

Done, just added tests as well

@noahbkim
Copy link
ContributorAuthor

@Eclips4 who should I ping to get final review? Is it alright if I @ the blamed reviewers?

@Eclips4
Copy link
Member

@Eclips4 who should I ping to get final review? Is it alright if I @ the blamed reviewers?

I guess@vstinner is the right person to review this 😄
However, you should move your changes from whatsnew/3.13.rst to whatsnew/3.14.rst since 3.13 branch is in feature-freeze mode, and this feature will appear only in the 3.14 version.

@Eclips4
Copy link
Member

Eclips4 commentedMay 28, 2024
edited
Loading

However, you should move your changes from whatsnew/3.13.rst to whatsnew/3.14.rst since 3.13 branch is in feature-freeze mode, and this feature will appear only in the 3.14 version.

The same applies for theversionchanged directives.

@noahbkim
Copy link
ContributorAuthor

@AA-Turner merged your changes, happy to do anything else necessary, apologies for the inconvenience of the organization branch. Not sure where your comment went (I saw it in my email), but thanks for helping push this through.

@AA-Turner
Copy link
Member

AA-Turner commentedMay 1, 2025
edited
Loading

Thanks@noahbkim, no worries! Are you able to make the changes toconfig_set_import_time etc to reserve values >=2?

We also recently changed our CLA bot, please can you click on the red 'not signed' and ensure everything is up to date?

A

@noahbkim
Copy link
ContributorAuthor

I think I've messed up my branch, I'm going to attempt to squash everything so I can overwrite my commit email correctly.

@AA-Turner
Copy link
Member

Ok, please ping for a review when you've sorted the branch out! If it's easier, feel free to just open a new PR, but a force push here should also work.

A

@noahbkimnoahbkimforce-pushed thefeature/import-cache-2 branch frome53dea1 to0ed040cCompareMay 1, 2025 20:24
@noahbkim
Copy link
ContributorAuthor

@AA-Turner I've squashed my changes and given everything a second pass.

It looks like I caused some kind of regression intest_embed.py, but none of the expected invariants make sense to me (I'm not sure why we expectimport_time to be set to1 in any of the listed circumstances). Looking through the blame was not particularly illuminating. My suspicion is that these tests have been migrated/updated several times without regard for what they're actually testing (or perhaps implicit behavior surrounding-Ximporttime has changed since the test suite's addition?).

Zeroingimport_time in said cases returns everything to passing, but a second pair of eyes on that particular bit would be greatly appreciated.

I have about 6 tests failing locally, one of which prevents me from compiling with--enable-optimizations (test_json) but I can't imagine those are related. I'll check if my merge base is behind + fix the CI lint issue.

@noahbkimnoahbkimforce-pushed thefeature/import-cache-2 branch from0ed040c to62d09cdCompareMay 1, 2025 20:31
@noahbkimnoahbkimforce-pushed thefeature/import-cache-2 branch from62d09cd to58d07beCompareMay 2, 2025 14:41
import nt
return nt._supports_virtual_terminal()
except (ImportError, AttributeError):
nt._supports_virtual_terminal()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
nt._supports_virtual_terminal()
returnnt._supports_virtual_terminal()

This is missing a return and would never enable virtual terminal anymore.

@AA-Turner
Copy link
Member

@noahbkim I've pushed a series of commits to the3.14-importtime=2 branch on my fork. I've created a PR to HRT's fork (hudson-trading#1), which if merged will automatically update this PR with those commits. Please review them and let me know what you think. In short:

  • Fixed an issue where the header was appearing after the first import:
    .\python.bat -SX importtime=2 -c 'import os; import os'Running Debug|x64 interpreter...import time: cached    | cached     | builtinsimport time: self [us] | cumulative | imported packageimport time:       325 |        325 | winreg
  • Minor style changes to the pyrepl changes this PR introduces, and also the issue@chris-eibl noted.
  • Simplify initalisation forconfig.import_time & fix a bug where explicit setting of the member was overridden (see changes totest_embed).
  • Add some more test cases
  • Update documentation, including the man page & adding you toMisc/ACKS.

When these changes are addressed, we can run the buildbots on this PR and check that everything looks alright. Thanks!

A

@AA-Turner
Copy link
Member

ping@noahbkim (feature freeze is tomorrow)

`-X importtime=2` feedback(Thanks so much,@AA-Turner)
@noahbkimnoahbkimforce-pushed thefeature/import-cache-2 branch fromc0d3c1f to6d50b0cCompareMay 5, 2025 15:30
@noahbkim
Copy link
ContributorAuthor

@AA-Turner I am incredibly grateful for the work you've done to get this over the finish line. Thanks again, I'll have an eye on Github/my email all day in case you need anything else.

AA-Turner reacted with heart emoji

@AA-TurnerAA-Turner added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 5, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@AA-Turner for commit6d50b0c 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F118655%2Fmerge

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 5, 2025
@AA-Turner
Copy link
Member

Buildbots show leaks intest_datetime which seem unrelated.

Copy link
Member

@AA-TurnerAA-Turner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks!

A

@AA-TurnerAA-Turner merged commitc4bcc6a intopython:mainMay 6, 2025
107 of 120 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra left review comments

@picnixzpicnixzpicnixz left review comments

@chris-eiblchris-eiblchris-eibl left review comments

@methanemethanemethane approved these changes

@AA-TurnerAA-TurnerAA-Turner approved these changes

@Eclips4Eclips4Eclips4 approved these changes

@vstinnervstinnerAwaiting requested review from vstinner

@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

@pablogsalpablogsalAwaiting requested review from pablogsalpablogsal is a code owner

@ambvambvAwaiting requested review from ambvambv is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

10 participants
@noahbkim@Eclips4@picnixz@AA-Turner@bedevere-bot@ambv@methane@JelleZijlstra@serhiy-storchaka@chris-eibl

[8]ページ先頭

©2009-2025 Movatter.jp