-
Notifications
You must be signed in to change notification settings - Fork 188
fix: Gemini thoughts not correctly accumulated when streaming enabled #514
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
base: main
Are you sure you want to change the base?
Conversation
bbf6d0c to
33ac74f
Compare
Poggecci
left a comment
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.
Hey Owen. Thank you for the contribution!
This PR seems to both resolve the thoughts accumulation issue, but also changes what content we send in our requests to the model. Is there a reason you've coupled these two changes? The original rationale behind stripping thoughts was a naive form of context management (although the relevance of this is fully dependent on the GenAI SDK's handling of the thoughts we provide), but since we've had this behavior since release, a change to it merits its own PR.
Happy to approve if just the accumulation is kept in this PR or if we have some further discussion on the matter.
| } else { | ||
| if (accumulatedThoughtText.length() > 0 | ||
| && GeminiUtil.shouldEmitAccumulatedText(currentProcessedLlmResponse)) { | ||
| LlmResponse aggregatedTextResponse = |
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.
aggregatedThoughtResponse here?
|
@Poggecci thank you for taking a look. The issue with stripping thoughts is that will ultimately lead to empty parts being sent to the gemini API now that we are accumulating thought-only parts. I can add some more processing to remove those from the llm request here adk-java/core/src/main/java/com/google/adk/models/Gemini.java Lines 208 to 235 in 7f12064
In the python-adk if I look at pre-processing before https://github.com/google/adk-python/blob/4a842c5a1334c3ee01406f796651299589fe12ab/src/google/adk/models/google_llm.py#L149-L154 if stream:
responses = await self.api_client.aio.models.generate_content_stream(
model=llm_request.model,
contents=llm_request.contents,
config=llm_request.config,
)I see nothing removing thoughts - https://github.com/google/adk-python/blob/4a842c5a1334c3ee01406f796651299589fe12ab/src/google/adk/models/google_llm.py#L300-L326. async def _preprocess_request(self, llm_request: LlmRequest) -> None:
if self._api_backend == GoogleLLMVariant.GEMINI_API:
# Using API key from Google AI Studio to call model doesn't support labels.
if llm_request.config:
llm_request.config.labels = None
if llm_request.contents:
for content in llm_request.contents:
if not content.parts:
continue
for part in content.parts:
# Create copies to avoid mutating the original objects
if part.inline_data:
part.inline_data = copy.copy(part.inline_data)
_remove_display_name_if_present(part.inline_data)
if part.file_data:
part.file_data = copy.copy(part.file_data)
_remove_display_name_if_present(part.file_data)
# Initialize config if needed
if llm_request.config and llm_request.config.tools:
# Check if computer use is configured
for tool in llm_request.config.tools:
if isinstance(tool, types.Tool) and tool.computer_use:
llm_request.config.system_instruction = None
await self._adapt_computer_use_tool(llm_request) |
516d136 to
a3f0560
Compare
a3f0560 to
cbddffb
Compare
c1754f1 to
eb26d88
Compare
eb26d88 to
e71adcb
Compare
Fixes #510