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

Observer pattern #6

Open
davidbrochart opened this issue Feb 17, 2023 · 14 comments
Open

Observer pattern #6

davidbrochart opened this issue Feb 17, 2023 · 14 comments

Comments

@davidbrochart
Copy link
Member

In jupyter-widgets/ipywidgets#3293 (comment) @tlambert03 mentioned psygnal, that could be used to implement the equivalent of traitlets.
I think it is a good idea, but I'm not sure how it could fit here. For instance, take the Switch model, currently implemented as:

from .ypywidgets import Widget


class Switch(Widget):

    def __init__(self, value: bool = False, open_comm: bool = True) -> None:
        super().__init__(name="switch", open_comm=open_comm)
        self.yvalue = self.ydoc.get_map("value")
        self._set(value)

    def _set(self, value: bool) -> None:
        with self.ydoc.begin_transaction() as t:
            self.yvalue.set(t, "value",  value)

    @property
    def value(self) -> bool:
        return self.yvalue["value"]

    @value.setter
    def value(self, value: bool):
        if value == self.value:
            return

        self._set(value)

    def toggle(self):
        self.value = not self.value

Its YDoc consists of a YMap named value, with a single entry in it, also named value, which contains the boolean value of the switch. Here the observer pattern is manually implemented using a getter and a setter function for the value attribute.
If we were to use psygnal, we would need a way to point to the YDoc's structure (self.yvalue and value entry in it) from the psygnal's attribute.
I think that's the reason why traitlets use Python's descriptor protocol, which allows to get the name of an attribute at runtime.
Also, I'm not sure how we could connect nested YDoc structures (e.g. a YMap can contain other Y structures), but I don't think it was possible with traitlets anyway, so that would be an improvement.

@tlambert03
Copy link

thanks for the pig @davidbrochart! here are some general thoughts that i hope will be helpful:

