Use sentinel value for inheriting parameter slots

[jbednar]

[Updated description to match the currently implemented version, not the original commits in this PR, for which see the history.]

Addressing the parameter inheritance issues at #97 and #113 by distinguishing between "not set by the user" and "set by the user" (including for None), using a sentinel value Undefined distinct from None and other valid values. Notes:

1. ParameterizedMetaClass.__param_inheritance(): Most Parameter constructor arguments are there to allow the users to specify the initial value of a slot (e.g. default or bounds). Previously, slot values specified as None in a Parameter constructor were inherited from parameters of the same name in superclasses, which fails when None is an allowed value, and also fails to inherit if default values are boolean or other non-None values. Now, inheritance checks for slot values of Undefined instead and if found inherits the value from superclass values.
2. Undefined: Nearly all constructor arguments for slot values have now been updated to have a value of param.Undefined, so that they will inherit from superclasses. This value was previously named _Undefined, used internally only, but now renamed since it's part of the public API.
3. Parameter._slot_defaults: Previously, slots with defaults other than None like 0.0 or (False,True) stored those defaults in the constructor's signature, but now that the signature shows only Undefined, we have to store those values somewhere else. Added a dictionary _slot_defaults to Parameter containing those values, used when inheritance "bottoms out" and no value has been specified by the user in a constructor at any level. Subclasses can extend this dictionary like _slot_defaults = dict_update(Parameter._slot_defaults, default=0.0, inclusive_bounds=(True,True)) to override or add new defaults. Several Parameter subclasses now no longer even need constructors, as the constructor previously existed solely to capture and declare those default values.
4. Parameter.__getattribute__: __param_inheritance is applied when a Parameterized object is constructed, filling in slot values from superclasses. Note that slots are also used in "unbound" parameters not (yet) owned by any Parameterized. In particular, the Parameter constructor must complete before the Parameter can be bound into its owning Parameterized class, and so any code in the constructor must be able to run even though the Undefined slot values have not yet been filled in with their values. Specifically, the constructors typically validate the various slot values, and this validation needs to complete for construction to succeed. Previous commits either deferred validation until binding time (which broke many tests that assumed validation happened immediately) or explicitly wrapped every access to a slot with code that checks for Undefined and substitutes the default (which was highly verbose and only worked for None default values anyway). Now, Parameter.__getattribute__ is overridden to catch Undefined values and look them up in _slot_defaults (with None if not found there), so that slot values can be Undefined to allow inheritance without the programmer or user ever having to reason about that value.
5. self.<slotvalue> vs. <constructorarg>. Previously, the various constructors referred fairly randomly either to the constructor argument (e.g. default) or the slot value once it has been put into the slot (e.g. self.default), since those were typically equal. The behavior of those two things now differs, when the value is Undefined -- default is Undefined, while self.default is None (or whatever default value was declared). So, each of the Parameter constructors has been updated as needed to use self.<slotvalue> so that there is no need to handle Undefined, or else left as <constructorarg> in the rare case when needing to check for Undefined explicitly.

Fixes #97 and #113.

With these changes, default values and nearly all other slots now respect None, which they did not do previously:

import param

class A(param.Parameterized):
    x = param.Parameter(5, doc="A.x_doc")
    y = param.Parameter(2, doc="A.y_doc")
    z = param.Parameter(2, allow_None=True)

class B(A):
    x = param.Parameter(doc="B.x_doc")
    y = param.Parameter(8)
    z = param.Parameter(None, allow_None=True)

b = B()
print(B.param.x.doc, B.param.y.doc)
b.x, b.y, b.z

(prior to this PR, the output would have been (5, 8, 2)). They also handle cases like Number where the default is non-None:

class A(param.Parameterized):
    x = param.Number(5, doc="A.x_doc")
    y = param.Number(2, doc="A.y_doc")
    z = param.Number(2, allow_None=True)

class B(A):
    x = param.Number(doc="B.x_doc")
    y = param.Number(8)
    z = param.Number(None, allow_None=True)

b = B()
print(B.param.x.doc, B.param.y.doc)
b.x, b.y, b.z

To do:

