Pydantic __eq__ and __hash__ doesn't work properly with functools.cached_property

[QuentinSoubeyranAqemia]

Initial Checks

• I confirm that I'm using Pydantic V2

Description

Bug

While porting an app to Pydantic v2, I notice that the implementation of __eq__ doesn't work properly on Model with functools.cached_property: equal instances can compare unequal.

From the documentation on computed fields, it seems pydantic should support functools.cached_property. I'm trying to compute an attribute value lazily and only once, because it is very expensive. functools.cached_property seems like the perfect tool for that.

Example 1 below demonstrate the problem, and Example 2 shows that this can introduce subtle bugs when using dict on "equal" objects, as dict relies on a correct __eq__ and __hash__.

Analysis

Line 831 from pydantic's implementation of __eq__ seems to assumes that __dict__ only ever contains pydantic's field values.

pydantic/pydantic/main.py

Lines 829 to 834 in c6e2f89

	return (
	self_type == other_type
	and self.__dict__ == other.__dict__
	and self.__pydantic_private__ == other.__pydantic_private__
	and self.__pydantic_extra__ == other.__pydantic_extra__
	)

I think this is not true in the general case, as __dict__ is an instance state that is shared between pydantic and all other mechanism that need to act on the instance itself.
Indeed, functools.cached_property stores the computed value in the instance __dict__, because it is the only place it can reasonably store it:

https://github.com/python/cpython/blob/1f885df2a580360c5de69cc41191f3c6bfaaeb35/Lib/functools.py#L1005

Assuming that __dict__ only contains pydantic's own fields has already been mentionned in #6422

Possible fix

I think the fix below limits the comparison of __dict__ to pydantic's own fields. I don't know much about pydantic source code, so there might be some rough edges I overlooked. I used a sentinel object as a default value to avoid crashing on missing fields, not sure it is actually necessary.

    def __eq__(self, other):
        ...
        sentinel = object()
        return (
            self_type == other_type
            and all(
                getattr(self, field, sentinel) == getattr(other, field, sentinel)
                for field in self.model_fields
            )
            and self.__pydantic_private__ == other.__pydantic_private__
            and self.__pydantic_extra__ == other.__pydantic_extra__
        )

Related issues

• model_dump() shows Pydantic serializer warnings after call @cached_property method #6422 had a more obscure problem due to assuming __dict__ contains only pydantic fields

Additional context

Comparing the full __dict__ seems to be a change from pydantic V1. I would be interested in knowing why the full __dict__ is compared in V2.

Both attrs and the stdlib dataclasses work seamlessly with functools.cahced_property and only compare their own fields:

dataclasses __eq__

https://github.com/python/cpython/blob/1f885df2a580360c5de69cc41191f3c6bfaaeb35/Lib/dataclasses.py#L1086-L1099

attrs __eq__

https://www.attrs.org/en/stable/comparison.html#comparison

Example Code

# ---------- Example 1 -------------
import functools

import pydantic
    

class Model(pydantic.BaseModel):
    attr: int

    @functools.cached_property
    def cached(self) -> int:
        return 0


obj1 = Model(attr=1)
obj2 = Model(attr=1)

assert obj1 == obj2  # works

obj1.cached

assert obj1 == obj2  # raises an error

# ---------- Example 2 -------------

import functools

import pydantic


class Model(pydantic.BaseModel):
    model_config = pydantic.ConfigDict(frozen=True)

    attr: int

    @functools.cached_property
    def cached(self) -> int:
        return 0




obj1 = Model(attr=1)
obj2 = Model(attr=1)
d = {obj1: True}

assert d[obj2]

obj1.cached

assert d[obj2]  # raise an error

Python, Pydantic & OS Version

Note: installed pydantic with "pip install git+https://github.com/pydantic/pydantic.git"


             pydantic version: 2.3.0
        pydantic-core version: 2.7.0
          pydantic-core build: profile=release pgo=false
                 install path: ~/miniconda3/envs/dev-sandbox/lib/python3.8/site-packages/pydantic
               python version: 3.8.17 | packaged by conda-forge | (default, Jun 16 2023, 07:11:34)  [Clang 14.0.6 ]
                     platform: macOS-10.16-x86_64-i386-64bit
     optional deps. installed: ['typing-extensions']

[lig]

@QuentinSoubeyranAqemia thank you for the bug report.

Looking at your examples this will produce equal entities:

obj1 = Model(attr=1)
obj2 = Model(attr=1)

assert obj1 == obj2  # works

obj1.cached
obj2.cached  # adding this makes the next check pass

assert obj1 == obj2

