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

[#595] Improve artifact/parameter access #639

Merged
merged 14 commits into from
May 26, 2023
Merged

Conversation

flaviuvadan
Copy link
Collaborator

Pull Request Checklist

Description of PR

@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!

Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
@flaviuvadan flaviuvadan added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels May 26, 2023
@flaviuvadan flaviuvadan changed the base branch from main to fv/add-exceptions May 26, 2023 02:35
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
Signed-off-by: Flaviu Vadan <flaviuvadan@gmail.com>
@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #639 (3379201) into main (6bbc07a) will increase coverage by 0.0%.
The diff coverage is 48.4%.

@@          Coverage Diff          @@
##            main    #639   +/-   ##
=====================================
  Coverage   72.5%   72.5%           
=====================================
  Files         43      43           
  Lines       2983    2973   -10     
  Branches     572     568    -4     
=====================================
- Hits        2163    2158    -5     
+ Misses       668     663    -5     
  Partials     152     152           
Impacted Files Coverage Δ
src/hera/workflows/_mixins.py 68.6% <45.1%> (-3.5%) ⬇️
src/hera/workflows/steps.py 79.1% <100.0%> (+13.0%) ⬆️
src/hera/workflows/task.py 73.2% <100.0%> (+6.1%) ⬆️

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

@flaviuvadan flaviuvadan changed the title Improve artifact/parameter access [#595] Improve artifact/parameter access May 26, 2023
Comment on lines +728 to +731
if isinstance(self.template, Templatable):
template = self.template._build_template()
else:
template = self.template
Copy link
Collaborator

Choose a reason for hiding this comment

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

A general point here - we've been finding it hard to work with TemplateRefs that we know output certain parameters, so we might want to relax/change this validation. Something we'll look to address in a later PR, JFYI

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 see. Do you imagine this can actually accommodate template refs by actually fetching the template based on the ref if self.template is a string, and search the params like that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

fetching the template

This would be pretty awesome, and along the lines of what we're thinking - if we're able to statically generate stubs from a given WorkflowTemplate, so that we have a class that inherits from a Hera TemplateRef and works with our functions like get_parameter.

class TemplateRef(IOMixin):
    pass  # let's say we've implemented this


from hera.workflow import TemplateRef


class MyTemplateRefStub(TemplateRef):  # we generate this in some hera.workflow.template_ref_stubs module or something
    inputs = [Parameter("a-parameter", default="some-default")]
    outputs = [Parameter("an-output-parameter")]
    ... etc

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.

Really like the changes! I saw that we can reduce duplication a bit more - see #644 and feel free to merge it in

Small PR on top of a WIP to avoid making suggestions/commits

---------

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
Base automatically changed from fv/add-exceptions to main May 26, 2023 13:24
# Conflicts:
#	scripts/service.py
#	src/hera/events/service.py
#	src/hera/exceptions/__init__.py
#	src/hera/workflows/service.py
@flaviuvadan flaviuvadan enabled auto-merge (squash) May 26, 2023 13:30
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.

LGTM

@flaviuvadan flaviuvadan disabled auto-merge May 26, 2023 13:44
@flaviuvadan flaviuvadan merged commit da9a0ef into main May 26, 2023
@flaviuvadan flaviuvadan deleted the fv/artifact-access branch May 26, 2023 14:29
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.

Better get_parameter/get_artifact support
3 participants