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

Incorrect ForwardRef class lookup in inherited models #6742

Closed
1 task done
Tishka17 opened this issue Jul 18, 2023 · 8 comments · Fixed by #6823 or #6838
Closed
1 task done

Incorrect ForwardRef class lookup in inherited models #6742

Tishka17 opened this issue Jul 18, 2023 · 8 comments · Fixed by #6823 or #6838
Assignees
Labels
bug V2 Bug related to Pydantic V2

Comments

@Tishka17
Copy link

Tishka17 commented Jul 18, 2023

Initial Checks

  • I confirm that I'm using Pydantic V2

Description

Let's suppose parent class has fields with ForwardRef and calls model_rebuild. It correctly build.
Let's imagine we have child class in different module. In this case for that class pydantic tries to lookup types of parent fields again, but uses current scope instead of scope of parent class.

In all files below it is expected that Child.a will have the type fw_parent.A as it is declared. But it isn't

When running main.py the error is:

pydantic.errors.PydanticUserError: `Child` is not fully defined; you should define `A`, then call `Child.model_rebuild()`.

It looks like pydantic tries to rebuild parent fields while it has info about them in parent class.

When running main2.py the error is:

pydantic.errors.PydanticSchemaGenerationError: Unable to generate pydantic-core schema for <class '__main__.A'>. Set `arbitrary_types_allowed=True` in the model_config to ignore this error or implement `__get_pydantic_core_schema__` on your type to fully support it.

It looks like pydantic tries to use wrong A class from wrong file.

Example Code

# fw_parent.py
from typing import Optional

from pydantic import BaseModel


class Model(BaseModel):
    a: Optional["A"]


class A(BaseModel):
    pass


Model.model_rebuild()

# main.py
from fw_parent import Model

class Child(Model):
    pass


Child(a=None)


# main2.py
from fw_parent import Model

class A:
    pass

class Child(Model):
    pass


Child(a=None)

Python, Pydantic & OS Version

pydantic version: 2.0.3
        pydantic-core version: 2.3.0 release build profile
                 install path: /home/tishka17/src/tmp/venv/lib/python3.10/site-packages/pydantic
               python version: 3.10.6 (main, May 29 2023, 11:10:38) [GCC 11.3.0]
                     platform: Linux-5.19.0-46-generic-x86_64-with-glibc2.35
     optional deps. installed: ['email-validator', 'typing-extensions']

Selected Assignee: @dmontagu

@JrooTJunior
Copy link

@samuelcolvin, any news on this issue?

@hramezani
Copy link
Member

@JrooTJunior we are working on this issue and trying to provide a fix as soon as possible

@samuelcolvin
Copy link
Member

This is expected since class Child(Model): looks in the current module namespace for types.

Please see #6823 which proposes a fix solution for this - basically I've added a defer_model_build to config which means you can avoid model construction on the class Child(Model): line, after that you can call Child.model_construction(_types_namespace=fw_parent.__dict__).

Please let me know asap if you're not happy with #6823, otherwise I'll try to get it included in the next release.

@Tishka17
Copy link
Author

Tishka17 commented Jul 24, 2023

This is expected since class Child(Model): looks in the current module namespace for types.

Child model MUST look in current namespace only when detecting types for ITS fields. NOT parent fields.

I understand that you might have some limitations on how you implemented this, but for provided example the expected result is quite obvious.

Moreover if you get rid of ForwaredRef, moving class A upper in the file it is expected that child class will not be affected, but it is.

Additionally, I expect that in here Child.a and Child.aa will have different types:

# fw_parent.py
from pydantic import BaseModel

class Model(BaseModel):
    a: "A"

class A(BaseModel):
    pass

Model.model_rebuild()

# fw_child.py
from pydantic import BaseModel
from fw_parent import Model

class A(BaseModel):
   pass

class Child(Model):
    aa: A

# main.py
from fw_parent import A as ParentA
from fw_child import A as ChildA, Child
Child(a=ParentA(), aa=ChildA())

@JrooTJunior
Copy link

I'm not totally understand why inheritance was broken in V2 because it works as well on V1.

Why child class can't copy already resolved fields from the parent class?

@dmontagu
Copy link
Contributor

@JrooTJunior I agree it probably should. Let me investigate a bit and see if I can improve this.

@dmontagu
Copy link
Contributor

dmontagu commented Jul 25, 2023

@Tishka17 I believe your code example behaves properly with #6838 — any feedback on whether it resolves the issues for the people following this thread would be appreciated.

Note that even with #6838 you may need to explicitly call model_rebuild in the appropriate module when defining models with forward references across modules, or the forward ref might be evaluated in the wrong module. (The necessary call to model_rebuild was done in the examples provided above though.)

I suspect there will be a solution to this that will eliminate the need for that model_rebuild once PEP649 is accepted/implemented and we can get annotations evaluated in the proper namespaces more easily, but until then, I think it may be better to accept this awkwardness rather than make the implementation more complex to handle it seamlessly.

@dmontagu dmontagu removed the unconfirmed Bug not yet confirmed as valid/applicable label Jul 25, 2023
@Tishka17
Copy link
Author

Looks ok for me, I cannot find an example how to break it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug V2 Bug related to Pydantic V2
Projects
None yet
5 participants