Fix issues with with_param #1236

alicederyn · 2024-10-10T08:01:17Z

Pull Request Checklist

Fixes Script annotation inputs doesn't play well with with_param #861 and fixes Issue using with_param and Pydantic IO #1234
Tests added
Documentation/examples added
Good commit messages and/or PR title

Description of PR
Currently, the logic implementing with_param assumes that Argo Parameters exactly match script function parameters, including the Python parameter name. This does not support the experimental script_annotations and script_pydantic_io features.

This PR extracts common logic for creating an IO object from parameter/field metadata into a construct_io_from_annotation function, which copies any IO annotation it finds, sets the name if missing, and defaults to a Parameter if no annotation is found. It then uses this to generalize _get_param_items_from_source, including:

pulling the parameters from a Pydantic Input type instead, if present
respecting the name in the Parameter annotation, if present
ignoring output=True annotations
ignoring Artifact annotations

Many places need to inspect a parameter or field to construct a Parameter or Artifact based on the name and other annotations. Extract the single-field logic from _construct_io_from_fields to simplify many callsites that were using get_workflow_annotation and stitching in the name and default Parameter themselves. This will also allow us to reliably provide more features in future, such as inferring an enum parameter for a Literal. Signed-off-by: Alice Purcell <[email protected]>

Add three unit tests of the working current behaviour to prevent regression. Signed-off-by: Alice Purcell <[email protected]>

Support Parameter/Artifact annotations in _get_param_items_from_source. If a Parameter annotation is provided, the name will override the name of the Python parameter; additionally, if it is an output parameter, or if there is an Artifact annotation, it will be skipped. Signed-off-by: Alice Purcell <[email protected]>

Support Pydantic Input subclasses in _get_param_items_from_source. Signed-off-by: Alice Purcell <[email protected]>

Add examples reproducing issues argoproj-labs#861 (using with_param with an annotated input) and argoproj-labs#1234 (using with_param with a Pydantic Input type). Signed-off-by: Alice Purcell <[email protected]>

alicederyn · 2024-10-10T08:02:44Z

src/hera/workflows/_meta_mixins.py

+                # as the default value ones are captured by the automatically generated `Parameter` fields for positional
+                # kwargs. Otherwise, we assume that the user sets the value of the parameter via the `with_param` field
+                io = construct_io_from_annotation(p.name, p.annotation)
+                if isinstance(io, Parameter) and io.default is None and not io.output:


I wasn't sure whether it made sense to include Artifacts here or not; if this was the wrong decision, LMK and I'll fix it.

for me it looks reasonable while reading callee function (it is _get_deduped_params_from_source, and returns List[Parameter])

Perhaps we should be raising an error instead?

We only want to inspect parameters in this function. We shouldn't raise an error (as Artifact defaults work differently)

codecov · 2024-10-10T08:03:56Z

Codecov Report

Attention: Patch coverage is 12.24490% with 43 lines in your changes missing coverage. Please review.

Project coverage is 45.8%. Comparing base (a08f744) to head (e5708fe).

Files with missing lines	Patch %	Lines
src/hera/workflows/_meta_mixins.py	8.6%	21 Missing ⚠️
src/hera/workflows/script.py	7.1%	13 Missing ⚠️
src/hera/shared/_type_util.py	16.6%	5 Missing ⚠️
src/hera/workflows/_mixins.py	25.0%	3 Missing ⚠️
src/hera/workflows/io/_io_mixins.py	50.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1236     +/-   ##
=======================================
- Coverage   45.9%   45.8%   -0.1%     
=======================================
  Files         60      60             
  Lines       4082    4094     +12     
  Branches     857     858      +1     
=======================================
+ Hits        1875    1877      +2     
- Misses      2175    2185     +10     
  Partials      32      32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alicederyn · 2024-10-10T08:35:20Z

It looks like the code coverage is broken, because it's complaining about lines not being tested that definitely are.

alicederyn · 2024-10-10T08:41:37Z

src/hera/workflows/io/_io_mixins.py

-    If a field has a Parameter or Artifact annotation, a copy will be returned, with name added if missing.
-    Otherwise, a Parameter object will be constructed.
+    If a field has a Parameter or Artifact annotation, a copy will be returned, with missing
+    fields filled out based on other metadata. Otherwise, a Parameter object will be constructed.


Note: I changed this docstring because the core logic is now in a different function, so it will be easy to miss changes in future and bitrot. I intend to change the behaviour as part of fixing #1173, for instance, to set the enum field for Literals.

Signed-off-by: Alice Purcell <[email protected]>

jeongukjae

other things looks good

src/hera/shared/_type_util.py

jeongukjae · 2024-10-10T11:00:47Z

src/hera/workflows/_meta_mixins.py

+                # as the default value ones are captured by the automatically generated `Parameter` fields for positional
+                # kwargs. Otherwise, we assume that the user sets the value of the parameter via the `with_param` field
+                io = construct_io_from_annotation(p.name, p.annotation)
+                if isinstance(io, Parameter) and io.default is None and not io.output:


