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

Add a test demonstrating initvar works with inheritance #5859

Merged
merged 9 commits into from
May 25, 2023

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented May 24, 2023

Closes #5496.

It seems that at some point since the 2.0a4 release, this got fixed, but I've added a test written from the code in the issue. (This test fails in 2.0a4.)

This got fixed for python 3.11 after #5760, but not for other python versions..

Selected Reviewer: @adriangb

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 24, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a1615bf
Status: ✅  Deploy successful!
Preview URL: https://ac9f893b.pydantic-docs2.pages.dev
Branch Preview URL: https://initvar-inheritance.pydantic-docs2.pages.dev

View logs

@dmontagu
Copy link
Contributor Author

Okay nevermind, it's just resolved in 3.11 :(

For what it's worth, it started working in 3.11 after #5760. I'll keep working through this.

@@ -211,3 +211,8 @@ def create_dataclass(cls: type[Any]) -> type[PydanticDataclass]:


__getattr__ = getattr_migration(__name__)

if (3, 8) <= sys.version_info < (3, 11):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, dataclasses.InitVar[<type>] raises errors when returned by typing.get_type_hints:

>           raise TypeError(f"{msg} Got {arg!r:.100}.")
E           TypeError: Forward references must evaluate to types. Got dataclasses.InitVar[int].

The issue is, even if you were willing to rely on dataclasses to tell you what is or isn't an InitVar, you really need this function (or something similar) to allow you to resolve forward references to get what the generic parameter resolves to.

I think monkeypatching InitVar like this is preferable to copying the whole logic of get_type_hints with a tiny tweak to not error if the result isn't callable. But I suppose I could be convinced away from this if we think it's really problematic.

@dmontagu
Copy link
Contributor Author

please review

if (3, 8) <= sys.version_info < (3, 11):
# Monkeypatch dataclasses.InitVar so that typing doesn't error if it occurs as a type when evaluating type hints
# This is not necessary in 3.11, and we don't support InitVar in python < 3.8.
dataclasses.InitVar.__call__ = lambda: None # type: ignore
Copy link
Member

@adriangb adriangb May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this gets used before pydantic gets imported? I guess it just doesn't work right?

Copy link
Contributor Author

@dmontagu dmontagu May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would error if you tried to use typing.get_type_hints first, but currently I don't think there is a way to get to the point of building a dataclass schema without importing something from pydantic.dataclasses, at least the following works:


from pydantic import BaseModel

initvars = []
@dataclasses.dataclass
class A:
    x: 'dataclasses.InitVar[int]'
    def __post_init__(self, x):
        initvars.append(x)

class B(BaseModel):
    a: A

B.model_validate({'a': {'x': 2}})

assert initvars == [2]

B.model_validate({'a': {'x': 'abc'}})  # errors

Do you think we need to be more careful about ensuring the monkeypatch happens no matter what import path is followed? We could do it in pydantic.__init__ or similar, that would be a little more bulletproof I guess.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean what about:

from dataclasses import dataclass, InitVar

@dataclass
class A:
    x: InitVar[int]  # calls `InitVar.__call__`?

# maybe in another module
from pydantic import BaseModel

class B(BaseModel):
    a: A

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InitVar.__call__ doesn't actually get called — the typing.get_type_hints function errors if it doesn't think the annotations are actually types. It doesn't think InitVar[int] is a type pre-3.11 I guess, but one thing you can do to get it to make it think something is a type is to make that thing callable (it has a boolean check if callable(maybe_type): in which case it lets you use that annotation and not error). That's why this monkeypatch works — as long as get_type_hints doesn't get called on the type until after the monkeypatch is applied (which I think is the case because that doesn't happen until you are building the schema).

@adriangb
Copy link
Member

Please go ahead and merge

@dmontagu dmontagu enabled auto-merge (squash) May 25, 2023 20:48
@dmontagu dmontagu merged commit 82e28c6 into main May 25, 2023
50 checks passed
@dmontagu dmontagu deleted the initvar-inheritance branch May 25, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V2 pydantic dataclass: InitVar and inheritance don't work together
2 participants