holoviz / param

Param: Make your Python code clearer and more reliable by declaring Parameters
https://param.holoviz.org
BSD 3-Clause "New" or "Revised" License
410 stars 69 forks source link

`name` of a Parameterized instance overridden when is the default of a `readonly/constant` Parameter #937

Closed maximlt closed 1 month ago

maximlt commented 2 months ago
import param

class A(param.Parameterized):
    x = param.Number()

a1 = A(name="FOO1")
a2 = A(name="FOO2")

class B(param.Parameterized):
    const = param.Parameter(a1, constant=True)
    ro = param.Parameter(a2, readonly=True)

b = B()

print(f'{b.const.name=}')
print(f'{b.ro.name=}')

Param 1.13:

b.const.name='A00005'
b.ro.name='FOO2'

Now:

b.const.name='A00005'
b.ro.name='A00006'

The name of a Parameterized instance set as the default of a readonly Parameter is unexpectedly overridden. Bisecting pointed at https://github.com/holoviz/param/pull/776 that stopped automatically setting instantiate to True when constant is True.

However, I'm also surprised to see the name being updated for a constant Parameter! As currently the default value of a Parameter is deep copied only when instantiate=True. So in the example above, instantiate is always False, there's no deep copy ofa1ora2`.

https://github.com/holoviz/param/blob/dae8fc4a7b47079468ad5da0b486a779e963351b/param/parameterized.py#L1922-L1926

So the change I'd suggest would be to override the Parameter name only when a deep copy is required which doesn't break any test and would lead to this output for the example:

b.const.name='FOO1'
b.ro.name='FOO2'

The piece of code in Parameterized._instantiate_param that overrides the name of a Parameterized class on instantiation originates from the first commit on Github. One could argue that, even when deep-copying, overriding name isn't correct and could be left as a responsibility of the users implementing their Parameterized classes.


Did I already say I'd like name to be removed from Param? :)

jbednar commented 2 months ago

Did I already say I'd like name to be removed from Param?

That's a different issue. Maybe Parameterized can be renamed to ParamObject and its name handling code moved into a new subclass now named Parameterized. That way downstream libraries can choose whether or not they wish their objects to have names, (e.g. from param import ParamObject as Parameterized) making it easier to retain backwards compatibility while removing the special name handling in practice. Feel free to file a separate issue and/or PR!

Meanwhile, for this specific issue, I do agree the behavior is both surprising and wrong. The proposed fix seems better, but my brain starts to overheat if I try to think of whether it truly addresses the issue. I'd ask another Param developer like @philippjfr .

philippjfr commented 1 month ago

Maybe Parameterized can be renamed to ParamObject and its name handling code moved into a new subclass now named Parameterized.

Agreed, although not 100% aligned on the naming.