From 8cb375a966f103c0b27007c1e3f9554c2353355b Mon Sep 17 00:00:00 2001 From: Elliot Gunton Date: Wed, 18 Sep 2024 09:21:00 +0100 Subject: [PATCH] Error on Parameter default kwarg usage for inputs (#1197) **Pull Request Checklist** - [x] Fixes #812 - [x] Tests added - [x] Documentation/examples added - [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR title **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 https://github.com/argoproj-labs/hera/pull/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. --------- Signed-off-by: Elliot Gunton --- .../experimental/new_dag_decorator_params.md | 2 +- .../new_steps_decorator_with_parallel_steps.md | 2 +- docs/user-guides/script-annotations.md | 3 ++- .../experimental/new_dag_decorator_params.py | 2 +- .../new_steps_decorator_with_parallel_steps.py | 2 +- src/hera/shared/_global_config.py | 1 + src/hera/workflows/io/_io_mixins.py | 13 ++++++++++--- src/hera/workflows/script.py | 15 +++++++++------ tests/test_decorators.py | 3 ++- tests/test_script_annotations.py | 17 ++++++++++++----- 10 files changed, 40 insertions(+), 20 deletions(-) diff --git a/docs/examples/workflows/experimental/new_dag_decorator_params.md b/docs/examples/workflows/experimental/new_dag_decorator_params.md index 4e8db0246..87ab9b605 100644 --- a/docs/examples/workflows/experimental/new_dag_decorator_params.md +++ b/docs/examples/workflows/experimental/new_dag_decorator_params.md @@ -45,7 +45,7 @@ class ConcatInput(Input): - word_a: Annotated[str, Parameter(name="word_a", default="")] + word_a: Annotated[str, Parameter(name="word_a")] = "" word_b: str concat_config: ConcatConfig = ConcatConfig(reverse=False) diff --git a/docs/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.md b/docs/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.md index 63f620193..d68391714 100644 --- a/docs/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.md +++ b/docs/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.md @@ -30,7 +30,7 @@ class ConcatInput(Input): - word_a: Annotated[str, Parameter(name="word_a", default="")] + word_a: Annotated[str, Parameter(name="word_a")] = "" word_b: str diff --git a/docs/user-guides/script-annotations.md b/docs/user-guides/script-annotations.md index b897f77e3..1a41af384 100644 --- a/docs/user-guides/script-annotations.md +++ b/docs/user-guides/script-annotations.md @@ -45,7 +45,8 @@ def echo_all( ``` The fields allowed in the `Parameter` annotations are: `name`, `enum`, and `description`, `name` will be set to the -variable name if not provided (when exporting to YAML or viewing in the Argo UI, the `name` variable will be used). +variable name if not provided (when exporting to YAML or viewing in the Argo UI, the `name` variable will be used). A +`default` must be set using standard Python syntax, i.e. `x: int = 42`. ## Artifacts diff --git a/examples/workflows/experimental/new_dag_decorator_params.py b/examples/workflows/experimental/new_dag_decorator_params.py index d06b61299..2a7b719de 100644 --- a/examples/workflows/experimental/new_dag_decorator_params.py +++ b/examples/workflows/experimental/new_dag_decorator_params.py @@ -35,7 +35,7 @@ class ConcatConfig(BaseModel): class ConcatInput(Input): - word_a: Annotated[str, Parameter(name="word_a", default="")] + word_a: Annotated[str, Parameter(name="word_a")] = "" word_b: str concat_config: ConcatConfig = ConcatConfig(reverse=False) diff --git a/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.py b/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.py index a26d134ef..822749768 100644 --- a/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.py +++ b/examples/workflows/experimental/new_steps_decorator_with_parallel_steps.py @@ -20,7 +20,7 @@ def setup() -> SetupOutput: class ConcatInput(Input): - word_a: Annotated[str, Parameter(name="word_a", default="")] + word_a: Annotated[str, Parameter(name="word_a")] = "" word_b: str diff --git a/src/hera/shared/_global_config.py b/src/hera/shared/_global_config.py index a93b3731b..a5c42353c 100644 --- a/src/hera/shared/_global_config.py +++ b/src/hera/shared/_global_config.py @@ -202,6 +202,7 @@ def _set_defaults(cls, values): _SCRIPT_ANNOTATIONS_FLAG = "script_annotations" _SCRIPT_PYDANTIC_IO_FLAG = "script_pydantic_io" _DECORATOR_SYNTAX_FLAG = "decorator_syntax" +_SUPPRESS_PARAMETER_DEFAULT_ERROR_FLAG = "suppress_parameter_default_error" # A dictionary where each key is a flag that has a list of flags which supersede it, hence # the given flag key can also be switched on by any of the flags in the list. Using simple flat lists diff --git a/src/hera/workflows/io/_io_mixins.py b/src/hera/workflows/io/_io_mixins.py index a7f2bdd1a..be7afe948 100644 --- a/src/hera/workflows/io/_io_mixins.py +++ b/src/hera/workflows/io/_io_mixins.py @@ -7,9 +7,10 @@ else: from typing_extensions import Self -from pydantic.fields import FieldInfo -from hera.shared._pydantic import _PYDANTIC_VERSION, get_field_annotations, get_fields +from hera.shared import global_config +from hera.shared._global_config import _SUPPRESS_PARAMETER_DEFAULT_ERROR_FLAG +from hera.shared._pydantic import _PYDANTIC_VERSION, FieldInfo, get_field_annotations, get_fields from hera.shared._type_util import get_workflow_annotation from hera.shared.serialization import MISSING, serialize from hera.workflows._context import _context @@ -87,8 +88,14 @@ def _get_parameters(cls, object_override: Optional[Self] = None) -> List[Paramet if param.default is not None: warnings.warn( "Using the default field for Parameters in Annotations is deprecated since v5.16" - "and will be removed in a future minor version, use a Python default value instead" + "and will be removed in a future minor version, use a Python default value instead. " ) + if not global_config.experimental_features[_SUPPRESS_PARAMETER_DEFAULT_ERROR_FLAG]: + 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' + ) if object_override: param.default = serialize(getattr(object_override, field)) elif field_info.default is not None and field_info.default != PydanticUndefined: # type: ignore diff --git a/src/hera/workflows/script.py b/src/hera/workflows/script.py index 063ec6f47..058f5a1b9 100644 --- a/src/hera/workflows/script.py +++ b/src/hera/workflows/script.py @@ -43,6 +43,7 @@ from hera.shared._global_config import ( _SCRIPT_ANNOTATIONS_FLAG, _SCRIPT_PYDANTIC_IO_FLAG, + _SUPPRESS_PARAMETER_DEFAULT_ERROR_FLAG, _flag_enabled, ) from hera.shared._pydantic import _PYDANTIC_VERSION, root_validator, validator @@ -511,15 +512,17 @@ class will be used as inputs, rather than the class itself. artifacts.append(new_object) elif isinstance(new_object, Parameter): if new_object.default is not None: + # TODO: in 5.18 remove the flag check and `warn`, and raise the ValueError directly (minus "flag" text) warnings.warn( "Using the default field for Parameters in Annotations is deprecated since v5.16" - "and will be removed in a future minor version, use a Python default value instead" + "and will be removed in a future minor version, use a Python default value instead. " ) - # TODO: raise error if override flag not enabled in 5.17: - # if not global_config.experimental_features["..."]: - # raise ValueError( - # "default cannot be set via the Parameter's default, use a Python default value instead" - # ) + if not global_config.experimental_features[_SUPPRESS_PARAMETER_DEFAULT_ERROR_FLAG]: + 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' + ) if func_param.default != inspect.Parameter.empty: # TODO: remove this check in 5.18: if new_object.default is not None: diff --git a/tests/test_decorators.py b/tests/test_decorators.py index 3200cf4ed..cca81ea31 100644 --- a/tests/test_decorators.py +++ b/tests/test_decorators.py @@ -229,8 +229,9 @@ def test_dag_is_runnable(): ) -def test_steps_with_parallel_steps_is_runnable(): +def test_steps_with_parallel_steps_is_runnable(global_config_fixture): """The steps function, even with a parallel context, should be runnable as Python code.""" + global_config_fixture.experimental_features["suppress_parameter_default_error"] = True from tests.workflow_decorators.steps import WorkerInput, WorkerOutput, worker assert worker(WorkerInput(value_a="hello", value_b="world")) == WorkerOutput( diff --git a/tests/test_script_annotations.py b/tests/test_script_annotations.py index d2faccf58..0c7e994ea 100644 --- a/tests/test_script_annotations.py +++ b/tests/test_script_annotations.py @@ -68,6 +68,8 @@ def test_double_default_throws_a_value_error(global_config_fixture): """Test asserting that it is not possible to define default in the annotation and normal Python.""" # GIVEN + global_config_fixture.experimental_features["suppress_parameter_default_error"] = True + @script() def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2): print(an_int) @@ -83,13 +85,14 @@ def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2): assert "default cannot be set via both the function parameter default and the Parameter's default" in str(e.value) -@pytest.mark.skip(reason="Code change required in next Hera version") -def test_parameter_default_throws_a_value_error(global_config_fixture): - """Test asserting that it is not possible to define default in the annotation.""" +def test_parameter_default_without_suppression_throws_a_value_error(global_config_fixture): + """Test asserting that it is not possible to define default in the annotation and normal Python.""" # GIVEN + global_config_fixture.experimental_features["suppress_parameter_default_error"] = False + @script() - def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2): + def echo_int(an_int: Annotated[int, Parameter(default=1)]): print(an_int) global_config_fixture.experimental_features["script_annotations"] = True @@ -100,7 +103,11 @@ def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2): w.to_dict() - assert "default cannot be set via the Parameter's default, use a Python default value instead" in str(e.value) + assert ( + "default cannot be set via the Parameter's default, use a Python default value instead" + "You can suppress this error by setting " + 'global_config.experimental_features["suppress_parameter_default_error"] = True' + ) in str(e.value) @pytest.mark.parametrize(