one of the main differences between traitlets and psygnal is that traitlets implements both the observer pattern and the dataclass pattern, while psygnal focuses mostly on carefully implementing the observer pattern, and wants to let the user pick their own modern dataclass pattern, though it does bring some in-library awareness of specific dataclass libs (currently dataclasses.dataclass, pydantic.BaseModel, attrs.define, and msgspec.Struct with the psygnal.evented decorator.

So, the first important question here is: what pattern will you use to declare what the mutable ("dataclass-like") fields are for any given model (i.e. how do we get away from the @property.setter for each field here) ... this is probably something that you'd implement in Widget itself (where you would also have something that iterates over fields and connects to ydoc with ydoc.get_map('field_name'). And then Switch would just be a dataclass-like declaration.

So, as an example here that omits the Comm object and only focuses on updating the YMap...

If you wanted to use pydantic as your dataclass structure, then you could use psygnal.EventedModel

from pydantic import PrivateAttr
from psygnal import EventedModel, EmissionInfo
import y_py as Y


class Base(EventedModel):
    _ydoc: Y.YDoc = PrivateAttr(default_factory=Y.YDoc)
    _ymap: Y.YMap = PrivateAttr()

    def __init__(self, **data) -> None:
        super().__init__(**data)
        self._ymap = self._ydoc.get_map("my_map")

        # do initial sync with ydoc
        with self._ydoc.begin_transaction() as t:
            self._ymap.update(t, self.dict())

        # events is a psygnal.SignalGroup, that contains a SignalInstance
        # for each field on the model. However, it can *also* be directly connect
        # to a callback, which will be called for *any* event on the model
        # with the EmissionInfo object passed as the first argument.
        self.events.connect(self._on_event)

    def _on_event(self, info: EmissionInfo) -> None:
        # called anytime any field changes
        field_name = info.signal.name
        # info.args is a tuple of the arguments "emitted"
        # in the most common case of a single argument, we unwrap it here.
        new_val = info.args[0] if len(info.args) == 1 else list(info.args)
        with self._ydoc.begin_transaction() as t:
            self._ymap.set(t, field_name, new_val)


class Switch(Base):
    value: bool = False

    def toggle(self):
        self.value = not self.value
In [3]: s = Switch()

In [4]: s._ymap
Out[4]: YMap({'value': False})

In [5]: s.dict()
Out[5]: {'value': False}

In [6]: s.toggle()

In [7]: s._ymap
Out[7]: YMap({'value': True})

In [8]: s.dict()
Out[8]: {'value': True}

or, implementing the same thing with dataclasses.dataclass instead of pydantic:

from psygnal import evented, SignalGroup, EmissionInfo
from dataclasses import dataclass, asdict
import y_py as Y


class Base:
    if TYPE_CHECKING:
        # this is what @evented adds
        # the typing here is a little funny as a super-class
        # but that's another conversation :)
        events: SignalGroup

    def __post_init__(self):
        # same thing that __init__ did above.  # this could go in a base class
        self._ydoc = Y.YDoc()
        self._ymap = self._ydoc.get_map("my_map")
        with self._ydoc.begin_transaction() as t:
            self._ymap.update(t, asdict(self))
        self.events.connect(self._on_event)

    def _on_event(self, info: EmissionInfo) -> None:
        # same as above
        field_name = info.signal.name
        new_val = info.args[0] if len(info.args) == 1 else list(info.args)
        with self._ydoc.begin_transaction() as t:
            self._ymap.set(t, field_name, new_val)


@evented
@dataclass
class Switch(Base):
    value: bool = False

    def toggle(self):
        self.value = not self.value
In [3]: s = Switch()

In [4]: s
Out[4]: Switch(value=False)

In [5]: s._ymap
Out[5]: YMap({'value': False})

In [6]: s.toggle()

In [7]: s._ymap
Out[7]: YMap({'value': True})

In [8]: s
Out[8]: Switch(value=True)

let me know if that helps a bit, and/or where pain points still exist or arise

@tlambert03
Copy link

an additional note: the main thing that the psygnal.evented does is

  1. determine what flavor of dataclass we're working with so we can iterate over the fields (source here)
  2. builds a psygnal.SignalGroup (named 'events' by default) that contains a SignalInstance for each field in the dataclass
  3. overrides __setattr__ on the class (just like you did in pyceptive) to emit an event from the corresponding field on the signal group.

@davidbrochart
Copy link
Member Author

That looks awesome!
I guess we would probably choose Pydantic, because it's great (and fast) at data validation, but maybe the other libraries do that too?
I will play with the code you've posted, thanks @tlambert03 !

@tlambert03
Copy link

tlambert03 commented Feb 17, 2023

yeah, I love pydantic too, and it seems poised to only get better with v2.

For this particular application (with a lot of serialization and deserialization), i think msgspec also looks very promising: it really specializes in serialization and speed (though perhaps rust-pydantic will match it?). It also does validation, but i don't think it does the same degree of data parsing & coercion that pydantic does. And, i don't know, maybe for your application y-py is already taking care of all your serialization needs? and one double-edged-sword of msgspec is that it's implemented at a rather low level, and it can be a bit difficult to use as a base class if you're looking to do a lot of customization (for example, I don't think you can override __init__ at the moment: jcrist/msgspec#70)

@jcrist
Copy link

jcrist commented Feb 17, 2023

Hi! Hope you don't mind me chiming in here.

though perhaps rust-pydantic will match it?

The last time I benchmarked msgspec against pydantic v2, msgspec was between 7 and 20x faster at decoding. Whether that matters is definitely application specific. I'd also expect with the recent 4.5 million they got that they'll eventually narrow that gap, but I'd be surprised if they're ever measurably faster. At this point msgspec is mostly bottlenecked by cpython allocation costs that can't be avoided by any tool.

it can be a bit difficult to use as a base class if you're looking to do a lot of customization

Please open an issue about any rough edges you find! I definitely want to make msgspec as easy to use as possible. Happy to help!


Anyway, please don't feel pressured at all to use msgspec here (or elsewhere). All of these tools have their places and tradeoffs, and whether msgspec's performance improvements matter is definitely application specific.

@tlambert03
Copy link

hey @jcrist! Don't mind at all, very nice to see you here :) love msgspec!

Please open an issue about any rough edges you find!

I would love to discuss the custom init thing a bit. I saw your very reasonable question on jcrist/msgspec#70 asking "what would you use post_init for" ... and my internal answer to myself was so broad (along the lines of "erm ... any custom stuff?") that I didn't think it would add much to the conversation 😂

Generally speaking, I'd like to just be able to do something after initialization of Struct... would you like me to post to that issue? or make a new one?

@jcrist
Copy link

jcrist commented Feb 17, 2023

Thanks! And that issue was misusing __post_init__ for something that would be better handle elsewhere; if you have a real use case for __post_init__ I'd love to hear about it! Please open a new issue where we can discuss further :).

@davidbrochart
Copy link
Member Author

@tlambert03 I've been using psygnal in ypywidgets, mostly copying your code above, and it works fine except that sometimes I get an error:

EmitLoopError('calling <built-in method _slot_relay of SwitchSignalGroup object at 0x7f9c26a74c20> with args=(False,) caused EmitLoopError in emit loop.')

Do you know what could go wrong?

@tlambert03
Copy link

tlambert03 commented Feb 21, 2023

it means that there was an exception in your callback function (but it's not an informative enough message)...
the current dev version of psygnal (and the next release) will have a better error message that shows the text of exception that occurred.

easiest approach for now might be to install from github for the moment?
pip install git+https://github.com/pyapp-kit/psygnal

@davidbrochart
Copy link
Member Author

I opened #7, if you want to take a look. I suspect there is an infinite loop?

easiest approach for now might be to install from github for the moment?

OK, I'll try that.

@tlambert03
Copy link

tlambert03 commented Feb 21, 2023

also ... look higher up in the stack trace. is the cause of the exception printed in the penultimate stack? (with a "The above exception was the direct cause of the following exception:" message)

@davidbrochart
Copy link
Member Author

I run in an environment where it's not easy to see what's happening, but by printing the traceback when the exception is raised I saw that it's a y-py issue, so not related to psygnal. Thanks!

@tlambert03
Copy link

good to know, thanks! I'll get that version out with the better traceback soon

@tlambert03
Copy link

side-note: by all means let me know if you run into any other pain-points with psygnal or EventedModel specifically

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

No branches or pull requests

3 participants