Skip to content
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

TypeError: _eval_type() missing 1 required positional argument: 'type_params' with import module #118418

Closed
enefry opened this issue Apr 30, 2024 · 18 comments · Fixed by #118695
Closed
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes topic-typing type-bug An unexpected behavior, bug, or error

Comments

@enefry
Copy link

enefry commented Apr 30, 2024

Bug report

What happened?

import openai
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.12/site-packages/openai/__init__.py", line 8, in <module>
    from . import types
  File "/usr/local/lib/python3.12/site-packages/openai/types/__init__.py", line 5, in <module>
    from .batch import Batch as Batch
  File "/usr/local/lib/python3.12/site-packages/openai/types/batch.py", line 7, in <module>
    from .._models import BaseModel
  File "/usr/local/lib/python3.12/site-packages/openai/_models.py", line 35, in <module>
    from ._utils import (
  File "/usr/local/lib/python3.12/site-packages/openai/_utils/__init__.py", line 3, in <module>
    from ._utils import (
  File "/usr/local/lib/python3.12/site-packages/openai/_utils/_utils.py", line 24, in <module>
    from .._compat import parse_date as parse_date, parse_datetime as parse_datetime
  File "/usr/local/lib/python3.12/site-packages/openai/_compat.py", line 48, in <module>
    from pydantic.v1.typing import (
  File "/usr/local/lib/python3.12/site-packages/pydantic/v1/__init__.py", line 7, in <module>
    from .env_settings import BaseSettings
  File "/usr/local/lib/python3.12/site-packages/pydantic/v1/env_settings.py", line 23, in <module>
    class BaseSettings(BaseModel):
  File "/usr/local/lib/python3.12/site-packages/pydantic/v1/main.py", line 178, in __new__
    annotations = resolve_annotations(namespace.get('__annotations__', {}), namespace.get('__module__', None))
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/pydantic/v1/typing.py", line 400, in resolve_annotations
    value = _eval_type(value, base_globals, None)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
import fastapi
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.12/site-packages/fastapi/__init__.py", line 7, in <module>
    from .applications import FastAPI as FastAPI
  File "/usr/local/lib/python3.12/site-packages/fastapi/applications.py", line 16, in <module>
    from fastapi import routing
  File "/usr/local/lib/python3.12/site-packages/fastapi/routing.py", line 22, in <module>
    from fastapi import params
  File "/usr/local/lib/python3.12/site-packages/fastapi/params.py", line 5, in <module>
    from fastapi.openapi.models import Example
  File "/usr/local/lib/python3.12/site-packages/fastapi/openapi/models.py", line 68, in <module>
    class Contact(BaseModelWithConfig):
  File "/usr/local/lib/python3.12/site-packages/pydantic/_internal/_model_construction.py", line 197, in __new__
    set_model_fields(cls, bases, config_wrapper, types_namespace)
  File "/usr/local/lib/python3.12/site-packages/pydantic/_internal/_model_construction.py", line 474, in set_model_fields
    fields, class_vars = collect_model_fields(cls, bases, config_wrapper, types_namespace, typevars_map=typevars_map)
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/pydantic/_internal/_fields.py", line 131, in collect_model_fields
    type_hints = get_cls_type_hints_lenient(cls, types_namespace)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/pydantic/_internal/_typing_extra.py", line 226, in get_cls_type_hints_lenient
    hints[name] = eval_type_lenient(value, globalns, localns)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/pydantic/_internal/_typing_extra.py", line 238, in eval_type_lenient
    return eval_type_backport(value, globalns, localns)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/pydantic/_internal/_typing_extra.py", line 255, in eval_type_backport
    return typing._eval_type(value, globalns, localns)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: _eval_type() missing 1 required positional argument: 'type_params'

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

@enefry enefry added the type-crash A hard crash of the interpreter, possibly with a core dump label Apr 30, 2024
@sobolevn
Copy link
Member

sobolevn commented Apr 30, 2024

Well, this has several points.

  1. This is a private API. It can change at any point and we don't provide any guarantees about it.
  2. But, people are clearly using this private API in rather big projects.
  3. 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.
  4. But, there are still versions of 3.12 available that won't have this default. So, you would need a compat layer anyway.
  5. 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

This is hard :)

sobolevn added a commit to sobolevn/cpython that referenced this issue Apr 30, 2024
@sobolevn
Copy link
Member

See my sobolevn@d7483de commit that adds None default. I am not sure that we should go for it.

@sobolevn sobolevn added type-bug An unexpected behavior, bug, or error topic-typing 3.12 bugs and security fixes 3.13 bugs and security fixes and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Apr 30, 2024
@enefry
Copy link
Author

enefry commented Apr 30, 2024

🤔
Yes, I think default value will more better for exists code

Well, this has several points.

  1. This is a private API. It can change at any point and we don't provide any guarantees about it.
  2. But, people are clearly using this private API in rather big projects.
  3. 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.
  4. But, there are still versions of 3.12 available that won't have this default. So, you would need a compat layer anyway.
  5. 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

This is hard :)

@enefry enefry closed this as completed Apr 30, 2024
@sobolevn sobolevn reopened this Apr 30, 2024
@sobolevn
Copy link
Member

You closed the issue too early :)

@AlexWaygood
Copy link
Member

@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? :-)

@enefry
Copy link
Author

enefry commented Apr 30, 2024

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? :-)

@AlexWaygood
Copy link
Member

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.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 30, 2024

To summarise, then:

A change to typing.py authored by me a couple of weeks ago (1e3e7ce), 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 #114053 (comment)). 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 (sobolevn@d7483de). 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.

@sobolevn
Copy link
Member

We can do that if we have to, but I don't much like the idea.

Yes, this is what I said in #118418 (comment)

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 :)

@JelleZijlstra
Copy link
Member

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.

@AlexWaygood
Copy link
Member

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 1e3e7ce). 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.

@sobolevn
Copy link
Member

sobolevn commented Apr 30, 2024

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.

@AlexWaygood
Copy link
Member

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 1e3e7ce. 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 #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.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 30, 2024
…al_type` (pythonGH-118431)

(cherry picked from commit 4a5ad84)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@terryjreedy
Copy link
Member

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.

AlexWaygood pushed a commit that referenced this issue Apr 30, 2024
…val_type` (GH-118431) (#118436)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@AlexWaygood
Copy link
Member

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.

@sobolevn
Copy link
Member

sobolevn commented Apr 30, 2024

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?

@AlexWaygood
Copy link
Member

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 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.

@samuelcolvin
Copy link
Contributor

Hi everyone, sorry I somehow missed the notification for this one.

@AlexWaygood thanks so much for accommodating my egregious use of a private API.

We'll get Pydantic working with 3.12.4 and 3.13 ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
6 participants