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

Update script to handle Optional and Union input parameters #1160

Merged
merged 14 commits into from
Aug 20, 2024

Conversation

jeongukjae
Copy link
Collaborator

Pull Request Checklist

Description of PR
Currently, script decorator cannot handle Union or Optional input parameters because of the error in issubclass, so updated it to be able to handle it as expected.


original PR: #1147

Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
@jeongukjae jeongukjae added semver:patch A change requiring a patch version bump type:bug A general bug labels Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.4%. Comparing base (d553546) to head (f24a7c9).
Report is 185 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1160     +/-   ##
=======================================
- Coverage   81.7%   81.4%   -0.3%     
=======================================
  Files         54      60      +6     
  Lines       4208    4732    +524     
  Branches     889    1004    +115     
=======================================
+ Hits        3439    3856    +417     
- Misses       574     646     +72     
- Partials     195     230     +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
.github/workflows/cicd.yaml Outdated Show resolved Hide resolved
tests/script_runner/parameter_with_complex_types.py Outdated Show resolved Hide resolved
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
jeongukjae and others added 3 commits August 20, 2024 16:08
Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Co-authored-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Ukjae Jeong <JeongUkJae@gmail.com>
Signed-off-by: Ukjae Jeong <JeongUkJae@gmail.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.

Nice! 🚀

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.

Sorry, a minor change back of function name

src/hera/workflows/_runner/util.py Outdated Show resolved Hide resolved
src/hera/workflows/_runner/util.py Outdated Show resolved Hide resolved
Co-authored-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Ukjae Jeong <JeongUkJae@gmail.com>
@sambhav sambhav merged commit 523a9b9 into argoproj-labs:main Aug 20, 2024
19 of 20 checks passed
@jeongukjae jeongukjae deleted the issue-1012 branch August 21, 2024 10:56
sambhav added a commit that referenced this pull request Sep 2, 2024
**Pull Request Checklist**
- [x] ~~Fixes #<!--issue number goes here-->~~ this PR follows up on
#1160
- [x] Tests added
- [x] Documentation/examples added : this is refactoring. no need
- [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR
title

**Description of PR**

Currently, we are checking annotated, parameter/artifact, or other types
in different ways.
In this PR, I added `hera._utils.type_util` module and unified them into
one place.
Followings can be expected:
- If there's another metadata along with Parameter or Artifact, they
should be ignored. (e.g. `Annotated[type, metadata1, metadata2,
Parameter(...)]`, this can be happened when `Annotated`s are nested)
- `Optional` parameter handling should be more precise which is added in
#1160.

Suggestions on function names/module names are welcome.

---------

Signed-off-by: Ukjae Jeong <jeongukjae@gmail.com>
Signed-off-by: Ukjae Jeong <JeongUkJae@gmail.com>
Co-authored-by: Elliot Gunton <elliotgunton@gmail.com>
Co-authored-by: Sambhav Kothari <skothari44@bloomberg.net>
Co-authored-by: Alice <Alice.Purcell.39@gmail.com>
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.

Support Optional/Union types in script runner
3 participants