wrap agent outputs in a literalmap #3143
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: Samhita Alla <[email protected]>
Code Review Agent Run Status
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3143 +/- ##
==========================================
+ Coverage 76.85% 76.99% +0.13%
==========================================
Files 206 213 +7
Lines 21851 22348 +497
Branches 2837 2837
==========================================
+ Hits 16794 17207 +413
- Misses 4269 4354 +85
+ Partials 788 787 -1 ☔ View full report in Codecov by Sentry. |
samhita-alla
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy,
thomasjpfan and
Future-Outlier
as code owners
thomasjpfan
reviewed
Feb 19, 2025
plugins/flytekit-aws-sagemaker/flytekitplugins/awssagemaker_inference/boto3_agent.py
Show resolved
Hide resolved
Signed-off-by: Samhita Alla <[email protected]>
pingsutw
previously approved these changes
Feb 19, 2025
Code Review Agent Run #813c1aActionable Suggestions - 1
Review Details
AI Code Review powered by |
Signed-off-by: Samhita Alla <[email protected]>
Changelist by BitoThis pull request implements the following key changes.
|
flyte-bot
reviewed
Feb 19, 2025
| @@ -157,6 +156,7 @@ async def test_openai_batch_agent(mock_retrieve, mock_create, mock_context): | |||
| mock_retrieve.return_value = batch_retrieve_result | |||
| resource = await agent.get(metadata) | |||
| assert resource.phase == TaskExecution.SUCCEEDED | |||
| assert isinstance(resource.outputs, literals.LiteralMap) | |||
There was a problem hiding this comment.
Consider adding more specific output assertions
Consider adding more specific assertions to validate the structure and content of the LiteralMap outputs. This could help catch potential issues with the output format early.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| assert isinstance(resource.outputs, literals.LiteralMap) | |
| assert isinstance(resource.outputs, literals.LiteralMap) | |
| assert 'result' in resource.outputs.literals | |
| assert isinstance(resource.outputs.literals['result'].scalar.primitive.string_value, str) |
Code Review Run #813c1a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #0f8cc0Actionable Suggestions - 0Review Details
AI Code Review powered by |
eapolinario
approved these changes
Feb 19, 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
Why are the changes needed?
This is necessary because although the result is already a literal, it is being converted to a literal again. Since literal isn't a valid Python type, it ends up being pickled. This issue surfaced in Flytekit 1.15.0.
What changes were proposed in this pull request?
Wrapping agent outputs in a
LiteralMapso they don't get converted to literals again.How was this patch tested?
Tested the OpenAI plugin with the updated code.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
Fixed a bug in agent output handling where results were being converted to literals multiple times, affecting Python type validation in Flytekit 1.15.0. Implemented proper LiteralMap wrapping for agent outputs in AWS SageMaker and OpenAI plugins to prevent double literal conversion. Modified AWS SageMaker plugin test cases to enhance output type validation and added conditional checks for LiteralMap instances with improved mock return value handling.Unit tests added: True
Estimated effort to review (1-5, lower is better): 1