argoproj-labs / hera

Hera is an Argo Python SDK. Hera aims to make construction and submission of various Argo Project resources easy and accessible to everyone! Hera abstracts away low-level setup details while still maintaining a consistent vocabulary with Argo. ⭐️ Remember to star!
https://hera.rtfd.io
Apache License 2.0
550 stars 105 forks source link

Support inherited fields for RunnerInput, other niceties #1093

Closed mitrydoug closed 2 months ago

mitrydoug commented 2 months ago

Pull Request Checklist

Summary

Currently, users cannot leverage inheritance when defining RunnerInput models that utilize Annotated fields. For example:

class ResourceIndex(RunnerInput):
    resource_name: Annotated[str, Parameter(name="resource-name")]
    resource_namespace: Annotated[str, Parameter(name="resource-namespace")]

class GetStatusInput(ResourceIndex):
    wait: Annotated[bool, Parameter(name="wait")]
    timeout_s: Annotated[int, Parameter(name="timeout")]

This would fail at runtime with a KeyError at this line, because apparently cls.__annotations__ are not inherited by child classes.

This PR enables the above inheritance structure by utilizing the existing get_field_annotations instead of my_runner_input.__annotations__.

Parameter Default Names

This PR also changes logic so that Input field Parameters adopt the name of their field if no name is provided. For example, in

class MyInput(Input):
    my_int: Annotated[int, Parameter(description="this is my int")]

The parameter is converted to Parameter(name="my_int", description="this is my int"). This avoids an error from here which is thrown when trying to generate a workflow yaml.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.8%. Comparing base (d553546) to head (c4653ef). Report is 81 commits behind head on main.

Files Patch % Lines
src/hera/workflows/io/_io_mixins.py 66.6% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1093 +/- ## ======================================= + Coverage 81.7% 81.8% +0.1% ======================================= Files 54 60 +6 Lines 4208 4641 +433 Branches 889 975 +86 ======================================= + Hits 3439 3800 +361 - Misses 574 613 +39 - Partials 195 228 +33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mitrydoug commented 2 months ago

applying a simplifying commit and additional test

mitrydoug commented 2 months ago

applying one more change, please do not merge

samj1912 commented 2 months ago

@mitrydoug can you please update the PR description with the motivation behind the implementation and also add some comments to the code describing "why" we are performing certain operations? For eg the copy.

mitrydoug commented 2 months ago

I removed the logic about finding a unique Parameter/Artifact since it became clear to me that the remainder of the codebase assumes Parameters/Artifacts to be at index 1 of Annotated, so I wanted to stay consistent with that.

@samj1912 I have updated my PR description, and commented on the copy.

From my end, this is ready to go.

elliotgunton commented 2 months ago

the remainder of the codebase assumes Parameters/Artifacts to be at index 1

Yeah we haven't heard any complaints from people who want to use more Annotations, so if/when they do we should convert all these parts of the codebase at once