Script annotations output #741
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dejamiko
requested review from
flaviuvadan,
sambhav and
elliotgunton
as code owners
Codecov Report
@@ Coverage Diff @@
## main #741 +/- ##
=======================================
+ Coverage 77.9% 78.4% +0.4%
=======================================
Files 45 45
Lines 3475 3622 +147
Branches 677 725 +48
=======================================
+ Hits 2708 2840 +132
- Misses 580 588 +8
- Partials 187 194 +7
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
flaviuvadan
reviewed
Aug 8, 2023
flaviuvadan
reviewed
Aug 8, 2023
flaviuvadan
reviewed
Aug 8, 2023
flaviuvadan
reviewed
Aug 8, 2023
flaviuvadan
reviewed
Aug 8, 2023
tests/script_annotations_outputs/script_annotations_output_param_and_artifact_in_func.py
Show resolved
Hide resolved
flaviuvadan
reviewed
Aug 8, 2023
tests/script_annotations_outputs/script_annotations_output_param_and_artifact_mix.py
Show resolved
Hide resolved
flaviuvadan
reviewed
Aug 8, 2023
elliotgunton
reviewed
Aug 9, 2023
elliotgunton
reviewed
Aug 9, 2023
src/hera/workflows/runner.py
Outdated
Comment on lines
142
to
146
| output_dir = Path( | ||
| str(global_config.experimental_features["hera_output_directory"]) | ||
| if global_config.experimental_features["hera_output_directory"] | ||
| else "hera/outputs" | ||
| ) |
There was a problem hiding this comment.
Very nice thought to use the experimental features for users to play with the directory explicitly, but I'm thinking if we really need it? 🤔 Also, on the technical side, the type of the dict is:
experimental_features: Dict[str, bool] = field(default_factory=lambda: defaultdict(bool))So we need to think about how we can specify this directory, or if we can change the types.
Also a very small suggestion if we keep this approach is to add an s on outputs: hera_outputs_directory to match the default
There was a problem hiding this comment.
Could we make another field on the global config vs use the experimental features + use the experimental features to check for allowed usage?
There was a problem hiding this comment.
I think we should use
hera/src/hera/shared/_global_config.py
Line 118 in 49140d0
elliotgunton
reviewed
Aug 9, 2023
elliotgunton
reviewed
Aug 9, 2023
elliotgunton
reviewed
Aug 9, 2023
elliotgunton
force-pushed
the
script-annotations-output
branch
from
2a4528d
to
937d089
Compare
|
@dejamiko you don't need to call _get_class_defaults, the hera mixin automatically handles it. See hera/src/hera/shared/_global_config.py Line 157 in 49140d0 |
added 2 commits
August 17, 2023 10:52
Add support for output annotations for scripts. Allow for the users to define them in the function return type as well as function parameters. Automatically mount the volumes and save the files for the runner constructor. Signed-off-by: Mikolaj Deja <[email protected]>
Add runner and example tests, check that results are saved in the expected directories and that the correct yaml is generated. Restructure test naming and file structure, add docstrings and fix workflow naming. Signed-off-by: Mikolaj Deja <[email protected]>
dejamiko
force-pushed
the
script-annotations-output
branch
from
adcb03b
to
7ce1210
Compare
dejamiko
commented
Aug 17, 2023
dejamiko
force-pushed
the
script-annotations-output
branch
from
ae658ad
to
62df288
Compare
src/hera/workflows/script.py
Outdated
| already_set_artifacts = {a.name for a in current_io.artifacts or []} | ||
|
|
||
| for param in func_parameters: | ||
| if param.name not in already_set_params and param.name not in already_set_artifacts: |
There was a problem hiding this comment.
Curious why we need to check param.name not in already_set_artifacts (and artifact.name not in already_set_params below)
There was a problem hiding this comment.
This is because if you have a function with an artifact
@script(inputs=Artifact(name="i", path="/tmp/i"))
def consume(i):
import pickle
with open("/tmp/i", "rb") as f:
i = pickle.load(f)
print(i)i would be interpreted as both an Artifact and a Parameter. The check in the other direction is not necessary.
dejamiko
force-pushed
the
script-annotations-output
branch
from
62df288
to
abfb812
Compare
dejamiko
force-pushed
the
script-annotations-output
branch
from
b6d5f80
to
13e1cc7
Compare
Signed-off-by: Mikolaj <[email protected]>
dejamiko
force-pushed
the
script-annotations-output
branch
from
13e1cc7
to
8c0becf
Compare
4 tasks
elliotgunton
added a commit
that referenced
this pull request
Sep 1, 2023
**Pull Request Checklist** - [x] Fixes bugs in #741 - [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 Artifacts/Parameters contained in function return annotations or function parameter annotations where marked as output don't output their paths in the yaml. The runner also does not pre-load function input parameters marked as outputs as Path objects for the given kwarg. This PR fixes output annotations for these use cases and makes the tests more robust. The "advanced-hera-features" doc was also edited. I also fixed the codebase for use with VSCode's pytest debugger. --------- Signed-off-by: Elliot Gunton <[email protected]>
kimjunil
pushed a commit
to kimjunil/hera
that referenced
this pull request
Sep 3, 2023
**Pull Request Checklist** - [x] Fixes bugs in argoproj-labs#741 - [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 Artifacts/Parameters contained in function return annotations or function parameter annotations where marked as output don't output their paths in the yaml. The runner also does not pre-load function input parameters marked as outputs as Path objects for the given kwarg. This PR fixes output annotations for these use cases and makes the tests more robust. The "advanced-hera-features" doc was also edited. I also fixed the codebase for use with VSCode's pytest debugger. --------- Signed-off-by: Elliot Gunton <[email protected]> Signed-off-by: KimJunil <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
This PR implements the output annotation behaviour described in #740. The case with output params in the function signature as
Pathis not tested and this can be done with #652.