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

Do not trigger event if identity of parameter value is unchanged #901

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

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jan 11, 2024

Seems like a crazy oversight, not all objects can be compared for equality so we should always check for identity first. The current behavior was added in #279, but it does not provide any arguments for why we would want to trigger events if the object identity is unchanged.

@philippjfr philippjfr changed the title Make Comparator check for identity Do not trigger event if identity of parameter value is unchanged Jan 11, 2024
@jbednar
Copy link
Member

jbednar commented Jan 11, 2024

It certainly sounds safe to add this check.

@maximlt
Copy link
Member

maximlt commented Jan 11, 2024

I thought we didn't compare for identity because of mutable/complex objects. Take the code below, right now the debug callback is called. With your changes, it no longer is.

import param

class Dummy: pass

dummy = Dummy()

class Foo(param.Parameterized):
    d = param.ClassSelector(class_=Dummy)
    
    @param.depends('d', watch=True)
    def debug(self):
        print('updated d')

foo = Foo(d=dummy)
dummy.changed = True
foo.d = dummy  # triggers `debug` now, not with this PR

We could decide that's the right course of action, if so, I don't think it should be released in a patch release.

@philippjfr
Copy link
Member Author

philippjfr commented Jan 11, 2024

Thanks, that's a good point. Personally I don't think this was intentional but I appreciate that it's a significant change in behavior. For mutable objects the correct way to trigger an event should always be to explicitly .param.trigger an event. That is because if we ever did register an equality comparison for a particular type of object then the event would no longer get triggered because the equality check will always pass if the object has the same identity.

@philippjfr
Copy link
Member Author

philippjfr commented Jan 11, 2024

As an example to illustrate this point, the two mutables that we do have equality comparisons defined for (list and dict) do not trigger an event:

import param

class Foo(param.Parameterized):
    d = param.List()
    
    @param.depends('d', watch=True)
    def debug(self):
        print('updated d')

mutable = []

foo = Foo(d=mutable)
mutable.append(1)
foo.d = mutable # does not trigger `debug`

@philippjfr
Copy link
Member Author

The docs about trigger also make this point:

Usually, a Watcher will be invoked only when a parameter is set (and only if it is changed, by default). What if you want to trigger a Watcher in other cases? For instance, if a parameter value is a mutable container like a list and you add or change an item in that container, Param’s set method will never be invoked, because in Python the container itself is not changed when the contents are changed. In such cases, you can trigger a watcher explicitly, using .param.trigger(*param_names).

@maximlt
Copy link
Member

maximlt commented Jan 11, 2024

Yes, your points make sense to me.

We often see Param users, and in particular Panel users using Param, asking why updating their list/dict doesn't execute their callbacks. Expanding this behavior to "complex" (there's got to be a better name for this) will need to be carefully documented (with a big warning!).

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

Successfully merging this pull request may close these issues.

3 participants