Closed andy-sweet closed 1 year ago
thanks as always for the super clear issue! :)
I think we should fix this (without forcing the workaround of making them properties), but it could be done in a bunch of ways, let me know which of the following sounds best to you:
property_dependencies
to include fields would be the simplest and the most explicit. we would just remove this exception, and allow:
class Config:
validate_assignment = True
property_dependencies = {"y": ["x"]}
It would be the most performant, and the least magical, but it would mean events.y
will always be emitted whenever x
is changed, regardless of whether the change in x
actually changed the value of y
. (for what it's worth, that's already the case with property dependencies anyway)
__setattr__
is called on a field, we'd need to fetch the values of all dependent fields before and after actually setting the field, then compare each of them to determine whether an emission is required. So, could be costly in terms of performance.property_dependencies
configuration would be to detect whether the class had a root validator, then simply check all fields in the model before and after setting any field. That's of course the worst in terms of performance, but "just does the right thing" without requiring the user to notice this special case.thoughts?
I think (1) then (2) seem like the most attractive to me because they're the simplest to implement. For (1), the property_dependencies
name is a bit unfortunate - maybe a rename to field_dependencies
or just dependencies
might be worth it?
As far as the performance hit of (2), it doesn't seem too bad because that will only be incurred if the user explicitly declared the dependency. And I think this particular usage/bug is probably uncommon. Plus if we're only doing this for fields, I think the performance hit doesn't have to be huge. If we're doing for it properties where we may need to calculate its value before/after rather than just read it, it's more of a problem. Therefore, (5) seems a bit unnecessary.
I don't love (3) because it might useless for most users and the performance penalty could be higher since we're checking all the values before and after.
great, I agree on all accounts. I implemented (2) in #234 (without the config of (5)) and it didn't seem to nudge any of the benchmarks. I agree that the naming is now unfortunate, and I'm up for changing it. I like dependencies
, but since the namespace is shared with all the other pydantic stuff, perhaps event_dependencies
is safer/clearer? or do you like field_dependencies
better than event_dependencies
?
renamed to field_dependencies
main
at the time of writing (a4a3348fb6f81c3aeefc8a1c723360c9a26db23e)Description
When I define an
EventedModel
with a root validator that coerces some field values based on others, changes in derived/constrained values do not trigger events to be emitted.Coming here from an issue on napari where similar problems with its similar
EventedModel
cause problems forDims
. No fix for that yet.What I Did
Here's the simplest and also somewhat artificial reproducer I could write, where a model has two ints (
x
andy
) and the root validator ensuresx >= y
.I think this is a bug because I'd expect the change in
y
to emit an event.Thoughts
I'm not sure this is a critical bug because I've at least had half-baked thoughts about alternatives/workarounds (e.g. define
x
andy
as properties and then useproperty_dependencies
). Taking that half-baked idea a little further, does it make any sense to allowproperty_dependencies
to includefields
as well as properties?Other ideas/workarounds are welcome!