for me it looks reasonable while reading callee function (it is _get_deduped_params_from_source, and returns List[Parameter])

jeongukjae · 2024-10-10T11:02:56Z

src/hera/workflows/io/_io_mixins.py

-        if annotation := get_workflow_annotation(annotations[field]):
-            # Copy so as to not modify the fields themselves
-            annotation_copy = annotation.copy()
-            annotation_copy.name = annotation.name or field
-            yield field, field_info, annotation_copy
-        else:
-            yield field, field_info, Parameter(name=field)
+        yield field, field_info, construct_io_from_annotation(field, annotations[field])


src/hera/workflows/script.py

    for name, p in inspect.signature(source).parameters.items():
-        annotation = get_workflow_annotation(p.annotation)
-        if not annotation or not annotation.output:
+        annotation = construct_io_from_annotation(name, p.annotation)


Co-authored-by: Ukjae Jeong <[email protected]> Signed-off-by: Alice <[email protected]>

jeongukjae

looks good to me :)

elliotgunton · 2024-10-11T09:32:19Z

src/hera/workflows/_meta_mixins.py

+    for param in non_default_parameters:
+        param.value = "{{" + ("item" if len(non_default_parameters) == 1 else f"item.{param.name}") + "}}"


This seems harder to read/understand IMO

elliotgunton · 2024-10-11T09:38:01Z

examples/workflows/experimental/script_runner_io_dynamic_fanout.py

@@ -0,0 +1,38 @@
+"""


I don't think we need to add these two examples - they are not demonstrating something unique - script annotations/pydantic IO have their own examples, and the syntax for with_param is more simply shown in the loops examples, where the syntax in the DAG construction is the same.

Fair, I added them to exercise the code rather than as examples per se. Is there a better place to put this kind of end-to-end YAML output test?

You could do a roundtrip to_dict/from_dict like in the test_script_annotations tests

hera/tests/test_script_annotations.py

Lines 373 to 374 in a08f744

workflow_dict = workflow.to_dict()

assert workflow == Workflow.from_dict(workflow_dict)

alicederyn added 5 commits October 9, 2024 19:32

Add three tests for _get_param_items_from_source

4451959

Add three unit tests of the working current behaviour to prevent regression. Signed-off-by: Alice Purcell <[email protected]>

Support Pydantic inputs and with_param

2f820a0

Support Pydantic Input subclasses in _get_param_items_from_source. Signed-off-by: Alice Purcell <[email protected]>

Add examples reproducing raised issues

3e0177e

Add examples reproducing issues argoproj-labs#861 (using with_param with an annotated input) and argoproj-labs#1234 (using with_param with a Pydantic Input type). Signed-off-by: Alice Purcell <[email protected]>

alicederyn commented Oct 10, 2024

View reviewed changes

alicederyn added semver:patch A change requiring a patch version bump type:bug A general bug labels Oct 10, 2024

alicederyn marked this pull request as ready for review October 10, 2024 08:35

alicederyn requested review from flaviuvadan, sambhav and elliotgunton as code owners October 10, 2024 08:35

alicederyn commented Oct 10, 2024

View reviewed changes

Add docstring

c0a3b63

Signed-off-by: Alice Purcell <[email protected]>

jeongukjae reviewed Oct 10, 2024

View reviewed changes

Remove unnecessary else before return

e5708fe

Co-authored-by: Ukjae Jeong <[email protected]> Signed-off-by: Alice <[email protected]>

jeongukjae approved these changes Oct 10, 2024

View reviewed changes

elliotgunton reviewed Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with with_param #1236

Fix issues with with_param #1236

alicederyn commented Oct 10, 2024

alicederyn Oct 10, 2024

jeongukjae Oct 10, 2024

alicederyn Oct 10, 2024

elliotgunton Oct 11, 2024

codecov bot commented Oct 10, 2024 •

edited

Loading

alicederyn commented Oct 10, 2024

alicederyn Oct 10, 2024

jeongukjae left a comment

jeongukjae Oct 10, 2024

jeongukjae Oct 10, 2024

This comment was marked as resolved.

jeongukjae left a comment

elliotgunton Oct 11, 2024

elliotgunton Oct 11, 2024

alicederyn Oct 11, 2024

elliotgunton Oct 11, 2024

		for param in non_default_parameters:
		param.value = "{{" + ("item" if len(non_default_parameters) == 1 else f"item.{param.name}") + "}}"

	workflow_dict = workflow.to_dict()
	assert workflow == Workflow.from_dict(workflow_dict)

Fix issues with with_param #1236

Are you sure you want to change the base?

Fix issues with with_param #1236

Conversation

alicederyn commented Oct 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

alicederyn commented Oct 10, 2024

Choose a reason for hiding this comment

jeongukjae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

jeongukjae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2024 •

edited

Loading