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

Ensure constant attribute is inherited #1017

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Feb 11, 2025

Fixes #1001

Let's have a good look at the downstream tests before releasing 2.3.0, this is a nasty bug.

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.26%. Comparing base (031a4f4) to head (3b4bf9e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1017   +/-   ##
=======================================
  Coverage   87.25%   87.26%           
=======================================
  Files           9        9           
  Lines        4928     4930    +2     
=======================================
+ Hits         4300     4302    +2     
  Misses        628      628           

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

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Glad to see this one caught! I think that was my oversight orginally. Sorry about that!

if constant is True or readonly is True: # readonly => constant
self.constant = True
else:
self.constant = constant
Copy link
Member

Choose a reason for hiding this comment

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

Ooh; good catch! I wonder if there were any other cases where it wouldn't inherit properly due to computation like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if there were any other cases where it wouldn't inherit properly due to computation like this.

Good question 🙃 !

@maximlt
Copy link
Member Author

maximlt commented Feb 11, 2025

Glad to see this one caught! I think that was my oversight orginally. Sorry about that!

Any dependency between Parameter attributes (looking at you Selector) makes attribute inheritance more complex. In this case, one could argue there isn't really a need to automatically set constant=True when readonly=True!

@jbednar
Copy link
Member

jbednar commented Feb 11, 2025

Any dependency between Parameter attributes (looking at you Selector) makes attribute inheritance more complex.

That's true.

In this case, one could argue there isn't really a need to automatically set constant=True when readonly=True!

But then we'd have to wonder what it means for constant=False and readonly=True, which seems like a bizarre case to even consider. Maybe it would have been best to handle that on attribute access, not via defaults?

@maximlt
Copy link
Member Author

maximlt commented Feb 12, 2025

But then we'd have to wonder what it means for constant=False and readonly=True

I think I could live with that as readonly takes precedence over constant in my mind. Though it's easy for me as I've internalized what readonly and constant mean but for most users, their effect isn't obvious, readonly meaning can_never_be_updated and constant meaning cannot_be_updated_after_instantiation.

Maybe it would have been best to handle that on attribute access, not via defaults?

I don't know and don't want to think about it too much :D

EDIT:

Coming back to the first point, readonly takes precedence over constant not only in my mind but in Param too. parameter = Parameter(readonly=True, constant=False) doesn't raise any error or emit any warning, it internally sets constant to True.

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.

constant Parameter attribute not inherited
2 participants