Skip to content

Conversation

@fjetter
Copy link
Member

@fjetter fjetter commented Mar 21, 2025

I believe this is an important first step towards making the Expr classes more traditional classes. The concept with _parameters, etc. has been causing issues in the past in the context of inheritance (not to mention the confusion around recursion errors and other things)

This change also further highlights a couple of anti patterns where some implementations set attributes on the classes without them being registered as parameters which poses challenges to serialization and tokenization

This needs #11835 since otherwise I cannot properly subclass Expr for the test

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @fjetter

@fjetter fjetter merged commit bd3a7d6 into dask:main Mar 21, 2025
9 of 22 checks passed
@fjetter fjetter deleted the expr_setattr branch March 24, 2025 13:26
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.

2 participants