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 Parameter re-validates default if slot changes #820

Merged
merged 6 commits into from
Sep 16, 2023

Conversation

philippjfr
Copy link
Member

This PR adds validation when overriding slot values on a Parameter in a subclass. It does this by checking if a superclass defines a slot value and if that slot value is overridden by the subclass. It attempts to minimize the number of actual lookups by aborting early if we already know that re-validation has to happen and by skipping checks on various slot values which do not affect validation.

Fixes #714

@philippjfr philippjfr requested review from jbednar and maximlt August 21, 2023 20:57
@philippjfr
Copy link
Member Author

I had to disable re-validation of None values because the automatic appending of an unknown value on Selector opens a whole rabbit hole in regard to the validation.

param/parameterized.py Outdated Show resolved Hide resolved
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.

I'm having a good bit of trouble following the logic here, so hopefully we can work through it together. I think I probably wrote some of the confusing bits (e.g. what's "new" about what I think I was the one to call "new_param"?), but as the code has gotten more complex I'm now lost. The basic idea seems sound, i.e. to see if any slots have changed and thus whether we need to re-validate the default value.

param/parameterized.py Outdated Show resolved Hide resolved
param/parameterized.py Outdated Show resolved Hide resolved
@philippjfr
Copy link
Member Author

@maximlt Can you either merge or ask for further clarification? You are release manager :)

@maximlt
Copy link
Member

maximlt commented Sep 15, 2023

I was waiting for #826 to be merged! Now it's done I'll rebase this and merge if it goes well.

@maximlt maximlt merged commit 779f734 into main Sep 16, 2023
10 checks passed
@maximlt maximlt deleted the validate_changing_slots branch September 16, 2023 08:04
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.

Inherited default is not validated
3 participants