feat: support positional args for ref entity #3104
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
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
machichima
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
samhita-alla,
thomasjpfan and
Future-Outlier
as code owners
Code Review Agent Run #942e72Actionable Suggestions - 1
Review Details
AI Code Review powered by |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3104 +/- ##
===========================================
+ Coverage 76.85% 95.24% +18.38%
===========================================
Files 206 85 -121
Lines 21851 3803 -18048
Branches 2837 0 -2837
===========================================
- Hits 16794 3622 -13172
+ Misses 4269 181 -4088
+ Partials 788 0 -788 ☔ View full report in Codecov by Sentry. |
Changelist by BitoThis pull request implements the following key changes.
|
flyte-bot
reviewed
Feb 2, 2025
flytekit/core/reference_entity.py
Outdated
| @@ -187,7 +186,7 @@ def construct_node_metadata(self) -> _workflow_model.NodeMetadata: | |||
| return _workflow_model.NodeMetadata(name=extract_obj_name(self.name)) | |||
|
|
|||
| def compile(self, ctx: FlyteContext, *args, **kwargs): | |||
| return create_and_link_node(ctx, entity=self, **kwargs) | |||
| return create_and_link_node(ctx, self, *args, **kwargs) | |||
There was a problem hiding this comment.
Consider impact of keyword to positional change
Consider if the change from entity=self to positional argument self could impact any callers expecting keyword arguments. The function signature in create_and_link_node expects entity as a keyword parameter.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| return create_and_link_node(ctx, self, *args, **kwargs) | |
| return create_and_link_node(ctx, entity=self, *args, **kwargs) |
Code Review Run #942e72
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
pingsutw
reviewed
Feb 4, 2025
| @@ -200,10 +199,6 @@ def __call__(self, *args, **kwargs): | |||
| # nothing. Subsequent tasks will have to know how to unwrap these. If by chance a non-Flyte task uses a | |||
| # task output as an input, things probably will fail pretty obviously. | |||
| # Since this is a reference entity, it still needs to be mocked otherwise an exception will be raised. | |||
| if len(args) > 0: | |||
There was a problem hiding this comment.
Could we convert args to kwargs here instead? Then, we don't need to change the signature of create_and_link_node
There was a problem hiding this comment.
It will be great to have a small test for it
There was a problem hiding this comment.
Sure! I will modify it and add some tests
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #204b34Actionable Suggestions - 2
Additional Suggestions - 10
Review Details
AI Code Review powered by |
flyte-bot
reviewed
Feb 9, 2025
flytekit/core/reference_entity.py
Outdated
| # Check if we have more arguments than expected | ||
| if len(args) > len(interface.inputs): | ||
| raise AssertionError( | ||
| f"Received more arguments than expected in function '{entity.name}'. Expected {len(interface.inputs)} but got {len(args)}" |
There was a problem hiding this comment.
Fix undefined variable in error message
The error message in the AssertionError references an undefined entity.name. Consider using self.name instead since this is within the ReferenceEntity class.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| f"Received more arguments than expected in function '{entity.name}'. Expected {len(interface.inputs)} but got {len(args)}" | |
| f"Received more arguments than expected in function '{self.name}'. Expected {len(interface.inputs)} but got {len(args)}" |
Code Review Run #204b34
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
flyte-bot
reviewed
Feb 9, 2025
flytekit/core/reference_entity.py
Outdated
| # Convert args to kwargs | ||
| for arg, input_name in zip(args, interface.inputs.keys()): | ||
| if input_name in kwargs: | ||
| raise AssertionError(f"Got multiple values for argument '{input_name}' in function '{entity.name}'") |
There was a problem hiding this comment.
Fix undefined variable in duplicate arg message
Similar to the previous issue, the error message uses entity.name which is undefined. Consider using self.name for consistency.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| raise AssertionError(f"Got multiple values for argument '{input_name}' in function '{entity.name}'") | |
| raise AssertionError(f"Got multiple values for argument '{input_name}' in function '{self.name}'") |
Code Review Run #204b34
Is this a valid issue, or was it incorrectly flagged by the Agent?
- it was incorrectly flagged
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #90a123Actionable Suggestions - 0Review Details
AI Code Review powered by |
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
machichima
dismissed stale reviews from pingsutw and Future-Outlier
via
8544ace
Code Review Agent Run #e70869Actionable Suggestions - 0Additional Suggestions - 10
Review Details
AI Code Review powered by |
Signed-off-by: machichima <[email protected]>
Signed-off-by: machichima <[email protected]>
Code Review Agent Run #8bdf2aActionable Suggestions - 0Review Details
AI Code Review powered by |
Future-Outlier
approved these changes
Feb 21, 2025
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.
Tracking issue
Similar to issue: flyteorg/flyte#5320, but for reference entities
Why are the changes needed?
Support positional args for reference task, workflow, and launch plan.
Originally, following code will fail with error like:
FlyteAssertion: USER:AssertionError: error=Cannot call reference entity with args - detected 2 positional args (Promise(node:.x.[]), Promise(node:.y.[]))What changes were proposed in this pull request?
Write values in positional arguments (args) into kwargs
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR implements comprehensive Flytekit enhancements including: positional argument support for reference entities, improved task caching with configurable policies, shared memory support in containers, better Ray task configuration and pod template handling, enhanced remote execution monitoring, and updated agent implementations with LiteralMap for output handling. These changes focus on improving system reliability, maintainability, and authentication mechanisms with expanded test coverage.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5