add sandbox client and python sdk #5
Conversation
1. Restructure sandbox creation with pod templates; 2. Add resource tracker to avoid resource leakage; 3. Add logging mechanisms;
Summary of ChangesHello @Tweakzx, 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 introduces a brand new Python SDK and client for interacting with a sandbox service, completely replacing an older implementation. The new SDK is built with a focus on Kubernetes for provisioning ephemeral sandboxes, offering a hybrid approach for interaction: RESTful APIs for managing sandbox lifecycles (create, get, list, delete), and SSH/SFTP capabilities tunneled over HTTP CONNECT for secure command execution and file transfers within the sandboxes. This update provides a more structured, robust, and feature-rich client for sandbox integration. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces an old Python SDK with a new one. The new SDK is a significant improvement, but I've found several critical issues, including a circular import, incorrect method calls, and uninitialized attributes that will cause runtime errors. There are also some high-severity issues like a lack of SSL/TLS for tunneled connections and unreliable resource cleanup. Additionally, a generated file has been committed, and there are some medium-severity issues like hardcoded paths, dead code, and platform-dependent path joining. Please address these issues to ensure the SDK is robust and secure.
| def upload_file(self, local_path: str, remote_path: str) -> Dict[str, Any]: | ||
| if not self.is_running(): | ||
| raise exceptions.SandboxNotReadyError(f"Sandbox {self.id} is not running") | ||
| return self._client.upload_file(local_path, remote_path) |
There was a problem hiding this comment.
The upload_file method is implemented to take local_path, but it passes this path directly to self._client.upload_file(), which expects file content as a string, not a path. This will cause a runtime error. The method should read the file from local_path and pass its content to the client. Additionally, the return type hint Dict[str, Any] is incorrect as the client's upload_file method returns None.
Here's a suggested implementation:
def upload_file(self, local_path: str, remote_path: str) -> None:
if not self.is_running():
raise exceptions.SandboxNotReadyError(f"Sandbox {self.id} is not running")
with open(local_path, 'r') as f:
content = f.read()
self._client.upload_file(content, remote_path)
sdk_python/sandbox_client/client.py
Outdated
| remote_path: Path on remote server to upload to | ||
| ssh_client: SSH client instance (uses current connection if not provided) | ||
| """ | ||
| ssh_client = self.ssh_client or self.connect_ssh |
There was a problem hiding this comment.
self.connect_ssh is a method, but it's not being called here. This will result in ssh_client being assigned the method object itself, which is truthy, but it won't establish a connection. This will cause an error in subsequent operations. You need to call the method with ().
| ssh_client = self.ssh_client or self.connect_ssh | |
| ssh_client = self.ssh_client or self.connect_ssh() |
| response.raise_for_status() | ||
| return response.json() | ||
| except requests.exceptions.RequestException as e: | ||
| self.logger.error(f"Session query failed: {str(e)}") |
There was a problem hiding this comment.
The get_sandbox, get_sandboxs, and delete_sandbox methods use self.logger, but logger is not initialized as an attribute of the SandboxClient class. This will cause an AttributeError at runtime. Please initialize it in __init__, for example by using get_logger from services.log.
sdk_python/models/sandbox_info.py
Outdated
| from typing import Optional, Dict, Any | ||
| from models.sandbox_info import PodState | ||
| from sandbox_client.client import SandboxClient | ||
| from services import exceptions |
There was a problem hiding this comment.
These imports are misplaced. According to PEP 8, all imports should be at the top of the file. More critically, line 21 (from models.sandbox_info import PodState) is a circular import, as it imports from the file itself. This will cause a runtime error. Please move the necessary imports (SandboxClient, exceptions) to the top of the file and remove this block.
sdk_python/sandbox_client/client.py
Outdated
| def __del__(self): | ||
| """Destructor that automatically cleans up resources""" | ||
| self.cleanup() |
There was a problem hiding this comment.
Using __del__ for resource cleanup is unreliable because its execution is not guaranteed by the Python language specification. This can lead to resource leaks (e.g., open sockets). It is much safer to implement the context manager protocol (__enter__ and __exit__) for deterministic cleanup.
| #result = sandbox.execute_command(command=cmd) | ||
| result = sandbox.execute_command(command=cmd) |
There was a problem hiding this comment.
This commented-out line appears to be a leftover from development. If it's no longer needed, please remove it to keep the code clean.
| #result = sandbox.execute_command(command=cmd) | |
| result = sandbox.execute_command(command=cmd) | |
| result = sandbox.execute_command(command=cmd) |
sdk_python/sandbox_client/client.py
Outdated
| except requests.exceptions.RequestException as e: | ||
| self.logger.error(f"Session query failed: {str(e)}") | ||
|
|
||
| def get_sandboxs(self) -> List[Dict[str, Any]]: |
There was a problem hiding this comment.
The method name get_sandboxs should be get_sandboxes to follow standard English pluralization and for consistency with the API endpoint (/sandboxes).
| def get_sandboxs(self) -> List[Dict[str, Any]]: | |
| def get_sandboxes(self) -> List[Dict[str, Any]]: |
| current_dir += '/' | ||
| continue | ||
|
|
||
| current_dir = os.path.join(current_dir, dir) |
There was a problem hiding this comment.
Using os.path.join to construct a remote path is not platform-independent. On Windows, it will use backslashes, which will likely be incorrect for the remote SFTP server (which is usually a Unix-like system). You should use posixpath.join to ensure forward slashes are always used for remote paths. You will also need to add import posixpath at the top of the file.
| current_dir = os.path.join(current_dir, dir) | |
| current_dir = posixpath.join(current_dir, dir) |
|
|
||
| # 9. Download result file | ||
| log.info("Step 9: Downloading output file...") | ||
| local_path = "/tmp/pico_output.json" |
There was a problem hiding this comment.
The local path /tmp/pico_output.json is hardcoded. This is not portable and might fail on systems where /tmp is not writable or doesn't exist (like Windows). It's better to use the tempfile module to create temporary files in a platform-independent way.
| local_path = "/tmp/pico_output.json" | |
| local_path = os.path.join(tempfile.gettempdir(), "pico_output.json") |
| import threading | ||
| from collections import defaultdict | ||
| from typing import Dict | ||
|
|
||
|
|
||
| class ResourceTracker: | ||
| def __init__(self): | ||
| self._resources = defaultdict(dict) | ||
| self._lock = threading.Lock() | ||
|
|
||
| def track(self, sandbox_id: str, resource_type: str, resource_id: str): | ||
| with self._lock: | ||
| self._resources[sandbox_id][resource_type] = resource_id | ||
|
|
||
| def release(self, sandbox_id: str) -> Dict[str, str]: | ||
| with self._lock: | ||
| return self._resources.pop(sandbox_id, {}) | ||
|
|
||
| def get_resources(self, sandbox_id: str) -> Dict[str, str]: | ||
| with self._lock: | ||
| return self._resources.get(sandbox_id, {}).copy() |
There was a problem hiding this comment.
This ResourceTracker class and the entire file seem to be unused in the new SDK. If this is not intended for future work, it should be removed to avoid having dead code in the repository.
sdk_python/sandbox.py
Outdated
| self._active_sandboxes: Dict[str, SandboxInstance] = {} | ||
| self._lock = threading.RLock() | ||
|
|
||
| def create_sandbox(self, config: Optional[Dict[str, Any]] = None) -> SandboxInstance: |
sdk_python/sandbox.py
Outdated
| ttl = int(config.get("ttl", SandboxClient.DEFAULT_TTL)) | ||
|
|
||
| session_id = self._client.create_sandbox(ttl=ttl, image=image) | ||
| session_info = self._client.get_sandbox(session_id) |
There was a problem hiding this comment.
This seems redundant, we can make create_sandbox return info, no need to call get_sandbox again,
| ) | ||
|
|
||
| with self._lock: | ||
| self._active_sandboxes[sandbox.id] = sandbox |
There was a problem hiding this comment.
please open a issue: active sandbox is only recorded when create now, this does not perfectly relect the sandbox status.
sdk_python/constants.py
Outdated
| DEFAULT_SSH_KEY_PATH = "/root/.ssh/authorized_keys" | ||
|
|
||
| # Sandbox Constants | ||
| DEFAULT_SANDBOX_IMAGE = "hub.oepkgs.net/conch/openeuler-python" |
There was a problem hiding this comment.
? please use the image @YaoZengzeng built
sdk_python/models/pod_templates.py
Outdated
Signed-off-by: lizhixuan <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR removes the old session-based SDK documentation and test files while adding a new Python SDK implementation with a refactored client architecture. The changes transition from session-oriented terminology to sandbox-oriented terminology, introducing a new SandboxSDK wrapper class and updated SandboxClient implementation.
Key changes:
- Removed legacy SDK files including documentation, examples, integration tests, and requirements
- Added new Python SDK implementation in
sdk-python/directory with modular architecture - Introduced
SandboxSDKclass as a higher-level wrapper aroundSandboxClient
Reviewed Changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/test_sessions_integration.py | Removed integration test suite for old sessions SDK |
| sdk/sandbox_sessions_sdk.py | Removed comprehensive SDK implementation with REST and SSH/SFTP support |
| sdk/requirements.txt | Removed dependency specification file |
| sdk/examples/tunnel_ssh_example.py | Removed SSH tunnel usage example |
| sdk/examples/test_sessions_sdk.py | Removed SDK examples and test cases |
| sdk/SESSIONS_SDK.md | Removed complete SDK documentation |
| sdk/QUICK_REFERENCE.md | Removed quick reference guide |
| sdk/DESIGN.md | Removed design specification document |
| sdk-python/sandbox_sdk.py | Added new SDK wrapper class with sandbox management |
| sdk-python/sandbox.py | Added Sandbox class representing individual sandbox instances |
| sdk-python/pyproject.toml | Added project configuration with dependencies |
| sdk-python/log.py | Added logging utility module |
| sdk-python/exceptions.py | Added custom exception definitions |
| sdk-python/examples.py | Added usage examples for new SDK |
| sdk-python/constants.py | Added configuration constants |
| sdk-python/client_test.py | Added client test script |
| sdk-python/client.py | Added new SandboxClient implementation |
| downloaded_os-release.txt | Added sample downloaded file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print(f"\nFailed to retrieve sandbox with ID: {sandbox.id}") | ||
|
|
||
| print("8. Get all active sandboxes") | ||
| active_sandboxes = sdk.get_sandboxes() |
There was a problem hiding this comment.
Method get_sandboxes() is called but doesn't exist in SandboxSDK. The class only defines list_sandboxes() which returns a dictionary. This will cause an AttributeError at runtime.
| active_sandboxes = sdk.get_sandboxes() | |
| active_sandboxes = sdk.list_sandboxes() |
| print(url) | ||
| print(response) |
There was a problem hiding this comment.
Debug print statements should be removed from production code. Use the existing logger instance (self.logger.debug()) instead if debugging output is needed.
| print(url) | |
| print(response) | |
| self.logger.debug(f"Request URL: {url}") | |
| self.logger.debug(f"Response: {response}") |
| self.private_key: Optional[paramiko.RSAKey] = None | ||
| self.logger = get_logger(f"{__name__}.SandboxClient") | ||
|
|
||
| def __exit__(self): |
There was a problem hiding this comment.
The __exit__ method signature is incorrect for context manager protocol. It must accept three parameters: exc_type, exc_val, and exc_tb. The current signature will cause a TypeError when used in a with statement.
| def __exit__(self): | |
| def __exit__(self, exc_type, exc_val, exc_tb): |
| self._active_sandboxes[sandbox.id] = sandbox | ||
|
|
||
| self.logger.info(f"Sandbox {sandbox.id} created") |
There was a problem hiding this comment.
Property sandbox.id is used but the Sandbox class defines this attribute as self.id. While this works, it's inconsistent with the pattern used elsewhere in the codebase where sandbox_id is used (e.g., line 22: sandbox_id=session_id). Consider using sandbox_id consistently throughout.
| self._active_sandboxes[sandbox.id] = sandbox | |
| self.logger.info(f"Sandbox {sandbox.id} created") | |
| self._active_sandboxes[sandbox.sandbox_id] = sandbox | |
| self.logger.info(f"Sandbox {sandbox.sandbox_id} created") |
| return response.json() | ||
| except requests.exceptions.RequestException as e: | ||
| self.logger.error(f"Session query failed: {str(e)}") | ||
|
|
There was a problem hiding this comment.
The exception is caught and logged but not re-raised, and the method has no explicit return statement in this path. This will return None implicitly, which is inconsistent with the return type hint Optional[Dict[str, Any]] and may cause confusion as it's unclear whether None means an error occurred or the session wasn't found (line 133 also returns None for 404).
| return None |
| return True | ||
| except requests.exceptions.RequestException as e: | ||
| self.logger.error(f"Session deletion failed: {str(e)}") | ||
|
|
There was a problem hiding this comment.
The exception is caught and logged but the method doesn't return a value in the error case, causing an implicit None return. The method's return type is bool, so this should explicitly return False to indicate failure.
| return False |
| author_email = "[email protected]" | ||
|
|
||
| [dependencies] | ||
| kubernetes = "^27.2.0" |
5 participants
No description provided.