I think that there could be cases where objects with a cached property being already calculated and not shouldn't be considered equal. In general, we don't know what the result of calculating a cached property would be on another instance. Also, it's not clear to me that Pydantic is allowed calling that calculation implicitly in the __eq__ method.

I would argue that this logic is implementation specific and would recommend implementing a custom base class for your models implementing desired logic.

[samuelcolvin]

I'd be happy to consider changing it, the question is whether this could represent a breaking change?

The reason we used __dict__ was that it's faster than iterating over model fields.

[QuentinSoubeyranAqemia]

Thank you for your answers! I have a few things to reply to, so I splitted my message by what part is refered to

---

The reason we used dict was that it's faster than iterating over model fields.

That make sense, thank you for the context !
If the change measurably degrades performances:

• would making a dict subset in a dict-comprehension be fast enough ? (I could try that if you have a performance benchmark)
• use a rust hook to alleviating the costn? (e.g. a rust code to compare python dict only on certain keys instead of the list-comprehension, or another solution)
• dynamically generate an __eq__ at class-creation time, like attrs and dataclasses do? Possibly reusing part of the stdlib's dataclasses for that?

---

I'd be happy to consider changing it, the question is whether this could represent a breaking change?

Technically it is a breaking change, I suppose.

In practice, this depends on how often the assumption "the __dict__ only contains pydantic's fields" holds. The behavior only changes when the assertion is false. I suppose this is rare outside using @functools.cached_property, else this bug would have been more prominent?

[QuentinSoubeyranAqemia]

Looking at your examples this will produce equal entities

While this indeed work on the minimal-repro, I don't think it is applicable in production. For instance, to use a dict mapping model instances to some values, you would need to:

• iterate on the dict keys(), thus loosing all performance advantage in using lookup tables
• known about all cached properties on the object used as keys, even when the keys are mixed types (they can be detected dynamically I suppose, but yet more work)
• compute all properties in advance before comparison, thus defeating the performance purpose of @cached_property

It would be more efficient to use a simple @property (no need to iterate on keys since it is ignored in comparisons, no need to compute all properties in advance), but the property being a performance bottleneck is already why I'm using cached_property.

---

I think that there could be cases where objects with a cached property being already calculated and not shouldn't be considered equal. In general, we don't know what the result of calculating a cached property would be on another instance

Do you have a concrete example where a cached property being computed or not should change the equality comparison, all other fields being equal? It seems to me that if the property can be meaningfully cached, by definition it shouldn't change, it's like a lazy constant attribute?

When I'm using @cached_property, given the cached_property python doc mention of immutable objects and cached_property being entirely determined by other attributes:

• used than on frozen/immutable objects for performance reasons, so:
  • the cached property value is entirely determined by the other field values (this is also how the python doc shows an example between cached_property and lru_cache)
  • the other field values never changes ("effectively immutable")
• I expect @cached_property to behave the same as @property, but with performance improvements. Pydantic doesn't compare @property, I think therefore it shouldn't compare @cached_property.

Also, it's not clear to me that Pydantic is allowed calling that calculation implicitly in the eq method.

I agree that pydantic should not trigger the computation of cached_property during equality comparisons. I was not proposing that: cached_property are not part of pydantic's runtime fields (I think?) and should be ignored by the __eq__, the same way @property are ignored by the __eq__.

---

I would argue that this logic is implementation specific and would recommend implementing a custom base class for your models implementing desired logic.

Indeed, using a custom base model with a custom __eq__ is what I resorted to after porting to pydantic v2. Though it doesn't feel very elegant to need to overwrite custom __eq__ to use the stdlib.

[alexmojaki]

I expect @cached_property to behave the same as @property, but with performance improvements. Pydantic doesn't compare @property, I think therefore it shouldn't compare @cached_property.

I think this is an excellent point. Put differently, if @cached_property somehow worked another way and didn't affect __dict__ and thus __eq__ then I don't think it'd be considered a bug that __eq__ returns True even if a cached property has only been accessed on one side. If that would somehow be considered a bug, then it's a bug right now with @property.

The reason we used __dict__ was that it's faster than iterating over model fields.

How about replacing:

self.__dict__ == other.__dict__ 

with:

(
    self.__dict__ == other.__dict__ or (
        self.__dict__.keys() != other.__dict__.keys() and
        (slower check using model_fields or something)
    )
)

This should be:

1. Essentially the same speed when the dicts are equal
2. Still pretty good when the dicts are not equal but the keys are equal, since I imagine self.__dict__ == other.__dict__ should often be faster than self.__dict__.keys() != other.__dict__.keys() since it doesn't have to compare values, although that depends on several things.
3. Only triggering the slow check in rare cases like this one, since we've already checked self_type == other_type and instances of the same type will usually have the same .__dict__.keys().

