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-102567: Add -X importtime=2 for logging an importtime message for already-loaded modules by noahbkim · Pull Request #118655 · python/cpython

Conversation

@noahbkim

@noahbkim noahbkim commented May 6, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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 uuid
import time: cached    | cached     | builtins
import time: cached    | cached     | linecache
import time: cached    | cached     |   _io
import time: cached    | cached     |   os
import time: cached    | cached     |   sys
import time: cached    | cached     |   enum
import time:       594 |        594 |   _uuid
import time:      2428 |       3022 | uuid

With -Ximporttime:

>>> import uuid
import time:       351 |        351 |   _uuid
import 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/[email protected]/thread/GEISYQ5BXWGKT33RWF77EOSOMMMFUBUS/

  • Issue: Add -X importtime=2 for additional logging when an imported module is already loaded #102567

Sorry, something went wrong.

Uh oh!

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

davidwitten reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@noahbkim
noahbkim requested review from ericsnowcurrently and kumaraditya303 as code owners May 6, 2024 15:40
@bedevere-app bedevere-app Bot added the awaiting review label May 6, 2024
@bedevere-app bedevere-app Bot mentioned this pull request May 6, 2024
Add -X importtime=2 for additional logging when an imported module is already loaded #102567
Closed
@Eclips4

Eclips4 commented May 6, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Hello!
This PR needs to add:

  1. A NEWS entry
  2. A whatsnew entry
  3. A new paragraph in the Doc/cmdline for the -X option.
  4. New test case in the Lib/test/test_cmd_line
All reactions

Sorry, something went wrong.

Uh oh!

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

@noahbkim

noahbkim commented May 6, 2024

Copy link
Copy Markdown
Contributor Author

Hello! This PR needs to add:

  1. A NEWS entry
  2. A whatsnew entry
  3. A new paragraph in the Doc/cmdline for the -X option.
  4. New test case in the Lib/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 no importtime 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 corresponding foo row appears in the output?

Thanks again!

All reactions

Sorry, something went wrong.

Uh oh!

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

@Eclips4

Eclips4 commented May 6, 2024

Copy link
Copy Markdown
Member

Hello! This PR needs to add:

  1. A NEWS entry
  2. A whatsnew entry
  3. A new paragraph in the Doc/cmdline for the -X option.
  4. New test case in the Lib/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 no importtime 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 corresponding foo row appears in the output?

Thanks again!

Yes, it would be sufficient :)

All reactions

Sorry, something went wrong.

Uh oh!

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

@Eclips4

Eclips4 commented May 6, 2024

Copy link
Copy Markdown
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)

All reactions

Sorry, something went wrong.

Uh oh!

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

@noahbkim

noahbkim commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@noahbkim

noahbkim commented May 28, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@Eclips4

Eclips4 commented May 28, 2024

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@Eclips4

Eclips4 commented May 28, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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 the versionchanged directives.

All reactions

Sorry, something went wrong.

Uh oh!

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

@noahbkim
noahbkim requested review from a team, AlexWaygood, JelleZijlstra, barneygale, brettcannon, corona10, encukou, ericvsmith, erlend-aasland, ethanfurman, freakboy3742, jaraco, pfmoore, pradyunsg, rhettinger, sethmlarson and terryjreedy as code owners July 11, 2024 16:26
@noahbkim

noahbkim commented Jan 13, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

All checks except for mypy should pass. The issue reported by mypy is the following idiom

try:
    import posix
except ImportError:
    posix = None

Since this is used pretty widely throughout the standard library, I figured it should be left as is and the warning ignored. I could suppress the warning instead.

Update: woah, I lied, something has changed since my last push. I'll fix these failures today.

All reactions

Sorry, something went wrong.

Uh oh!

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

@picnixz

picnixz commented Jan 13, 2025

Copy link
Copy Markdown
Member

Since this is used pretty widely throughout the standard library, I figured it should be left as is and the warning ignored. I could suppress the warning instead.

You can mypy-ignore it I think. Otherwise, future modifications in that file would also be flagged, right?

All reactions

Sorry, something went wrong.

Uh oh!

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

picnixz
picnixz reviewed Jan 13, 2025
View reviewed changes
Comment thread Lib/_pyrepl/unix_console.py Resolved

Uh oh!

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

AA-Turner
AA-Turner reviewed Apr 1, 2025
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

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

Comment thread Lib/_pyrepl/unix_console.py Resolved

Uh oh!

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

AA-Turner
AA-Turner reviewed Apr 7, 2025
View reviewed changes
Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

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

@python-cla-bot

python-cla-bot Bot commented Apr 14, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

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

AA-Turner
AA-Turner reviewed May 1, 2025
View reviewed changes
Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

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

Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

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

Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

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

Comment thread Doc/using/cmdline.rst Resolved

Uh oh!

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

Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

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

@noahbkim

noahbkim commented May 1, 2025

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented May 1, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Thanks @noahbkim, no worries! Are you able to make the changes to config_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

All reactions

Sorry, something went wrong.

Uh oh!

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

@noahbkim

noahbkim commented May 1, 2025

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented May 1, 2025

Copy link
Copy Markdown
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

All reactions

Sorry, something went wrong.

Uh oh!

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

@noahbkim

noahbkim commented May 1, 2025

Copy link
Copy Markdown
Contributor Author

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

