-
Notifications
You must be signed in to change notification settings - Fork 258
feat: use reasoning field in StreamingChunk for Google GenAI #2900
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
Changes from 1 commit
70af8fa
648bfd3
510a336
bd3d479
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -489,6 +489,38 @@ def test_convert_google_chunk_to_streaming_chunk_real_example(self, monkeypatch) | |
| assert streaming_chunk.tool_calls[5].id is None | ||
| assert streaming_chunk.tool_calls[5].index == 5 | ||
|
|
||
| def test_convert_google_chunk_to_streaming_chunk_with_thought(self, monkeypatch): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you try using actual objects from Google API in this test?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call — updated the test to use actual |
||
| """Test that thought parts populate StreamingChunk.reasoning instead of meta.""" | ||
| monkeypatch.setenv("GOOGLE_API_KEY", "test-api-key") | ||
| component = GoogleGenAIChatGenerator() | ||
| component_info = ComponentInfo.from_component(component) | ||
|
|
||
| # Simulate a chunk with a thought part (reasoning-only chunk) | ||
| mock_thought_part = Mock() | ||
| mock_thought_part.text = "Let me think about this..." | ||
| mock_thought_part.thought = True | ||
| mock_thought_part.function_call = None | ||
|
|
||
| mock_content = Mock() | ||
| mock_content.parts = [mock_thought_part] | ||
| mock_candidate = Mock() | ||
| mock_candidate.content = mock_content | ||
| mock_candidate.finish_reason = None | ||
|
|
||
| mock_chunk = Mock() | ||
| mock_chunk.candidates = [mock_candidate] | ||
| mock_chunk.usage_metadata = None | ||
|
|
||
| streaming_chunk = _convert_google_chunk_to_streaming_chunk( | ||
| chunk=mock_chunk, index=0, component_info=component_info, model="gemini-2.5-flash" | ||
| ) | ||
|
|
||
| # Reasoning should be in the reasoning field, not in meta | ||
| assert streaming_chunk.reasoning is not None | ||
| assert streaming_chunk.reasoning.reasoning_text == "Let me think about this..." | ||
| assert "reasoning_deltas" not in streaming_chunk.meta | ||
| assert streaming_chunk.content == "" | ||
|
|
||
| def test_aggregate_streaming_chunks_with_reasoning(self): | ||
| """Test the _aggregate_streaming_chunks_with_reasoning function for reasoning content aggregation.""" | ||
|
|
||
|
|
@@ -515,9 +547,6 @@ def test_aggregate_streaming_chunks_with_reasoning(self): | |
| } | ||
| final_chunk.reasoning = ReasoningContent(reasoning_text="I should greet the user politely") | ||
|
|
||
| # Add reasoning deltas to the final chunk meta (this is how the real method works) | ||
| final_chunk.meta["reasoning_deltas"] = [{"type": "reasoning", "content": "I should greet the user politely"}] | ||
|
|
||
| # Test aggregation | ||
| result = _aggregate_streaming_chunks_with_reasoning([chunk1, chunk2, final_chunk]) | ||
|
|
||
|
|
||
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.
My impression is that we can use the
extrafield ofReasoningContent(code) to storethought_signature_deltas.In my opinion, we did something similar in #2849 for Anthropic redacted thinking and thinking signature.
Since all this info is related to reasoning, I'd like to have it grouped into
ReasoningContent.But I haven't tried, and I don't know if this is simple to implement or if it would make the code much more complex. So please try and let me know.
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.
Thanks for the suggestion! I've moved
thought_signature_deltasintoReasoningContent.extrafor reasoning chunks, consistent with the Anthropic approach in #2849.One caveat:
StreamingChunkenforces mutual exclusivity betweencontentandreasoning(raisesValueErrorin__post_init__), so for text/tool-call chunks that also carry thought signatures, the signatures still go inmeta. The aggregation logic reads from both sources. This keeps all reasoning-related info grouped intoReasoningContentwhere possible without breaking the constraint.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.
Seeing the code, I now realize that Thought Signatures can be included in non-reasoning response parts.
For this reason, I recommend going back to the previous version of your code where
thought_signature_deltasare always stored inmeta. This would be simpler and consistent.I'd just add a comment on top of the code line explaining that this data can be part of non-reasoning content parts.
Makes sense?
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.
Makes sense! Reverted in bd3d479 —
thought_signature_deltasare now always stored inmetawith a comment explaining that thought signatures can appear in both reasoning and non-reasoning response parts.