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

Ensure class watchers are not inherited by instance parameter #833

Merged
merged 6 commits into from Sep 16, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Sep 15, 2023

In #826 we discovered some weird logic that was copying over watchers from class to instance parameters. After discussing this we all agreed this does not make sense, this is because slots are not shared between class and instance parameters and therefore it does not make sense that an instance parameter would inherit the watchers from a completely different Parameter instance. Worse yet, once an instance was created the watcher would be removed from the class which means that modifying the class Parameter slot value would no longer trigger the watcher.

The correct behavior is actually quite simple, watchers should not be copied over, instead the Parameter slot watchers should be reset when the instance parameter is created. This ensure that while the instance still shares the Parameter object with the class it correctly fires if a slot on the class Parameter is changed and when an instance parameter is created only newly created watchers on the instance will be watched. Since calling inst.param.watch will automatically create instance parameters for any parameters that are watched this means that the behavior will be correct in all cases both instances and classes will always correctly trigger the appropriate watchers.

Fixes #829

# Reset watchers since class parameter watcher should not execute
# on instance parameters
p.watchers = {}

Copy link
Member

Choose a reason for hiding this comment

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

That's a very pleasing simplification! Hope it will work well in practice.

@philippjfr
Copy link
Member Author

philippjfr commented Sep 15, 2023

So it turns out the copying was effectively a hack to allow watchers that are set up during instance creation (i.e. primarily ones set up using @param.depends on a method) to be copied over after the instance is fully set up. I'm going to revert my latest changes which were forcing the creation of an instance parameter even before the class was fully initialized since it can potentially cause bad side-effects should someone try to set up a watcher before they call super(Parameterized self).__init__. Instead I will declare the instance initialized before we call _update_deps which is what is responsible for setting up the dependencies. This will ensure that watchers will be correctly installed on the instance parameter rather than the class parameter.

param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Sep 16, 2023

I ran Panel and HoloViews unit test suites successfully against this branch. Merging, thanks!

@maximlt maximlt merged commit c087fb9 into main Sep 16, 2023
10 checks passed
@maximlt maximlt deleted the cls_param_slot_watchers branch September 16, 2023 08:46
@holoviz holoviz deleted a comment from maximlt Sep 16, 2023
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.

Class-level watchers and copying watchers behaviors
3 participants