Eliminating `_update_state`?

[jbednar]

https://github.com/holoviz/param/pull/794 introduced a new Parameter._update_state method that runs after the Parameter is installed into a Parameterized, to support any Parameter-specific computations that cannot complete until then.

Adding this mechanism was required once we used the Sentinel approach (param.Undefined) to provide systematic support for inheritance of Parameter slot values, including slots that depend on the Parameter value, since that value might be inherited and thus cannot be fully determined until the Parameter is installed into the Parameterized class hierarchy that owns it. I.e., first the constructor for a Parameter completes, then it gets installed into a Parameterized, and only then can the values of any Undefined slots be known.

It's difficult to trace all of this through and I keep forgetting the details, so I'm filing this issue to capture for posterity precisely what we would lose if we did not have _update_state. Specifically, we would lose the ability to have a computed default for the Selector.check_on_set slot. A Selector can either have a fixed list of allowable items, or it can be a handy way to keep track of all the previous items used so far (e.g. for populating a Selector widget in a GUI), and check_on_set determines which of those behaviors will be provided. Most people don't currently have to set the check_on_set slot explicitly, because when they provide an explicit objects slot value, check_on_set defaults to True , and otherwise check_on_set defaults to False. I.e. if we have objects, assume those are the only objects allowed, and if we don't, assume we're just collecting values.

That's all easy enough to implement, but the tricky bit is that when we do want to collect values, we need to collect the initial default value as well, and that value is not necessarily available until after slot inheritance, which is done after the constructor completes. So @maximlt implemented _update_state to provide a hook for recording the fact that the default value needs to be put onto the objects list/dict, but not actually doing it eagerly in the constructor because (a) the value isn't actually known yet, and (b) even if we do know the value (e.g. when it's provided explicitly in the constructor call), populating it eagerly will break the logic for the check_on_set default value, because the objects list/dict will appear to have been populated by the user.

So, what would it take to eliminate this quite hairy logic? I think the results would be an annoyance for users and a compatibility issue, but they would have been reasonable policies to have if we'd done them from the start. Specifically, we'd:

• Require a user of Selector to specify check_on_set explicitly. It would have to default to a simple True for safety, to ensure that checking is done when expected. If a user doesn't want checking, they'd have to explicitly change check_on_set to False, rather than simply not supplying objects, or they will get an exception.
• If we do that, we should probably make a conceptually similar change to Parameter.allow_None. allow_None normally defaults to False, but it is automatically updated to True if the default value (whether inherited or not) is None. This is a convenience that reflects the fact that most uses of a None value are for a default, awaiting a user filling in a real value later, which means that right now almost no user ever needs to specify allow_None explicitly. With this change, users would be required to specify allow_None=True explicitly whenever they use a default of None, or they will get an exception.

If we went with these more explicit but less friendly policies, we'd eliminate _update_state (about 10 lines of mysterious code), __compute_selector_checking_default (8 lines), and _set_allow_None and related allow_None defaults handling (12 lines). 30 lines of code isn't much, but it's all code that is deeply confusing to trace through, so as a programmer I'd be very happy to see it go. Plus as a user it's much more explicit and easy to reason without it; I just have to deal with the exception when it's raised, and it should be obvious what to do.

Still, the current behavior is convenient, and making such a change now seems like it would disrupt quite a lot of existing code (anything using Selectors without explicit objects lists or any Parameter with a default value of None). Does anyone think it's worth the disruption? Moving to 2.0 as we are currently doing seems like the only possible chance for such a disruption, if we have a consensus behind it.

[maximlt]

Thanks for the great write-up! It's an exact recollection of what happened and why I had to add _update_state, to deal with dynamically computed Parameter attributes in an inheritance context.

I'd be in favor of requiring users to specific check_on_set and making that change now before Param 2.0:

• check_on_set isn't used so much, the effect of this change isn't going to have a large impact
• a Selector without any objects value and an implicit check_on_set=False is a bit of a weird case, as this disables validation entirely. I actually suspect some users start with an empty list of objects and populate it afterwards (e.g. from a database query), without realizing that check_on_set has automatically been set to False.

Dealing with allow_None is a lot more controversial, having it set automatically when default=None is quite handy and is used all over the place, with dozens and dozens of occurrences in HoloViews and Panel. I would actually not mind having to be explicit, I'm on the fence though about making such an impactful change without any deprecation period.

To complete the list of attributes than can be dynamically computed, there's alsoTuple.length from the length of the default value, unless it's None in which case it must be provided explicitly.

[jbednar]

I think the Tuple length imputation is safe and reasonably clean, so it sounds like @maximlt votes for changing check_on_set to have a fixed default of True, but to keep allow_None imputation as it is. Ok by me; my concern is about disrupting current usage, and if representatives of Panel and HoloViews are ok with that, ok by me! @philippjfr ?

[philippjfr]

Thanks for the detailed writeups both of you. I fully agree with @maximlt's summary and agree that making check_on_set be an explicit setting is fine but allow_None would be far too disruptive. My only other comment is that I really don't understand the naming of check_on_set and I wish we could call it something more sensible.

[jbednar]

Ok, sounds good. That option controls whether a given value is validated against the list of allowed values, i.e., it checks that the value is allowed when someone sets the value. Suggestions for another name, now that people will need to learn about this option?

[philippjfr]

Not sure I have anything good. In the Panel context this will line up with Autocomplete and MultiChoice widgets which allow either restricting the options or adding new options. There it's called restrict but that isn't really clear enough in this context, restrict_objects could be okay, add_on_set is slightly clearer than check_on_set but also not great, add_unseen is clearer but I don't like it. I got nothing good.

[jbednar]

That's an argument for keeping the status quo, but if anyone does have a really clear name, we can add that as an alias when we come up with it and make our docs focus on that instead.

[philippjfr]

Agreed, my vote is to take no action on renaming check_on_set or any major reworking of _update_state for the 2.0 release.

[jbednar]

We're agreed about keeping the name of check_on_set, but my understanding is that above you were agreeing with simplifying its semantics, and removing _update_state is a consequence of that simplification. I.e. _update_state exists only to achieve those semantics, and we would not need preserve it otherwise. And as _update_state has not ever been released, now is the time to delete it, if we are going to delete it. Unless I got confused somewhere?

[philippjfr]

No my misunderstanding, that all sounds fine. In the end _update_state is just an implementation detail though so no opinion whether to remove it, if it gets eliminated by making those changes then that's good too.

[maximlt]

I'm going to try to remove _update_state now, making check_on_set static. I wouldn't be so sure that it is the only reason for _update_state to exist. Getting the Selector Parameter, and its subclasses, to work in an inheritance context is definitely not easy. _update_state is an undocumented private method, if I don't manage to get rid of it in a reasonable amount of time, I'm fine releasing Param 2.0 with that, since anyway the release notes won't mention it and it can be removed anytime. Of course, the sooner the better if it's meant to be removed.

[jbednar]

It's undocumented, but if it's required for any user-visible behavior, better for us to find that out now!

[maximlt]

Also, I get the idea to clean up some method if we can. But providing a way to update the state of a Parameter doesn't sound too bad to me. Parameters could all be lazily created and be finalized when bound to a Parameterized class.

[jbednar]

Sure. On balance if we don't need that I want to prioritize someone being able to read Param's code and understand what happens when, which is quite difficult given the Parameter itself, its metaclass, Parameterized, and its metaclass, so anything that can be omitted from that control flow is a win.