Only compare pydantic fields in BaseModel.__eq__ instead of whole __dict__

[QuentinSoubeyranAqemia]

Change Summary

This PR fixes #7444. It is focused on fixing pydantic.BaseModel.__eq__, see #7786 for pydantic.BaseModel.__hash__.

Related issue number

• Fix Pydantic __eq__ and __hash__ doesn't work properly with functools.cached_property #7444

Checklist

• The pull request title is a good summary of the changes - it will be used in the changelog
• Unit tests for the changes exist
• Tests pass on CI
• Documentation reflects the changes where applicable
• My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @lig

[QuentinSoubeyranAqemia]

I performed a benchmark on various implementations, here are the results:

• All performance report as expressed as a ratio of the current performance, dubbed "baseline"
• I had to comment out the dict-comprehension implementation, it was too slow to run the benchmark in a timely manner
• Having a fast-path allows to not degrade performance in the common case (left column, fast-path line is close to baseline)
• To trigger the specialized code, the test fakes a functools.cached_property by manually editting the instances' __dict__. Of note, I had to add the same key to both instances, with different values, else the default __dict__ comparison is very fast due to the dict not having the same key set. This comparison is unfair, because the bugged default implem has a fast-path. The different values are necessary to trigger the specialized code.
• If the fast-path allows to have unchanged performance in the default case, it appears to have performance cost in the functools case. This is IMHO an artifact of the testing: in the real case:
  • either the two object didn't pre-compute the same set of cached properties. Then, the keys of the left and right instances' __dict__ are different, and thus the __dict__ comparison is very fast (see point above) and doesn't cost a full dict-comparison
  • or the two object computed the same set of cached properties. Then, if there fields are equals, their cached properties are usually equals (cached properties are a function of the actual fields), and thus the fast-path doesn't trigger.

All that being said, I think @alexmojaki 's ideas of a fast-path and operator.itemgetter constitue a good performance compromise.

[QuentinSoubeyranAqemia]

The fix cause some edge-case to fail. Using the deprecated pydantic.BaseModel.copy() method can yield a weird instance where some fields with a default value are missing.

I'm unclear on what to do here. I could use collections.ChainMap to chain the instance's __dict__ and the class' __dict__, but perhaps that is a problem in pydantic.BaseModel.copy ?
The test seems to be making sure some deprecated behavior still works, so I'd be happy to have some inputs from pydantic devs as to what the best course of action is.

[alexmojaki]

It's not just about deprecated behaviour. Pydantic allows deleting attributes even if they don't have a default:

from pydantic import BaseModel

class A(BaseModel):
    b: int

a = A(b=1)
print(a.__dict__)
del a.b
print(a.__dict__)

For frozen models I opened and resolved #7784 to deal with this, so __hash__ doesn't have to worry about it. But you definitely need to worry about it for __eq__, since I see no sign of deleting attributes being deprecated. And I guess maybe I need to worry about .copy() for __hash__, thanks for catching that. Though I do wonder what the point of a model with missing fields is, whether it's reached by del or copy.

I think that this should still be almost as fast in the common case:

try:
    # fast path
except KeyError:
    # slow path

[QuentinSoubeyranAqemia]

I don't think the above code is valid: deleting the attribute violates the Liskov Substitution Principle, since the class used a type annotation to promise an attribute named a will always be present on instances.

This is not related to pydantic: you can remove pydantic altogether from your example, and it would still violate the LSP. If I where to write the obvious, manual __eq__ function for this class (which is essentially what dataclasses.dataclass does), it would also raise an AttributeError.

Though since legacy pydantic interface can produce those invalid objects, I think it is better to not hard-crash and handle those. I'll use your trick (again!) from #7786 to have a low-cost fallback.

[QuentinSoubeyranAqemia]

please review

Not sure if the benchmark should be kept in the code-base. Also not sure where, or if, to add a mention of this in the docs.

[alexmojaki]

I think you must have a different version of black installed, or it's pointing to the wrong configuration, causing a bunch of code to get reformatted, maybe to fit in a shorter line length. Then CI doesn't complain about it because of the new trailing commas. But you should probably revert the formatting-only changes.

[QuentinSoubeyranAqemia]

Thanks for catching that! That was the black VScode extension doing, it has its own configuration

[alexmojaki]

Also need to revert changes in tests/test_edge_cases.py

[QuentinSoubeyranAqemia]

Thanks again, I forgot about that file

[QuentinSoubeyranAqemia]

I cleaned the commit history with reset+re-adding things

[QuentinSoubeyranAqemia]

Added a fast-path for standard-case (i.e. no additional keys in __dict__ to ignore) unequal items. Previously, if the two __dict__ compared unequal, we would always trigger the slow __dict__ filtering code. This no longer happens when there are no keys to ignore.

[alexmojaki]

I don't think you need to worry about the commit history unless you think it'll make it easier to review, since all PRs are squash-merged.

[QuentinSoubeyranAqemia]

Good to know! I've seen some people do review commit-by-commit, so I try to have commits represent steps that would help the reviewer if they do it that way

[sydney-runkle]

@alexmojaki,

I've merged #7786, so feel free to continue work here 👍. Thanks!

[QuentinSoubeyranAqemia]

Thanks, I'll adapt the PR to the merging of #7786 when I get a moment !

[codspeed-hq]

CodSpeed Performance Report

Merging #7825 will not alter performance

_{Comparing QuentinSoubeyranAqemia:gh-7444 (d8f481b) with main (eb90b59)}

Summary

✅ 10 untouched benchmarks

[QuentinSoubeyranAqemia]

@alexmojaki @dmontagu @sydney-runkle
This is ready for review
I have adapted to #7786 merge, and shared the code.

I went with fully-typed flavor, so that correct usage of the SafeGetItemProxy utility can be checked by MyPy. If you'd prefer a non-typed with # type: ignore comments, or something in-between, feel free to ask in the review and I'll adapt !
I'm really not sure which is easier to maintain

[sydney-runkle]

@QuentinSoubeyranAqemia,

Thanks so much for your work on this 🙏. I'm going to focus on bug fixes for the newly released 2.5 today, and will take a close look at this once we get those across the line (tomorrow).

Your work here is super appreciated 🚀 !

[dmontagu]

@QuentinSoubeyranAqemia If I'm understanding correctly, it looks like we only use the SafeGetItemProxy to do comparisons and equality checks, and not to actually retrieve data in APIs where knowing the returned value's type is important. Given that, I think we should drop the use of the enum and generic, and just annotate SafeGetItemProxy.__getitem__ as returning Any. In a future where we try to use SafeGetItemProxy to retrieve data we can change it to be more type safe, but as is, I think the complexity isn't worth it given I don't think we are using it anywhere that the values aren't of type Any anyway, and I think it adds a lot of cognitive overhead to the implementation.

[sydney-runkle]

@QuentinSoubeyranAqemia,

Thanks again. Following up on your typing question - the "fully-typed flavor" is great. This generally makes for cleaner code, and imo makes things easier to maintain as types are more clear.

[brasie]

Hi, thanks for this fix. Do you have an estimate when this will make it into a release? I was surprised it didn't make it into v2.5.2.

[sydney-runkle]

@brasie,

This will be included in v2.6.0 - we only included things in the 2.5 patch releases that fixed bugs introduced by 2.5!

[brasie]

I see, thanks for the update.