fix(python): add timeout parameter to generated RPC methods#580
fix(python): add timeout parameter to generated RPC methods#580Rasaboun wants to merge 1 commit intogithub:mainfrom
Conversation
Rasaboun
force-pushed
the
fix/python-rpc-timeout-param
branch
2 times, most recently
from
371af5c to
5af8c3d
Compare
Every generated async RPC method now accepts an optional `timeout` keyword argument that is forwarded to `JsonRpcClient.request()`. This lets callers override the default 30s timeout for long-running RPCs like `session.fleet.start` without bypassing the typed API. Fixes github#539
Rasaboun
force-pushed
the
fix/python-rpc-timeout-param
branch
from
5af8c3d to
7a0753b
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Python SDK’s generated typed RPC surface so callers can override the default JSON-RPC request timeout (notably for long-running blocking RPCs like session.fleet.start) while keeping the default behavior unchanged when no timeout is provided.
Changes:
- Extend Python RPC codegen to add a keyword-only
timeout: Optional[float] = Noneparameter to every generated RPC method and forward it toJsonRpcClient.request(). - Add a generated
_timeout_kwargs()helper to conditionally includetimeoutonly when provided. - Add Python unit tests covering default vs. custom timeout forwarding.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
scripts/codegen/python.ts |
Updates the Python RPC generator to add timeout to generated method signatures and forward it via _timeout_kwargs(). |
python/copilot/generated/rpc.py |
Regenerated output reflecting the new timeout parameter and forwarding behavior across RPC wrappers. |
python/test_rpc_timeout.py |
Adds unit tests to validate that timeout is not forwarded by default and is forwarded when explicitly provided. |
| lines.push(` | ||
| def _timeout_kwargs(timeout: Optional[float]) -> dict: | ||
| """Build keyword arguments for optional timeout forwarding.""" | ||
| if timeout is not None: | ||
| return {"timeout": timeout} | ||
| return {} | ||
|
|
||
| `); | ||
| lines.push(typesCode); |
There was a problem hiding this comment.
The generated _timeout_kwargs helper is emitted before the quicktype-generated import block (typesCode), which results in rpc.py defining a function in between import sections (and before some imports). Consider emitting the helper after typesCode (or at least after all imports) so the generated module keeps imports at the top and won’t trip tools that enforce import ordering if copilot/generated is ever linted/type-checked.
| @pytest.mark.asyncio | ||
| async def test_timeout_on_other_methods(self): | ||
| client = AsyncMock() | ||
| client.request = AsyncMock(return_value={"mode": "plan"}) | ||
| api = ModeApi(client, "sess-1") | ||
|
|
||
| await api.set(SessionModeSetParams(mode=Mode.PLAN), timeout=120.0) | ||
|
|
||
| _, kwargs = client.request.call_args | ||
| assert kwargs["timeout"] == 120.0 |
There was a problem hiding this comment.
Current tests only cover session-scoped methods that have params. Since the codegen change also touches (1) server-scoped APIs and (2) methods with no params (e.g., ModelsApi.list()), it would be good to add at least one test covering a no-params method and/or a server-scoped method to ensure timeout forwarding works in those branches too.
Summary
scripts/codegen/python.tsso every generated RPC method gets an optionaltimeoutkeyword argument forwarded toJsonRpcClient.request()_timeout_kwargshelper in the generated preamble to conditionally build the**kwargspython/copilot/generated/rpc.py(not hand-edited)Usage
Motivation
FleetApi.start()inherits a hardcoded 30s timeout fromJsonRpcClient.request(), butsession.fleet.startis a blocking RPC that only returns once the fleet completes — routinely >30s. Users are forced to bypass the typed API.Fixes #539
Note on
fleet.startsemanticssession.fleet.startblocks until the entire fleet completes and always returnsstarted=True, making the return value meaningless. Consider makingfleet.startreturn immediately once the fleet is deployed, and let callers wait viaSESSION_IDLEevents. This would also eliminate the need for large timeouts.Test plan
npm run generate:pythonsucceedsrpc.pydiff:timeoutparam on every method,_timeout_kwargshelper at toppytest test_rpc_timeout.py)ruff check .clean