Fix/transcript finish #3324
Fix/transcript finish #3324
Conversation
Summary of ChangesHello @ChrisQlasty, 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 where the ADK (Agent Development Kit) was failing to properly propagate the Highlights
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
|
adk-bot
added
the
live
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of the finished flag not being passed for input/output transcriptions by relaxing the validation conditions in GeminiLlmConnection.receive. The change is simple and effective. The accompanying unit test, test_receive_transcript_finished, properly verifies this new behavior for both input and output transcriptions. I've added a couple of suggestions to the new test code to improve its conciseness and maintainability by reducing code duplication. Overall, this is a good fix.
| class Msg: | ||
|
|
||
| def __init__(self): | ||
| self.server_content = mock.Mock() | ||
| sc = self.server_content | ||
| sc.model_turn = None | ||
| if tx_direction == 'input': | ||
| sc.input_transcription = finished_tx | ||
| sc.output_transcription = None | ||
| else: | ||
| sc.input_transcription = None | ||
| sc.output_transcription = finished_tx | ||
| sc.interrupted = False | ||
| sc.turn_complete = False | ||
| self.tool_call = None | ||
| self.session_resumption_update = None | ||
|
|
||
| async def gen(): | ||
| yield Msg() |
There was a problem hiding this comment.
For improved readability and conciseness, you can create the mock message object directly by setting attributes on a mock.Mock() instance, instead of defining a local Msg class. This makes the test setup more straightforward and less verbose.
msg = mock.Mock()
msg.tool_call = None
msg.session_resumption_update = None
msg.server_content.model_turn = None
msg.server_content.interrupted = False
msg.server_content.turn_complete = False
msg.server_content.input_transcription = (
finished_tx if tx_direction == 'input' else None
)
msg.server_content.output_transcription = (
finished_tx if tx_direction == 'output' else None
)
async def gen():
yield msg| if tx_direction == 'input': | ||
| tx_resps = [r for r in responses if r.input_transcription] | ||
| else: | ||
| tx_resps = [r for r in responses if r.output_transcription] | ||
|
|
||
| if tx_direction == 'input': | ||
| assert tx_resps, 'Excpected input transcription response' | ||
| assert tx_resps[0].input_transcription.finished is True | ||
| assert not tx_resps[0].input_transcription.text | ||
| else: | ||
| assert tx_resps, 'Expected output transcription response' | ||
| assert tx_resps[0].output_transcription.finished is True | ||
| assert not tx_resps[0].output_transcription.text |
There was a problem hiding this comment.
The assertion logic is duplicated for the 'input' and 'output' cases. You can refactor this to be more DRY (Don't Repeat Yourself) by using a variable for the attribute name and consolidating the checks. This makes the test easier to read and maintain. This change also corrects a typo in the assertion message ('Excpected' -> 'Expected').
attr_name = f'{tx_direction}_transcription'
tx_resps = [r for r in responses if getattr(r, attr_name)]
assert tx_resps, f'Expected {tx_direction} transcription response'
transcription = getattr(tx_resps[0], attr_name)
assert transcription.finished is True
assert not transcription.text2 participants
Problem:
ADK seems to not pass output/input transcription
finishedflag from the Gemini.Solution:
Relaxation of checking conditions of valid llm_response for input/output transcription.
Testing Plan
Unit test
test_receive_transcript_finishedchecks if input/output transcription message with no text butfinishedflag is received.Unit Tests:
Manual End-to-End (E2E) Tests:
Configure Gemini Agent to produce input & output transcriptions. Observe incoming transcript messages - to see if the finished flag appears at the end agents & users statement.
Checklist
Additional context
The mentioned
finishedflag can be obtained in native Gemini APIs like via Websocket but not via ADK.