Conversation
|
I am OK with this, @rogercloud could give a review. |
|
if any implementation is introduced, this PR title might need to be modified. |
yyquiet
changed the title
Add abstract base classes for sandbox lifecycle management: - Sandbox: Interface for sandbox instance operations - SandboxService: Interface for sandbox lifecycle management - Support for template-based creation (image/snapshot) - File operations (upload/download/read/write) - Code execution (Python/JavaScript) - Snapshot management
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Sandbox feature code review. Found 9 issues (see inline comments).
Summary:
- Style: 3 (naming, shadowing, redundant imports)
- Minor: 3 (redundant imports)
- Design: 2 (private access, snapshot interface)
- UX: 1 (silent exception)
- Validation: 1 (input validation)
Overall: Code is fundamentally sound and ready to merge. All issues are minor.
|
|
||
|
|
||
| @dataclass | ||
| class SandBoxTemplate: |
There was a problem hiding this comment.
Issue #1: Naming Inconsistency
SandBoxTemplate uses SandBox (two words, capital B) but the module is sandbox (one word, lowercase). Suggestion: Rename to SandboxTemplate.
| # First copy to temp directory (if copying directly to mount directory, it won't be readable on host) | ||
| # Note: Cannot use /tmp as it's a tmpfs mount, copy_in will fail | ||
| # Use /var/tmp or other non-tmpfs directory | ||
| import uuid |
There was a problem hiding this comment.
Issue #2: Redundant import uuid at line 231 - already imported at module level (line 12).
| raise FileExistsError(f"Local file already exists: {local_path}") | ||
|
|
||
| # First copy to temp directory (avoid volume mount issues) | ||
| import uuid |
There was a problem hiding this comment.
Issue #3: Redundant import uuid at line 273 - already imported at module level (line 12).
| """Get sandbox status information.""" | ||
| try: | ||
| # Update state in real-time | ||
| box_info = await self._box._runtime.get_info(self._name) |
There was a problem hiding this comment.
Issue #4: Private member access _box._runtime violates encapsulation. Add comment explaining why or request public API.
| # Update state in real-time | ||
| box_info = await self._box._runtime.get_info(self._name) | ||
| self._info.state = _get_state(box_info) | ||
| except Exception as e: |
There was a problem hiding this comment.
Issue #5: Silent exception handling - returns cached info without indicating it might be stale.
| Configuration parameters for creating a sandbox. | ||
| """ | ||
|
|
||
| cpus: Optional[int] = 1 |
There was a problem hiding this comment.
Issue #6: Missing input validation for cpus and memory. Add post_init validation.
| @pytest.mark.asyncio(loop_scope="module") | ||
| async def test_list_sandboxes(self): | ||
| """Test listing sandboxes""" | ||
| list = ["test-list-1", "test-list-2"] |
There was a problem hiding this comment.
Issue #7: Variable list shadows built-in. Rename to sandbox_names.
| # Create sandbox | ||
| template = SandBoxTemplate(_type="image", image="python:slim") | ||
|
|
||
| import tempfile |
There was a problem hiding this comment.
Issue #8: Redundant import tempfile - already imported at line 9.
| """ | ||
|
|
||
|
|
||
| class SandboxService(abc.ABC): |
There was a problem hiding this comment.
Issue #9: No interface to check snapshot support. Add supports_snapshots property to SandboxService interface.
4 participants
Add abstract base classes for sandbox lifecycle management:
Add implementation based on BoxLite.