Moving the whole check to after checking __pydantic_private__ and __pydantic_extra__ might also help.

[alexmojaki]

Ah, but you'd also need to update __hash__, i.e. set_default_hash_func.<locals>.hash_func, and no similar trick will work there.

[QuentinSoubeyranAqemia]

Good catch, modifying __eq__ means updating __hash__ too, to preserve the property "object comparing equal must have the same hash".
I found at least this line needs to be modified:

pydantic/pydantic/_internal/_model_construction.py

Line 400 in c6e2f89

return hash(self.__class__) + hash(tuple(self.__dict__.values()))

It similarly should only use pydantic's fields.

Looks like the key performance bottleneck will be to efficiently take a subset of a dict? Is there any performance benchmark for pydantic that could be used to experiment with that?

[alexmojaki]

I opened #7786 to show that hashing can be handled without hurting performance too much.

[QuentinSoubeyranAqemia]

The problem also affects __hash__ as detected by @alexmojaki . This cause the following code to fail, where a frozen object stored in a dict cannot be retrieved because its hash changes.

This is because pydantic only enforces pydantic fields to be frozen, but hashes the whole __dict__, which isn't frozen. This would be fixed by #7786 .

Should I open a new issue with this problem ?

import functools
import random

import pydantic


class Model(pydantic.BaseModel):
    model_config = pydantic.ConfigDict(frozen=True)

    attr: int

    @functools.cached_property
    def prop(self) -> int:
        return random.randint(0, 999)


obj1 = Model(attr=1)
print(hash(obj1))

mapping = {obj1: True}
mapping[obj1]

obj1.prop
print(hash(obj1))
mapping[obj1]  # KeyError

[alexmojaki]

I think it's essentially the same problem, it just needs to be fixed in two places. The original "Example 2" already covered this.

[QuentinSoubeyranAqemia]

I don't think example 2 actually covers it. This is dependent on the internals of dict, which I'm not sure I'm getting right, but this is what I understand happens:

• In example 2, the obj1 is put into a hash-bucket corresponding to its old hash.
• we side-effect-modify the __dict__ of obj1, thus changing its hash. This doesn't move the object in the dict internal structure
• we query the dict with obj2, which has the same old hash. The dict looks into the old hash bucket, effectively comparing the old hashes together. Then it compares each object in the hash-bucket to obj2. This fails because obj1 != obj2, but I don't think the hash is used anymore at that point.

Given that all objects with the same hash ends up in the same bucket, I don't think the comparison is done first on hash, then with __eq__, as pulling the bucket is already doing an implicit hash comparison. Then perhaps the fast-comparison is still done, because several hashes end-up in the same bucket.

[dmontagu]

I personally think it makes sense to just replace self.__dict__ == other.__dict__ in the BaseModel.__eq__ with the relevant check for only fields. (i.e., {k: self.__dict__.get(k) for k in self.model_fields} == {k: other.__dict__.get(k) for k in other.model_fields}, or perhaps something fancier/faster using operator.itemgetter as in @alexmojaki's #7786).

Even if it's not quite as fast, I agree with the claims made here that @cached_property should behave as closely to @property as possible and not cause instances to not compare equal, same as the various other libraries referenced in the issue report.

[alexmojaki]

Thanks @dmontagu for the vote, it felt like there was still some doubt before but now it sounds like we should go forward with a change.

@QuentinSoubeyranAqemia you've been amazingly thorough and diligent with this stuff, can you open a PR for __eq__ and benchmark various approaches?

[QuentinSoubeyranAqemia]

I'll take some time to experiment with __eq__ implementations, yes !

[QuentinSoubeyranAqemia]

Opened #7825, still working on it
EDIT: #7825 is ready for review (marked as such)

[QuentinSoubeyranAqemia]

Well, apparently there is an edge case where the deprecated pydantic.BaseModel.copy() can make an instance that is missing fields. I'd like some input on that in #7825

[alexmojaki]

@lig @dmontagu please can you review #7825 and #7786?

[dmontagu]

@alexmojaki I've reviewed both, they basically seem good — thank you both for the detailed performance investigations and nuanced, correct(🤞) implementations.

I had note on #7786 that doesn't necessarily need to be addressed before merging (my most recent comment), and I think that can be merged, and then I think #7825 can be merged once @alexmojaki's comments on the PR are addressed.

I'll leave it to @sydney-runkle to decide exactly when to merge, I think the plan was docs only changes prior to 2.5.0, but given it's a bugfix maybe it can be in a quick 2.5.1.

[alexmojaki]

🎆 🎉 💃