-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix runconfig options endpoint #4264
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,7 +113,12 @@ def _event_state_delta(state_delta: dict[str, Any]): | |
|
|
||
|
|
||
| # Define mocked async generator functions for the Runner | ||
| async def dummy_run_live(self, session, live_request_queue): | ||
| async def dummy_run_live( | ||
| self, | ||
| session, | ||
| live_request_queue, | ||
| run_config: Optional[RunConfig] = None, | ||
| ): | ||
| yield _event_1() | ||
| await asyncio.sleep(0) | ||
|
|
||
|
|
@@ -1411,5 +1416,48 @@ def test_builder_save_rejects_traversal(builder_test_client, tmp_path): | |
| assert not (tmp_path / "app" / "tmp" / "escape.yaml").exists() | ||
|
|
||
|
|
||
| def test_run_live_accepts_new_config_options(test_app, test_session_info): | ||
| """Test that /run_live endpoint accepts proactive_audio, affective_dialog, and session_resumption query parameters.""" | ||
| # First, create a session to use | ||
| session_url = ( | ||
| f"/apps/{test_session_info['app_name']}/users/" | ||
| f"{test_session_info['user_id']}/sessions/{test_session_info['session_id']}" | ||
| ) | ||
| test_app.post(session_url, json={"state": {}}) | ||
|
|
||
| # The actual websocket test would require more elaborate setup. | ||
| # Here we verify that the endpoint accepts the new query parameters | ||
| # by checking FastAPI's route signature. | ||
| from fastapi import Query as FastApiQuery | ||
|
|
||
| # Find the /run_live route | ||
| run_live_route = None | ||
| for route in test_app.app.routes: | ||
| if hasattr(route, "path") and route.path == "/run_live": | ||
| run_live_route = route | ||
| break | ||
|
|
||
| assert run_live_route is not None, "/run_live route not found" | ||
|
|
||
| # Check that the endpoint function signature includes the new parameters | ||
| import inspect | ||
|
|
||
| endpoint_fn = run_live_route.endpoint | ||
| sig = inspect.signature(endpoint_fn) | ||
| params = sig.parameters | ||
|
|
||
| # Verify the new parameters exist | ||
| assert "proactive_audio" in params, "proactive_audio parameter not found" | ||
| assert "affective_dialog" in params, "affective_dialog parameter not found" | ||
| assert ( | ||
| "session_resumption" in params | ||
| ), "session_resumption parameter not found" | ||
|
|
||
| # Verify they have default values of False | ||
| assert params["proactive_audio"].default.default is False | ||
| assert params["affective_dialog"].default.default is False | ||
| assert params["session_resumption"].default.default is False | ||
|
|
||
|
Comment on lines
+1419
to
+1460
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is a good start as it verifies that the new query parameters are present in the endpoint's signature with the correct defaults. However, it doesn't test the core logic of this change, which is the correct creation of the I recommend adding a test that verifies the Here is an example of what such a test could look like: from unittest.mock import patch, MagicMock
from google.adk.agents.run_config import RunConfig
from google.genai import types
import asyncio
@patch("google.adk.cli.adk_web_server.AdkWebServer.get_runner_async")
def test_run_live_creates_correct_run_config(
mock_get_runner, test_app, test_session_info
):
"""Test that /run_live creates RunConfig with correct options."""
# Setup session
session_url = (
f"/apps/{test_session_info['app_name']}/users/"
f"{test_session_info['user_id']}/sessions/{test_session_info['session_id']}"
)
test_app.post(session_url, json={"state": {}})
# Mock the runner and its run_live method
mock_runner = MagicMock()
async def empty_agen(*args, **kwargs):
if False: yield
mock_runner.run_live.return_value = empty_agen()
mock_get_runner.return_value = mock_runner
# Test with all options enabled
with test_app.websocket_connect(
f"/run_live?app_name={test_session_info['app_name']}"
f"&user_id={test_session_info['user_id']}"
f"&session_id={test_session_info['session_id']}"
"&proactive_audio=true&affective_dialog=true&session_resumption=true"
) as websocket:
pass # Just need to establish connection to trigger the endpoint logic
mock_get_runner.assert_called_with(test_session_info["app_name"])
mock_runner.run_live.assert_called_once()
# Check the run_config passed to run_live
call_args, call_kwargs = mock_runner.run_live.call_args
run_config = call_kwargs.get("run_config")
assert isinstance(run_config, RunConfig)
assert isinstance(run_config.proactivity, types.ProactivityConfig)
assert run_config.proactivity.proactive_audio is True
assert run_config.enable_affective_dialog is True
assert isinstance(run_config.session_resumption, types.SessionResumptionConfig)
# Reset mock and test default values
mock_runner.reset_mock()
with test_app.websocket_connect(
f"/run_live?app_name={test_session_info['app_name']}"
f"&user_id={test_session_info['user_id']}"
f"&session_id={test_session_info['session_id']}"
) as websocket:
pass
mock_runner.run_live.assert_called_once()
call_args, call_kwargs = mock_runner.run_live.call_args
run_config = call_kwargs.get("run_config")
assert run_config.proactivity is None
assert run_config.enable_affective_dialog is None
assert run_config.session_resumption is None |
||
|
|
||
| if __name__ == "__main__": | ||
| pytest.main(["-xvs", __file__]) | ||
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.
The expression
affective_dialog or Noneis a bit ambiguous and could be clearer. It maps aFalsevalue from the query parameter toNonefor theRunConfig. IfFalseandNonehave different meanings forenable_affective_dialog(e.g.,Falseexplicitly disables it, whileNoneuses a default behavior), this could be a subtle bug.Given the PR description states "All parameters default to False for backward compatibility", it seems more direct and readable to pass the boolean value as-is. This would pass
Falsewhen the query parameter isfalseor omitted, which aligns better with the stated goal.