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

Script annotations artifacts #718

Merged

Conversation

dejamiko
Copy link
Contributor

@dejamiko dejamiko commented Jul 20, 2023

Pull Request Checklist

Description of PR
In Hera we are able to specify Artifacts in inputs:

@script(inputs=Artifact(name="my-artifact", path="/tmp/file"))
def read_artifact():
    with open("/tmp/file") as a_file:
        print(a_file.read())

But by using annotations we can avoid repeating the path of the file, and access the artifact as if it's a regular Python argument of the given type:

@script()
def read_artifact(an_artifact: Annotated[Path, Artifact(name="my-artifact", path="/tmp/file")]):
    print(an_artifact.read_text())

The path duplication is no longer necessary since an_artifact is a Path object.

The fields allowed in the Artifact annotations are: name, path, and loader.
For Artifacts, we allow three types of loaders. Using ArtifactLoader, we have file, json,
and None.
With no loader, the path attribute of Artifact is extracted and can be subsequently used in the function body by referring to the function parameter. This can be seen above.
When the loader is set to file, the function parameter will be the contents of the file
stored at path.

@script()
def read_artifact(
    an_artifact: Annotated[str, Artifact(name="my-artifact", path=ARTIFACT_PATH, loader=ArtifactLoader.file)]
) -> str:
    return an_artifact

This loads the contents of the file at ARTIFACT_PATH to the argument an_artifact and subsequently can be used as a string inside the function.

When the loader is set to json, the contents of the file at path are read and parsed to json.

class MyArtifact(BaseModel):
    a = "a"
    b = "b"


@script()
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

Here, we have a json representation of MyArtifact stored at ARTIFACT_PATH. We can load it with ArtifactLoader.json and then use an_artifact as an instance of MyArtifact inside the function.

@elliotgunton elliotgunton marked this pull request as draft July 20, 2023 14:50
@elliotgunton
Copy link
Collaborator

We'll need to merge #708 first, and then you can rebase this branch to still merge to main. atm this PR includes all the #708 changes

@elliotgunton elliotgunton added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Jul 24, 2023
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #718 (dea7696) into main (d835fcf) will increase coverage by 0.2%.
Report is 1 commits behind head on main.
The diff coverage is 88.5%.

@@           Coverage Diff           @@
##            main    #718     +/-   ##
=======================================
+ Coverage   77.7%   77.9%   +0.2%     
=======================================
  Files         45      45             
  Lines       3388    3472     +84     
  Branches     649     676     +27     
=======================================
+ Hits        2633    2707     +74     
- Misses       574     578      +4     
- Partials     181     187      +6     
Files Changed Coverage Δ
src/hera/workflows/__init__.py 100.0% <ø> (ø)
src/hera/workflows/container_set.py 92.0% <ø> (ø)
src/hera/workflows/cron_workflow.py 90.0% <ø> (ø)
src/hera/workflows/dag.py 81.8% <ø> (ø)
src/hera/workflows/task.py 80.1% <ø> (ø)
src/hera/workflows/workflow.py 79.6% <ø> (ø)
src/hera/workflows/workflow_template.py 97.1% <ø> (ø)
src/hera/workflows/script.py 86.8% <81.6%> (-2.7%) ⬇️
src/hera/workflows/runner.py 74.4% <88.2%> (+0.3%) ⬆️
src/hera/workflows/artifact.py 97.7% <97.2%> (-0.3%) ⬇️
... and 1 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dejamiko dejamiko marked this pull request as ready for review July 24, 2023 15:47
src/hera/workflows/runner.py Outdated Show resolved Hide resolved
src/hera/workflows/runner.py Outdated Show resolved Hide resolved
src/hera/workflows/script.py Outdated Show resolved Hide resolved
src/hera/workflows/script.py Outdated Show resolved Hide resolved
src/hera/workflows/script.py Outdated Show resolved Hide resolved
tests/test_runner.py Show resolved Hide resolved
src/hera/workflows/artifact.py Outdated Show resolved Hide resolved
src/hera/workflows/artifact.py Outdated Show resolved Hide resolved
tests/test_runner.py Outdated Show resolved Hide resolved
tests/test_unit/test_script.py Outdated Show resolved Hide resolved
),
]
) -> str:
return my_artifact
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this return work by default if someone does not use the experimental script runner?

Copy link
Contributor Author

@dejamiko dejamiko Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably won't; I didn't really make sure the examples make sense when ran on Argo, I focused on generating the same yaml as the old version

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone's not using the runner then I think this would return an empty string? Not using the runner with Artifact annotations seems to be the wrong thing to be doing

Mikolaj Deja added 2 commits August 3, 2023 10:45
Signed-off-by: Mikolaj Deja <mdeja2@bloomberg.net>
Move parameter examples to tests

Signed-off-by: Mikolaj Deja <mdeja2@bloomberg.net>
@dejamiko dejamiko force-pushed the script-annotations-artifacts branch from b1c3028 to 3fcf7cb Compare August 3, 2023 10:49
src/hera/workflows/runner.py Outdated Show resolved Hide resolved
src/hera/workflows/runner.py Outdated Show resolved Hide resolved
src/hera/workflows/task.py Outdated Show resolved Hide resolved
src/hera/workflows/workflow.py Outdated Show resolved Hide resolved
src/hera/workflows/workflow_template.py Outdated Show resolved Hide resolved
Mikolaj Deja added 2 commits August 3, 2023 11:23
Signed-off-by: Mikolaj Deja <mdeja2@bloomberg.net>
Signed-off-by: Mikolaj Deja <mdeja2@bloomberg.net>
Signed-off-by: Mikolaj Deja <mdeja2@bloomberg.net>
@elliotgunton elliotgunton enabled auto-merge (squash) August 3, 2023 12:01
Copy link
Collaborator

@elliotgunton elliotgunton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wooo! 🚀

@elliotgunton elliotgunton merged commit a088622 into argoproj-labs:main Aug 3, 2023
13 checks passed
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 this pull request may close these issues.

Support annotations for input artifacts for script
3 participants