Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better get_parameter/get_artifact support #595

Closed
3 of 4 tasks
elliotgunton opened this issue Apr 26, 2023 · 0 comments · Fixed by #639
Closed
3 of 4 tasks

Better get_parameter/get_artifact support #595

elliotgunton opened this issue Apr 26, 2023 · 0 comments · Fixed by #639
Assignees
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement

Comments

@elliotgunton
Copy link
Collaborator

elliotgunton commented Apr 26, 2023

A goal of Hera should be to avoid the use of Argo expression syntax for Parameters and Artifacts, as without seeing the yaml itself it's not obvious what the keys are. E.g in the script_auto_infer example, a good syntax for the artifact would be:

    with DAG(name="d"):
        p = produce()
        c = consume(arguments=Artifact(name="i", from_=p.get_artifact("result")))
        p >> c

Currently, there's no Hera syntax for fetching a Workflow argument, example taking inspiration from https://argoproj.github.io/argo-workflows/workflow-inputs/

@script(constructor="inline")
def echo(message: str) -> None:
    print(message)

with WorkflowTemplate(
    name="my-wt-with-args",
    entrypoint="main",
    arguments=[
        Parameter(name="arg-1"),
        Parameter(name="arg-2"),
    ],
) as wt:
    Steps(name="main"):
        echo(
            name="test",
            arguments={
                "message": "{{workflow.parameters.arg-1}}",
            },
        )

Something like wt.get_argument("arg-1") would be better.


Addressing this issue would also be a good time to consolidate the get_parameter[s_as] functions that are duplicated (except for "steps." vs "tasks." in obj.value assignment this was a bug addressed in #624) in Step and Task


We've also had feedback that knowing whether to use value or value_from can be confusing (e.g. you could do value="{{inputs.parameters.my-value}} or (I think) value_from=ValueFrom(expression="inputs.parameters.my-value") - maybe we can figure out a way to assign to one of these automatically based on the input? Both of these are still using Argo expressions though.


Tasks:

Examples to update:

  • script_argument_passing.py
  • global_parameters.py
@elliotgunton elliotgunton added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Apr 26, 2023
@elliotgunton elliotgunton self-assigned this Apr 26, 2023
elliotgunton added a commit that referenced this issue Apr 26, 2023
Prepping example for #595, will make the changes to improve the Hera
syntax in another PR

Signed-off-by: Elliot Gunton <[email protected]>
elliotgunton added a commit that referenced this issue Apr 26, 2023
Fixes #591 with a script example to mirror `artifact_passing.py`
upstream example.

Will also be useful as an example of artifact passing noted in #595

Signed-off-by: Elliot Gunton <[email protected]>
elliotgunton added a commit that referenced this issue May 9, 2023
**Description of PR**

Currently, using `get_parameter` on a step fails due to the validator as
value/value_from are mutually exclusive, e.g.
```py
            Step(
                name="do-retry",
                template=...
                when=f"{retry_step.get_parameter('retry-param')}==retry",
            )
```

This PR copies the fix for Task from #565 for Step.

Only doing a small fix as the duplicated function code should be
addressed in #595

Signed-off-by: Elliot Gunton <[email protected]>
flaviuvadan added a commit that referenced this issue May 26, 2023
<!-- Thank you for submitting a PR to Hera! 🚀 -->

**Pull Request Checklist**
- [x] Fixes #595 
- [ ] Tests added
- [x] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title
<!-- Also remember to sign off commits or the DCO check will fail on
your PR! -->

**Description of PR**
<!-- If not linked to an issue, please describe your changes here -->

@elliotgunton and I bounced some thoughts on improving access to
parameters/artifact via #595 and #613. This PR adds the implementation
@elliotgunton suggested to the template invokator class. Open to having
these on a different mixin!

<!-- Piece of cake! ✨🍰✨ -->

---------

Signed-off-by: Flaviu Vadan <[email protected]>
Signed-off-by: Elliot Gunton <[email protected]>
Co-authored-by: Sambhav Kothari <[email protected]>
Co-authored-by: Elliot Gunton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump type:enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant