param icon indicating copy to clipboard operation
param copied to clipboard

Callback triggered when depending on a subobject not declared

Open maximlt opened this issue 2 years ago • 12 comments

From https://github.com/holoviz/panel/issues/3580

I didn't expect that setting the test_param attribute in the example below would trigger the callback, but it does.

import param

class Sub(param.Parameterized):
    s = param.String()

class P(param.Parameterized):
    test_param = param.Parameter()

    def __init__(self, **params):
        self._sub = Sub()
        super().__init__(**params)

    @param.depends('_sub.s', watch=True)
    def cb(self):
        print('cb called')

p = P()
p.test_param = 'should not trigger'
# 'cb called'

A simple fix consists in adding _sub = param.Parameter() to the Parameters of P.

maximlt avatar May 31 '22 22:05 maximlt

@jlstevens do you know if this pattern, i.e. depending on a subobject that is not one of the Parameters of the class but is supported by Param? I think it is as I've seen some usage of this in Panel but I'd like a confirmation if possible.

https://github.com/holoviz/panel/blob/120019e4318ac51bc2b9d0a1b2eb2239c8a0c9ad/panel/widgets/input.py#L891-L904

maximlt avatar May 31 '22 22:05 maximlt

@philippjfr tells me that people are using that behavior commonly "in the wild", but it wasn't part of the initial design as I ever understood it.

jbednar avatar Jun 01 '22 02:06 jbednar

Right, depending on a subobject's parameters is supposed to be allowed...as you point out, the weird thing to me here is that self._sub is not a parameter.

I dislike this behavior but if people are now relying on it, best not to change it imho.

jlstevens avatar Jun 02 '22 15:06 jlstevens

A simple fix consists in adding _sub = param.Parameter() to the Parameters of P.

In this case I don't see any reason not to declare _sub as a Parameter, because then you don't need to write an __init__ method at all. Plus it's then clear to anyone that _sub is available as a Parameter to be depended upon.

That said, if we do continue to support depending on attributes that are not Parameters, then I agree that the behavior illustrated is surprising and not at all desirable. I think it would take some debugging to figure out, e.g. by raising an Exception in the callback and tracing back to figure out how it got executed. My guess is that _sub.s is somehow being initialized in some way that is triggered by setting test_param, but I don't know why that would be. Will probably involve some head scratching!

jbednar avatar Jun 02 '22 18:06 jbednar

My intuition is that we get a mix of two things:

  • even if _sub is not a Parameter the callback is correctly registered on _sub.s with watch=True
  • methods in a Parameterized class automatically depend on all the Parameters if they're not decorated with depends, with the behavior of watch=False

We get a mix of these two things, so the callback is triggered on changes of _sub.s and any other declared Parameter value, with watch=True.

Total guess here, could be completely wrong.

maximlt avatar Jun 02 '22 18:06 maximlt

methods in a Parameterized class automatically depend on all the Parameters if they're not decorated with depends,

That doesn't seem to apply in this case, which does have depends.

jbednar avatar Jun 02 '22 19:06 jbednar

import param

class Sub(param.Parameterized):
    s = param.String()

class P(param.Parameterized):
    test_param = param.Parameter()
    _sub = Sub()

    @param.depends('_sub.s', watch=True)
    def cb(self):
        print('cb called')

p = P()
p.test_param = 'should not trigger'
# 'cb called'

same issue with the following code snippet. This used to work in an earlier version of param

timheb avatar Jun 03 '22 11:06 timheb

I think this behavior is related to _Undefined. If I set param.parameterized._Undefined = None at the start of the script, it doesn't call the cb function.

code
import param

param.parameterized._Undefined = None


class Sub(param.Parameterized):
    s = param.String()


class P(param.Parameterized):
    test_param = param.Parameter()

    def __init__(self, **params):
        self._sub = Sub()
        super().__init__(**params)

    @param.depends("_sub.s", watch=True)
    def cb(self):
        print("cb called")


p = P()
p.test_param = "should not trigger"

hoxbro avatar Jun 15 '22 21:06 hoxbro

I also saw a colleague of mine use a similar pattern as the first example.

I bacame aware of it because an app was slower than it should. I found out a function was run too many times. Took way too much time to figure out it was because the class was holding a Parametrized instance in an attribute not declared as a parameter.

MarcSkovMadsen avatar Jun 16 '22 05:06 MarcSkovMadsen

@philippjfr do you think the pattern I showed in the example should be supported or it happens to work by chance? i.e., the fact that one can use depends on a sub parameter whose parent has been added as an attribute in __init__, before the call to super().__init__?

I'm not very enthusiastic about it, it isn't particularly nice to explain (cf my last sentence...). Instead I'd rather enforce the fact that one can only depend on objects that have been declared at the class level.

maximlt avatar Jun 24 '22 09:06 maximlt

I'm also not enthusiastic about it but the fact that it worked means that we probably have a fairly large class of users relying on it. I think we have two options:

  1. Make it officially supported
  2. Add a very clear and descriptive warning that explains why you shouldn't do this and guide users to the correct usage

philippjfr avatar Jun 24 '22 09:06 philippjfr

Agreed. I was hoping to take a look at fixing up support for this case today, but if I don't manage it today, it will be some time before I can investigate.

jbednar avatar Jun 24 '22 09:06 jbednar

Indeed, it has been "some time", in this case nearly a year. :-/ I have no idea what my plan might have been in 2022, but I can verify that the incorrect behavior reported by @maximlt still applies. :-(

jbednar avatar May 12 '23 21:05 jbednar