-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Fix: Process function_response from separate request by searching full history Fixes #3531 #3553
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?
Fix: Process function_response from separate request by searching full history Fixes #3531 #3553
Conversation
…ync function_response Fixes google#3531
|
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. |
Summary of ChangesHello @miguelabdonsh, 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 a critical regression that prevented the correct processing of asynchronous function responses in the system. By extending the search scope for corresponding function calls to the entire session history, the change ensures that 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
|
|
Response from ADK Triaging Agent Hello @miguelabdonsh, thank you for your contribution! Before we can merge this PR, you'll need to sign a Contributor License Agreement (CLA). You can find more information in our contribution guidelines. Once the CLA is signed, we can proceed with the review. Thanks! |
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.
Code Review
This pull request addresses a regression where a function_response arriving in a separate request could not find its corresponding function_call. The fix correctly expands the search to the full session history. The addition of a new unit test to validate this asynchronous scenario is a great improvement. However, I've identified a critical issue in the implementation where an index from the full history is incorrectly used on a filtered list of events, which could lead to errors. Please see my detailed comment.
| search_events = all_events if all_events else events | ||
| function_call_event_idx = -1 | ||
| # look for corresponding function call event reversely | ||
| for idx in range(len(events) - 2, -1, -1): | ||
| event = events[idx] | ||
| for idx in range(len(search_events) - 2, -1, -1): | ||
| event = search_events[idx] | ||
| function_calls = event.get_function_calls() | ||
| if function_calls: | ||
| for function_call in function_calls: |
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.
There appears to be a critical issue with how the index from this search is used. The function_call_event_idx is calculated by iterating over search_events (which can be the full history all_events), but this index is later used to slice and access elements in events (the filtered list).
Since events is a processed and filtered version of search_events, their indices are not interchangeable. This will likely lead to an IndexError or incorrect logic when the lists differ in length or content.
To resolve this, after finding the matching event at idx in search_events, you should find the corresponding index of that same event within the events list (for example, by matching event.id). This new, correct index should then be used for all subsequent operations on events.
Problem
When sending a function_response via a separate
/run_sserequest (e.g., asyncjob completion callback),
_rearrange_events_for_latest_function_responseraised:
ValueError: No function call event found for function responses ids
The function only searched events in the current request context
(
filtered_events), not the full session history. When a function_responsearrived in a separate request, it couldn't find the matching function_call from
the previous request.
Regression from ADK 1.17.0 → 1.18.0.
Solution
Modified
_rearrange_events_for_latest_function_responseto search the fullsession history (
rewind_filtered_events) instead of only current requestevents.
Changes in
src/google/adk/flows/llm_flows/contents.py:- Added
all_eventsparameter (line 128)- Search in
all_eventsif provided, otherwise fall back toevents(line165-169)
- Pass rewind_filtered_events when calling the function (line 440-441)
Testing
✅ All 30 existing tests in
test_contents*.pypass✅ New test
test_function_response_in_separate_request.pyverifies fix✅ Test simulates async scenario: function_call in early session history,
function_response arrives later