Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add concrete keyword to descendents #1027

Merged
merged 6 commits into from
Feb 20, 2025
Merged

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Feb 16, 2025

Somewhat related to #651, the idea being to steer users towards descendents instead of concrete_descendents (to deprecate it ultimately but it'll take a little while as it's used in a lot of holoviz code bases).

Copy link

codecov bot commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.26%. Comparing base (274632a) to head (a313999).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
param/_utils.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
- Coverage   87.27%   87.26%   -0.02%     
==========================================
  Files           9        9              
  Lines        4936     4939       +3     
==========================================
+ Hits         4308     4310       +2     
- Misses        628      629       +1     

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

@jlstevens
Copy link
Contributor

The concept here makes sense to me.

Discussing this with Maxime, we decided there are two paths we can take:

  1. Have concrete_descendents warn the user when a key is clobbered, directing the user to using descendents with concrete=True
  2. Work the usual deprecation cycle where we eventually get rid of concrete_descendents entirely (i.e. start warning users of .concrete_descendents to use descendents with the keyword instead)

These things are orthogonal and I think the first suggestion is a good idea regardless. I do see the argument for deprecating concrete_descendents (i.e. cleaning up the API a bit) , especially given that most usage seems to call .values() to get a list-like iterator anyway.

This PR is necessary whether we decide to deprecate concrete_descendents or not so I'll go ahead and approve it now.

Copy link
Contributor

@jlstevens jlstevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@@ -518,26 +518,52 @@ def _is_number(obj):
else: return False


def _is_abstract(class_):
def _is_abstract(class_: type) -> bool:
if inspect.isabstract(class_):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I haven't looked where this is used; if it is not in many places, ignore me).

Is there a penalty related to this check? E.g. if this is in a hotpath will this add significant overhead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timed it and it's 2-3x slower than the try/except that follows. That's also not a particularly hot path I think.

@maximlt maximlt merged commit 30edc3f into main Feb 20, 2025
17 checks passed
@maximlt maximlt deleted the add_concrete_to_descendents branch February 20, 2025 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants