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

Make better union validation decisions based on extra behavior #1335

Open
sydney-runkle opened this issue Jun 19, 2024 · 7 comments
Open

Make better union validation decisions based on extra behavior #1335

sydney-runkle opened this issue Jun 19, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@sydney-runkle
Copy link
Member

sydney-runkle commented Jun 19, 2024

For example:

class ModelA(BaseModel):
    pass

class ModelB(BaseModel, extra='allow'):
    pass

TypeAdapter(ModelA | ModelB).validate_python({'x': 1})
#> ModelA()

Is the current behavior, but that doesn't seem like the best decision.

See #1332 and #1334 for recent context on union validation decision improvements.

@sydney-runkle sydney-runkle added bug Something isn't working and removed unconfirmed labels Jun 19, 2024
@davidhewitt
Copy link
Contributor

It seems to me like the solution here would be to have another counter extra_fields_set 🙈

@sydney-runkle
Copy link
Member Author

Indeed. Honestly a good first issue for someone looking to start in rust and pydantic-core, given that we just set the model for this in the PRs linked above.

@sydney-runkle
Copy link
Member Author

@erohmensing you might be interested in this!

@erohmensing
Copy link

Thanks for the tag @sydney-runkle! This makes sense, would be happy to take a look.

@davidhewitt is there any reason why you'd think of a counter over a bool for extra_fields_set? From my understanding the models either allow extras or don't, but maybe there's some more complicated custom config logic where you can customize the shape of allowed extras that I'm not aware of?

@sydney-runkle
Copy link
Member Author

@erohmensing,

I think a bool would be fine for the reasons above that you mentioned. In some ways, a bool is better, because I don't think an arbitrary count of extra fields set is indicative of a better match if two models have the same extra config setting.

Also, consider:

from pydantic import BaseModel, TypeAdapter

class ModelA(BaseModel):
    a: int
    b: int

class ModelB(ModelA, extra='allow'):
    pass

print(repr(TypeAdapter(ModelA | ModelB).validate_python({'a': 1, 'b': 1, 'x': 1})))
#> ModelA(a=1, b=1)

This is the current behavior - I think the desired behavior would be ModelB. This is a case where we'd have fields_set_count information (a tie), and could default to the extra metric as a tiebreaker. That being said, maybe exactness should take priority over our new metric?

@erohmensing
Copy link

erohmensing commented Jun 20, 2024

@sydney-runkle

This is the current behavior - I think the desired behavior would be ModelB

I agree!

That being said, maybe exactness should take priority over our new metric?

do you mean in the case of like

from pydantic import BaseModel, TypeAdapter

class ModelA(BaseModel):
    a: int
    b: int

class ModelB(ModelA, extra='allow'):
    pass

print(repr(TypeAdapter(ModelA | ModelB).validate_python({'a': 1, 'b': 1)))

should be Model A? If so, agreed - that seems to be the case and I think it would make sense for it to stay like that

@sydney-runkle
Copy link
Member Author

@erohmensing, are you still interested in contributing here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants