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

Error on Parameter default kwarg usage for input annotations #1197

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

elliotgunton
Copy link
Collaborator

@elliotgunton elliotgunton commented Sep 11, 2024

Pull Request Checklist

Description of PR
Currently, using the default kwarg for a Parameter in an input Annotation or Hera's Pydantic Input class logs a warning message, added in #1102. This PR makes the code path raise an error, which can be supressed using the suppress_parameter_default_error experimental feature flag for 1 minor version until it is removed.

* Add temporary experimental feature flag to suppress error

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
* Update examples to remove `default=""` usage

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton added semver:minor A change requiring a minor version bump type:informational Provides information or notice to the community labels Sep 11, 2024
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton marked this pull request as ready for review September 11, 2024 09:06
raise ValueError(
"default cannot be set via the Parameter's default, use a Python default value instead. "
"You can suppress this error by setting "
f'global_config.experimental_features["{_SUPPRESS_PARAMETER_DEFAULT_ERROR_FLAG}"] = True'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be an experimental flag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean? Should the error be supressed another way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think so. This is a flag but not really an experiment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was intending this to only be a one-off flag for 1 minor version, so I'd rather not add another feature flag syntax just for 1 flag for 1 version (and then have to keep it around). Plus the supression flag is a nicety instead of the intended purpose of "experimental" letting us make breaking changes, but Annotated became a mainstay so I understand the need in this case.

In any case, the global_config.experimental_features dictionary should be able to hold whatever we need it to hold related to any experimental features - we haven't put any enforced rules on it.

experimental_features: Dict[str, bool] = field(default_factory=lambda: defaultdict(bool))
"""an indicator holder for any Hera experimental features to use"""

Copy link
Collaborator

Choose a reason for hiding this comment

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

FWIW I think experimental flag is ok as long as we have a clear deadline/version by which we're going to remove this. Small but do we need a warning and a ValueError? I sense the warning will get lost when the exception is raised

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@flaviuvadan Good idea - I can add an explicit version (5.18?) in the warning and error text?

the warning will get lost

For the first time seeing the error, yes, the warning will probably not be seen, but then after turning the flag on, we still want the user to see the logged warning.

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton merged commit 8cb375a into main Sep 18, 2024
20 checks passed
@elliotgunton elliotgunton deleted the remove-parameter-default branch September 18, 2024 08:21
@elliotgunton elliotgunton changed the title Error on Parameter default kwarg usage for inputs Error on Parameter default kwarg usage for input annotations Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:informational Provides information or notice to the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input Parameter annotations should only be able to set default via standard Python
4 participants