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: Improve import time of annotationlib#132028
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
DavidCEllis commented Apr 2, 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.
AA-Turner commented Apr 2, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@DavidCEllis what do you think of AA-Turner@opt-annotationlib? Instead of the lazy object, it uses self-replacing functions, so that the 'lazy' cost is only paid once. I think for def_Stringifier(*args, **kwds): # This function replaces itself with the real class when first calledglobal_Stringifierfromannotationlib._stringifierimportStringifieras_Stringifierreturn_Stringifier(*args, **kwds)A |
annotationlib by deferring imports of ast and functoolsannotationlibDavidCEllis commented Apr 2, 2025 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I don't mind the self replacing functions if that's a more recognisable pattern. I personally prefer the class as there's no observable placeholder object that needs to be replaced. Inspecting will only give you the actual function/module and you can't make a reference to the self-replacing function that doesn't get replaced. [Edit: I also like that it puts information about all of the imports at the top of the module, so you can see that the module may import The lazy object also only pays the cost once by assigning to the object after the first import - it's basically a by-hand written version of an instance of my general lazy importer module. |
AA-Turner commented Apr 2, 2025
Benchmarking again with the recent changes to I'll leave it up to @JelleZijlstra to decide which he prefers. Current HEADThis PR ('lazy')Self replacing functionsUse a local import for |
JelleZijlstra commented Apr 3, 2025
I'm not sure this is worth it any more:
|
DavidCEllis commented Apr 3, 2025
Probably fair on the scale of things, although perhaps you could skip the Lines 808 to 814 in d30052a
I'll note that needing |
JelleZijlstra commented Apr 4, 2025
Good idea regarding functools, I just did that myself in #132059 since the change is so simple. I feel deferring the However, I do think it's realistic to defer |
DavidCEllis commented Apr 4, 2025
With the STRING format you do need With Example with this PR: importsysfromannotationlibimportget_annotations, FormatclassNoForwardRef: a: intclassYesForwardRef: a: Anyprint(get_annotations(NoForwardRef, format=Format.FORWARDREF)) print("ast"insys.modules) print(get_annotations(YesForwardRef, format=Format.FORWARDREF)) print("ast"insys.modules){'a':<class'int'>} False{'a': ForwardRef('Any')} TrueI think it's likely many tools that currently get annotations directly will need to switch to using Still arguable if it goes too far with the improvement to |
DavidCEllis commented Apr 11, 2025
I think this can be closed. The The module could still be broken up into a package, but I feel that about a fair chunk of the larger slower stdlib modules and doing what this branch does feels more 'hacky' than structured at this point. |
This PR converts
annotationlib.pyinto a package withannotationlib/__init__.pyandannotationlib/_stringifier.py.Discussed here: https://discuss.python.org/t/pep-749-implementing-pep-649/54974/63
This is done in order to move the definition of
_Stringifierinto a new submodule in order to defer the import ofastin the main module.The outcome of this is that
astshould only be imported if any of the following occur:get_annotations(obj, format=Format.STRING)1 is used and the output is not an empty dictget_annotations(obj, format=Format.FORWARDREF)is used and there are actually forward referencesforwardref.__forward_arg__is called andforwardref.__ast_node__ is not NoneNote: I've used a class with a
__getattr__method as a way of deferring imports in the current PR but I'd be happy to change that to something else if there's a more standard pattern.My machine isn't a super stable benchmarking machine so take these only as rough estimates (they're slightly different to those posted in the discuss thread as it's a different run).
This branch:
Main:
Footnotes
or any of the similar methods such as
call_annotate_function↩