Warn when concrete_descendents clobbers classes

[maximlt]

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 89.18%. Comparing base (a35358b) to head (4a9f3f5). :warning: Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1035      +/-   ##
==========================================
+ Coverage   89.16%   89.18%   +0.01%     
==========================================
  Files           9        9              
  Lines        4679     4686       +7     
==========================================
+ Hits         4172     4179       +7     
  Misses        507      507              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maximlt]

@jlstevens the PyPy tests have failed which raised an interesting question. concrete_descendents is used internally in ClassSelector.get_range (see the code below). One of the tests that fail defines a parameter with ClassSelector(class_=(int, str)) and then calls get_range(). On PyPy, there's somehow an enum that appears twice in the concrete descendants of int, leading to that warning being emitted.

I feel like if we want to fix this for good, we need to update get_range to return a list of descendents and no longer a dict, i.e. use descendents(parent, concrete=True) instead of concrete_descendents(parent). To implement that:

• we could add a new as_list keyword to get_range that is False by default
• warn whenever get_range is called with as_list is False, stating that in the future as_list will become True by default, users can opt-in by already calling get_range(as_list=True).

What do you think?

class ClassSelector(Parameter):
    ...
    def get_range(self):
        """
        Return the possible types for this parameter's value.

        (I.e. return `{name: <class>}` for all classes that are
        concrete_descendents() of `self.class_`.)

        Only classes from modules that have been imported are added
        (see concrete_descendents()).
        """
        classes = self.class_ if isinstance(self.class_, tuple) else (self.class_,)
        all_classes = {}
        for cls in classes:
            all_classes.update(concrete_descendents(cls))
        d = OrderedDict((name, class_) for name,class_ in all_classes.items())
        if self.allow_None:
            d['None'] = None
        return d

EDIT: This is following a discussion started here https://github.com/holoviz/param/pull/1027#issuecomment-2668638758

[jbednar]

I don't know the cleanest way to get out of this mess, but presumably it's also worth (briefly) considering a multidict, which would preserve the interface but allow duplicate keys? That might well cause other problems later, of course, if someone tries to use the multidict as a regular dict.

[maximlt]

True, I didn't consider multidict until now. I'll chat with Jean-Luc this week more about all this!

[maximlt]

Reading this again https://github.com/holoviz/param/pull/1035#issuecomment-2674486583, I no longer think that's the right approach as I am not confident that there's a sane way to change the type of what Selector.get_range() returns.

Instead, I'd suggest either:

1. Status quo for Selector.get_range() (e.g. by just swallowing the warnings emitted by the updated concrete_descendents or by using an implementation that doesn't emit a warning)
2. Update concrete_descendents to no longer clobber the keys but instead append the duplicate ones with an increment (MyClass, MyClass1, MyClass2)

I might just go for 1) now to unblock this.

[maximlt]

Unrelated test failures, merging.