gh-146553: Fix infinite loop in typing.get_type_hints() on circular __wrapped__#146554
gh-146553: Fix infinite loop in typing.get_type_hints() on circular __wrapped__#146554raminfp wants to merge 4 commits intopython:mainfrom
Conversation
brianschubert
left a comment
brianschubert
left a comment
There was a problem hiding this comment.
I wonder if it would make sense to lazily import inspect here instead of having two implementations of essentially the same thing? inspect is a heavy import, but if you're calling get_type_hints you're already doing some sort of runtime signature introspection, in which case the import cost may be less of a concern.
@raminfp Please be mindful of https://devguide.python.org/getting-started/generative-ai/. I see you have been warned about relying on LLM generated PRs in the past. If you do decide to use LLMs, please make sure you take the time to scrutinize the output and compare it with the existing code before submitting a PR. In particular, you should try to match the style of the existing code (e.g. don't use _private_names for local variables).
|
@brianschubert Thank you for the review and for pointing this out. I do use LLM assistance when working on contributions (testing, security check, design and etc), but I wasn't sufficiently familiar with the relevant |
Lib/typing.py
Outdated
| import inspect | ||
| nsobj = inspect.unwrap(obj) |
There was a problem hiding this comment.
Everytime I see local imports for inspect, I feel that we need to have a lower-level inspect utililty that can have less functions but more common to avoid having to import inspect.
What I can suggest for this specific PR is to use lazy import inspect instead at the top-level to make it a bit cleaner. For backports, we'll however need to use a local import. Note that we have other places with a local import to inspect, which you can eventually remove since we would have both a lazy and a local import (which would be redundant). I'll let @JelleZijlstra decide whether to want lazy imports.
If we switch to lazy imports, we'll also need to update the lazy import tests somewhere to check that inspect is lazily imported.
There was a problem hiding this comment.
Maybe inspect.unwrap should live in functools instead, it's closely related to functools.wraps after all. But it's probably too late for that. Lazy importing sounds good to me.
…rt in get_type_hints()
Lib/typing.py
Outdated
| @functools.cache | ||
| def _lazy_load_unwrap(): | ||
| # Import unwrap lazily so as not to slow down the import of typing.py | ||
| # Cache the result so we don't slow down get_type_hints() unnecessarily | ||
| from inspect import unwrap | ||
| return unwrap | ||
|
|
||
|
|
||
| _cleanups.append(_lazy_load_unwrap.cache_clear) |
There was a problem hiding this comment.
I think we can just use lazy import inspect now and get rid of those lazy load stuff?
There was a problem hiding this comment.
There was some controversy about using PEP 810 too widely in the stdlib https://discuss.python.org/t/concerns-about-x-lazy-imports-none/106203
I don't have a strong opinion myself, to be clear
There was a problem hiding this comment.
@picnixz Done, replaced both _lazy_load_getattr_static and _lazy_load_unwrap with a _LazyInspect class following the existing _LazyAnnotationLib pattern already in the file.
There was a problem hiding this comment.
There was some controversy about using PEP 810 too widely in the stdlib
Jelle said he was fine so I think we can do the switch unless it's breaking something. @raminfp please check that the concerns expressed in the DPO thread are not concerns, but do so without using an LLM.
…azy inspect import
| class _LazyInspect: | ||
| def __getattr__(self, attr): | ||
| global _lazy_inspect | ||
| import inspect | ||
| _lazy_inspect = inspect | ||
| return getattr(inspect, attr) | ||
|
|
||
| _lazy_inspect = _LazyInspect() |
There was a problem hiding this comment.
Why do we have this construction? Please don't use an LLM to generate this. Just use lazy import inspect and check if there is nothing wrong with that construction as per the DPO thread.
There was a problem hiding this comment.
For annotationlib, we'll clean it up either in a follow-up if codeowners do want it, or we will keep it as is until we need to modify annotationlib calls somewhere.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
The intended fix (using inspect.unwrap()) is clear, but after several rounds of review I'm still uncertain about the exact lazy import pattern expected. The guidance "just use lazy import inspect" wasn't concrete enough for me to implement confidently, especially given the concerns raised about PEP 810 in the linked DPO thread. |
|
This is really like an LLM auto-reply. @raminfp The issues you report are worth my time as they are crash-related, but if you use an LLM to write your PRs without interacting with reviewers, it wastes both our time. In the future, please do notlet agents running in the background. Human interaction is necessary in a PR and if you do not want to follow those guidelines, I will not review your PRs anymore (and I have already warned you about this in the past). |
5 participants
#146553
typing.get_type_hints()via Circular__wrapped__#146553