It looks like I caused some kind of regression in test_embed.py, but none of the expected invariants make sense to me (I'm not sure why we expect import_time to be set to 1 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?).

Zeroing import_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.

All reactions

Sorry, something went wrong.

Uh oh!

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

chris-eibl
chris-eibl reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/unix_console.py Resolved

Uh oh!

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

@noahbkim
Add -X importtime=2 for logging an importtime message for already-loa…
58d07be
…ded modules
chris-eibl
chris-eibl reviewed May 3, 2025
View reviewed changes
Comment thread Lib/_pyrepl/windows_console.py Outdated
import nt
return nt._supports_virtual_terminal()
except (ImportError, AttributeError):
nt._supports_virtual_terminal()

@chris-eibl chris-eibl May 3, 2025

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.

Suggested change
nt._supports_virtual_terminal()
return nt._supports_virtual_terminal()

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@AA-Turner

AA-Turner commented May 3, 2025

Copy link
Copy Markdown
Member

@noahbkim I've pushed a series of commits to the 3.14-importtime=2 branch on my fork. I've created a PR to HRT's fork (https://github.com/hudson-trading/cpython/pull/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     | builtins
    import time: self [us] | cumulative | imported package
    import time:       325 |        325 | winreg
    
  • Minor style changes to the pyrepl changes this PR introduces, and also the issue @chris-eibl noted.
  • Simplify initalisation for config.import_time & fix a bug where explicit setting of the member was overridden (see changes to test_embed).
  • Add some more test cases
  • Update documentation, including the man page & adding you to Misc/ACKS.

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

A

All reactions

Sorry, something went wrong.

Uh oh!

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

AA-Turner added 5 commits May 4, 2025 03:04
@AA-Turner
Print importtime header before first cached import
b081980
@AA-Turner
_pyrepl style changes
ee7a4a0
@AA-Turner
Simplify config->import_time initialisation
0226aa1
@AA-Turner
Update tests, add a few more cases
0b0c56d
@AA-Turner
Documentation
f9c07ce
@AA-Turner

AA-Turner commented May 5, 2025

Copy link
Copy Markdown
Member

ping @noahbkim (feature freeze is tomorrow)

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner AA-Turner mentioned this pull request May 5, 2025
gh-102567: Add -X importtime=2 for logging an importtime message for already-loaded modules #133443
Closed
@noahbkim
Merge pull request #1 from AA-Turner/3.14-importtime=2
6d50b0c
`-X importtime=2` feedback

(Thanks so much, @AA-Turner)
@noahbkim

noahbkim commented May 5, 2025

Copy link
Copy Markdown
Contributor Author

@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
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot

bedevere-bot commented May 5, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @AA-Turner for commit 6d50b0c 🤖

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@AA-Turner

AA-Turner commented May 6, 2025

Copy link
Copy Markdown
Member

Buildbots show leaks in test_datetime which seem unrelated.

All reactions

Sorry, something went wrong.

Uh oh!

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

AA-Turner
AA-Turner approved these changes May 6, 2025
View reviewed changes

@AA-Turner AA-Turner 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.

Thanks!

A

Sorry, something went wrong.

Uh oh!

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

All reactions
@hugovk hugovk mentioned this pull request Jul 7, 2025
gh-102567: Add missing newline to --help-all #136391
Merged
hugovk
hugovk reviewed Jul 7, 2025
View reviewed changes
Comment thread Python/initconfig.c
Comment on lines +315 to +316
-X importtime[=2]: show how long each import takes; use -X importtime=2 to\
log imports of already-loaded modules; also PYTHONPROFILEIMPORTTIME\n\

@hugovk hugovk Jul 7, 2025

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.

This is missing a newline:

Suggested change
-X importtime[=2]: show how long each import takes; use -X importtime=2 to\
log imports of already-loaded modules; also PYTHONPROFILEIMPORTTIME\n\
-X importtime[=2]: show how long each import takes; use -X importtime=2 to\n\
log imports of already-loaded modules; also PYTHONPROFILEIMPORTTIME\n\
$ python3.14 --help-all
...
-X frozen_modules=[on|off]: whether to use frozen modules; the default is "on"
         for installed Python and "off" for a local build;
         also PYTHON_FROZEN_MODULES
-X importtime[=2]: show how long each import takes; use -X importtime=2 to         log imports of already-loaded modules; also PYTHONPROFILEIMPORTTIME
-X int_max_str_digits=N: limit the size of int<->str conversions;
         0 disables the limit; also PYTHONINTMAXSTRDIGITS
-X no_debug_ranges: don't include extra location information in code objects;
         also PYTHONNODEBUGRANGES
-X perf: support the Linux "perf" profiler; also PYTHONPERFSUPPORT=1
...

Please see PR #136391 for a fix.

Sorry, something went wrong.

Uh oh!

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

All reactions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@hugovk hugovk hugovk left review comments
@picnixz picnixz picnixz left review comments
@chris-eibl chris-eibl chris-eibl left review comments
@methane methane methane approved these changes
@AA-Turner AA-Turner AA-Turner approved these changes
@Eclips4 Eclips4 Eclips4 approved these changes
@vstinner vstinner Awaiting requested review from vstinner
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka
@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner
@ambv ambv Awaiting requested review from ambv ambv is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

11 participants

@noahbkim @Eclips4 @picnixz @AA-Turner @bedevere-bot @ambv @methane @JelleZijlstra @hugovk @serhiy-storchaka @chris-eibl
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 01402520 — 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.