-
-
Notifications
You must be signed in to change notification settings - Fork 102
[Platform][Gemini] Fix choice conversion logic for executableCode
and codeExecutionResult
#421
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
[Platform][Gemini] Fix choice conversion logic for executableCode
and codeExecutionResult
#421
Conversation
94dac63
to
fa4614e
Compare
src/platform/tests/Bridge/Gemini/CodeExecution/ResultConverterTest.php
Outdated
Show resolved
Hide resolved
executableCode
and codeExecutionResult
friendly ping @valtzu |
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.
Pull Request Overview
This PR fixes a crash in the Gemini ResultConverter when handling responses containing only executableCode
and codeExecutionResult
parts, particularly when all parts are marked as thought: true
. The fix prevents RuntimeException: "Unsupported finish reason STOP" by implementing proper aggregation logic for multi-part responses.
- Enhanced choice conversion logic to handle both single-part (legacy) and multi-part responses
- Added aggregation of executable code into formatted content and preservation of successful execution results
- Comprehensive test coverage for the previously failing scenario with thought-flagged parts
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/platform/src/Bridge/Gemini/Gemini/ResultConverter.php | Enhanced convertChoice method to aggregate multiple parts containing executable code and execution results |
src/platform/tests/Bridge/Gemini/CodeExecution/ResultConverterTest.php | Added test case covering the crash scenario with thought-flagged parts containing code execution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/platform/tests/Bridge/Gemini/CodeExecution/ResultConverterTest.php
Outdated
Show resolved
Hide resolved
executableCode
and codeExecutionResult
executableCode
and codeExecutionResult
src/platform/tests/Bridge/Gemini/CodeExecution/ResultConverterTest.php
Outdated
Show resolved
Hide resolved
"thought": true | ||
}, | ||
{ | ||
"codeExecutionResult": { |
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.
So this response contains 2 failed and one OK?
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.
Yes, this response matches the requested prompt:
'Count how many times each word appears in this text using the Python code execution tool:
"Symfony est un framework PHP. Symfony est utilisé dans de nombreux projets. PHP est un langage puissant."
Display the result as a dictionary sorted by frequency, then explain the result.'
It seems Gemini iteratively fixed its code until it executed correctly.
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.
so would be good to have a fixture (and test) for failed, too
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.
Finally, I removed the isSuccessfulCodeExecution
condition, we want to display all the LLM’s thoughts to the user, right?"
First of all, thanks for working on this – when the In my opinion the output should not contain the generated & executed code by default, since the last message/ The If I'm building a customer-facing chat bot which should be able to answer to Somewhere we have this feature: |
7b50efb
to
a2f4c4d
Compare
}, | ||
{ | ||
"codeExecutionResult": { | ||
"outcome": "OUTCOME_OK", |
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.
When I tried it via the API, after codeExecutionResult.outcome: "OUTCOME_OK"
there is a normal text
part, i.e.
{
"text": "The 20th Fibonacci number is 6765.\n\nThe nearest palindrome to 6765 is 6776."
}
Did you not get it in the response? If you did, let's add it to the fixture?
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.
It's weird, I never saw this part before, but you right I have it in API response, I will take this part into account.
I think text
is not in the response when we have parts with thoughts
key.
Thanks a lot for clarifying! I see your point now. Since I like the idea of making this configurable. |
The rebase looks wrong |
a2f4c4d
to
4ea8008
Compare
At the moment the code iterates through the contentParts array and looks for a successful code execution marker.
|
src/platform/src/Bridge/Gemini/Gemini/CodeExecutionResultOutcome.php
Outdated
Show resolved
Hide resolved
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.
Only one minor, afterwards good to merge. Thanks
…nd `codeExecutionResult`
8fe9603
to
bbff633
Compare
Thanks for fixing this bug @pentiminax. |
Thanks for reviewing @OskarStark. |
Yes, can you have a look? |
This PR was squashed before being merged into the main branch. Discussion ---------- [Platform][VertexAI] Update `ResultConverter` | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Docs? | no | License | MIT Same as #421 but for VertexAI bridge This PR fixes a crash in the Gemini ResultConverter when handling responses that only contained executableCode and codeExecutionResult parts. Rather than duplicating the code, I created a trait that is used in both bridges. I also reused the same test fixtures. Commits ------- a46ddcf [Platform][VertexAI] Update `ResultConverter`
This PR fixes a crash in the Gemini ResultConverter when handling responses that only contained executableCode and codeExecutionResult parts.
Before:
After:
What’s Changed
New unit tests:
Why
Impact