Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
execute_sqlinintegrations/bigquery/query_tool.pyis a plaindefcalling a blocking BigQuery client, and ADK awaits a sync tool inline on the running loop. Nothing else in the process runs until the query returns, so on a server handling more than one session the others stall behind a call that is only waiting on the network. Spanner had the same problem and was fixed in 1dbceccf; Bigtable in 72f3e7e1. BigQuery has noasyncio.to_threadanywhere.Solution:
Make
execute_sqla coroutine and run the existing_execute_sqlhelper withawait asyncio.to_thread(...), the same pattern Spanner and Bigtable use._execute_sqlstays sync becauseforecast,analyze_contributionanddetect_anomaliescall it directly; those three block the loop too, but that's a separate change.get_execute_sqland the docstring-swapping helper returnAwaitable[dict]now, and the write-mode variants are clones ofexecute_sql.__code__, so they become coroutines with it.Testing Plan
Unit Tests:
One regression test added to
tests/unittests/integrations/bigquery/test_bigquery_query_tool.py:test_execute_sql_leaves_the_event_loop_free_while_querying- the stubbed client waits insidequery_and_waiton athreading.Eventthat only a concurrently scheduled asyncio task can set. If the query ran on the loop thread the release could never happen (bounded by a 10s timeout, so a regression fails instead of hanging CI).The rest of the diff converts the existing
execute_sqltests to coroutines.test_tool_call_doesnt_change_global_settingsandtest_tool_call_doesnt_mutate_job_labelsare parametrized over bothexecute_sqland the three ML tools that stay sync, so they await only when the result is awaitable.toxacross 3.10-3.14 is clean apart from the twotest_import_loading.pyallowlist cases on 3.11-3.13, which fail identically on unchangedmainhere.Manual End-to-End (E2E) Tests:
Run the repro script from #7160: it stands in a BigQuery client that takes 2 seconds and counts how often a 100 ms heartbeat task gets to run while the query is in flight. Before this change the heartbeat does not tick at all until the query returns; with it the interval holds.
Checklist
Additional context
Only
execute_sqlhere. The other BigQuery tools still run the blocking client on the loop and can follow the same pattern separately.