Closed enefry closed 2 weeks ago
Well, this has several points.
type_params=None
default in _eval_type
, it won't change the code semantics. But, it will help multiple projects._eval_type
without type_params
(or with proposed None
default) may just hide the bug (when working with PEP695's functions / classes) instead of being correct in all casesThis is hard :)
See my https://github.com/sobolevn/cpython/commit/d7483de262e53059671946b147c1fe62986582c7 commit that adds None
default. I am not sure that we should go for it.
🤔 Yes, I think default value will more better for exists code
Well, this has several points.
- This is a private API. It can change at any point and we don't provide any guarantees about it.
- But, people are clearly using this private API in rather big projects.
- And we can help them by simply providing
type_params=None
default in_eval_type
, it won't change the code semantics. But, it will help multiple projects.- But, there are still versions of 3.12 available that won't have this default. So, you would need a compat layer anyway.
- And using
_eval_type
withouttype_params
(or with proposedNone
default) may just hide the bug (when working with PEP695's functions / classes) instead of being correct in all casesThis is hard :)
You closed the issue too early :)
@enefry, I can't reproduce this issue based on the snippet you gave above:
(3123-env) % python -m pip freeze
annotated-types==0.6.0
anyio==4.3.0
certifi==2024.2.2
distro==1.9.0
fastapi==0.110.3
h11==0.14.0
httpcore==1.0.5
httpx==0.27.0
idna==3.7
openai==1.24.0
pydantic==2.7.1
pydantic_core==2.18.2
sniffio==1.3.1
starlette==0.37.2
tqdm==4.66.2
typing_extensions==4.11.0
(3123-env) % python --version
Python 3.12.3
(3123-env) % python
Python 3.12.3 (main, Apr 30 2024, 10:12:02) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import openai
>>> import fastapi
>>> exit()
Could you post a minimal, reproducible example please? :-)
you may checkout commit before this commit See my https://github.com/sobolevn/cpython/commit/d7483de262e53059671946b147c1fe62986582c7 commit that adds None default. I am not sure that we should go for it.
my library version is : fastapi>=0.110.1 openai>=1.14.0
@enefry, I can't reproduce this issue based on the snippet you gave above:
(3123-env) % python -m pip freeze annotated-types==0.6.0 anyio==4.3.0 certifi==2024.2.2 distro==1.9.0 fastapi==0.110.3 h11==0.14.0 httpcore==1.0.5 httpx==0.27.0 idna==3.7 openai==1.24.0 pydantic==2.7.1 pydantic_core==2.18.2 sniffio==1.3.1 starlette==0.37.2 tqdm==4.66.2 typing_extensions==4.11.0 (3123-env) % python --version Python 3.12.3 (3123-env) % python Python 3.12.3 (main, Apr 30 2024, 10:12:02) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import openai >>> import fastapi >>> exit()
Could you post a minimal, reproducible example please? :-)
Thanks @enefry, I can reproduce with a fresh build of the tip of the 3.12 branch. Apologies -- I assumed you were experiencing the bug with a released version of Python 3.12.
To summarise, then:
A change to typing.py
authored by me a couple of weeks ago (https://github.com/python/cpython/commit/1e3e7ce11e3b0fc76e981db85d27019d6d210bbc), which I backported to 3.12, is going to break pydantic when the change is included as part of Python 3.12.4. According to PEP-693, Python 3.12.4 will be released on 4th June.
The change in question fixed a bug where NameError
would be raised if you called get_type_hints()
on a class that used PEP-695 type parameters in a module that had from __future__ import annotations
at the top of the file (see https://github.com/python/cpython/issues/114053#issue-2080696512). I assumed it was safe to backport the change, since it only changed the signature of private, undocumented functions which I would consider implementation details. But apparently pydantic is using typing._eval_type()
, and will be broken by the change.
I want to say that "this is what happens if you make use of undocumented implementation details" and close the issue as "not a bug". But obviously pydantic is a very popular framework, and it's not great that it would be broken by a patch release of CPython. Breaking pydantic means that other very popular libraries such as openai
and fastapi
also break, which nobody wants.
@sobolevn has floated adding a default value for the new parameter for _eval_type
(https://github.com/sobolevn/cpython/commit/d7483de262e53059671946b147c1fe62986582c7). We can do that if we have to, but I don't much like the idea. There's no principled reason for the parameter to have a default value -- it would be too easy to forget to pass a value to the parameter, which would lead to incorrect behaviour when calling the function on a PEP-563 stringified annotation that references a PEP-695 type parameter.
Cc. @samuelcolvin for awareness.
We can do that if we have to, but I don't much like the idea.
Yes, this is what I said in https://github.com/python/cpython/issues/118418#issuecomment-2084500304
And using _eval_type without type_params (or with proposed None default) may just hide the bug (when working with PEP695's functions / classes) instead of being correct in all cases
Maybe there's a better way :)
This bug is especially bad because it triggers at import time, meaning that affected libraries cannot be used at all in combination with affected versions of CPython.
I vote for adding the default on 3.12. We should avoid breaking half the ecosystem in a bugfix release.
For 3.13 maybe the best solution would be to expose a new public function for whatever Pydantic needs _eval_type
for. However, it's a bit late for that, and in the interest of stability I'd prefer to also add the default in 3.13.
Let's say that we add a default value for the parameter. I'd assume that pydantic will also get incorrect behaviour that could result in a NameError
if they call typing._eval_type()
without passing a value for that parameter on a class that uses PEP-695 type params in a module that has from __future__ import annotations
(i.e., pydantic would get the behaviour get_type_hints
had prior to https://github.com/python/cpython/commit/1e3e7ce11e3b0fc76e981db85d27019d6d210bbc). I'd like to give the pydantic maintainers a little bit of time to confirm that that is really what they want before we make the change. Since 3.12.4 won't be released until June, we have some time here.
I guess this would be the compat code to workaround this problem:
if sys.version_info < (3, 12, 5): # or whatever version it would be
# Python has a bug in its `_eval_type` function.
locals_namespace = {} # whatever they are
if sys.version_info >= (3, 12):
# Python since 3.12.0 and until 3.12.4 also has a bug with NameError
# for types using PEP695 type params:
locals_namespace.update({
param.__name__: param
for param in cls_or_func.__type_params__
})
result = typing._eval_type(type_, globals_namespace, locals_namespace, recursive_guard=...)
else:
result = typing._eval_type(
type_, globals_namespace, locals_namespace,
cls_or_func.__type_params__, # since 3.12.5 `_eval_type` has a correct way to do it
recursive_guard=...,
)
PR is on its way.
Okay, if it was "just" pydantic that was going to be broken with this, then I would have argued that they'd have plenty of time to patch their code before the release of Python 3.12.4, and that adding a default value for this parameter would just mean that pydantic would have the same bug in their use of _eval_type
that we had prior to https://github.com/python/cpython/commit/1e3e7ce11e3b0fc76e981db85d27019d6d210bbc. But it's not just pydantic. Unfortunately, typing._eval_type
has many users outside the stdlib, much though I wish it didn't:
We can't afford to break this many libraries in a patch release, so I now agree that we have no choice except to add a default value to the new parameter. After https://github.com/python/cpython/pull/118431 is merged, however, I would like to propose that we immediately make a Python 3.13-only change to start issuing a DeprecationWarning
if None
is passed to this parameter. Failing to pass a value to this parameter shouldn't be encouraged: it's going to lead to incorrect behaviour if _eval_type
is called on a stringified annotation that references a PEP-695 type parameter.
Alternate suggestion: in 3.12.4 and successors, default to a private sentinel so as to detect defaulting versus explicitly passing None. If not overriden, convert it to None and issue a deprecation warning. Keep the proper signature in 3.13. There will be months for users of the private function to adjust before the latter's release. After .0b1, we can discuss making the function public, or making some other change.
Alternate suggestion: in 3.12.4 and successors, default to a private sentinel so as to detect defaulting versus explicitly passing None. If not overriden, convert it to None and issue a deprecation warning.
Unfortunately, lots of users run tests with -Werror
in CI, so I think adding a deprecation warning in a patch release could be just as breaking as adding a new parameter without a default. So I think the earliest we can add this warning is Python 3.13.
Possibly we could consider being aggressive with the deprecation period, but I think if we've conceded that this change could break quite a few libraries (even if it shouldn't), we may as well go through the standard two-cycle deprecation period for removing the default.
I propose not to touch _eval_type
anymore. Let's just provide a new public API (since this one is already public de facto). And just ship the backport in typing_extensions
?
I propose not to touch
_eval_type
anymore. Let's just provide a new public API (since this one is already public de facto). And just ship the backport intyping_extensions
?
I don't think that's sufficient: there are already many libraries using the private API, and even if we provide a public API for them in 3.14, it will take time for them to migrate to the new public API. In the meantime, the footgun will continue to exist in the private API that they're already using.
I've put up #118695 to deprecate failing to pass a value to the new parameter. It's a pretty simple change and I think we should go ahead with it.
Bug report
What happened?
CPython versions tested on:
3.12
Operating systems tested on:
Linux
Output from running 'python -VV' on the command line:
Python 3.12.3+ (heads/3.12-dirty:817190c, Apr 29 2024, 12:17:20) [GCC 11.4.0]
Linked PRs