Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-118761: Optimise import time for ast#131953
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
AA-Turner commented Mar 31, 2025 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
eli-schwartz commented Apr 1, 2025
And with this PR we are back down to 671 lines. The massive jump is from when unparse was added. This feels nice all on its own, to avoid so much code that is often not needed. |
JelleZijlstra left a comment
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.
Thank you!
A few thoughts:
- An alternative approach could be to make
asta package, with the unparse code inast/_unparse.pyand the rest of the functionality inast/__init__.py. Pro: Doesn't need a new top-level module; avoids the (probably theoretical) compatibility concern that someone could have their own top-level_ast_unparsemodule that interferes with the stdlib one. Con: Makes the directory structure more complex. - I'm not sure we need to keep getattr support for the private attributes, like the Precedence enum and the various constants. I'd do that only if there's evidence they are being used in third-party code, especially widely used packages. Otherwise we just make our lives harder if we want to refactor the unparse code in the future.
This reverts commit 460491e.
AA-Turner commented Apr 1, 2025
Reverted.
I'd suggest doing that in a follow-up PR, if desirable. I think for now this is the simplest approach. If it provides any reassurance, GitHub has no results for the |
f20f02e into python:mainUh oh!
There was an error while loading. Please reload this page.
vstinner commented Apr 2, 2025
Oh, I was reviewing the change while it has been merged :-) Well, it LGTM. I had minor suggestions, but they don't matter anymore. Nice optimization. |
AA-Turner commented Apr 2, 2025
Sorry! Happy to open a follow-up PR for any of your suggestions? |
vstinner commented Apr 2, 2025
Benchmark: 13.1 ms => 18.8 ms (-5.7 ms). Before: After: |
vstinner commented Apr 2, 2025
Well, I don't know if it would better, but an alternative to adding defunparse(ast_obj): import_ast_unparsereturn_ast_unparse.unparse(ast_obj) |
danielhollas commented Apr 2, 2025
The |
danielhollas commented Apr 2, 2025
Hmm, unless I am missing something, it seems that with this change ❯ pythonPython3.12.7 (main, Oct12024, 00:00:00) [GCC13.3.120240913 (RedHat13.3.1-3)] onlinuxType"help", "copyright", "credits"or"license"formoreinformation. >>>fromastimport*>>>unparse<functionunparseat0x7f7f8ebc9c60> ❯ ./pythonPython3.14.0a6+ (heads/main:f20f02e6b58, Apr22025, 17:41:39) [GCC13.3.120240913 (RedHat13.3.1-3)] onlinuxType"help", "copyright", "credits"or"license"formoreinformation. >>>fromastimport*>>>unparseTraceback (mostrecentcalllast): File"<python-input-1>", line1, in<module>unparseNameError: name'unparse'isnotdefined. Didyoumean: 'parse'? |
AA-Turner commented Apr 2, 2025
I think (untested) that the |
AA-Turner commented Apr 2, 2025
I've opened #132024 for follow-ups. |
JelleZijlstra commented Apr 2, 2025
Alternative followup in #132025. |
Attempting @JelleZijlstra's suggestion in #118761 (comment).
Running
python -Ximporttime -Sc "import ast"ten times with the current and proposed modules, I get 14.1ms for the current module and 1.48ms for the new, for a ~12ms speed up. I've left numbers out of the NEWS entry for now, as I've only tested on a Windows PC -- Linux may have different performance profiles.Full Times
(using the last line of each output)
Current
Proposed