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

New container decorator #1068

Merged
merged 10 commits into from
Jun 12, 2024
Merged

New container decorator #1068

merged 10 commits into from
Jun 12, 2024

Conversation

elliotgunton
Copy link
Collaborator

@elliotgunton elliotgunton commented May 15, 2024

Pull Request Checklist

Description of PR
Adds the container decorator, and the special path function on Outputs

@elliotgunton elliotgunton added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels May 15, 2024
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from abdfc10 to 6e0e2bc Compare May 15, 2024 14:10
@elliotgunton elliotgunton mentioned this pull request May 15, 2024
4 tasks
@elliotgunton elliotgunton force-pushed the new-steps-decorator branch from 3ce3d10 to ff6eac6 Compare May 15, 2024 14:29
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 6e0e2bc to 240e50d Compare May 15, 2024 14:44
@elliotgunton elliotgunton force-pushed the new-steps-decorator branch from ff6eac6 to 9ef804f Compare May 15, 2024 14:47
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 240e50d to 380c9d0 Compare May 15, 2024 14:50
Base automatically changed from new-steps-decorator to main May 15, 2024 19:52
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 380c9d0 to ae0409c Compare May 16, 2024 09:42
@elliotgunton elliotgunton marked this pull request as draft May 16, 2024 11:40
@elliotgunton elliotgunton force-pushed the new-container-decorator branch 3 times, most recently from e87960c to e573df0 Compare May 29, 2024 10:09
@elliotgunton elliotgunton marked this pull request as ready for review May 29, 2024 10:11
@elliotgunton elliotgunton linked an issue May 29, 2024 that may be closed by this pull request
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from e573df0 to d15badf Compare May 31, 2024 02:36
def advanced_hello_world(my_input: MyInput, template: Optional[Container] = None) -> MyOutput:
output: MyOutput = MyOutput(container_greeting=f"Hello {my_input.user}")

if template:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I like this pattern of doing if template.

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 don't love it but it was the best I could find for linter/developer sanity - magically adding the arg in the decorator is a bit too opaque even for devs IMO

@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 9eb99d7 to 9c574fb Compare June 3, 2024 06:06
@sambhav
Copy link
Collaborator

sambhav commented Jun 3, 2024

@elliotgunton how about instead of using template, we can use the function itself?

def my_container(my_args:...):
    my_container.<attribute>

@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 9c574fb to db04b42 Compare June 3, 2024 10:30
@elliotgunton
Copy link
Collaborator Author

@samj1912 is there a sensible way to add container_call_wrapper.template = container_template as a type-hintable attribute to the function? (From some searching it seems not)

image

Otherwise I don't think it's a nice user experience. For the template: Optional[Container] = None argument to the function, in terms of user experience it at least kind of acts as an explicit opt-in for advanced users, who would probably prefer nice code completion over nice syntax. (If we can add type hints to the function attribute then I'm 100% for it!)

Another idea is some global value that we can set before calling the user function?

from hera.workflows import current_template  # no idea on naming 😅

@w.container(command=["sh", "-c"])
def advanced_hello_world(my_input: MyInput) -> MyOutput:
    output: MyOutput = MyOutput(container_greeting=f"Hello {my_input.user}")
    current_template.args = [f"echo Hello {my_input.user} | tee {output.path(output.container_greeting)}"]
    return output

Copy link
Collaborator

@flaviuvadan flaviuvadan left a comment

Choose a reason for hiding this comment

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

impl wise this looks good to me. We already chatted about the user experience in one of the maintainer syncs. Anything else to review @elliotgunton? Happy to approve bc it lgtm!

@sambhav
Copy link
Collaborator

sambhav commented Jun 10, 2024

I would prefer to go back to the drawing board for this specific feature as I'm not sure of the UX it promotes. Happy to release the other parts that are already merged. We barely use container templates in Hera anyway compared to scripts.

@elliotgunton
Copy link
Collaborator Author

@samj1912 I'm not sure there is a better balance of UX for the "advanced container" use case from the proposal, as the proposal itself broke the paradigm of config-as-code (contained within type signatures etc), instead running the function to create the container config/spec. I can remove the code for the advanced use case if the basic one is fine?

@sambhav
Copy link
Collaborator

sambhav commented Jun 12, 2024

Sure, let's start with that

* TODO: fix typing for advanced use case

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
elliotgunton and others added 5 commits June 12, 2024 16:38
Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
* Use varname's argname to get the attribute name, then inspect the
attribute's annotation, which should be a Parameter or Artifact

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 7b5ae85 to 5c8a02e Compare June 12, 2024 08:46
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 5c8a02e to 6f5cdcc Compare June 12, 2024 08:55
@elliotgunton elliotgunton enabled auto-merge (squash) June 12, 2024 08:56
* Within a DAG/Steps function, when calling functions that are decorated
as templates belonging to other [Cluster]WorkflowTemplates, we generate
a step/task that sets the TemplateRef based on the WorkflowTemplate name
and the template name, as well as setting cluster_scope. Arguments are
set as normal

**Pull Request Checklist**
- [x] Fixes #1096 
- [x] Tests added
- [ ] Documentation/examples added
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton disabled auto-merge June 12, 2024 10:04
@elliotgunton elliotgunton enabled auto-merge (squash) June 12, 2024 10:05
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
@elliotgunton elliotgunton force-pushed the new-container-decorator branch from 75df498 to 4507151 Compare June 12, 2024 10:13
@elliotgunton elliotgunton linked an issue Jun 12, 2024 that may be closed by this pull request
@elliotgunton elliotgunton merged commit 73861b6 into main Jun 12, 2024
20 checks passed
@elliotgunton elliotgunton deleted the new-container-decorator branch June 12, 2024 10:44
@sambhav
Copy link
Collaborator

sambhav commented Jun 12, 2024

@elliotgunton if it's simple, can we also extend this to other template types?

@elliotgunton
Copy link
Collaborator Author

@samj1912 as in stubbing the function for inputs/outputs? Yes that should be okay. I'll make a new issue for it

@sambhav
Copy link
Collaborator

sambhav commented Jun 14, 2024

@elliotgunton Yes, given a template being able to set its properties in the decorator and the i/o in the signature.

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.

HEP0001 - Automatic TemplateRef Step/Task HEP0001 - new container decorator
3 participants