update array node model #3156
update array node model #3156
Conversation
Signed-off-by: Paul Dittamo <[email protected]>
Code Review Agent Run Status
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3156 +/- ##
===========================================
- Coverage 76.59% 35.80% -40.79%
===========================================
Files 211 212 +1
Lines 22008 22028 +20
Branches 2865 2865
===========================================
- Hits 16856 7888 -8968
- Misses 4361 14027 +9666
+ Partials 791 113 -678 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Paul Dittamo <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
Code Review Agent Run Status
|
Signed-off-by: Paul Dittamo <[email protected]>
pvditt
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #73d43eActionable Suggestions - 2
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| @@ -395,6 +395,7 @@ def __init__( | |||
| execution_mode=None, | |||
| is_original_sub_node_interface=False, | |||
| data_mode=None, | |||
| bound_inputs=None, | |||
There was a problem hiding this comment.
Consider adding type hints for the bound_inputs parameter in the constructor. This would help with code maintainability and IDE support.
Code suggestion
Check the AI-generated fix before applying
- node: "Node",
- bound_inputs=None,
+ node: "Node",
+ bound_inputs: typing.Optional[typing.List[_Binding]] = None,
Code Review Run #73d43e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| def bound_inputs(self) -> Set[str]: | ||
| return set() |
There was a problem hiding this comment.
The bound_inputs property appears to be returning an empty set while there is an instance variable self._bound_inputs that seems to be the intended return value. Consider returning self._bound_inputs instead of an empty set.
Code suggestion
Check the AI-generated fix before applying
| def bound_inputs(self) -> Set[str]: | |
| return set() | |
| def bound_inputs(self) -> Set[str]: | |
| return self._bound_inputs |
Code Review Run #73d43e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Tracking issue
ref: https://linear.app/unionai/issue/COR-2662/update-sdk-to-support-bound-inputs-for-mapping-over-launch-plans
Why are the changes needed?
Checking this in for future support for array_node_map_task to fix flyteorg/flyte#4325
Need to put this here for union.map to bind inputs for when mapping over launch plans
What changes were proposed in this pull request?
Set bound input field in array node spec
How was this patch tested?
Ran in sandbox w/ union sdk changes
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
Implementation of bound inputs support in array node mapping, specifically for mapping over launch plans. The changes encompass adding bound_inputs property to ArrayNode class, updating workflow model for bound inputs handling, and modifying the translator. The PR also includes an update to flyteidl dependency to version 1.15.1.Unit tests added: False
Estimated effort to review (1-5, lower is better): 1