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

Allow mounting volume for outputs #780

Merged
merged 2 commits into from
Sep 26, 2023
Merged

Allow mounting volume for outputs #780

merged 2 commits into from
Sep 26, 2023

Conversation

elliotgunton
Copy link
Collaborator

@elliotgunton elliotgunton commented Sep 18, 2023

  • Still use /tmp for script outputs but allow mounting own volume with volume_for_outputs passed to Runner constructor
  • Fix examples using volumes incorrectly and simplify where possible

@elliotgunton elliotgunton added semver:patch A change requiring a patch version bump type:bug A general bug labels Sep 18, 2023
@elliotgunton elliotgunton marked this pull request as draft September 18, 2023 10:20
@elliotgunton elliotgunton marked this pull request as ready for review September 18, 2023 10:24
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

Merging #780 (c5a42e5) into main (bb3c7c0) will increase coverage by 0.0%.
Report is 1 commits behind head on main.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #780   +/-   ##
=====================================
  Coverage   78.2%   78.3%           
=====================================
  Files         45      45           
  Lines       3639    3643    +4     
  Branches     732     734    +2     
=====================================
+ Hits        2847    2853    +6     
+ Misses       594     593    -1     
+ Partials     198     197    -1     
Files Coverage Δ
src/hera/workflows/_mixins.py 79.6% <100.0%> (ø)
src/hera/workflows/runner.py 79.7% <ø> (ø)
src/hera/workflows/script.py 86.1% <100.0%> (+0.7%) ⬆️

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

@@ -1061,7 +1061,7 @@ def _get_artifact(self, name: str, subtype: str) -> Artifact:

obj = next((output for output in artifacts if output.name == name), None)
if obj is not None:
return Artifact(name=name, path=obj.path, from_=f"{{{{{subtype}.{self.name}.outputs.artifacts.{name}}}}}")
Copy link
Contributor

@dejamiko dejamiko Sep 18, 2023

Choose a reason for hiding this comment

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

This return object looks strange to me.

It has a given name and the from_ has the same name in the path? We don't return the Artifact but rather create a new one with a populated from_ string. At a glance, it is unclear why this doesn't return the old Artifact object but creates a new one with a reference.

If we are doing that, for clarity, we might want to change the function slightly to combine the get_artifact with as_name and use the provided "new name" for the return object. We can make the new_name equal to old_name for convenience but I feel like this would be clearer to the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it looks strange, but it makes a bit more sense from the user's perpective e.g. for parameters we have with_name which kind of matches our new callable syntax, and is used like:

t1 = Task(name="a", template=out)
t2 = Task(name="b", template=in_, arguments=t1.get_parameter("x").with_name("a"))

And for artifacts we have as_name (which we should probably deprecate and use with_name as well):

out = output_artifact(arguments={"a_number": 3})
use_artifact(arguments=[out.get_artifact("successor_out").as_name("successor_in")])

Your suggestion would be a small change to e.g.

out = output_artifact(arguments={"a_number": 3})
use_artifact(arguments=[out.get_artifact("successor_out", with_name="successor_in")])

Which I'm open to, but I'd rather nail that change down in the new callable syntax PR, this line change was just to remove the path which isn't used for artifacts-as-arguments.

@elliotgunton elliotgunton marked this pull request as draft September 24, 2023 20:27
* Do not use /tmp for script outputs
* Allow mounting own volume with use_volume_for_outputs
* Fix examples using volumes incorrectly and simplify where possible
* Fix passing artifacts as argument (remove path), and simplify examples

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Allows users to set outputs_directory and volume_for_outputs
separately, meaning they can use a different /tmp path, mount
a new empty dir, or use an existing volume, all of which now
have tests

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
@elliotgunton elliotgunton marked this pull request as ready for review September 26, 2023 09:41
@samj1912 samj1912 merged commit 932ba29 into main Sep 26, 2023
15 checks passed
@samj1912 samj1912 deleted the cleanup-volumes branch September 26, 2023 12:06
@elliotgunton elliotgunton changed the title Fixup volumes Allow mounting volume for outputs Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants