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: signal alias in field metadata #265

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

getzze
Copy link
Contributor

@getzze getzze commented Feb 8, 2024

solves #261
supersedes #260

Add a keyword argument to SignalGroupDescriptor to specify signal aliases (if the alias is None, no signal is emitted when changing the field). The signal aliases table is stored in the SignalGroup class.

Also allow for specifying per-field options for Signals using the metadata of the fields (different implementation in the different packages).

It solves #261 elegantly and makes subclassing more automatic:

from typing import ClassVar
from attrs import define, field
from psygnal import SignalGroupDescriptor, EmissionInfo


@define
class Model:
    events: ClassVar = SignalGroupDescriptor(
        signal_suffix="_changed", 
        signal_aliases={"_controller": None, "_name": "name_changed", "name": None},
    )

    _controller: str = field()
    _name: str = field(kw_only=True)

    @property
    def name(self) -> str:
        return self._name

    @name.setter
    def name(self, value: str) -> None:
        # Generate a unique name among siblings
        siblings = [self.controller]
        value = f"{value}_1" if value in siblings else value
        self._name = value

    @property
    def controller(self) -> str:
        return self._controller

m = Model("ctt", name="c")
@m.events.connect
def on_any_change(info: EmissionInfo):
    print(f"signal {info.signal.name!r} emitted {info.args}")

m.name = "ctt"; m.name
# >> signal 'name_changed' emitted ('ctt_1', 'c')

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: Patch coverage is 92.56198% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 99.57%. Comparing base (f6cebcb) to head (f5d4fa4).

Files Patch % Lines
src/psygnal/_dataclass_utils.py 91.00% 9 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #265      +/-   ##
===========================================
- Coverage   100.00%   99.57%   -0.43%     
===========================================
  Files           22       22              
  Lines         2009     2111     +102     
===========================================
+ Hits          2009     2102      +93     
- Misses           0        9       +9     

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

Copy link

codspeed-hq bot commented Feb 9, 2024

CodSpeed Performance Report

Merging #265 will degrade performances by 24.23%

Comparing getzze:alias (f5d4fa4) with main (f6cebcb)

Summary

❌ 4 regressions
✅ 62 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main getzze:alias Change
test_dataclass_group_create[attrs] 2.3 ms 2.6 ms -10.21%
test_dataclass_group_create[dataclass] 2 ms 2.3 ms -11.31%
test_dataclass_group_create[msgspec] 2.4 ms 2.7 ms -12.51%
test_dataclass_group_create[pydantic] 2.3 ms 3.1 ms -24.23%

@tlambert03
Copy link
Member

hey @getzze, thanks again for all your work on this. it's greatly appreciated.

I took a quick look yesterday and I like a lot of what I see, and also have a couple minor reservations that I want to think through a bit more (such establishing any new patterns for field-specific metadata, such as the Annotated pattern for pydantic2). But in general, I think you're right: it's a very flexible solution that solves most of the issues you've pointed out so far.

One other thing I'll mention is the library fieldz. I have a couple other libraries where I want to be able to support a bunch of different dataclass-like patterns, and fieldz grew out of that. I hadn't used it yet here to avoid an additional dependency, but with this PR, it might be time to consider using fieldz.fields() instead of the the iter_fields() method that i had, and which you've extended in this PR. (it's possible we'll need to make a PR or two to fieldz to make sure we're grabbing field-specific metadata in a way that works for psygnal too)

it's been a busy week here, so sorry that I haven't commented yet, but will have a deeper look soon. (In the meantime, don't worry too much about the mypyc compiled tests failing)

@getzze
Copy link
Contributor Author

getzze commented Feb 9, 2024

Now it fails only with compile=True, so I'm done :)

I didn't find any official way to add metadata to fields in Pydantic v2, but in the forums people seem to pass the metadata as a dict in the second argument of Annotated. It works but this mechanism was not thought to be used like that indeed: https://docs.pydantic.dev/latest/api/fields/#pydantic.fields.FieldInfo

It would be great to add the iter_fields_with_options function to fieldz, although as you said that would be an extra dependency.

Sorry for the large PR, it has all the part about metadata for individual fields but also the part about signal aliasing.
The signal aliasing mechanism could help solve #260 and #262 if SignalGroup is redesigned as a container with a Signal proxy attribute and a list of attached Signals. I'll explain better in #262

@@ -426,13 +536,15 @@ def __get__(
if instance is None:
return self

signal_group = self._get_signal_group(owner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do this to make it work, but I think it makes sense in general so signal_group is not recomputed for each new instance. This should not affect much performances as the call to _build_dataclass_signal_group was cached anyway.

@@ -372,7 +381,7 @@ def signals(self) -> Mapping[str, SignalInstance]:

def __len__(self) -> int:
"""Return the number of signals in the group (not including the relay)."""
return len(self._psygnal_signals)
return len(self._psygnal_instances)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should go to a separate PR

@getzze
Copy link
Contributor Author

getzze commented Mar 6, 2024

I rebased to main. I just realized that one part of this PR should have gone to v0.10 release, I can make a separate PR ([EDIT] #289 ).
Do you still think it makes sense to make SignalGroup a Mapping ?

About this PR, I restructured it to include 3 features:

  1. Skip a field: do not create the signal associated with it
  2. Alias a signal: a field called "name", the signal is called "name_changed"
  3. Do not trigger the signal when field is changed.

Maybe 3 is not really needed actually.

There is two ways of doing that:

  • defining dicts and lists of fields to skip and alias to pass as arguments to @evented or SignalGroupDescriptor
  • defining in the metadata of each field.

The metadata part can go to another PR (or even to fieldz as you already mentioned).

Another change brought by this PR is the collect_fields argument to SignalGroupDescriptor. Before, if you specified a signal_group_class to a SignalGroupDescriptor, the fields were not added to it. So you could not provide special signal names, not related to the fields of the dataclass, and at the same time automatically create the signals attached to the fields. With this PR, signal_group_class defines the base SignalGroup type (by default it is just SignalGroup) and specify if you want to also collect the fields with collect_fields. This could go in another PR ([EDIT] #291 ).

Tell me what you think about it when you have time 🙂

@getzze
Copy link
Contributor Author

getzze commented Mar 11, 2024

Once #291 is merged, I'll split this PR, first add the signal_aliases option, then add the metadata on the fields.

@tlambert03
Copy link
Member

Once #291 is merged, I'll split this PR, first add the signal_aliases option, then add the metadata on the fields.

sounds good! the metadata on the fields is the one i'm the most anxious about :) but we'll find something that works

@getzze
Copy link
Contributor Author

getzze commented Mar 11, 2024

sounds good! the metadata on the fields is the one i'm the most anxious about :) but we'll find something that works

Yes, I realized mypyc was not happy about the metadatas, so it will require more work.

adapt to SignalRelay
@getzze getzze changed the title Allow for signal aliasing feat: signal alias in field metadata Mar 22, 2024
@getzze
Copy link
Contributor Author

getzze commented Mar 22, 2024

With #299 merged, this PR adds parsing of field metadata to define aliases directly in the fields declarations.

I don't know if it's worth adding, maybe it's too much work to maintain (like keep up with the dataclasses-like librairies API changes). At least it should be low priority.
It also needs some work to make it work with the compiled version and to improve the speed.

@tlambert03
Copy link
Member

thanks for the update here @getzze, I do think i'm going to take you up on the offer to allow this to be low priority :)
i like the general idea... and indeed there are parts of the EventedModel config that would be better off as a Field() metadata rather than model metadata.

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.

2 participants