2.0.0rc6 - param.ListSelector Issue

jgriessler commented 9 months ago

ALL software version info

param.2.0.0rc6 (and older 2.0 rc)

Description of expected behavior and the observed behavior

param.ListSelector with check_on_set=False incorrectly updates the .objects when assigning an existing or new value to the parameter. It adds the value as a list to the end of the .objects, even if it already exists. Works fine on 1.13.0.

Complete, minimal, self-contained example code that reproduces the issue

import param

class Test(param.Parameterized):
    mylist = param.ListSelector(objects=['a', 'b', 'c'], check_on_set=False,)

mytest = Test()
mytest.mylist = ['a']
print(mytest.param.mylist.objects)
['a', 'b', 'c', ['a']]

jgriessler commented 9 months ago

I compared the 1.13 and 2.0.rc versions - looks like the ListSelector validation stuff has been restructured and the problem is that the ListSelector uses the Selector._ensure_value_is_in_objects() method although that expects a value, not the list of values (hence it doesn't find a match and just appends the list at the end).

jbednar commented 9 months ago

Damn! I could swear we had already addressed that issue while preparing 2.0. Unfortunately I am travelling and cannot easily look into this soon, but hopefully @maximlt can spot the issue and address it (and add another test!).

maximlt commented 9 months ago

Hi @jgriessler ! Thanks for trying out the release candidate and reporting this bug, that I can reproduce. We indeed thought we fixed it in https://github.com/holoviz/param/pull/817 but not entirely apparently. Definitely a release blocker!

To be honest, we didn't really expect anyone to exert the release candidates! Out of curiosity, and only if you don't mind, I'd have a couple of questions for you. Are you a Panel user trying out the latest version? What's your use case with ListSelector(check_on_set=False) ?

jgriessler commented 9 months ago

Yes, using latest Panel. The app is just some private fun stuff, not mission critical, though use param quite a bit (and panel for driving the UI, bokeh mostly for timeseries charts, holoviews, hvplot for adhoc exploration (and pandas, xarray for the data).

Most if not all of the many classes are based on param.parameterized (I really like the realtime checking, watch-triggers and automatic widget creation via panel etc).

What I use params for (besides real-time checking and automatic widgets):

lots of dependencies (mostly via @param.depends, but also some via param.watch, less often via pn.bind) to refresh data, UI, charts ... and some basic workflow pipelines
storing/reloading states/configurations/setups of classes via param json serialization
parameters I often dynamically adjust options etc on Selectors or only define them after init/startup (eg. if the User configures a new UI Tile or creates a new screener plot layout or filter, ..)

Everything seems to still work after the upgrade to 2.0.0rc5 (and rc6) with the exception of the following use case:

my screener class gets data based on a list of filters that is dynamically allocated based on the data-schema given for the data to be accessed. What data to access and any predefined filters+values is loaded in from .json param serialized setup file saved by the user previously.

So the list of filters/fields is Unknown at class instantiate, means ListSelector.objects is empty (and hence check_on_set=False automatically) at that time. Only once I load the setup I adjust the .objects and load any pre-selected values from the setup as well.

maximlt commented 8 months ago

Thanks a lot for these details @jgriessler, that's good feedback, we don't really know how people use Param and I'm happy to see you're using in exactly the same way we do!

I've found a fix for the issue you reported that will be released in Param 2.0.0rc7 soon.

On your use case with ListSelector.objects being empty on instantiation, the logic might also be approached this way (not quite sure this fits to your use case but that can be helpful to others anyway):

import param

class A(param.Parameterized):
    ls = param.ListSelector()

    def __init__(self, **params):
        super().__init__(**params)
        self.param['ls'].objects = list(range(5))
        # check_on_set automatically set to False if
        # objects is empty on Parameter instantiation
        self.param['ls'].check_on_set = True
        self.ls = [0]

a = A()
a.ls

jgriessler commented 8 months ago

Thanks for the suggestion, funny enough, I realized yesterday after reading through the #817 and the other PRs referenced there that I should actually set .check_on_set back to True after filling in the .objects. If I hadn't left that gap open, I might not have stumbled across the bug at all :-)

Reading through the older PR I realized that the whole handling of the ListSelector is actually more complex than I thought, quite different use-cases, especially in combination with Panel. I do sometimes come across the use-case mentioned in #807 where I want the user to select something out of A, B, C, D but also start with a '' to "force" the user to select something (and being able to check if he selected something or trigger a callback on change)

maximlt commented 8 months ago

We recently discussed preventing check_on_set to be automatically set, except that we had too much of our code that broke with this change and decided if this was done it'd have to go through a deprecation cycle to warn users about that change.

Indeed, the whole handling of ListSelector and Selector in general is complex, way too complex in my opinion :D

holoviz / param