Skip to content

Commit

Permalink
Remove override for default usage in Parameter annotations (#1278)
Browse files Browse the repository at this point in the history
**Pull Request Checklist**
- [x] Fixes #812 (final step)
- [x] Tests adjusted
- [x] Documentation/examples added previously
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**
Currently, when using the default attribute in Parameters
(`Parameter(default="x")`) users can supress the error with the
`suppress_parameter_default_error` experimental feature flag. If users
have not updated their code (which is a very easy change) they will now
get errors when upgrading Hera, the warning has been there for 2 minor
versions and this is an experimental feature - we will be able to
graduate it in the next version once this is released.

---------

Signed-off-by: Elliot Gunton <[email protected]>
  • Loading branch information
elliotgunton authored Nov 27, 2024
1 parent 2f640b6 commit 133bc54
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 47 deletions.
1 change: 0 additions & 1 deletion src/hera/shared/_global_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,6 @@ 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
Expand Down
14 changes: 2 additions & 12 deletions src/hera/workflows/io/_io_mixins.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import sys
import warnings
from typing import TYPE_CHECKING, Iterator, List, Optional, Tuple, Type, Union

if sys.version_info >= (3, 11):
Expand All @@ -8,8 +7,6 @@
from typing_extensions import Self


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 construct_io_from_annotation, get_workflow_annotation
from hera.shared.serialization import MISSING, serialize
Expand Down Expand Up @@ -80,16 +77,9 @@ def _get_parameters(cls, object_override: Optional[Self] = None) -> List[Paramet
for field, field_info, param in _construct_io_from_fields(cls):
if isinstance(param, Parameter):
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. "
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 object_override:
param.default = serialize(getattr(object_override, field))
elif field_info.default is not None and field_info.default != PydanticUndefined: # type: ignore
Expand Down
19 changes: 2 additions & 17 deletions src/hera/workflows/script.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import inspect
import sys
import textwrap
import warnings
from abc import abstractmethod
from functools import wraps
from typing import (
Expand Down Expand Up @@ -43,7 +42,6 @@
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
Expand Down Expand Up @@ -509,23 +507,10 @@ class will be used as inputs, rather than the class itself.
artifacts.append(io)
elif isinstance(io, Parameter):
if io.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. "
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 io.default is not None:
raise ValueError(
"default cannot be set via both the function parameter default and the Parameter's default"
)
io.default = serialize(func_param.default)

if origin_type_issupertype(func_param.annotation, NoneType) and io.default != "null":
Expand Down
30 changes: 14 additions & 16 deletions tests/test_script_annotations.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from hera.shared._pydantic import _PYDANTIC_VERSION
from hera.workflows import Workflow, script
from hera.workflows.io import Input
from hera.workflows.parameter import Parameter
from hera.workflows.steps import Steps

Expand Down Expand Up @@ -64,14 +65,12 @@ def test_script_annotations_artifact_regression(module_name, global_config_fixtu
_compare_workflows(workflow_old, output_old, output_new)


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."""
def test_parameter_default_throws_a_value_error(global_config_fixture):
"""Test asserting that it is not possible to define default in the annotation."""

# GIVEN
global_config_fixture.experimental_features["suppress_parameter_default_error"] = True

@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
Expand All @@ -82,18 +81,21 @@ def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2):

w.to_dict()

assert "default cannot be set via both the function parameter default and the Parameter's default" in str(e.value)
assert ("default cannot be set via the Parameter's default, use a Python default value instead") in str(e.value)


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."""
def test_pydantic_input_with_default_throws_a_value_error(global_config_fixture):
"""Test asserting that it is not possible to define default in the annotation in a Hera Input class."""

# GIVEN
global_config_fixture.experimental_features["suppress_parameter_default_error"] = False
global_config_fixture.experimental_features["script_pydantic_io"] = True

class ExampleInput(Input):
an_int: Annotated[int, Parameter(default=1)]

@script()
def echo_int(an_int: Annotated[int, Parameter(default=1)]):
print(an_int)
def echo_int(pydantic_input: ExampleInput):
print(pydantic_input.an_int)

global_config_fixture.experimental_features["script_annotations"] = True
with pytest.raises(ValueError) as e:
Expand All @@ -103,11 +105,7 @@ def echo_int(an_int: Annotated[int, Parameter(default=1)]):

w.to_dict()

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)
assert ("default cannot be set via the Parameter's default, use a Python default value instead") in str(e.value)


@pytest.mark.parametrize(
Expand Down
2 changes: 1 addition & 1 deletion tests/workflow_decorators/steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,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


Expand Down

0 comments on commit 133bc54

Please sign in to comment.