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.

https://github.com/holoviz/param/blob/76c7026346b73951dcf4308b6302f0742b0e83e7/param/init.py#L1090-L1101

[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 (https://github.com/holoviz/param/issues/519) which can motivate the creation of another function.