Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support arbitrary field dependents #340

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Nov 21, 2024

This is a proof of principle PR that would support the feature that @sjdemartini is requesting in #337. namely:

  • computed fields can gain events
  • computed fields (really, currently any field) can list non-field attribute names as their dependents for checking event emission.

@Czaki, I would appreciate your eyes on this.

  1. I just "made it work" ... so there's likely some minor refactoring that should be done here to polish it (particularly in the _check_if_values_changed_and_emit_if_needed area that you had previously worked on)
  2. Feel free to comment in general on the feature request. I think @sjdemartini motivated it quite nicely his comment, Adding additional signals (e.g. for computed_field) #337 (comment) ... so please read that for context; and for a quick use case example, see the new test I added in test_private_field_dependents

closes #337

@tlambert03
Copy link
Member Author

tests are failing during building of mypyc, due to pydantic/pydantic#10907

Copy link

codspeed-hq bot commented Nov 21, 2024

CodSpeed Performance Report

Merging #340 will not alter performance

Comparing tlambert03:computed-fields (ed0a7e0) with main (6db5219)

Summary

✅ 67 untouched benchmarks

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.95%. Comparing base (6db5219) to head (ed0a7e0).

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #340      +/-   ##
===========================================
- Coverage   100.00%   99.95%   -0.05%     
===========================================
  Files           21       21              
  Lines         2095     2105      +10     
===========================================
+ Hits          2095     2104       +9     
- Misses           0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@tlambert03 tlambert03 marked this pull request as ready for review November 22, 2024 01:45
Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can we not use the same machinery like current property setter/getter (put things in self._changes_queue)?

Comment on lines +132 to +133
name: pydantic.fields.FieldInfo(annotation=f.return_type, frozen=False)
for name, f in cls.model_computed_fields.items()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see test with pydantic 1.x, Should be here: if hasattr(cls, "model_computed_fields")?

# An attribute is changing that is not in the SignalGroup
# if it has field dependents, we must still continue
# to check the _changes_queue
must_continue = name in self.__field_dependents__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
must_continue = name in self.__field_dependents__
must_continue = must_continue or name in self.__field_dependents__

As it may happen in the middle of for loop

Comment on lines 582 to 583
# primary changes should contains only fields
# that are changed directly by assignment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not fully understand. When we need to emit a signal. This comment still statement that self._primary_changes are only fields changed by direct assignment.
And documentation of https://docs.pydantic.dev/2.0/usage/computed_fields/ do not mention the assigment of this property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i wasn't entirely clear on the _primary_changes, and _changes_queue attributes here. So maybe I did it wrong. we need to emit a signal here when a non-field attribute changes, provided that some other evented field lists that attribute as one of its "dependents". So, in the example provided by the original requester:

from psygnal import EventedModel
from pydantic import computed_field, PrivateAttr

class MyModel(EventedModel):
    _items_dict: dict[str, int] = PrivateAttr(default_factory=dict)

    @computed_field
    def item_names(self) -> list[int]:
        return list(self._items_dict.keys())

    def add_item(self, name: str, value: int) -> None:
        # this next assignment should trigger the emission of `self.events.item_names`
        # because `_items_dict` was listed as a field_dependency of `item_names`
        self._items_dict = {**self._items_dict, name: value}

    class Config:
        field_dependencies = {"item_names": ["_items_dict"]}

if I did that incorrectly (or stupidly) with the existing _primary_changes/_changes_queue pattern let me know

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this might be the first case where the "primary change" is to a non-evented attribute (_items_dict)... and so it didn't quite fit into the existing pattern. It's the first case where a _primary_change isn't actually able to itself be evented

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.

Maybe we should add a dummy emiter (callback that do nothing, and raises an exception on connection) for private attributes that are listed in field_dependencies? Then we will move the complexity from setattr to constructor call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's worth trying, lemme see how it goes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Adding additional signals (e.g. for computed_field)
2 participants