-
Notifications
You must be signed in to change notification settings - Fork 310
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
Fix sync node execution for map over lp #3151
Conversation
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Code Review Agent Run Status
|
Signed-off-by: Troy Chiu <[email protected]>
Signed-off-by: Troy Chiu <[email protected]>
Code Review Agent Run Status
|
Signed-off-by: Troy Chiu <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3151 +/- ##
==========================================
- Coverage 78.18% 76.59% -1.60%
==========================================
Files 211 211
Lines 21994 22008 +14
Branches 2862 2865 +3
==========================================
- Hits 17195 16856 -339
- Misses 3960 4361 +401
+ Partials 839 791 -48 ☔ View full report in Codecov by Sentry. |
Code Review Agent Run #f8d228Actionable Suggestions - 4
Review Details
|
Changelist by BitoThis pull request implements the following key changes.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a comment about a breaking change, but no need to be more accommodating other than just list it in the changelog.
def sync_task_execution( | ||
self, execution: FlyteTaskExecution, entity_definition: typing.Optional[FlyteTask] = None | ||
self, execution: FlyteTaskExecution, entity_interface: typing.Optional[TypedInterface] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a breaking change, right? We should call it out in the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Thank you for catching this.
Why are the changes needed?
Currently if you use flyteremote to sync node execution which contains an array node mapping over a lp, you will get an error
What changes were proposed in this pull request?
Add case for map over lp in sync node execution.
Check all the applicable boxes
Summary by Bito
This PR addresses a bug in FlyteRemote's node execution synchronization by implementing proper handling of launch plan references within array nodes. The changes include improved type hints and interface-based execution synchronization, ensuring correct processing of array nodes containing workflow nodes with launch plan references.Unit tests added: False
Estimated effort to review (1-5, lower is better): 2