holoviz / holoviz/param

Eliminating `_update_state`?

Open
#830 13 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
521
Forks
86
Avg merge
1d 13h
Merged PRs (30d)
35

Description

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](https://github.com/holoviz/param/pull/605) 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.