• [ ] Deal with the remaining 14 or so Parameter constructor arguments that still have default values other than Undefined (e.g. allow_None and instantiate) in Parameter, Selector, ClassSelector, List, MultiFileSelector, Event, etc.). For most of these, the final value for the slot is calculated in the constructor based on other slot values, and thus can't be captured as a single static default value in the _slot_defaults. I assume they'll either need another mechanism (calling a method for their value?) or to declare that they are not inherited.
• [ ] Should we verify compatibility between types across the inheritance hierarchy (#456)? Maybe we could enforce "is-a" in cases where the type changes (or maybe always?) by validating that the default value in the subclass is accepted as valid by the superclass, catching e.g. a List changing to an Integer, while still allowing types to change across the hierarchy when required (as in #456).
• [ ] Should we raise an error for a "default" value that ultimately inherited as Undefined? Currently will silently return None.
• [ ] Could add a mechanism to fix #1 and #574, about allowing a user to specify that a value is required, e.g. by defining a new sentinel Required for a user to use as the default value for a parameter and raising an error in Parameterized's constructor if that value is detected. I don't think that Undefined will work here, since most or all Parameters ultimately define a default value, and Undefined will eventually be replaced by that value.
• [ ] Add docs, including noting that parameter slot inheritance happens at Parameterized instantiation time.
• [ ] Add tests (e.g. starting from https://github.com/holoviz/param/issues/113)

[codecov-commenter]

Codecov Report

Merging #605 (20215a4) into master (fb6f4d9) will decrease coverage by 0.05%. The diff coverage is 97.22%.

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
- Coverage   81.95%   81.90%   -0.06%     
==========================================
  Files           4        4              
  Lines        3020     3050      +30     
==========================================
+ Hits         2475     2498      +23     
- Misses        545      552       +7     

Impacted Files	Coverage Δ
param/parameterized.py	82.34% <97.22%> (+0.16%)	:arrow_up:
param/serializer.py	84.78% <0.00%> (-2.18%)	:arrow_down:
numbergen/__init__.py	80.00% <0.00%> (+0.08%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fb6f4d9...20215a4. Read the comment docs.

[maximlt]

• [x] Just stumbled upon this issue which I think can be related to this PR https://github.com/holoviz/param/issues/583

[maximlt]

Recording some issues.

This snippet used to run fine and allow_None, despite being set to False in the Parameter constructor, is actually dynamically re-set to True internally (P().param.s.allow_None is True).

class P(param.Parameterized):
    s = param.String(default=None, allow_None=False)

On the branch in its current state running the same snippet raises an error:

ValueError: String parameter None only takes a string value, not value of type <class 'NoneType'>.

This happens in the validation code path of String. The code below runs fine on the main branch and on this branch.

class P(param.Parameterized):
    p = param.Parameter(default=None, allow_None=False)

inst = P()

inst.p is None and on this branch inst.param.p.allow_None is False while it's True on the main branch. Clearly there's something wrong.

Interestingly, the test suite passes despite this change.

(Yep and None instead of the Parameter name in String parameter None only takes... is a bug? I think I've already seen this sort of bug so not sure it's related to this PR)

[maximlt]

Replying to my own comment above https://github.com/holoviz/param/pull/605#issuecomment-1438862047.

This is the same behavior as main, which is correct, with allow_None being automatically set to True if the provided default is None.

import param

class P(param.Parameterized):
    s = param.String(default=None, allow_None=False)

inst = P()

print(repr(inst.s))
# None
print(inst.param.s.allow_None)
# True

[maximlt]

I think I am going to stop there for now :)

Early on I felt that the test suite was not good enough to capture all the changes that would be brought by this PR. Metadata inheritance wasn't really tested much, if at all. Users could rely on the fact that it didn't work, without that being captured by the test suite. See for instance this case in HoloViews https://github.com/holoviz/holoviews/pull/5667. I also observed that the test suite was lacking as I ran Panel's unit tests against this PR (after Jim's work) and it failed.

So I decided to add a lot of tests! These tests were all added on main first and then merged in this PR. With obviously some informative title:

• https://github.com/holoviz/param/pull/710
• https://github.com/holoviz/param/pull/716
• https://github.com/holoviz/param/pull/717
• https://github.com/holoviz/param/pull/719
• https://github.com/holoviz/param/pull/720

This included:

• Adding tests to previously untested Parameters
• Adding tests that check that the default values of an unbound Parameter, a class Parameter and an instance Parameter are the expected ones
• Adding tests to attempt to capture the behavior on main of Parameter metadata inheritance.

While all the added tests are important, the later ones in the list above are special for this PR, as some of them needed to be updated to reflect the new behavior. I would advise reviewers to carefully check these tests changes.

WARNING The Parameter metadata inheritance tests are very likely incomplete, there are a lot of Parameters to test and many possible variations in metadata inheritance. For instance, a sub Parameter can inherit metadata from multiple super Parameters, and can have some of its attributes being dynamically computed from a combination of inherited and directly provided ones. So I'm going to ask reviewers to check that the most important behavior you would like to be done by this PR are well captured, and work well. Look for tests with inheritance or behavior in their name, and the tests in API1/testparameterizedobject.py.

---

Overall, I find that I haven't really managed to find the recipe to enable metadata inheritance in dynamic cases, i.e. when some metadata value depends on some others. It felt every time a little custom, it'd be great to be able to better abstract that.

---

Should we raise an error for a "default" value that ultimately inherited as Undefined? Currently will silently return None

This PR now raises a KeyError when a Parameter has a slot that has no default value defined in _slot_defaults.

Add docs

Still on the TODO list, should be pretty straightforward though. We should probably also document how to write custom Parameters in a way that allows metadata inheritance for additional slots.

Should we verify compatibility between types across the inheritance hierarchy

It'd be nice, there must be some edge cases though. For instance, should that valid?

import param

class A(param.Parameterized):
    p = param.ClassSelector(class_=(list, dict))

class B(param.Parameterized):
    p = param.List()

It should be pretty straightforward to run the test suite of Panel/HoloViews/etc. from a branch that raises an error if the Parameter from which a metadata value is inherited (in _param_inheritance) is not a super class of the inheriting Parameter, and see what happens!

Could add a mechanism to fix https://github.com/holoviz/param/issues/1 and https://github.com/holoviz/param/issues/574, about allowing a user to specify that a value is required, e.g. by defining a new sentinel Required for a user to use as the default value for a parameter and raising an error in Parameterized's constructor if that value is detected. I don't think that Undefined will work here, since most or all Parameters ultimately define a default value, and Undefined will eventually be replaced by that value.

I haven't had a look at it yet. It looks like it shouldn't be too hard, but it can definitely be implemented later, even after 2.0.

---

A potentially important bit of information on a change made in this PR, I made Undefined an instance of an _Undefined class to raise an error when __bool__ is called, that helped me to catch bugs comparing it without using is. Hope it's fine! By the way I have no idea whether Undefined will be pickled correctly.

---

I would like to stress something that might be overlooked by this PR but that I judge pretty important. Now the Parameter signatures no longer have some of the default attribute values (like default), instead they just show param.Undefined, which probably has a rather ugly repr! This is going to affect a lot of users, who mostly don't care much about Parameter metadata inheritance. This is also going to affect the reference API doc of Param, that, even if it isn't great now, doesn't deserve to get worse. So I think we should think about how not to affect badly or even improve:

• [ ] signatures, both in VSCode and Jupyter
• [ ] docstrings, both in VSCode and Jupyer
• [ ] reference API doc

Some super quick and likely bad ideas include:

• overloading the signatures with typing
• def __init__(self, param.Undefined('somedefault'), ...), with Undefined having a repr that shows the repr of the provided default value, and no longer checking in the code is Undefined but isinstance(..., Undefined).

---

I have no clue about the performance impact of implementing Parameter.__getattribute__.

---

Given the potential effects of this PR and the fact that the test suite is certainly not catching all the changes, I advise we run the complete test suite of the following repos from that branch:

• [x] Panel
• [x] HoloViews
• [ ] GeoViews
• [x] Lumen

(I have just run the unit test suite of Panel and HoloViews)

EDIT: ran then excluding GeoViews and some examples tests, it went fine. The next test will be when an alpha version of Param gets released.

[maximlt]

Thanks for the review @jbednar, I applied your suggestions and renamed another function so that they follow a consistent naming scheme _compute_....

I also needed to make a small change to Boolean required after https://github.com/holoviz/param/pull/722 was merged into main.