CodSpeed Performance Report

Merging#1490 willnot alter performance

_{Comparingsneakers-the-rat:exclude-eq-exception (e80518a) withmain (1ced3e6)}

Summary

✅ 155 untouched benchmarks

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base(ab503cb) to head(e80518a).
Report is 197 commits behind head on main.

@@            Coverage Diff             @@##             main    #1490      +/-   ##==========================================- Coverage   90.21%   89.11%   -1.10%==========================================  Files         106      112       +6       Lines       16339    17826    +1487       Branches       36       40       +4     ==========================================+ Hits        14740    15886    +1146- Misses       1592     1920     +328- Partials        7       20      +13

I hope I am reading the PR template right that i am supposed to now make a comment saying "please review" because that is what i am doing :)

though silently ignoring exceptions can always potentially be confusing.

This is fair - although in this case I think it makes sense semantically: "exclude things that are equal to the default. If something cannot be meaningfully compared to the default such that it emits an error when you try, then that means that it probably isn't equal to the default." I think worst case is that we include a few strange custom-classes values in the output thatare technically equal to the default, but that is more of a burden on the implementers to implement equality operators (in this case, I can't, since i'm wrapping array libraries where there is strong and meaningful precedent to emit an error on==).

Equality comparisons for all the standard pydantic types should not raise an exception and always returnFalse if they are not equal, right? I am assuming since I couldn't find this issue having been raised before. So I wonder how many workflows we would break by allowing errored equality comparisons.

We should probably emit a warning when this happens though, that would be polite. If we want that I can read through the code and figure out how to do that (although it might take me a bit because i am new to rust)

we might want to document that exclude_defaults will serialize anyway if the default comparison fails.

I can of course add this to the docs if this is what we decide to go with! Since there are a few other ideas on the table (love the enthusiasm <3) i'll hold off for now.

we should expose a config value to override the default_eq comparison would be safer? Users could then supply a function which swallows exceptions (or does whatever else is suitable for their use case).

So are you thinking of something like this?

defmy_comparator(default:Any,value:Any)->bool:# do whatever i want herereturnrandom.random()>=0.5model.model_dump_json(exclude_default=True,default_eq=my_comparator)

If we go with that, it would be really great to be able to declare something like that in aSchemaSerializer or otherwise in acore_schema definition so that I could just declare a proper array equality function that's used by default when serializingNDArray types. It does sound like it has the potential to blow up in complexity tho, bc once it's there, you'd expect it in annotated field types, model config, etc...

All theeq checks are actually calls to python__eq__ methods, not some faster rust equality checking right? just asking because if they aren't, and there's some perf disadvantage, we might want to either make that behave like the python json "default" hook which only calls the function when there is an error using the normal serializer or add that as another option.

I suppose even more general, rather than just default_eq we could have an exclude_if function which checks the value and returns True / False. This would be a way to generalize exclude_defaults and exclude_none etc...

I would actually love this, but it doesn't necessarily resolve my goal of "make arrays behave as you would expect with all pydantic functionality" and would require an additional term for every serialization with arrays in it. maybe that's for another PR?

Bump on this, I see discussion going forward about the functional exclude filter here, which sounds like a good idea:pydantic/pydantic#10728

But this PR fixes an orthogonal problem specifically with the exclude_defaults flag, it's still breaking my tests for a custom type being compliant with all pydantic features, and im not sure where we landed on what I need to change here.

I think emitting a warning when a field is considered not a default by way of an exception being caught in comparing equality makes sense, and if maintainers agree then ill go ahead and add it, or lmk if we want other changes :)