From b5a260fa8204657b6e6f452ca4ee7da7a0b31a25 Mon Sep 17 00:00:00 2001 From: Elliot Gunton Date: Tue, 24 Oct 2023 16:32:40 +0100 Subject: [PATCH] Bug fixes for input Parameters and Artifacts in annotations (#811) **Pull Request Checklist** - [x] Fixes #809, fixes #810 - [x] Tests added - [x] Documentation/examples added - [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR title **Description of PR** Currently, output parameters can be specified without a name within the function parameters, but input parameters cannot. This PR makes the runner use the function parameter name as the `name` of a `hera.workflows.Parameter`. Input Artifacts can be specified without a path in the annotation, which creates the correct yaml, seen in #792, but at runtime the runner would not know what path the artifact is supposed to load from. Fixed and tested. This PR also cleans up the tests directory structure, removing a lot of duplicated code for runner/annotation tests. The distinction between these is now made clear in docstrings for `test_runner.py` and `test_script_annotations.py` - the runner tests should run the actual script function code to emulate the Argo cluster, while the script annotation tests check that function definitions produce correct yaml and are valid Python when using the annotations. --------- Signed-off-by: Elliot Gunton --- docs/examples/workflows/callable_script.md | 10 +- examples/workflows/callable-script.yaml | 4 +- examples/workflows/callable_script.py | 6 +- src/hera/workflows/artifact.py | 9 +- src/hera/workflows/runner.py | 113 ++++--- src/hera/workflows/script.py | 12 +- tests/conftest.py | 12 - tests/helper.py | 8 - tests/script_annotations/outputs.py | 81 +++++ .../script_annotations_artifact_file.py | 27 -- ...ipt_annotations_artifact_file_with_path.py | 27 -- .../script_annotations_artifact_object.py | 32 -- .../script_annotations_artifact_path.py | 26 -- ...ipt_annotations_output_artifact_in_func.py | 28 -- ...otations_output_custom_output_directory.py | 29 -- ...ript_annotations_output_in_func_no_name.py | 30 -- ...cript_annotations_output_multiple_calls.py | 31 -- ...tions_output_param_and_artifact_in_func.py | 30 -- ...notations_output_param_and_artifact_mix.py | 32 -- ...script_annotations_output_param_in_func.py | 28 -- .../annotated_outputs.py} | 30 +- tests/script_runner/artifact_loaders.py | 64 ++++ .../artifact_with_invalid_loader.py} | 12 +- tests/script_runner/parameter_inputs.py | 43 +++ .../script_runner/unknown_annotation_types.py | 16 + tests/test_runner.py | 299 +++++++++++++----- tests/test_script_annotations.py | 45 +-- tests/test_unit/test_script.py | 76 +++-- tests/test_unit/test_workflow.py | 5 +- 29 files changed, 602 insertions(+), 563 deletions(-) create mode 100644 tests/script_annotations/outputs.py delete mode 100644 tests/script_annotations_artifacts/script_annotations_artifact_file.py delete mode 100644 tests/script_annotations_artifacts/script_annotations_artifact_file_with_path.py delete mode 100644 tests/script_annotations_artifacts/script_annotations_artifact_object.py delete mode 100644 tests/script_annotations_artifacts/script_annotations_artifact_path.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_artifact_in_func.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_custom_output_directory.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_in_func_no_name.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_multiple_calls.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_param_and_artifact_in_func.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_param_and_artifact_mix.py delete mode 100644 tests/script_annotations_outputs/script_annotations_output_param_in_func.py rename tests/{script_annotations_outputs/script_annotations_output.py => script_runner/annotated_outputs.py} (82%) create mode 100644 tests/script_runner/artifact_loaders.py rename tests/{script_annotations_artifacts/script_annotations_artifact_wrong_loader.py => script_runner/artifact_with_invalid_loader.py} (65%) create mode 100644 tests/script_runner/parameter_inputs.py create mode 100644 tests/script_runner/unknown_annotation_types.py diff --git a/docs/examples/workflows/callable_script.md b/docs/examples/workflows/callable_script.md index a4ae93e1e..0f7566d3b 100644 --- a/docs/examples/workflows/callable_script.md +++ b/docs/examples/workflows/callable_script.md @@ -88,8 +88,8 @@ @script() - def function_kebab_object(input_values: Annotated[Input, Parameter(name="input-values")]) -> Output: - return Output(output=[input_values]) + def function_kebab_object(annotated_input_value: Annotated[Input, Parameter(name="input-value")]) -> Output: + return Output(output=[annotated_input_value]) with Workflow(name="my-workflow") as w: @@ -98,7 +98,7 @@ str_function(arguments={"input": Input(a=2, b="bar").json()}) another_function(arguments={"inputs": [Input(a=2, b="bar"), Input(a=2, b="bar")]}) function_kebab(arguments={"a-but-kebab": 3, "b-but-kebab": "bar"}) - function_kebab_object(arguments={"input-values": Input(a=3, b="bar")}) + function_kebab_object(arguments={"input-value": Input(a=3, b="bar")}) ``` === "YAML" @@ -140,7 +140,7 @@ template: function-kebab - - arguments: parameters: - - name: input-values + - name: input-value value: '{"a": 3, "b": "bar"}' name: function-kebab-object template: function-kebab-object @@ -217,7 +217,7 @@ source: '{{inputs.parameters}}' - inputs: parameters: - - name: input-values + - name: input-value name: function-kebab-object script: args: diff --git a/examples/workflows/callable-script.yaml b/examples/workflows/callable-script.yaml index 918c5b052..0163c6baa 100644 --- a/examples/workflows/callable-script.yaml +++ b/examples/workflows/callable-script.yaml @@ -34,7 +34,7 @@ spec: template: function-kebab - - arguments: parameters: - - name: input-values + - name: input-value value: '{"a": 3, "b": "bar"}' name: function-kebab-object template: function-kebab-object @@ -111,7 +111,7 @@ spec: source: '{{inputs.parameters}}' - inputs: parameters: - - name: input-values + - name: input-value name: function-kebab-object script: args: diff --git a/examples/workflows/callable_script.py b/examples/workflows/callable_script.py index 9cdcb9d9c..e639a6fff 100644 --- a/examples/workflows/callable_script.py +++ b/examples/workflows/callable_script.py @@ -78,8 +78,8 @@ def function_kebab( @script() -def function_kebab_object(input_values: Annotated[Input, Parameter(name="input-values")]) -> Output: - return Output(output=[input_values]) +def function_kebab_object(annotated_input_value: Annotated[Input, Parameter(name="input-value")]) -> Output: + return Output(output=[annotated_input_value]) with Workflow(name="my-workflow") as w: @@ -88,4 +88,4 @@ def function_kebab_object(input_values: Annotated[Input, Parameter(name="input-v str_function(arguments={"input": Input(a=2, b="bar").json()}) another_function(arguments={"inputs": [Input(a=2, b="bar"), Input(a=2, b="bar")]}) function_kebab(arguments={"a-but-kebab": 3, "b-but-kebab": "bar"}) - function_kebab_object(arguments={"input-values": Input(a=3, b="bar")}) + function_kebab_object(arguments={"input-value": Input(a=3, b="bar")}) diff --git a/src/hera/workflows/artifact.py b/src/hera/workflows/artifact.py index 56aa33b8d..2a4d84ef3 100644 --- a/src/hera/workflows/artifact.py +++ b/src/hera/workflows/artifact.py @@ -30,6 +30,8 @@ logger = logging.getLogger(__name__) logger.setLevel(logging.WARNING) +_DEFAULT_ARTIFACT_INPUT_DIRECTORY = "/tmp/hera/inputs/artifacts/" + class ArtifactLoader(Enum): """Enum for artifact loader options.""" @@ -78,7 +80,9 @@ class Artifact(BaseModel): """allows the specification of an artifact from a subpath within the main source.""" loader: Optional[ArtifactLoader] = None - """used in Artifact annotations for determining how to load the data""" + """used for input Artifact annotations for determining how to load the data. + + Note: A loader value of 'None' must be used with an underlying type of 'str' or Path-like class.""" output: bool = False """used to specify artifact as an output in function signature annotations""" @@ -87,6 +91,9 @@ def _check_name(self): if not self.name: raise ValueError("name cannot be `None` or empty when used") + def _get_default_inputs_path(self) -> str: + return _DEFAULT_ARTIFACT_INPUT_DIRECTORY + f"{self.name}" + def _build_archive(self) -> Optional[_ModelArchiveStrategy]: if self.archive is None: return None diff --git a/src/hera/workflows/runner.py b/src/hera/workflows/runner.py index c59a43dd4..5ce1b87d2 100644 --- a/src/hera/workflows/runner.py +++ b/src/hera/workflows/runner.py @@ -105,47 +105,67 @@ def _is_output_kwarg(key, f): ) -def _map_keys(function: Callable, kwargs: dict) -> dict: - """Change the kwargs's keys to use the Python name instead of the parameter name which could be kebab case. - - For Parameters, update their name to not contain kebab-case in Python but allow it in YAML. - For Artifacts, load the Artifact according to the given ArtifactLoader. +def _map_argo_inputs_to_function(function: Callable, kwargs: Dict) -> Dict: + """Map kwargs from Argo to the function parameters using the function's parameter annotations. + + For Parameter inputs: + * if the Parameter has a "name", replace it with the function parameter name + * otherwise use the function parameter name as-is + For Parameter outputs: + * update value to a Path object from the value_from.path value, or the default if not provided + + For Artifact inputs: + * load the Artifact according to the given ArtifactLoader + For Artifact outputs: + * update value to a Path object """ - if os.environ.get("hera__script_annotations", None) is None: - return {key.replace("-", "_"): value for key, value in kwargs.items()} - mapped_kwargs: Dict[str, Any] = {} + + def map_annotated_param(param_name: str, param_annotation: Parameter) -> None: + if param_annotation.output: + if param_annotation.value_from and param_annotation.value_from.path: + mapped_kwargs[param_name] = Path(param_annotation.value_from.path) + else: + mapped_kwargs[param_name] = _get_outputs_path(param_annotation) + # Automatically create the parent directory (if required) + mapped_kwargs[param_name].parent.mkdir(parents=True, exist_ok=True) + elif param_annotation.name: + mapped_kwargs[param_name] = kwargs[param_annotation.name] + else: + mapped_kwargs[param_name] = kwargs[param_name] + + def map_annotated_artifact(param_name: str, artifact_annotation: Artifact) -> None: + if artifact_annotation.output: + if artifact_annotation.path: + mapped_kwargs[param_name] = Path(artifact_annotation.path) + else: + mapped_kwargs[param_name] = _get_outputs_path(artifact_annotation) + # Automatically create the parent directory (if required) + mapped_kwargs[param_name].parent.mkdir(parents=True, exist_ok=True) + else: + if not artifact_annotation.path: + # Path was added to yaml automatically, we need to add it back in for the runner + artifact_annotation.path = artifact_annotation._get_default_inputs_path() + + if artifact_annotation.loader == ArtifactLoader.json.value: + path = Path(artifact_annotation.path) + mapped_kwargs[param_name] = json.load(path.open()) + elif artifact_annotation.loader == ArtifactLoader.file.value: + path = Path(artifact_annotation.path) + mapped_kwargs[param_name] = path.read_text() + elif artifact_annotation.loader is None: + mapped_kwargs[param_name] = artifact_annotation.path + for param_name, func_param in inspect.signature(function).parameters.items(): if get_origin(func_param.annotation) is Annotated: - annotated_type = get_args(func_param.annotation)[1] - - if isinstance(annotated_type, Parameter): - if annotated_type.output: - if annotated_type.value_from and annotated_type.value_from.path: - mapped_kwargs[param_name] = Path(annotated_type.value_from.path) - else: - mapped_kwargs[param_name] = _get_outputs_path(annotated_type) - # Automatically create the parent directory (if required) - mapped_kwargs[param_name].parent.mkdir(parents=True, exist_ok=True) - else: - mapped_kwargs[param_name] = kwargs[annotated_type.name] - elif isinstance(annotated_type, Artifact): - if annotated_type.output: - if annotated_type.path: - mapped_kwargs[param_name] = Path(annotated_type.path) - else: - mapped_kwargs[param_name] = _get_outputs_path(annotated_type) - # Automatically create the parent directory (if required) - mapped_kwargs[param_name].parent.mkdir(parents=True, exist_ok=True) - elif annotated_type.path: - if annotated_type.loader == ArtifactLoader.json.value: - path = Path(annotated_type.path) - mapped_kwargs[param_name] = json.load(path.open()) - elif annotated_type.loader == ArtifactLoader.file.value: - path = Path(annotated_type.path) - mapped_kwargs[param_name] = path.read_text() - elif annotated_type.loader is None: - mapped_kwargs[param_name] = annotated_type.path + func_param_annotation = get_args(func_param.annotation)[1] + + if isinstance(func_param_annotation, Parameter): + map_annotated_param(param_name, func_param_annotation) + elif isinstance(func_param_annotation, Artifact): + map_annotated_artifact(param_name, func_param_annotation) + else: + mapped_kwargs[param_name] = kwargs[param_name] else: mapped_kwargs[param_name] = kwargs[param_name] @@ -212,14 +232,12 @@ def _write_to_path(path: Path, output_value: Any) -> None: path.write_text(output_string) -def _runner(entrypoint: str, kwargs_list: Any) -> Any: - """Run a function with a list of kwargs. +def _runner(entrypoint: str, kwargs_list: List) -> Any: + """Run the function defined by the entrypoint with the given list of kwargs. Args: - entrypoint: The path to the script within the container to execute. - module: The module path to import the function from. - function_name: The name of the function to run. - kwargs_list: A list of kwargs to pass to the function. + entrypoint: The module path to the script within the container to execute. "package.submodule:function" + kwargs_list: A list of dicts with "name" and "value" keys, representing the kwargs of the function. Returns: The result of the function or `None` if the outputs are to be saved. @@ -240,7 +258,13 @@ def _runner(entrypoint: str, kwargs_list: Any) -> Any: key = cast(str, serialize(kwarg["name"])) value = kwarg["value"] kwargs[key] = value - kwargs = _map_keys(function, kwargs) + + if os.environ.get("hera__script_annotations", None) is None: + # Do a simple replacement for hyphens to get valid Python parameter names. + kwargs = {key.replace("-", "_"): value for key, value in kwargs.items()} + else: + kwargs = _map_argo_inputs_to_function(function, kwargs) + function = validate_arguments(function) function = _ignore_unmatched_kwargs(function) @@ -278,6 +302,7 @@ def _run(): # 2. Protect against files containing `null` as text with outer `or []` (as a result of using # `{{inputs.parameters}}` where the parameters key doesn't exist in `inputs`) kwargs_list = json.loads(args.args_path.read_text() or r"[]") or [] + assert isinstance(kwargs_list, List) result = _runner(args.entrypoint, kwargs_list) if not result: return diff --git a/src/hera/workflows/script.py b/src/hera/workflows/script.py index c33e518fa..b54ee7ec8 100644 --- a/src/hera/workflows/script.py +++ b/src/hera/workflows/script.py @@ -62,9 +62,6 @@ from typing_extensions import Annotated # type: ignore -_DEFAULT_ARTIFACT_INPUT_DIRECTORY = "/tmp/hera/inputs/artifacts/" - - class ScriptConstructor(BaseMixin): """A ScriptConstructor is responsible for generating the source code for a Script given a python callable. @@ -415,7 +412,9 @@ def _get_inputs_from_callable(source: Callable) -> Tuple[List[Parameter], List[A artifacts = [] for func_param in inspect.signature(source).parameters.values(): - if get_origin(func_param.annotation) is not Annotated: + if get_origin(func_param.annotation) is not Annotated or not isinstance( + get_args(func_param.annotation)[1], (Artifact, Parameter) + ): if ( func_param.default != inspect.Parameter.empty and func_param.kind == inspect.Parameter.POSITIONAL_OR_KEYWORD @@ -428,9 +427,6 @@ def _get_inputs_from_callable(source: Callable) -> Tuple[List[Parameter], List[A else: annotation = get_args(func_param.annotation)[1] - if not isinstance(annotation, (Artifact, Parameter)): - raise ValueError(f"The output {type(annotation)} cannot be used as an annotation.") - if annotation.output: continue @@ -441,7 +437,7 @@ def _get_inputs_from_callable(source: Callable) -> Tuple[List[Parameter], List[A if isinstance(new_object, Artifact): if new_object.path is None: - new_object.path = _DEFAULT_ARTIFACT_INPUT_DIRECTORY + f"{new_object.name}" + new_object.path = new_object._get_default_inputs_path() artifacts.append(new_object) elif isinstance(new_object, Parameter): diff --git a/tests/conftest.py b/tests/conftest.py index a81f37610..749c2f7e1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,4 @@ import os -from pathlib import Path -from shutil import rmtree import pytest @@ -19,13 +17,3 @@ def environ_annotations_fixture(): os.environ["hera__script_annotations"] = "" yield del os.environ["hera__script_annotations"] - - -@pytest.fixture -def tmp_path_fixture(): - # create a temporary directory - path = Path("test_outputs") - path.mkdir(exist_ok=True) - yield path - # destroy the directory - rmtree(path) diff --git a/tests/helper.py b/tests/helper.py index fcdec22de..25cefe452 100644 --- a/tests/helper.py +++ b/tests/helper.py @@ -1,9 +1 @@ ARTIFACT_PATH = "/tmp/file" - - -def my_function(a: int, b: str): - return a + b - - -def no_param_function(): - return "Hello there!" diff --git a/tests/script_annotations/outputs.py b/tests/script_annotations/outputs.py new file mode 100644 index 000000000..f405df95e --- /dev/null +++ b/tests/script_annotations/outputs.py @@ -0,0 +1,81 @@ +"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" + +try: + from typing import Annotated # type: ignore +except ImportError: + from typing_extensions import Annotated # type: ignore + +from pathlib import Path +from typing import Tuple + +from hera.shared import global_config +from hera.workflows import Artifact, Parameter, RunnerScriptConstructor, Workflow, script +from hera.workflows.steps import Steps + +global_config.experimental_features["script_runner"] = True +global_config.experimental_features["script_annotations"] = True + + +@script(constructor=RunnerScriptConstructor(outputs_directory="/user/chosen/outputs")) +def custom_output_directory( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[Path, Parameter(name="successor", output=True)], +) -> None: + successor.write_text(str(a_number + 1)) + + +@script(constructor="runner") +def output_artifact_as_function_parameter( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[Path, Artifact(name="successor", output=True)], +) -> None: + successor.write_text(a_number + 1) + + +@script(constructor="runner") +def output_parameter_as_function_parameter( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[Path, Parameter(name="successor", output=True)], +) -> None: + successor.write_text(str(a_number + 1)) + + +@script(constructor="runner") +def output_artifact_and_parameter_as_function_parameters( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[Path, Parameter(name="successor", output=True)], + successor2: Annotated[Path, Artifact(name="successor2", output=True)], +) -> None: + successor.write_text(a_number + 1) + successor2.write_text(a_number + 2) + + +@script(constructor="runner") +def outputs_in_function_parameters_and_return_signature( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[Path, Parameter(name="successor", output=True)], + successor2: Annotated[Path, Artifact(name="successor2", output=True)], +) -> Tuple[Annotated[int, Parameter(name="successor3")], Annotated[int, Artifact(name="successor4")],]: + successor.write_text(a_number + 1) + successor2.write_text(a_number + 2) + return a_number + 3, a_number + 4 + + +@script(constructor="runner") +def output_annotations_unnamed_in_function_parameters( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[Path, Parameter(output=True)], + successor2: Annotated[Path, Artifact(output=True)], +) -> None: + successor.write_text(a_number + 1) + successor2.write_text(a_number + 2) + + +with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: + with Steps(name="my-steps") as s: + custom_output_directory(arguments={"a_number": 3}) + output_artifact_as_function_parameter(arguments={"a_number": 3}) + output_parameter_as_function_parameter(arguments={"a_number": 3}) + output_artifact_and_parameter_as_function_parameters(arguments={"a_number": 3}) + outputs_in_function_parameters_and_return_signature(arguments={"a_number": 3}) + output_annotations_unnamed_in_function_parameters(arguments={"a_number": 3}) diff --git a/tests/script_annotations_artifacts/script_annotations_artifact_file.py b/tests/script_annotations_artifacts/script_annotations_artifact_file.py deleted file mode 100644 index 4a4d693cf..000000000 --- a/tests/script_annotations_artifacts/script_annotations_artifact_file.py +++ /dev/null @@ -1,27 +0,0 @@ -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - - -from tests.helper import ARTIFACT_PATH - -from hera.shared import global_config -from hera.workflows import Workflow, script -from hera.workflows.artifact import Artifact, ArtifactLoader -from hera.workflows.steps import Steps - -global_config.experimental_features["script_annotations"] = True -global_config.experimental_features["script_runner"] = True - - -@script(constructor="runner") -def read_artifact( - an_artifact: Annotated[str, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=ArtifactLoader.file)] -) -> str: - return an_artifact - - -with Workflow(generate_name="test-artifacts-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - read_artifact(arguments=[Artifact(name="my-artifact", from_="somewhere")]) diff --git a/tests/script_annotations_artifacts/script_annotations_artifact_file_with_path.py b/tests/script_annotations_artifacts/script_annotations_artifact_file_with_path.py deleted file mode 100644 index 4a4d693cf..000000000 --- a/tests/script_annotations_artifacts/script_annotations_artifact_file_with_path.py +++ /dev/null @@ -1,27 +0,0 @@ -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - - -from tests.helper import ARTIFACT_PATH - -from hera.shared import global_config -from hera.workflows import Workflow, script -from hera.workflows.artifact import Artifact, ArtifactLoader -from hera.workflows.steps import Steps - -global_config.experimental_features["script_annotations"] = True -global_config.experimental_features["script_runner"] = True - - -@script(constructor="runner") -def read_artifact( - an_artifact: Annotated[str, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=ArtifactLoader.file)] -) -> str: - return an_artifact - - -with Workflow(generate_name="test-artifacts-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - read_artifact(arguments=[Artifact(name="my-artifact", from_="somewhere")]) diff --git a/tests/script_annotations_artifacts/script_annotations_artifact_object.py b/tests/script_annotations_artifacts/script_annotations_artifact_object.py deleted file mode 100644 index 6f6b0867f..000000000 --- a/tests/script_annotations_artifacts/script_annotations_artifact_object.py +++ /dev/null @@ -1,32 +0,0 @@ -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pydantic import BaseModel -from tests.helper import ARTIFACT_PATH - -from hera.shared import global_config -from hera.workflows import Workflow, script -from hera.workflows.artifact import Artifact, ArtifactLoader -from hera.workflows.steps import Steps - -global_config.experimental_features["script_annotations"] = True -global_config.experimental_features["script_runner"] = True - - -class MyArtifact(BaseModel): - a = "a" - b = "b" - - -@script(constructor="runner") -def read_artifact( - an_artifact: Annotated[MyArtifact, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=ArtifactLoader.json)] -) -> str: - return an_artifact.a + an_artifact.b - - -with Workflow(generate_name="test-artifacts-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - read_artifact(arguments=[Artifact(name="my-artifact", from_="somewhere")]) diff --git a/tests/script_annotations_artifacts/script_annotations_artifact_path.py b/tests/script_annotations_artifacts/script_annotations_artifact_path.py deleted file mode 100644 index 70aa8f05a..000000000 --- a/tests/script_annotations_artifacts/script_annotations_artifact_path.py +++ /dev/null @@ -1,26 +0,0 @@ -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from tests.helper import ARTIFACT_PATH - -from hera.shared import global_config -from hera.workflows import Workflow, script -from hera.workflows.artifact import Artifact -from hera.workflows.steps import Steps - -global_config.experimental_features["script_annotations"] = True -global_config.experimental_features["script_runner"] = True - - -@script(constructor="runner") -def read_artifact(an_artifact: Annotated[Path, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=None)]) -> str: - return an_artifact.read_text() - - -with Workflow(generate_name="test-artifacts-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - read_artifact(arguments=[Artifact(name="my-artifact", from_="somewhere")]) diff --git a/tests/script_annotations_outputs/script_annotations_output_artifact_in_func.py b/tests/script_annotations_outputs/script_annotations_output_artifact_in_func.py deleted file mode 100644 index 24c4a4c78..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_artifact_in_func.py +++ /dev/null @@ -1,28 +0,0 @@ -"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" - -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from hera.shared import global_config -from hera.workflows import Artifact, Parameter, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True - - -@script(constructor="runner") -def script_artifact_in_fun_sig( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Artifact(name="successor", output=True)], -): - successor.write_text(a_number + 1) - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_artifact_in_fun_sig(arguments={"a_number": 3}) diff --git a/tests/script_annotations_outputs/script_annotations_output_custom_output_directory.py b/tests/script_annotations_outputs/script_annotations_output_custom_output_directory.py deleted file mode 100644 index ed6a60a01..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_custom_output_directory.py +++ /dev/null @@ -1,29 +0,0 @@ -"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" - -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from hera.shared import global_config -from hera.workflows import Parameter, RunnerScriptConstructor, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True -global_config.set_class_defaults(RunnerScriptConstructor, outputs_directory="/user/chosen/outputs") - - -@script(constructor="runner") -def script_param_in_fun_sig( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Parameter(name="successor", output=True)], -): - successor.write_text(str(a_number + 1)) - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_param_in_fun_sig(arguments={"a_number": 3}) diff --git a/tests/script_annotations_outputs/script_annotations_output_in_func_no_name.py b/tests/script_annotations_outputs/script_annotations_output_in_func_no_name.py deleted file mode 100644 index fe86cd972..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_in_func_no_name.py +++ /dev/null @@ -1,30 +0,0 @@ -"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" - -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from hera.shared import global_config -from hera.workflows import Artifact, Parameter, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True - - -@script(constructor="runner") -def script_param_in_fun_sig( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Parameter(output=True)], - successor2: Annotated[Path, Artifact(output=True)], -): - successor.write_text(a_number + 1) - successor2.write_text(a_number + 2) - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_param_in_fun_sig(arguments={"a_number": 3}) diff --git a/tests/script_annotations_outputs/script_annotations_output_multiple_calls.py b/tests/script_annotations_outputs/script_annotations_output_multiple_calls.py deleted file mode 100644 index 71ccdefd5..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_multiple_calls.py +++ /dev/null @@ -1,31 +0,0 @@ -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from hera.shared import global_config -from hera.workflows import Parameter, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True - - -@script(constructor="runner") -def script_param_in_fun_sig( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Parameter(name="successor", output=True)], -): - successor.save(a_number + 1) - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_param_in_fun_sig(arguments={"a_number": 2}) - with s.parallel() as ps: - script_param_in_fun_sig(arguments={"a_number": 3}) - script_param_in_fun_sig(arguments={"a_number": 4}) - script_param_in_fun_sig(arguments={"a_number": 5}) - script_param_in_fun_sig(arguments={"a_number": 6}) diff --git a/tests/script_annotations_outputs/script_annotations_output_param_and_artifact_in_func.py b/tests/script_annotations_outputs/script_annotations_output_param_and_artifact_in_func.py deleted file mode 100644 index a0ad7f8fc..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_param_and_artifact_in_func.py +++ /dev/null @@ -1,30 +0,0 @@ -"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" - -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from hera.shared import global_config -from hera.workflows import Artifact, Parameter, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True - - -@script(constructor="runner") -def script_param_in_fun_sig( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Parameter(name="successor", output=True)], - successor2: Annotated[Path, Artifact(name="successor2", output=True)], -): - successor.write_text(a_number + 1) - successor2.write_text(a_number + 2) - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_param_in_fun_sig(arguments={"a_number": 3}) diff --git a/tests/script_annotations_outputs/script_annotations_output_param_and_artifact_mix.py b/tests/script_annotations_outputs/script_annotations_output_param_and_artifact_mix.py deleted file mode 100644 index cf40f409e..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_param_and_artifact_mix.py +++ /dev/null @@ -1,32 +0,0 @@ -"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" - -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path -from typing import Tuple - -from hera.shared import global_config -from hera.workflows import Artifact, Parameter, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True - - -@script(constructor="runner") -def script_param_artifact_mixed( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Parameter(name="successor", output=True)], - successor2: Annotated[Path, Artifact(name="successor2", output=True)], -) -> Tuple[Annotated[int, Parameter(name="successor3")], Annotated[int, Artifact(name="successor4")],]: - successor.write_text(a_number + 1) - successor2.write_text(a_number + 2) - return a_number + 3, a_number + 4 - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_param_artifact_mixed(arguments={"a_number": 3}) diff --git a/tests/script_annotations_outputs/script_annotations_output_param_in_func.py b/tests/script_annotations_outputs/script_annotations_output_param_in_func.py deleted file mode 100644 index 5cc8325bc..000000000 --- a/tests/script_annotations_outputs/script_annotations_output_param_in_func.py +++ /dev/null @@ -1,28 +0,0 @@ -"""Test the correctness of the Output annotations. The test inspects the inputs and outputs of the workflow.""" - -try: - from typing import Annotated # type: ignore -except ImportError: - from typing_extensions import Annotated # type: ignore - -from pathlib import Path - -from hera.shared import global_config -from hera.workflows import Parameter, Workflow, script -from hera.workflows.steps import Steps - -global_config.experimental_features["script_runner"] = True -global_config.experimental_features["script_annotations"] = True - - -@script(constructor="runner") -def script_param_in_fun_sig( - a_number: Annotated[int, Parameter(name="a_number")], - successor: Annotated[Path, Parameter(name="successor", output=True)], -): - successor.save(a_number + 1) - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - script_param_in_fun_sig(arguments={"a_number": 3}) diff --git a/tests/script_annotations_outputs/script_annotations_output.py b/tests/script_runner/annotated_outputs.py similarity index 82% rename from tests/script_annotations_outputs/script_annotations_output.py rename to tests/script_runner/annotated_outputs.py index aebaa4273..1a3fcfd3c 100644 --- a/tests/script_annotations_outputs/script_annotations_output.py +++ b/tests/script_runner/annotated_outputs.py @@ -11,8 +11,7 @@ from tests.helper import ARTIFACT_PATH from hera.shared import global_config -from hera.workflows import Artifact, Parameter, Workflow, script -from hera.workflows.steps import Steps +from hera.workflows import Artifact, Parameter, script global_config.experimental_features["script_runner"] = True global_config.experimental_features["script_annotations"] = True @@ -86,7 +85,7 @@ def script_param_no_name(a_number) -> Annotated[int, Parameter()]: return a_number + 1 -@script(constructor="runner") +@script() def script_outputs_in_function_signature( a_number: Annotated[int, Parameter(name="a_number")], successor: Annotated[Path, Parameter(name="successor", output=True)], @@ -96,6 +95,18 @@ def script_outputs_in_function_signature( successor2.write_text(str(a_number + 2)) +@script() +def script_outputs_in_function_signature_with_path( + a_number: Annotated[int, Parameter(name="a_number")], + successor: Annotated[ + Path, Parameter(name="successor", value_from={"path": ARTIFACT_PATH + "/successor"}, output=True) + ], + successor2: Annotated[Path, Artifact(name="successor2", path=ARTIFACT_PATH + "/successor2", output=True)], +): + successor.write_text(str(a_number + 1)) + successor2.write_text(str(a_number + 2)) + + @script() def return_list_str() -> Annotated[List[str], Parameter(name="list-of-str")]: return ["my", "list"] @@ -115,16 +126,3 @@ def script_param_artifact_in_function_signature_and_return_type( successor.write_text(str(a_number + 1)) successor2.write_text(str(a_number + 2)) return a_number + 3, a_number + 4 - - -with Workflow(generate_name="test-outputs-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - empty_str_param() - none_param() - script_param(arguments={"a_number": 3}) - script_artifact(arguments={"a_number": 3}) - script_artifact_path(arguments={"a_number": 3}) - script_artifact_and_param(arguments={"a_number": 3}) - script_two_params(arguments={"a_number": 3}) - script_two_artifacts(arguments={"a_number": 3}) - script_param_artifact_in_function_signature_and_return_type(arguments={"a_number": 3}) diff --git a/tests/script_runner/artifact_loaders.py b/tests/script_runner/artifact_loaders.py new file mode 100644 index 000000000..ca69d93f0 --- /dev/null +++ b/tests/script_runner/artifact_loaders.py @@ -0,0 +1,64 @@ +try: + from typing import Annotated # type: ignore +except ImportError: + from typing_extensions import Annotated # type: ignore + +from pathlib import Path + +from pydantic import BaseModel +from tests.helper import ARTIFACT_PATH + +from hera.shared import global_config +from hera.workflows import script +from hera.workflows.artifact import Artifact, ArtifactLoader + +global_config.experimental_features["script_annotations"] = True +global_config.experimental_features["script_runner"] = True + + +class MyArtifact(BaseModel): + a = "a" + b = "b" + + +@script(constructor="runner") +def json_object_loader( + an_artifact: Annotated[MyArtifact, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=ArtifactLoader.json)] +) -> str: + return an_artifact.a + an_artifact.b + + +@script(constructor="runner") +def no_loader(an_artifact: Annotated[Path, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=None)]) -> str: + return an_artifact.read_text() + + +@script(constructor="runner") +def no_loader_as_string( + an_artifact: Annotated[str, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=None)] +) -> str: + """Getting the path as a string is allowed because the path in the Artifact class is a string""" + return Path(an_artifact).read_text() + + +@script(constructor="runner") +def no_loader_invalid_type( + an_artifact: Annotated[int, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=None)] +) -> str: + # Type of an_artifact must fail validation and not be allowed to become a Path + # The function code should not be reachable + pass + + +@script(constructor="runner") +def file_loader( + an_artifact: Annotated[str, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=ArtifactLoader.file)] +) -> str: + return an_artifact + + +@script(constructor="runner") +def file_loader_default_path( + an_artifact: Annotated[str, Artifact(name="my-artifact", loader=ArtifactLoader.file)] +) -> str: + return an_artifact diff --git a/tests/script_annotations_artifacts/script_annotations_artifact_wrong_loader.py b/tests/script_runner/artifact_with_invalid_loader.py similarity index 65% rename from tests/script_annotations_artifacts/script_annotations_artifact_wrong_loader.py rename to tests/script_runner/artifact_with_invalid_loader.py index 0f95d43a7..8c5c20df7 100644 --- a/tests/script_annotations_artifacts/script_annotations_artifact_wrong_loader.py +++ b/tests/script_runner/artifact_with_invalid_loader.py @@ -1,3 +1,5 @@ +"""This file causes a pydantic validation error when imported, so must be tested separately.""" + try: from typing import Annotated # type: ignore except ImportError: @@ -7,21 +9,15 @@ from tests.helper import ARTIFACT_PATH from hera.shared import global_config -from hera.workflows import Workflow, script +from hera.workflows import script from hera.workflows.artifact import Artifact -from hera.workflows.steps import Steps global_config.experimental_features["script_annotations"] = True global_config.experimental_features["script_runner"] = True @script(constructor="runner") -def read_artifact( +def invalid_loader( an_artifact: Annotated[str, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader="a different loader")] ) -> str: return an_artifact - - -with Workflow(generate_name="test-artifacts-", entrypoint="my-steps") as w: - with Steps(name="my-steps") as s: - read_artifact(arguments=[Artifact(name="my-artifact", from_="somewhere")]) diff --git a/tests/script_runner/parameter_inputs.py b/tests/script_runner/parameter_inputs.py new file mode 100644 index 000000000..1d5972d68 --- /dev/null +++ b/tests/script_runner/parameter_inputs.py @@ -0,0 +1,43 @@ +from typing import List + +try: + from typing import Annotated +except ImportError: + from typing_extensions import Annotated + +from pydantic import BaseModel + +from hera.shared import global_config +from hera.workflows import Parameter, script + +global_config.experimental_features["script_runner"] = True +global_config.experimental_features["script_annotations"] = True + + +class Input(BaseModel): + a: int + b: str = "foo" + + +class Output(BaseModel): + output: List[Input] + + +@script() +def annotated_basic_types( + a_but_kebab: Annotated[int, Parameter(name="a-but-kebab")] = 2, + b_but_kebab: Annotated[str, Parameter(name="b-but-kebab")] = "foo", +) -> Output: + return Output(output=[Input(a=a_but_kebab, b=b_but_kebab)]) + + +@script() +def annotated_object(annotated_input_value: Annotated[Input, Parameter(name="input-value")]) -> Output: + return Output(output=[annotated_input_value]) + + +@script() +def annotated_parameter_no_name( + annotated_input_value: Annotated[Input, Parameter(description="a value to input")] +) -> Output: + return Output(output=[annotated_input_value]) diff --git a/tests/script_runner/unknown_annotation_types.py b/tests/script_runner/unknown_annotation_types.py new file mode 100644 index 000000000..5e8eabb94 --- /dev/null +++ b/tests/script_runner/unknown_annotation_types.py @@ -0,0 +1,16 @@ +try: + from typing import Annotated # type: ignore +except ImportError: + from typing_extensions import Annotated # type: ignore + + +from hera.shared import global_config +from hera.workflows import script + +global_config.experimental_features["script_annotations"] = True +global_config.experimental_features["script_runner"] = True + + +@script(constructor="runner") +def unknown_annotations_ignored(my_string: Annotated[str, "some metadata"]) -> str: + return my_string diff --git a/tests/test_runner.py b/tests/test_runner.py index 626b7dc52..52a508e2e 100644 --- a/tests/test_runner.py +++ b/tests/test_runner.py @@ -1,9 +1,17 @@ +"""Test the runner with local functions. + +The functions should behave in the same way on the Argo cluster, meaning annotations +and import logic should be taken into account. The functions are not required to be a +part of a Workflow when running locally. +""" +import importlib import os from pathlib import Path -from typing import Dict, List, Literal +from typing import Dict, List from unittest.mock import MagicMock, patch import pytest +from pydantic import ValidationError import tests.helper as test_module from hera.shared import GlobalConfig @@ -37,12 +45,49 @@ ), ( "examples.workflows.callable_script:function_kebab_object", - [{"name": "input-values", "value": '{"a": 3, "b": "bar"}'}], + [{"name": "input-value", "value": '{"a": 3, "b": "bar"}'}], + '{"output": [{"a": 3, "b": "bar"}]}', + ), + ], +) +def test_runner_parameter_inputs( + entrypoint, + kwargs_list: List[Dict[str, str]], + expected_output, + global_config_fixture: GlobalConfig, + environ_annotations_fixture: None, +): + # GIVEN + global_config_fixture.experimental_features["script_annotations"] = True + global_config_fixture.experimental_features["script_runner"] = True + + # WHEN + output = _runner(entrypoint, kwargs_list) + # THEN + assert serialize(output) == expected_output + + +@pytest.mark.parametrize( + "entrypoint,kwargs_list,expected_output", + [ + ( + "tests.script_runner.parameter_inputs:annotated_basic_types", + [{"name": "a-but-kebab", "value": "3"}, {"name": "b-but-kebab", "value": "bar"}], + '{"output": [{"a": 3, "b": "bar"}]}', + ), + ( + "tests.script_runner.parameter_inputs:annotated_object", + [{"name": "input-value", "value": '{"a": 3, "b": "bar"}'}], + '{"output": [{"a": 3, "b": "bar"}]}', + ), + ( + "tests.script_runner.parameter_inputs:annotated_parameter_no_name", + [{"name": "annotated_input_value", "value": '{"a": 3, "b": "bar"}'}], '{"output": [{"a": 3, "b": "bar"}]}', ), ], ) -def test( +def test_runner_annotated_parameter_inputs( entrypoint, kwargs_list: List[Dict[str, str]], expected_output, @@ -60,151 +105,161 @@ def test( @pytest.mark.parametrize( - "entrypoint,kwargs_list,expected_files", + "function_name,kwargs_list,expected_files", [ ( - "tests.script_annotations_outputs.script_annotations_output:empty_str_param", + "empty_str_param", [], - [{"path": "tmp/hera/outputs/parameters/empty-str", "value": ""}], + [{"subpath": "tmp/hera/outputs/parameters/empty-str", "value": ""}], ), ( - "tests.script_annotations_outputs.script_annotations_output:none_param", + "none_param", [], - [{"path": "tmp/hera/outputs/parameters/null-str", "value": "null"}], + [{"subpath": "tmp/hera/outputs/parameters/null-str", "value": "null"}], + ), + ( + "script_param", + [{"name": "a_number", "value": "3"}], + [{"subpath": "tmp/hera/outputs/parameters/successor", "value": "4"}], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_param", + "script_artifact", [{"name": "a_number", "value": "3"}], - [{"path": "tmp/hera/outputs/parameters/successor", "value": "4"}], + [{"subpath": "tmp/hera/outputs/artifacts/successor", "value": "4"}], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_artifact", + "script_artifact_path", [{"name": "a_number", "value": "3"}], - [{"path": "tmp/hera/outputs/artifacts/successor", "value": "4"}], + [{"subpath": "file.txt", "value": "4"}], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_artifact_path", + "script_artifact_and_param", [{"name": "a_number", "value": "3"}], - [{"path": "file.txt", "value": "4"}], + [ + {"subpath": "tmp/hera/outputs/parameters/successor", "value": "4"}, + {"subpath": "tmp/hera/outputs/artifacts/successor", "value": "5"}, + ], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_artifact_and_param", + "script_two_params", [{"name": "a_number", "value": "3"}], [ - {"path": "tmp/hera/outputs/parameters/successor", "value": "4"}, - {"path": "tmp/hera/outputs/artifacts/successor", "value": "5"}, + {"subpath": "tmp/hera/outputs/parameters/successor", "value": "4"}, + {"subpath": "tmp/hera/outputs/parameters/successor2", "value": "5"}, ], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_two_params", + "script_two_artifacts", [{"name": "a_number", "value": "3"}], [ - {"path": "tmp/hera/outputs/parameters/successor", "value": "4"}, - {"path": "tmp/hera/outputs/parameters/successor2", "value": "5"}, + {"subpath": "tmp/hera/outputs/artifacts/successor", "value": "4"}, + {"subpath": "tmp/hera/outputs/artifacts/successor2", "value": "5"}, ], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_two_artifacts", + "script_outputs_in_function_signature", [{"name": "a_number", "value": "3"}], [ - {"path": "tmp/hera/outputs/artifacts/successor", "value": "4"}, - {"path": "tmp/hera/outputs/artifacts/successor2", "value": "5"}, + {"subpath": "tmp/hera/outputs/parameters/successor", "value": "4"}, + {"subpath": "tmp/hera/outputs/artifacts/successor2", "value": "5"}, ], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_outputs_in_function_signature", + "script_outputs_in_function_signature_with_path", [{"name": "a_number", "value": "3"}], [ - {"path": "tmp/hera/outputs/parameters/successor", "value": "4"}, - {"path": "tmp/hera/outputs/artifacts/successor2", "value": "5"}, + {"subpath": "successor", "value": "4"}, + {"subpath": "successor2", "value": "5"}, ], ), ( - "tests.script_annotations_outputs.script_annotations_output:script_param_artifact_in_function_signature_and_return_type", + "script_param_artifact_in_function_signature_and_return_type", [{"name": "a_number", "value": "3"}], [ - {"path": "tmp/hera/outputs/parameters/successor", "value": "4"}, - {"path": "tmp/hera/outputs/artifacts/successor2", "value": "5"}, - {"path": "tmp/hera/outputs/parameters/successor3", "value": "6"}, - {"path": "tmp/hera/outputs/artifacts/successor4", "value": "7"}, + {"subpath": "tmp/hera/outputs/parameters/successor", "value": "4"}, + {"subpath": "tmp/hera/outputs/artifacts/successor2", "value": "5"}, + {"subpath": "tmp/hera/outputs/parameters/successor3", "value": "6"}, + {"subpath": "tmp/hera/outputs/artifacts/successor4", "value": "7"}, ], ), ( - "tests.script_annotations_outputs.script_annotations_output:return_list_str", + "return_list_str", [], - [{"path": "tmp/hera/outputs/parameters/list-of-str", "value": '["my", "list"]'}], + [{"subpath": "tmp/hera/outputs/parameters/list-of-str", "value": '["my", "list"]'}], ), ( - "tests.script_annotations_outputs.script_annotations_output:return_dict", + "return_dict", [], - [{"path": "tmp/hera/outputs/parameters/dict-of-str", "value": '{"my-key": "my-value"}'}], + [{"subpath": "tmp/hera/outputs/parameters/dict-of-str", "value": '{"my-key": "my-value"}'}], ), ], ) def test_script_annotations_outputs( - entrypoint, + function_name, kwargs_list: List[Dict[str, str]], expected_files: List[Dict[str, str]], global_config_fixture: GlobalConfig, environ_annotations_fixture: None, - tmp_path_fixture: Path, + tmp_path: Path, monkeypatch, ): """Test that the output annotations are parsed correctly and save outputs to correct destinations.""" for file in expected_files: - assert not Path(tmp_path_fixture / file["path"]).is_file() + assert not Path(tmp_path / file["subpath"]).is_file() # GIVEN global_config_fixture.experimental_features["script_annotations"] = True global_config_fixture.experimental_features["script_runner"] = True - outputs_directory = str(tmp_path_fixture / "tmp/hera/outputs") + outputs_directory = str(tmp_path / "tmp/hera/outputs") global_config_fixture.set_class_defaults(RunnerScriptConstructor, outputs_directory=outputs_directory) - monkeypatch.setattr(test_module, "ARTIFACT_PATH", str(tmp_path_fixture)) + monkeypatch.setattr(test_module, "ARTIFACT_PATH", str(tmp_path)) os.environ["hera__outputs_directory"] = outputs_directory + # Force a reload of the test module, as the runner performs "importlib.import_module", which + # may fetch a cached version + import tests.script_runner.annotated_outputs as output_tests_module + + importlib.reload(output_tests_module) + # WHEN - output = _runner(entrypoint, kwargs_list) + output = _runner(f"{output_tests_module.__name__}:{function_name}", kwargs_list) # THEN assert output is None, "Runner should not return values directly when using return Annotations" for file in expected_files: - assert Path(tmp_path_fixture / file["path"]).is_file() - assert Path(tmp_path_fixture / file["path"]).read_text() == file["value"] + assert Path(tmp_path / file["subpath"]).is_file() + assert Path(tmp_path / file["subpath"]).read_text() == file["value"] @pytest.mark.parametrize( - "entrypoint,kwargs_list,exception", + "function_name,kwargs_list,exception", [ ( - "tests.script_annotations_outputs.script_annotations_output:script_two_params_one_output", + "script_two_params_one_output", [{"name": "a_number", "value": "3"}], "The number of outputs does not match the annotation", ), ( - "tests.script_annotations_outputs.script_annotations_output:script_param_incorrect_basic_type", + "script_param_incorrect_basic_type", [{"name": "a_number", "value": "3"}], "The type of output `successor`, `` does not match the annotated type ``", ), ( - "tests.script_annotations_outputs.script_annotations_output:script_param_incorrect_generic_type", + "script_param_incorrect_generic_type", [{"name": "a_number", "value": "3"}], "The type of output `successor`, `` does not match the annotated type `typing.Dict[str, str]`", ), ( - "tests.script_annotations_outputs.script_annotations_output:script_param_no_name", + "script_param_no_name", [{"name": "a_number", "value": "3"}], "The name was not provided for one of the outputs.", ), ], ) def test_script_annotations_outputs_exceptions( - entrypoint: Literal["tests.script_annotations_outputs.script_annotation…"], + function_name, kwargs_list: List[Dict[str, str]], - exception: Literal[ - "The number of outputs does not match the annotatio…", - "The type of output `successor`, `` do…", - "The name was not provided for one of the outputs.", - ], + exception, global_config_fixture: GlobalConfig, environ_annotations_fixture: None, ): @@ -215,7 +270,7 @@ def test_script_annotations_outputs_exceptions( # WHEN with pytest.raises(ValueError) as e: - _ = _runner(entrypoint, kwargs_list) + _ = _runner(f"tests.script_runner.annotated_outputs:{function_name}", kwargs_list) # THEN assert exception in str(e.value) @@ -224,16 +279,16 @@ def test_script_annotations_outputs_exceptions( "entrypoint,kwargs_list,expected_output", [ ( - "tests.script_annotations_outputs.script_annotations_output:script_param", + "tests.script_runner.annotated_outputs:script_param", [{"name": "a_number", "value": "3"}], "4", ) ], ) def test_script_annotations_outputs_no_global_config( - entrypoint: Literal["tests.script_annotations_outputs.script_annotation…"], + entrypoint, kwargs_list: Dict[str, str], - expected_output: Literal["4"], + expected_output, ): """Test that the output annotations are ignored when global_config is not set.""" # WHEN @@ -244,34 +299,39 @@ def test_script_annotations_outputs_no_global_config( @pytest.mark.parametrize( - "entrypoint,file_contents,expected_output", + "function,file_contents,expected_output", [ ( - "tests.script_annotations_artifacts.script_annotations_artifact_path:read_artifact", - "Hello there!", - "Hello there!", + "no_loader", + "First test!", + "First test!", + ), + ( + "no_loader_as_string", + "Another test", + "Another test", ), ( - "tests.script_annotations_artifacts.script_annotations_artifact_object:read_artifact", + "json_object_loader", """{"a": "Hello ", "b": "there!"}""", "Hello there!", ), ( - "tests.script_annotations_artifacts.script_annotations_artifact_file:read_artifact", - "Hello there!", - "Hello there!", + "file_loader", + "This file had a path", + "This file had a path", ), ( - "tests.script_annotations_artifacts.script_annotations_artifact_file_with_path:read_artifact", - "/this/is/a/path", - "/this/is/a/path", + "file_loader", + "/this/file/contains/a/path", # A file containing a path as a string (we should not do any further processing) + "/this/file/contains/a/path", ), ], ) -def test_script_annotations_artifacts( - entrypoint: Literal["tests.script_annotations_artifacts.script_annotati…"], - file_contents: Literal["Hello there!", '{"a": "Hello ", "b": "there!"}', "/this/is/a/path"], - expected_output: Literal["Hello there!", "/this/is/a/path"], +def test_script_annotations_artifact_inputs( + function, + file_contents, + expected_output, tmp_path: Path, monkeypatch, global_config_fixture: GlobalConfig, @@ -279,31 +339,97 @@ def test_script_annotations_artifacts( """Test that the input artifact annotations are parsed correctly and the loaders behave as intended.""" # GIVEN filepath = tmp_path / "my_file.txt" - filepath.write_text(file_contents) - import tests.helper as test_module monkeypatch.setattr(test_module, "ARTIFACT_PATH", str(filepath)) + + # Force a reload of the test module, as the runner performs "importlib.import_module", which + # may fetch a cached version + import tests.script_runner.artifact_loaders as module + + importlib.reload(module) + kwargs_list = [] global_config_fixture.experimental_features["script_annotations"] = True os.environ["hera__script_annotations"] = "" # WHEN - output = _runner(entrypoint, kwargs_list) + output = _runner(f"{module.__name__}:{function}", kwargs_list) # THEN assert serialize(output) == expected_output +def test_script_annotations_artifact_input_loader_error( + global_config_fixture: GlobalConfig, + environ_annotations_fixture: None, +): + """Test that the input artifact loaded with wrong type throws the expected exception.""" + # GIVEN + function_name = "no_loader_invalid_type" + kwargs_list = [] + global_config_fixture.experimental_features["script_annotations"] = True + global_config_fixture.experimental_features["script_runner"] = True + + # Force a reload of the test module, as the runner performs "importlib.import_module", which + # may fetch a cached version + import tests.script_runner.artifact_loaders as module + + importlib.reload(module) + + # WHEN + with pytest.raises(ValidationError) as e: + _ = _runner(f"{module.__name__}:{function_name}", kwargs_list) + + # THEN + assert "value is not a valid integer" in str(e.value) + + @pytest.mark.parametrize( - "entrypoint", - ["tests.script_annotations_artifacts.script_annotations_artifact_wrong_loader:read_artifact"], + "entrypoint,artifact_name,file_contents,expected_output", + [ + ( + "tests.script_runner.artifact_loaders:file_loader_default_path", + "my-artifact", + "Hello there!", + "Hello there!", + ), + ], ) +def test_script_annotations_artifacts_no_path( + entrypoint, + artifact_name, + file_contents, + expected_output, + tmp_path: Path, + monkeypatch, + global_config_fixture: GlobalConfig, +): + """Test that the input artifact annotations are parsed correctly and the loaders behave as intended.""" + # GIVEN + filepath = tmp_path / f"{artifact_name}" + filepath.write_text(file_contents) + + # Trailing slash required + monkeypatch.setattr("hera.workflows.artifact._DEFAULT_ARTIFACT_INPUT_DIRECTORY", f"{tmp_path}/") + + kwargs_list = [] + global_config_fixture.experimental_features["script_annotations"] = True + os.environ["hera__script_annotations"] = "" + + # WHEN + output = _runner(entrypoint, kwargs_list) + + # THEN + assert serialize(output) == expected_output + + def test_script_annotations_artifacts_wrong_loader( - entrypoint: Literal["tests.script_annotations_artifacts.script_annotati…"], global_config_fixture: GlobalConfig + global_config_fixture: GlobalConfig, ): """Test that the input artifact annotation with no loader throws an exception.""" # GIVEN + entrypoint = "tests.script_runner.artifact_with_invalid_loader:invalid_loader" kwargs_list = [] global_config_fixture.experimental_features["script_annotations"] = True os.environ["hera__script_annotations"] = "" @@ -316,6 +442,21 @@ def test_script_annotations_artifacts_wrong_loader( assert "value is not a valid enumeration member" in str(e.value) +def test_script_annotations_unknown_type(global_config_fixture: GlobalConfig): + # GIVEN + expected_output = "a string" + entrypoint = "tests.script_runner.unknown_annotation_types:unknown_annotations_ignored" + kwargs_list = [{"name": "my_string", "value": expected_output}] + global_config_fixture.experimental_features["script_annotations"] = True + os.environ["hera__script_annotations"] = "" + + # WHEN + output = _runner(entrypoint, kwargs_list) + + # THEN + assert serialize(output) == expected_output + + @pytest.mark.parametrize( "kwargs_list", [ diff --git a/tests/test_script_annotations.py b/tests/test_script_annotations.py index 698c73dfe..c7a99476c 100644 --- a/tests/test_script_annotations.py +++ b/tests/test_script_annotations.py @@ -1,3 +1,4 @@ +"""Test script annotations are built correctly within workflows.""" import importlib import pytest @@ -65,13 +66,14 @@ def test_script_annotations_artifact_regression(module_name, global_config_fixtu _compare_workflows(workflow_old, output_old, output_new) -@script() -def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2): - print(an_int) - - 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 + @script() + def echo_int(an_int: Annotated[int, Parameter(default=1)] = 2): + print(an_int) + global_config_fixture.experimental_features["script_annotations"] = True with pytest.raises(ValueError) as e: with Workflow(generate_name="test-default-", entrypoint="my-steps") as w: @@ -84,20 +86,20 @@ def test_double_default_throws_a_value_error(global_config_fixture): @pytest.mark.parametrize( - "module,expected_input,expected_output", + "function_name,expected_input,expected_output", [ ( - "script_annotations_output_param_in_func", + "output_parameter_as_function_parameter", {"parameters": [{"name": "a_number"}]}, {"parameters": [{"name": "successor", "valueFrom": {"path": "/tmp/hera/outputs/parameters/successor"}}]}, ), ( - "script_annotations_output_artifact_in_func", + "output_artifact_as_function_parameter", {"parameters": [{"name": "a_number"}]}, {"artifacts": [{"name": "successor", "path": "/tmp/hera/outputs/artifacts/successor"}]}, ), ( - "script_annotations_output_param_and_artifact_in_func", + "output_artifact_and_parameter_as_function_parameters", {"parameters": [{"name": "a_number"}]}, { "parameters": [{"name": "successor", "valueFrom": {"path": "/tmp/hera/outputs/parameters/successor"}}], @@ -105,7 +107,7 @@ def test_double_default_throws_a_value_error(global_config_fixture): }, ), ( - "script_annotations_output_param_and_artifact_mix", + "outputs_in_function_parameters_and_return_signature", {"parameters": [{"name": "a_number"}]}, { "parameters": [ @@ -119,7 +121,7 @@ def test_double_default_throws_a_value_error(global_config_fixture): }, ), ( - "script_annotations_output_in_func_no_name", + "output_annotations_unnamed_in_function_parameters", {"parameters": [{"name": "a_number"}]}, { "parameters": [{"name": "successor", "valueFrom": {"path": "/tmp/hera/outputs/parameters/successor"}}], @@ -127,7 +129,7 @@ def test_double_default_throws_a_value_error(global_config_fixture): }, ), ( - "script_annotations_output_custom_output_directory", + "custom_output_directory", {"parameters": [{"name": "a_number"}]}, { "parameters": [ @@ -137,23 +139,26 @@ def test_double_default_throws_a_value_error(global_config_fixture): ), ], ) -def test_script_annotations_outputs(module, expected_input, expected_output, global_config_fixture): - """Test that output annotations work correctly by asserting correct inputs and outputs.""" +def test_script_annotated_outputs(function_name, expected_input, expected_output, global_config_fixture): + """Test that output annotations work correctly by asserting correct inputs and outputs on the built workflow.""" # GIVEN global_config_fixture.experimental_features["script_annotations"] = True - workflow = importlib.import_module(f"tests.script_annotations_outputs.{module}").w + # Force a reload of the test module, as the runner performs "importlib.import_module", which + # may fetch a cached version + import tests.script_annotations.outputs as module + + importlib.reload(module) + workflow = importlib.import_module("tests.script_annotations.outputs").w # WHEN workflow_dict = workflow.to_dict() assert workflow == Workflow.from_dict(workflow_dict) assert workflow == Workflow.from_yaml(workflow.to_yaml()) - inputs = workflow_dict["spec"]["templates"][1]["inputs"] - outputs = workflow_dict["spec"]["templates"][1]["outputs"] - # THEN - assert inputs == expected_input - assert outputs == expected_output + template = next(filter(lambda t: t["name"] == function_name.replace("_", "-"), workflow_dict["spec"]["templates"])) + assert template["inputs"] == expected_input + assert template["outputs"] == expected_output def test_configmap(global_config_fixture): diff --git a/tests/test_unit/test_script.py b/tests/test_unit/test_script.py index 2ba5d2330..e8f846b74 100644 --- a/tests/test_unit/test_script.py +++ b/tests/test_unit/test_script.py @@ -1,17 +1,22 @@ -import importlib +try: + from typing import Annotated # type: ignore +except ImportError: + from typing_extensions import Annotated # type: ignore + +from pathlib import Path from hera.workflows import Workflow, script +from hera.workflows.artifact import Artifact from hera.workflows.script import _get_inputs_from_callable def test_get_inputs_from_callable_simple_params(): # GIVEN - entrypoint = "tests.helper:my_function" - module, function_name = entrypoint.split(":") - function = getattr(importlib.import_module(module), function_name) + def my_function(a: int, b: str): + return a + b # WHEN - params, artifacts = _get_inputs_from_callable(function) + params, artifacts = _get_inputs_from_callable(my_function) # THEN assert params is not None @@ -28,57 +33,41 @@ def test_get_inputs_from_callable_simple_params(): def test_get_inputs_from_callable_no_params(): # GIVEN - entrypoint = "tests.helper:no_param_function" - module, function_name = entrypoint.split(":") - function = getattr(importlib.import_module(module), function_name) + def no_param_function(): + return "Hello there!" # WHEN - params, artifacts = _get_inputs_from_callable(function) + params, artifacts = _get_inputs_from_callable(no_param_function) # THEN assert params == [] assert artifacts == [] -def test_get_inputs_from_callable_simple_artifact(tmp_path, monkeypatch): +def test_get_artifact_input(): # GIVEN - if not tmp_path.is_file(): - tmp_path = tmp_path / "my_file.txt" - - tmp_path.touch() - tmp_path.write_text("Hello there") - import tests.helper as test_module - - monkeypatch.setattr(test_module, "ARTIFACT_PATH", str(tmp_path)) - - entrypoint = "tests.script_annotations_artifacts.script_annotations_artifact_path:read_artifact" - module, function_name = entrypoint.split(":") - md = importlib.import_module(module) - importlib.reload(md) - function = getattr(md, function_name) + def input_artifact_function(an_artifact: Annotated[Path, Artifact(name="my-artifact", path="my-file.txt")]) -> str: + return an_artifact.read_text() # WHEN - params, artifacts = _get_inputs_from_callable(function) + params, artifacts = _get_inputs_from_callable(input_artifact_function) # THEN assert params == [] - assert artifacts is not None - assert isinstance(artifacts, list) assert len(artifacts) == 1 an_artifact = artifacts[0] assert an_artifact.name == "my-artifact" - assert an_artifact.path == str(tmp_path) - - -@script(name="my-alt-name") -def my_func(): - print("Hello world!") + assert an_artifact.path == "my-file.txt" def test_script_name_kwarg_in_decorator(): - # GIVEN my_func above + # GIVEN + @script(name="my-alt-name") + def my_func(): + print("Hello world!") + # WHEN with Workflow(name="test-script") as w: my_func() @@ -109,3 +98,22 @@ def my_func2(): assert len(w.templates) == 2 assert w.templates[0].name == "my-func" assert w.templates[1].name == "test" + + +def test_script_ignores_unknown_annotations(): + # GIVEN + @script() + def unknown_annotations_ignored(my_string: Annotated[str, "some metadata"]) -> str: + return my_string + + # WHEN + params, artifacts = _get_inputs_from_callable(unknown_annotations_ignored) + + # THEN + assert artifacts == [] + assert isinstance(params, list) + assert len(params) == 1 + parameter = params[0] + + assert parameter.name == "my_string" + assert parameter.default is None diff --git a/tests/test_unit/test_workflow.py b/tests/test_unit/test_workflow.py index f6c33eae4..bf1a85eae 100644 --- a/tests/test_unit/test_workflow.py +++ b/tests/test_unit/test_workflow.py @@ -50,13 +50,12 @@ def test_workflow_create(): ) -def test_workflow_to_file(tmpdir): +def test_workflow_to_file(tmp_path: Path): # GIVEN workflow = importlib.import_module("examples.workflows.coinflip").w - output_dir = Path(tmpdir) # WHEN - yaml_path = workflow.to_file(output_dir) + yaml_path = workflow.to_file(tmp_path) # THEN assert yaml_path.exists()