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

2.0.0rc6 - param.ListSelector Issue #872

Closed
jgriessler opened this issue Oct 6, 2023 · 7 comments · Fixed by #874
Closed

2.0.0rc6 - param.ListSelector Issue #872

jgriessler opened this issue Oct 6, 2023 · 7 comments · Fixed by #874
Labels
type-bug Bug report
Milestone

Comments

@jgriessler
Copy link

jgriessler commented Oct 6, 2023

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
Copy link
Author

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
Copy link
Member

jbednar commented Oct 7, 2023

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 maximlt added this to the 2.0 milestone Oct 8, 2023
@maximlt maximlt added the type-bug Bug report label Oct 8, 2023
@maximlt
Copy link
Member

maximlt commented Oct 8, 2023

Hi @jgriessler ! Thanks for trying out the release candidate and reporting this bug, that I can reproduce. We indeed thought we fixed it in #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
Copy link
Author

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
Copy link
Member

maximlt commented Oct 10, 2023

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):

image
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
Copy link
Author

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
Copy link
Member

maximlt commented Oct 11, 2023

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

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

Successfully merging a pull request may close this issue.

3 participants