holoviz/param

Selector regression in an inheritance context?

maximlt opened this issue · 3 comments

There's maybe a regression on the main branch brought by the sentinel PR, that happens when setting a Selector value on an instance of a subclass and that leads to the objects of the super class being updated.

Here's the most basic example:

class A(param.Parameterized):
    p = param.Selector()

class B(A):
    p = param.Selector()


b = B()
b.p = 2

# Before [], now [2]
print(A.param.p.objects)

And maybe a more realistic example:

import param

class A(param.Parameterized):
    p = param.Selector(objects=[1, 2], check_on_set=False)

class B(A):
    p = param.Selector(default=2)


b = B()
b.p = 3

# Before [1, 2], now [1, 2, 3]
print(A.param.p.objects)

That is probably again a problem of objects being a mutable object, this time being shared across a class and all of its subclasses and their instances. I come to wonder whether objects accepting a mutable object (list/dict) is actually a good idea, and if instead it wouldn't be best if it'd accept a factory function. cc @jbednar to gauge how blocking this is for the release of Param 2.0

When per_instance=True leads to copying the Parameter object, can we make it do a copy (not a deepcopy) of any mutable slot values (or all slot values, which should have the same result)? That seems cheap enough and preserves object identity for the items in the Selector while avoiding this problem.

The only mutable types I can see in slot values are all containers (_objects and names here, plus attribs for Composite, columns potentially on DataFrame, and search_paths on Path), so maybe we could shallow copy only containers, since arbitrary class instances aren't necessarily safe to copy if some Parameter did have a slot with such an item. For current Parameter types I think shallow copying everything, all mutable values, and containers would have the same result, so it may not matter much.

I'm pretty sure that for all those cases, sharing the contents of the container across separate Parameter objects would be seen as a bug and not expected behavior. I.e., if the user wanted independent Parameter objects, they will want independent containers, because if they wanted them all to share the same values they wouldn't have used per_instance parameters.

When instantiate=True leads to copying the Parameter object, can we make it do a copy (not a deepcopy) of any mutable slot values (or all slot values, which should have the same result)?

Yes there's nothing that prevents us from doing that when instantiate=True. However that doesn't resolve neither this issue nor #746 as they deal with the Selector Parameter which is set by default with instantiate=False, right?

I've corrected my message above to say per_instance where I had written instantiate; I always get those two confused. per_instance is the relevant one here, which controls instantiation of the Parameter object per instance, not instantiation of the default value into a current value (instantiate=True).