fix(models): aggregate streaming thought parts for all LiteLlm providers - #6898
Open
Ashfaqbs wants to merge 1 commit into
Open
fix(models): aggregate streaming thought parts for all LiteLlm providers#6898Ashfaqbs wants to merge 1 commit into
Ashfaqbs wants to merge 1 commit into
Conversation
The streaming finalizers passed the raw per-delta thought parts straight into the response, so a provider that streams reasoning token by token (e.g. xai/grok, OpenAI reasoning models via LiteLLM) produced one types.Part(thought=True) per delta in the aggregated response instead of one part per thinking block. _aggregate_streaming_thought_parts already did this joining, splitting on thought_signature to preserve Anthropic's per-block boundaries, but it was only wired into the Anthropic message- building path. Call it from both stream finalizers so every provider gets the same shape the non-streaming path already produces. Fixes google#6895
|
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. |
Author
|
recheck |
Contributor
|
I submitted a PR for this already: #6896 |
This file contains hidden or 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
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem:
LiteLlmstreaming builds the aggregated (non-partial)LlmResponseby joining the buffered text into a single part, but passes the buffered reasoning parts straight through unjoined. So for a provider that streams reasoning token-by-token (e.g.xai/grok-4.6, OpenAI reasoning models via LiteLLM), the final response — and therefore the persisted session event — holds onetypes.Part(thought=True)per streamed reasoning delta instead of one part per thinking block._aggregate_streaming_thought_partsalready does this joining (splitting onthought_signatureso Anthropic's per-block boundaries are preserved), but it was only wired into the Anthropic message-building path (_content_to_message_param), not into the two stream finalizers that build the response object itself.Solution:
Call
_aggregate_streaming_thought_parts(reasoning_parts)from both_finalize_tool_call_responseand_finalize_text_responseinsrc/google/adk/models/lite_llm.py, instead oflist(reasoning_parts). This makes every LiteLlm provider produce the same shape the non-streaming path already produces, and matches the aggregator's own docstring ("produces clean parts for session history and outbound requests").Testing Plan
Unit Tests:
Added
test_generate_content_async_stream_aggregates_reasoning_deltasintests/unittests/models/test_litellm.py, which streams three separatereasoning_contentdeltas (nothought_signature, matching how a non-Anthropic provider like xAI streams) followed by a text delta, and asserts the final non-partial response contains exactly onethought=Truepart with the joined text, instead of three.Also ran the full reasoning/thought-focused subset in isolation:
Manual End-to-End (E2E) Tests:
Not run — this is a pure data-shape bug in response aggregation, fully exercised by the unit test above (constructs the same per-token
ModelResponseStreamdeltas a live xAI/OpenAI-reasoning stream would produce and asserts on the resultingLlmResponse.content.parts). No live provider credentials were available to additionally verify against a real streaming API call.Checklist
Additional context
Both call sites previously read
thought_parts=list(reasoning_parts) if reasoning_parts else None; the fix replaceslist(reasoning_parts)with_aggregate_streaming_thought_parts(reasoning_parts). No behavior changes for the Anthropic path, which already routed through the aggregator via a separate code path (_content_to_message_param) for outbound requests — this PR fixes the inbound/session-persisted shape for all providers, including Anthropic's own aggregated response object.