widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.9k stars 140 forks source link

Event not firing when reactive variable is set #245

Open dhirschfeld opened 1 year ago

dhirschfeld commented 1 year ago

I'm trying to create a solara=1.19.0 component in jupyterlab=4.0.5 with ipywidgets=8.1.0 (on Windows)

I have from_date = sol.use_reactive(cast(Optional[str], None)) which will be set to (a string repr of) a date.

I have a Select widget which uses this reactive variable and triggers a cb_from_date callback:

sol.Select(
    label="Effective Date",
    value=from_date,
    values=effective_dates,
    on_value=cb_from_date,
)

When I first set the value of the reactive variable to_date.value = effective_dates[0] everything works as intended - a cascade of callbacks/events trigger from other (reactive) variables being set :tada:

What I'm observing is that when I update the value to_date.value = effective_dates[0], the cascade of callbacks/events aren't being triggered if effective_dates[0] is the same as it was previously - i.e. the callback isn't being triggered if to_date.value == effective_dates[0]

The effective dates are being updated from a database, but it's very often that effective_dates[0] won't change. Even if effective_dates[0] hasn't changed, I still want all the callbacks/events to fire as other things have changed and I want the state of the app to be updated.

Does that make sense?

Is this behaviour intended, or a bug? If intended, is there some way to ensure the callback gets triggered whenever the reactive variable is set, irrespective of whether it is being set to the same value it has already?

giswqs commented 1 year ago

It might be related to this https://github.com/jupyterlab/jupyterlab/issues/14829

dhirschfeld commented 1 year ago

I confirmed this behaviour by initially setting the variable to None before setting it to the correct value:

to_date.value = None
to_date.value = effective_dates[0]

I could make this work by putting a if value is None: return in the callback. I also confirmed by appending a hash to the date and stripping it in the callback. In both cases the callbacks were triggered correctly.

rob-vh commented 3 months ago

I have a similar observation in py.cafe This tries to react to changes in a dict. When I assign a new value to the dict and store it in the reactive, the first store triggers a render, the 2nd and further updates do not. If I first store None in the reactive, then stores trigger a render.

Is this bug related to the method used to detect differences in the old value and new value,e.g., does not support the data type and silently continues due to a try: except: with no qualification? Or is the old-value stored by reference, not by value, so changes to the dict also update the old-value?

Related to issue 680?

rob-vh commented 3 months ago

I have not traced this through, but set() for KernelStore toestand.py seems to improve performance by testing if the new value is the same as the old value

    def set(self, value: S):
        scope_dict, scope_id = self._get_dict()
        old = self.get()
        if equals(old, value):
            return
        scope_dict[self.storage_key] = value

are we sure that old is a copy of the old value, because otherwise we're comparing an object with itself (in my example of storing a dict)?

maartenbreddels commented 2 months ago

Hi,

reactive variables store a reference to the value you pass in, so if you mutate them, object equality and object identity will make it impossible for solara to know that something has changed.

Both problems:

Can be worked with using some kinda of wrapper object that also stores a version. I've demoed it here, together with possible pitfalls:

https://py.cafe/maartenbreddels/solara-reative-and-mutable

Let me know if this solves your problems.

rob-vh commented 2 months ago

@maartenbreddels I love how you've made my store sample work. But I wonder if toestand.py could be improved by storing the (old) value of the object as a shallow copy.

maartenbreddels commented 2 months ago

Mutation will always be considered bad practice, but we will (in dev mode for sure, probably not in production mode) make a deep copy (although probably configurable via code) to detect these user errors and give a proper warning if we notice a mutation, see: https://github.com/widgetti/solara/pull/595

What do you think of that?

rob-vh commented 2 months ago

Yup, that would help programmers, esp. when the 2 work-arounds are documented (linked from the warning message?).

maartenbreddels commented 2 months ago

Yes, the plan is to have the error message be self-documenting/solving or link to an explanation.