concrete_descendents clobbers descendents

[sdrobert]

ALL software version info

param v1.12.2, Colab notebook

Description of expected behavior and the observed behavior

Simply put, classes with the same name get clobbered in get_concrete_descendents().

I came across this bug when trying to run some unit tests with duplicated code (and class definitions) were in different files.

Complete, minimal, self-contained example code that reproduces the issue

import sys
import param

from param.parameterized import descendents

print(param.__version__)

class A(object):
  pass

class B(A):
  pass

b = B

print(b in param.concrete_descendents(A).values())

class B(A):
  pass

print(b in param.concrete_descendents(A).values(), b in descendents(A))
print(B in param.concrete_descendents(A).values(), B in descendents(A))
print(issubclass(b, A), issubclass(b, A))

Colab

[hoxbro]

I can reproduce the problem. And looking at the code, it is easy to see why it happens, as there is no uniqueness to the name of a class. The correct fix would properly be to use the class as key and name as value, though this is a breaking change.

Maybe the best thing is to test if there are duplicate class names in param.concrete_descendents and raise an exception if there are. This could raise exceptions in existing code, but at least it will indicate that something is gone wrong.

param/param/__init__.py

Lines 1090 to 1101 in 76c7026

	def concrete_descendents(parentclass):
	"""
	Return a dictionary containing all subclasses of the specified
	parentclass, including the parentclass. Only classes that are
	defined in scripts that have been run or modules that have been
	imported are included, so the caller will usually first do ``from
	package import *``.
	Only non-abstract classes will be included.
	"""
	return dict((c.__name__,c) for c in descendents(parentclass)
	if not _is_abstract(c))

[maximlt]

Hm yes the data structure used should probably not have been a dictionary but a list of tuple/namedtuple?

@philippjfr param.concrete_descendents is used quite a lot in Panel. What change would you be willing to accept, if any, to fix the reported issue?

[hoxbro]

Deprecating concrete_descendents with a warning and having a new function with a better data structure could be a solution.

It would allow people to shift to the new function and not break anyone using the current implementation.

[maximlt]

Yep that's another valid way to approach that 👍

[maximlt]

It also has another reported issue (#519) which can motivate the creation of another function.

[maximlt]

@philippjfr I see plenty of usage of concrete_descendents in HoloViews/Panel/Lumen/... and assume you're the number one contributor of this code :) This function has a couple of related issues:

• concrete_descendents ignoring abstract flag? #519
• Consider replacing our abstract class mechanism with Python's #84
• Error when setting ClassSelector class_=object #1013
• Document concrete_descendents #706

To fix this one, I still think the solution would be to return a list of class name and class object tuples instead of a dictionary. If we decide to do that, I guess the easiest approach would be to add concrete_descendents2. concrete_descendents could be updated to check if it clobbers names, and warn if it does with a message that points to concrete_descendents2.

Param and concrete_descendents have never been adapted to deal with Python abstract base classes. At the same time, concrete_descendents has never been limited to Parameterized classes only (which have their specific way of declaring themselves as abstract with the __abstract class attribute). So using concrete_descendents on a non-Parameterized class isn't quite correct. Should we restrict concrete_descendents to being applied to Parameterized classes only? I think I'd be in favor of that but I'm not sure how exactly it is used out there.

Let me know what you think :)

[maximlt]

Hmm, in fact, after looking at how concrete_descendents is used in the HoloViz code bases, in the majority of the occurrences, .values() is immediately called on the returned dictionary. So, an alternative might be to add a concrete keyword argument to param.descendents (used internally by concrete_descendents).

[maximlt]

Implemented that suggestion in #1027.

[maximlt]

concrete_descendents emits a warning when clobbering is detected. Prefer descendents(cls, concrete=True) to get a list of all the concrete descendant classes. The issue with ClassSelector.get_range still clobbering is tracked in #1087.