-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(models): handle mixed tool responses and text/media in LiteLLM ad… #4109
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
Conversation
…apter Signed-off-by: Akshat Kumar <[email protected]>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @Akshat8510, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue in the LiteLLM adapter where multipart content objects, particularly those combining tool responses with text or media, were not fully processed. The changes ensure that all components of a complex content object are correctly converted into a list of messages, enhancing the adapter's ability to support advanced vision-language and tool-use workflows without data loss. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Response from ADK Triaging Agent Hello @Akshat8510, thank you for your contribution! Before we can merge this pull request, we need you to sign our Contributor License Agreement (CLA). You can do so by following the instructions at https://cla.developers.google.com/. Once you've signed the CLA, the status check will be updated, and we can proceed with the review. Thanks! |
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.
Code Review
This pull request aims to fix an issue with handling mixed tool responses and other content types in the LiteLLM adapter. The approach of accumulating messages is correct, but the current implementation has some regressions. It introduces a less safe method for serializing tool responses, and more critically, it incorrectly handles thought and function_call parts in messages, leading to dropped information. Additionally, a large part of the original message processing logic has become unreachable dead code. I've provided detailed feedback on how to address these issues.
…responses while preserving original logic Signed-off-by: Akshat Kumar <[email protected]>
|
@gemini-code-assist /gemini review |
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.
Code Review
This pull request refactors the _content_to_message_param function to correctly handle mixed content containing both tool responses and other media like text. The change correctly separates tool responses from other parts and constructs a list of messages, fixing a bug where parts of the content were being silently dropped. My review identifies one critical issue: a large block of dead code that was not removed after the refactoring. I've also suggested improving the function's docstring to reflect its new, more complex behavior, which is important for future maintainability.
Signed-off-by: Akshat Kumar <[email protected]>
|
@gemini-code-assist /gemini review |
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.
Code Review
This pull request effectively refactors _content_to_message_param to correctly handle mixed content, addressing a bug where parts of multipart messages were being discarded. The new logic for separating and processing tool responses from other content is a significant improvement. I've identified a couple of issues: some leftover dead code from the refactoring and a bug in how reasoning content is constructed. Addressing these will make the implementation solid.
src/google/adk/models/lite_llm.py
Outdated
| content_parts.append(part) | ||
|
|
||
| message_content = await _get_content(content_parts, provider=provider) or None | ||
| reasoning_content = "\n".join([p.thought for p in reasoning_parts]) if reasoning_parts else 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.
There appears to be a bug in the construction of reasoning_content. It's currently using p.thought, which seems to be a boolean flag, instead of p.text, which holds the actual reasoning text. This will result in incorrect content for the thought field of the ChatCompletionAssistantMessage.
| reasoning_content = "\n".join([p.thought for p in reasoning_parts]) if reasoning_parts else None | |
| reasoning_content = "\n".join([p.text for p in reasoning_parts if p.text]) if reasoning_parts else None |
src/google/adk/models/lite_llm.py
Outdated
| if not final_messages: | ||
| return [] | ||
|
|
||
| return final_messages if len(final_messages) > 1 else final_messages[0] |
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.
Signed-off-by: Akshat Kumar <[email protected]>
|
@gemini-code-assist /gemini review |
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.
Code Review
This pull request refactors _content_to_message_param to correctly handle mixed content, including tool responses alongside text or media. The previous implementation would prematurely return upon encountering a tool response, discarding subsequent content. The new logic correctly accumulates all parts and constructs a list of messages. While the core logic of the fix is sound, a significant block of dead code from the previous implementation has been left in the function, which needs to be removed.
|
Closing this PR as I see the issue has been addressed in commit fdc286a. Glad to see the data-loss issue for mixed content is resolved! Looking forward to more contributions. |
1. Link to an existing issue (if applicable):
Problem:
The LiteLLM adapter in
_content_to_message_paramwould return early as soon as it encountered afunction_responsepart. In multipartContentobjects (common in Vision-Language workflows), this caused additional parts like text captions or image blobs to be silently discarded.Solution:
I modified
_content_to_message_paramto accumulate all parts into a list of messages instead of returning on the first tool hit.function_responseparts are converted to messages with thetoolrole._get_contenthelper and appended as a subsequent message (usually with theuserrole).Testing Plan
Unit Tests:
Summary of verification:
I ran a test suite covering three core scenarios:
Manual End-to-End (E2E) Tests:
I used a reproduction script to simulate the conversion of a
types.Contentobject containing both aFunctionResponseand atextpart.Terminal Logs:
Checklist