🐛 Fix RootModel equality logic
#5948
Conversation
|
please review |
|
chaining this one after #5943 |
| @@ -1096,14 +1096,14 @@ def _calculate_keys(self, *args: Any, **kwargs: Any) -> Any: | |||
|
|
|||
| class RootModel(BaseModel, typing.Generic[RootModelRootType]): | |||
| __pydantic_root_model__ = True | |||
| __pydantic_fields_set__ = {'root'} # It's fine having a set here as it will never change | |||
There was a problem hiding this comment.
Actually I think if we wanted it to be consistent with how it works for regular basemodel, this should depend on whether the default value was used. (If it needed to use the default, the value would be set())
There was a problem hiding this comment.
This is fair. Let me add a TODO here and get back to it in a separate PR.
There was a problem hiding this comment.
Could you change the comment on the line so that it isn't in conflict with the TODO comment above it? Just thinking it will be a bit confusing to come back to
There was a problem hiding this comment.
Also, I think we should fix this in the PR addressing the default setting for RootModel.
There was a problem hiding this comment.
It has been fixed already in #5949
| def __eq__(self, other: Any) -> bool: | ||
| if not isinstance(other, RootModel): | ||
| return NotImplemented | ||
| return self.model_fields['root'].annotation == other.model_fields['root'].annotation and super().__eq__(other) |
There was a problem hiding this comment.
What is this check on the annotations doing for us? I think we don't want to check the annotations are equal because that would mean that RootModel[Any](root=1) is not equal to RootModel[int](root=1), but that is the behavior that would be consistent with how generic BaseModel works. (You could similar concerns imagine for generic subclasses of RootModel)
There was a problem hiding this comment.
My thought was that this should be true RootModel[int](42) != RootModel[float](42), shouldn't it?
There was a problem hiding this comment.
I think having that return True is a price I'm willing to pay to get equality with Any. I mean, if you did:
from typing import Generic, TypeVar
from pydantic import BaseModel
T = TypeVar("T")
class MyModel(BaseModel, Generic[T]):
x: T
assert MyModel[int](x=42) == MyModel[float](x=42.0)it currently doesn't error. And I think there's a lot of stuff that behaves worse if you try to change this, because of things like reparametrizing generics, etc. I just think it's easiest if we have RootModel behave consistently with this
There was a problem hiding this comment.
I'm fine with the annotation but, but I wonder if we should do and self.root == other.root?
There was a problem hiding this comment.
@samuelcolvin we also should compare private fields and that is why there is the super call.
There was a problem hiding this comment.
I think comparing private fields is a good reason to keep the super().__eq__.
But also, you need to have the super().__eq__ to make sure the classes match — if you subclass RootModel[T] (which I would imagine is the reason why you are using a RootModel and not TypeAdapter in most cases), instance equality should require type equality. At least if we want to be consistent with BaseModel, and I think we should. (Well, you could drop the super().__eq__, but then I think we should copy that logic as well.)
I still think we should drop the annotation comparison thing because it's surprisingly inconsistent with other generic BaseModel subclasses, but it's not a hill I want to die on.
|
please review |
Deploying with
|
| Latest commit: |
c575e77
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://83fec789.pydantic-docs2.pages.dev |
| Branch Preview URL: | https://fix-root-model-eq.pydantic-docs2.pages.dev |
| def __eq__(self, other: Any) -> bool: | ||
| if not isinstance(other, RootModel): | ||
| return NotImplemented | ||
| return self.model_fields['root'].annotation == other.model_fields['root'].annotation and super().__eq__(other) |
There was a problem hiding this comment.
I'm fine with the annotation but, but I wonder if we should do and self.root == other.root?
|
please review @samuelcolvin I'm happy to discuss this |
There was a problem hiding this comment.
As noted, I still would like to drop the annotation comparison in the __eq__, but if I'm the only one that feels that way it doesn't need to hold up merging. (The thing about setting __pydantic_fields_set__ seems better addressed in the other PR so I'm okay with that here.)
|
No, I think your right, we should compare the generic annotations. Does equality work correctly for generic basmodels? Surely if it did, we wouldn't need a custom method for |
Change Summary
Fix crash on comparing RootModels for equality
Related issue number
None
Checklist
changes/<pull request or issue id>-<github username>.mdfile added describing change(see changes/README.md for details)
Selected Reviewer: @dmontagu