GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-161513: Resolve _wmi lazily in platform by xixi-box · Pull Request #161543 · python/cpython Conversation
|
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@xixi-box thanks!
You still can use lazy import _wmi, just catch the exception with something like:
try:
_wmi
except ImportError:
_wmi = None
could you look into this suggested by @sobolevn (looks more resonable if i'm not wrong)
one small suggetsion on top of that if maitainers think its worth, gating the import one platform
if sys.platform == "win32":
lazy import _wmi
else:
_wmi = None
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
also , please sign cla, and it would be great to add small test for this |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
22a4c30 to
81c0bbe
Compare
|
Thanks for the review! Done in the latest commit (81c0bbe):
Regarding CLA: it was already signed before your comment — the CLA check is green. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
are you sure 🤔 looks agent |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Adopt the 'lazy import _wmi' form suggested by sobolevn. A failed lazy import is retried on every access, so _wmi_query() binds _wmi = None on ImportError to record that it is unavailable. A failed WMI query still sets _wmi = False so it is not retried.
81c0bbe to
4128b12
Compare
|
You were right to double check — my bad. The CLA was green on the original commit, but when I force-pushed the update I accidentally re-authored the commit with my personal email, which isn't the one linked to the signed CLA. Re-authored it with my GitHub noreply address and the check is green again on 4128b12. Thanks for catching it! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
- Revert the OSError branch to _wmi = None: with lazy imports None no longer means 'not imported yet', so the False sentinel is unneeded. - Rewrite the NEWS entry to describe the user-facing perf effect. - Use ensure_lazy_imports() to test that importing platform does not import _wmi.
|
Fair point @Aniketsy — I do use AI tooling to help with this contribution (as disclosed in gh-161513), but everything is built and tested locally before it lands here, and the CLA slip above was on me for not re-checking the checks after force-pushing. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
- Run the lazy import test on all platforms under @support.cpython_only. - Tighten the comments around the deferred import. - State the measured import speedup in the NEWS entry.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Raise OSError directly on ImportError and do not cache query failures: the lazy import placeholder keeps the unavailable state.
| :mod:`platform` is now about 30% faster to import on Windows: the ``_wmi`` | ||
| extension module is loaded on first use instead of at import time. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| :mod:`platform` is now about 30% faster to import on Windows: the ``_wmi`` | |
| extension module is loaded on first use instead of at import time. | |
| :mod:`platform` is now about 30% faster to import on Windows. |
The rest is an implementation detail.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Applied — trimmed the NEWS entry to the user-facing performance statement.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Signed-off-by: wangshun <[email protected]>
|
@xixi-box are you an autonomous agent? I see you have https://github.com/xixi-box/oss-scout which says:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@StanFromIreland i feels the same, as if there is suggested changes, why not just apply |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for asking directly, @StanFromIreland — happy to be transparent. I'm wangshun, a human contributor. I work on these with AI assistance (disclosed in gh-161513), and the oss-scout tooling you found helps me track CI and review activity on my own PRs — but every commit and reply goes through me, and I verify things locally before pushing. As for the suggestion, @Aniketsy is right to nudge on it: I'm in mainland China (UTC+8), so the review landed late in my evening and the question then sat unanswered while I was away from the keyboard — that's on me. The NEWS entry was already trimmed to the suggested wording in 1110bca; I should have replied here sooner and will keep a closer eye on inline suggestions going forward. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Please reply in your own words, an AI can't write your comments for you. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This comment is written by myself. I am a college student from China who enjoys contributing to the open source community. Some of my coding work inevitably involves the use of AI, but I understand that every change has been made and tested locally. If this has caused any inconvenience to you, I apologize |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Nice, thanks for picking this up. One edge case I ran into while testing the same change on main: If an application installs a lazy imports filter that rejects this import, e.g. The guard @Aniketsy suggested above avoids it: if sys.platform == "win32": lazy import _wmi else: _wmi = None I checked both on a main build (cbaba02), simulating the non-Windows case: the current version raises at |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@azchohfi can you please create a new PR? We already had 45+ comments trying to fix the AI's output, but it clearly does not make any sense anymore :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
platform: import _wmi on first use
There was an error while loading. Please reload this page.