Fix api_timeout not being used in APIRemoteWorkspace #1206
Conversation
The api_timeout parameter was defined but not actually used in the HTTP client configuration. This caused all requests to use the default 60 second timeout from the base RemoteWorkspace class, preventing users from extending timeouts for slow local models. Changes: - Override the client property in APIRemoteWorkspace to use api_timeout for the read timeout in HTTP requests - Add comprehensive tests to verify api_timeout is properly applied - Ensure other timeout values (connect, write, pool) remain at defaults Fixes #1205 Co-authored-by: openhands <[email protected]>
| base_url=self.host, timeout=timeout, headers=self._headers | ||
| ) | ||
| self._client = client | ||
| return client |
There was a problem hiding this comment.
This code smells. @OpenHands, can you check the parent class implementation and suggest ways to simplify how we initialize the client so we don't end up overwriting the client initialized by the parent class multiple times?
We run into a similar situation in _start_or_attach_to_runtime when we do
self.host = self._runtime_url.rstrip("/")
self.api_key = self._session_api_key
self._client = None # Reset HTTP client with new host and API key
_ = self.client # Initialize client by accessing the property
assert self.client is not None
assert self.client.base_url == self.host
There was a problem hiding this comment.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Summary of Refactoring
I've successfully refactored the client initialization code to eliminate the code smell you identified. The changes have been committed and pushed to the PR branch.
Problem Identified
The original implementation had several issues:
- Code Duplication:
APIRemoteWorkspaceduplicated the entire 18-lineclientproperty just to change the timeout value - Fragile Reset Logic: Manual reset using
self._client = Nonefollowed by_ = self.clientwas not explicit or reusable - Multiple Initializations: Client was being initialized and reset multiple times during the lifecycle
Solution: Template Method Pattern
I applied the Template Method design pattern to make the code more maintainable:
1. Added _create_timeout() method in base classes (RemoteWorkspace and AsyncRemoteWorkspace)
- Returns default timeout configuration
- Can be overridden by subclasses to customize timeouts
- Eliminates need to duplicate entire client property
2. Added _reset_client() method in base classes
- Encapsulates client reset logic with proper cleanup
- Makes the reset operation explicit and reusable
- Properly closes existing client before resetting
3. Simplified APIRemoteWorkspace
- Removed the entire 18-line
clientproperty override - Added a simple 12-line
_create_timeout()override - Updated
_start_or_attach_to_runtime()to useself._reset_client()instead ofself._client = None
4. Updated AsyncRemoteWorkspace for consistency
- Applied the same pattern to the async version
- Previously had no timeout configuration at all
- Now consistent with sync version
Benefits
✅ Reduced Code Duplication: Child classes now only need to override _create_timeout() (12 lines) instead of the entire client property (18 lines)
✅ Clearer Intent: _reset_client() makes it obvious when and why we're resetting the client
✅ Better Maintainability: Changes to client initialization logic only need to be made in one place
✅ Extensibility: Easy for future subclasses to customize timeout or client creation behavior
✅ Consistency: Both sync and async versions follow the same pattern
✅ Backward Compatible: All 64 existing tests pass without modification
Files Changed
openhands-sdk/openhands/sdk/workspace/remote/base.py(+35, -6)openhands-sdk/openhands/sdk/workspace/remote/async_remote_workspace.py(+33, -1)openhands-workspace/openhands/workspace/remote_api/workspace.py(+16, -18)
The changes have been pushed to PR #1206.
This refactoring simplifies how subclasses customize the HTTP client configuration without duplicating the entire client property logic. Changes: - Add _create_timeout() method in base RemoteWorkspace classes that subclasses can override to customize timeout configuration - Add _reset_client() method to encapsulate client reset logic - Update APIRemoteWorkspace to override _create_timeout() instead of duplicating the entire client property - Apply same pattern to AsyncRemoteWorkspace for consistency - Replace manual client reset (_client = None) with _reset_client() call Benefits: - Eliminates code duplication (child classes no longer need to override the entire client property) - Makes client reset logic explicit and reusable - Follows template method design pattern for better maintainability - All existing tests pass, ensuring backward compatibility Co-authored-by: openhands <[email protected]>
Coverage Report •
|
|||||||||||||||||||||||||||||||||||
Description
Fixes #1205
The
api_timeoutparameter was defined inAPIRemoteWorkspacebut was not actually being used in the HTTP client configuration. This caused all requests to use the hardcoded 60-second timeout from the baseRemoteWorkspaceclass, preventing users from extending timeouts for slow local models or saturated systems.Changes
clientproperty override inAPIRemoteWorkspacethat uses theapi_timeoutfield for the HTTP client's read timeoutapi_timeoutvalues are properly appliedapi_timeoutof 60 seconds works correctlyTesting
All tests pass:
tests/workspace/test_api_remote_workspace.pyverify the timeout configurationImpact
Users can now properly configure longer API timeouts by setting the
api_timeoutparameter when creating anAPIRemoteWorkspace, which will help with scenarios like:@xingyaoww can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:9b5b643-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9b5b643-python) is a multi-arch manifest supporting both amd64 and arm649b5b643-python-amd64) are also available if needed