Fix multiple bugs in Hera Input and Output classes
#1193
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current code will silently skip fields with: - a Pydantic annotation but no workflow annotation - a workflow annotation with no name field Fix the code to ignore unrecognized annotations (this is a requirement of the Annotated spec) and to default the model name to the field name if none is given explicitly. Signed-off-by: Alice Purcell <alicederyn@gmail.com>
Signed-off-by: Alice Purcell <alicederyn@gmail.com>
Signed-off-by: Alice Purcell <alicederyn@gmail.com>
Signed-off-by: Alice Purcell <alicederyn@gmail.com>
Signed-off-by: Alice Purcell <alicederyn@gmail.com>
OutputMixin._get_outputs was mutating the original annotation instead of copying it; not defaulting the name to the field name; and not copying the model default into the annotation. Signed-off-by: Alice Purcell <alicederyn@gmail.com>
alicederyn
added
semver:patch
alicederyn
force-pushed
the
fix.io.mixin.bugs
branch
from
519f83d
to
5d9ce17
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1193 +/- ##
=======================================
+ Coverage 81.7% 86.0% +4.3%
=======================================
Files 54 60 +6
Lines 4208 4038 -170
Branches 889 838 -51
=======================================
+ Hits 3439 3475 +36
+ Misses 574 391 -183
+ Partials 195 172 -23 ☔ View full report in Codecov by Sentry. |
alicederyn
requested review from
flaviuvadan,
samj1912 and
elliotgunton
as code owners
alicederyn
force-pushed
the
fix.io.mixin.bugs
branch
from
5d9ce17
to
a87a7db
Compare
Multiple methods in _io_mixins resolve the same problem of iterating through all fields, constructing a workflow annotation based on the user's annotations, defaulting the name if unset or falling back to a Parameter if no annotation is found. Factor that logic out into an iterator, simplifying the various methods and reducing the risk of discrepancies and bugs in future. Signed-off-by: Alice Purcell <alicederyn@gmail.com>
alicederyn
force-pushed
the
fix.io.mixin.bugs
branch
from
a87a7db
to
d9363d5
Compare
jeongukjae
approved these changes
Sep 6, 2024
|
This also fixes #1165, updated the PR description |
This was referenced Sep 12, 2024
samj1912
pushed a commit
that referenced
this pull request
Sep 13, 2024
**Pull Request Checklist** - [x] Fixes #1198 - [ ] ~Tests added~ - [x] Documentation/examples added - [x] [Good commit messages](https://cbea.ms/git-commit/) and/or PR title **Description of PR** From issue description: * [Script basics "explicit" example](https://hera.readthedocs.io/en/stable/user-guides/script-basics/#script-decorator) passes `source=echo` in a Task - this is not possible * [Script annotations](https://hera.readthedocs.io/en/stable/user-guides/script-annotations/) is quite confusing to read. Examples are overly complex/wordy, some being incorrect. We can also show that `name` can be inferred when #1193 is merged * [expr guide](https://hera.readthedocs.io/en/stable/user-guides/expr/)'s first link is dead, also some formatting can be tidied up * README is duplicated due to mascot image link, but we can fix this. Also note the [PyPI page](https://pypi.org/project/hera/) has a broken image  --------- Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
Signed-off-by: Elliot Gunton <elliotgunton@gmail.com>
elliotgunton
approved these changes
Sep 17, 2024
elliotgunton
changed the title
Input and Output classes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Description of PR
Currently, the methods that consume annotations in InputMixin and OutputMixin have multiple bugs:
OutputMixin._get_outputsadditionally was:This PR pulls out a common function to iterate through fields and yield annotations, which:
This ensures all functions treat an explicit Parameter and a missing workflow annotation the same way, solving the second issue with
OutputMixin._get_outputs.