Warn when concrete_descendents clobbers classes

[maximlt]

No description provided.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.28%. Comparing base (30edc3f) to head (bb72745).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1035      +/-   ##
==========================================
+ Coverage   87.26%   87.28%   +0.01%     
==========================================
  Files           9        9              
  Lines        4939     4945       +6     
==========================================
+ Hits         4310     4316       +6     
  Misses        629      629              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[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 #1027 (comment)

[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 #1035 (comment), 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.