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 for input Parameters #708

Merged
merged 5 commits into from
Jul 24, 2023

Conversation

dejamiko
Copy link
Contributor

@dejamiko dejamiko commented Jul 11, 2023

Pull Request Checklist

Description of PR
This PR adds some basic support for annotations for script. We can avoid duplication of parameter names and default values. See the old version

@script(
    inputs=[
        Parameter(name="an_int", description="an_int parameter", default=1, enum=[1, 2, 3]), 
        Parameter(name="a_bool", description="a_bool parameter", default=True, enum=[True, False]), 
        Parameter(name="a_string", description="a_string parameter", default="a", enum=["a", "b", "c"])
    ]
)
def echo_all(an_int=1, a_bool=True, a_string="a"):
    print(an_int)
    print(a_bool)
    print(a_string)

vs the new one:

@script()
def echo_all(
    an_int: Annotated[int, Parameter(description="an_int parameter", default=1, enum=[1, 2, 3])], 
    a_bool: Annotated[bool, Parameter(description="a_bool parameter", default=True, enum=[True, False])], 
    a_string: Annotated[str, Parameter(description="a_string parameter", default="a", enum=["a", "b", "c"])]
):
    print(an_int)
    print(a_bool)
    print(a_string)

We also allow for renaming parameters, using YAML rules for naming

@script()
def echo(
    a_name: Annotated[str, Parameter(name="another-name")],
):
    print(a_name)

kebab-case support for runner

Since we now allow for parameters to have a new name and the naming rules come from YAML, we allow kebab-case names for parameters which could be used in a script runner. A mapping of kwargs was created that maps the parameter name from annotations to the one found in the python function signature.

@script()
def function_kebab(
    a_but_kebab: Annotated[int, Parameter(name="a-but-kebab")] = 2,
    b_but_kebab: Annotated[str, Parameter(name="b-but-kebab")] = "foo",
) -> Output:
    return Output(output=[Input(a=a_but_kebab, b=b_but_kebab)])

Signed-off-by: Elliot Gunton <egunton@bloomberg.net>
@dejamiko dejamiko force-pushed the script-annotations branch 2 times, most recently from d6e501c to 1b28454 Compare July 11, 2023 09:47
@elliotgunton elliotgunton added type:enhancement A general enhancement semver:minor A change requiring a minor version bump labels Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #708 (7b8c495) into main (c7374e7) will increase coverage by 0.2%.
The diff coverage is 93.7%.

@@           Coverage Diff           @@
##            main    #708     +/-   ##
=======================================
+ Coverage   77.4%   77.7%   +0.2%     
=======================================
  Files         45      45             
  Lines       3346    3386     +40     
  Branches     639     649     +10     
=======================================
+ Hits        2592    2632     +40     
+ Misses       576     573      -3     
- Partials     178     181      +3     
Impacted Files Coverage Δ
src/hera/workflows/runner.py 74.0% <87.5%> (+5.8%) ⬆️
src/hera/workflows/script.py 89.5% <96.0%> (+0.5%) ⬆️
src/hera/workflows/parameter.py 86.4% <100.0%> (+6.4%) ⬆️

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

@sambhav sambhav marked this pull request as draft July 11, 2023 17:34
@dejamiko dejamiko marked this pull request as ready for review July 17, 2023 15:04
@dejamiko dejamiko changed the title Script annotations Script annotations for InputParameters Jul 18, 2023
@dejamiko dejamiko changed the title Script annotations for InputParameters Script annotations for input Parameters Jul 18, 2023
examples/workflows/callable_script.py Outdated Show resolved Hide resolved
examples/workflows/callable_script.py Outdated Show resolved Hide resolved
examples/workflows/script_annotations_combined_old.py Outdated Show resolved Hide resolved
src/hera/workflows/parameter.py Outdated Show resolved Hide resolved
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
tests/test_runner.py Outdated Show resolved Hide resolved
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.

Two small comments! Looking good 💯

src/hera/workflows/script.py Outdated Show resolved Hide resolved
src/hera/workflows/script.py Outdated Show resolved Hide resolved
@elliotgunton elliotgunton mentioned this pull request Jul 20, 2023
4 tasks
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.

We need to add the old behaviour back in but otherwise LGTM!

src/hera/workflows/runner.py Show resolved Hide resolved
src/hera/workflows/runner.py Outdated Show resolved Hide resolved
Add support for easier declaration of script input parameters
using typing.Annotated. It supports enum, default, name,
and description. It is an experimental feature,
enabled by script_annotation flag in global_config.

Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
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.

More docs, slight change on env var then LGTM! :shipit:

src/hera/workflows/script.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
Signed-off-by: Mikolaj <57460148+dejamiko@users.noreply.github.com>
Signed-off-by: Mikolaj <57460148+dejamiko@users.noreply.github.com>
@elliotgunton elliotgunton enabled auto-merge (squash) July 24, 2023 11:09
@elliotgunton elliotgunton merged commit 97758e0 into argoproj-labs:main Jul 24, 2023
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 parameters for script
3 participants