[agent] Modal sandbox backend for coding_agent_rl (opt-in, E2B untouched) - #167
[agent] Modal sandbox backend for coding_agent_rl (opt-in, E2B untouched)#167aoshen02 wants to merge 2 commits into
Conversation
Adds `ModalSandbox` alongside `E2BSandbox` as a second concrete backend for the agent-rollout `Sandbox` Protocol. Purely additive: the E2B path is byte-unchanged and the Modal backend is opt-in via `VIME_AGENT_SANDBOX_BACKEND=modal`. Changes: - vime/agent/sandbox.py: new `ModalSandbox` (lazy `import modal`) mirroring E2BSandbox's surface. E2B->Modal gaps resolved: `user=` via `runuser -u` (env keys whitelisted through); `write_file(str|bytes|host Path)` streams over command stdin (2 MiB chunks) then chowns; image via `Image.from_registry` (+ REGISTRY_USERNAME/PASSWORD from DOCKER_* for private registries); cleanup always reaches `terminate`. - examples/coding_agent_rl/sandbox.py: `make_sandbox(image)` factory; both E2BSandbox call sites (work + eval sandbox) now backend-agnostic. - examples/coding_agent_rl/generate.py: `ADAPTER_URL_OVERRIDE` lets a reverse tunnel (e.g. cloudflared) supply a public adapter URL when the head has no directly routable host:port; relaxes the VIME_HEAD_HOST guard. - tests/test_agent_modal_sandbox.py: 34 unit tests with a faked `modal` (no network/dep) covering protocol conformance, config, image/app/create wiring, runuser/env, check/timeout/output-cap, write_file str/bytes/Path streaming + chown, read_file, terminate, and the factory. Reverse-network (coding_agent_rl runs Claude Code inside the sandbox dialing back to the head adapter) verified on real Modal: sandbox -> cloudflared tunnel -> head adapter works; direct-to-head-public-IP does not (NAT egress only), hence ADAPTER_URL_OVERRIDE + a tunnel for private clusters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new ModalSandbox backend as an alternative to E2BSandbox, selected via the VIME_AGENT_SANDBOX_BACKEND environment variable. It adds a make_sandbox factory to handle this selection, updates the generation and evaluation scripts to support it, and includes comprehensive unit tests for the new backend. Additionally, it supports ADAPTER_URL_OVERRIDE for routing in private clusters. Feedback on the changes highlights three key issues in the write_file implementation of ModalSandbox: potential event loop blocking from synchronous file I/O, deadlock risks due to unread stdout/stderr pipes, and a lack of stderr diagnostics in raised OSError exceptions. A code suggestion was provided to resolve these issues.
| if isinstance(content, Path): | ||
| with open(content, "rb") as fp: | ||
| while True: | ||
| chunk = fp.read(_MODAL_WRITE_CHUNK) | ||
| if not chunk: | ||
| break | ||
| proc.stdin.write(chunk) | ||
| await proc.stdin.drain.aio() | ||
| else: | ||
| data = content.encode() if isinstance(content, str) else content | ||
| proc.stdin.write(data) | ||
| await proc.stdin.drain.aio() | ||
| proc.stdin.write_eof() | ||
| await proc.stdin.drain.aio() | ||
| rc = await proc.wait.aio() | ||
| except Exception as e: | ||
| raise OSError(f"modal write_file({sandbox_path}) failed: {e!r}") from e | ||
| if rc != 0: | ||
| raise OSError(f"modal write_file({sandbox_path}) exit={rc}") |
There was a problem hiding this comment.
There are three main areas of improvement in the write_file implementation:
- Event Loop Blocking: Performing synchronous file operations (
openandread) inside anasync deffunction blocks the single-threaded asyncio event loop. For larger files (such as Node or Claude Code tarballs, or dataset assets), this can freeze other concurrent tasks and degrade rollout performance. Usingasyncio.to_threadoffloads these blocking calls to a thread pool. - Pipe Clogging / Deadlock Risk: Currently,
proc.stdoutandproc.stderrare never read. If the shell orcatcommand produces any unexpected output (e.g., login shell banners, warnings, or error messages) that exceeds the OS pipe buffer limit (typically 64KB), the process will block onwrite()and hang indefinitely. Reading them concurrently withproc.wait.aio()usingasyncio.gatherprevents this. - Error Diagnostics: If
catormkdirfails, the process exits with a non-zero code. Currently, the raisedOSErroronly contains the exit code, leaving developers with no visibility into the actual error message printed tostderr. Capturing and including the decodedstderrin the raisedOSErrormakes debugging write failures much easier.
if isinstance(content, Path):
fp = await asyncio.to_thread(open, content, "rb")
try:
while True:
chunk = await asyncio.to_thread(fp.read, _MODAL_WRITE_CHUNK)
if not chunk:
break
proc.stdin.write(chunk)
await proc.stdin.drain.aio()
finally:
await asyncio.to_thread(fp.close)
else:
data = content.encode() if isinstance(content, str) else content
proc.stdin.write(data)
await proc.stdin.drain.aio()
proc.stdin.write_eof()
await proc.stdin.drain.aio()
out, err, rc = await asyncio.gather(
proc.stdout.read.aio(),
proc.stderr.read.aio(),
proc.wait.aio(),
)
except Exception as e:
raise OSError(f"modal write_file({sandbox_path}) failed: {e!r}") from e
if rc != 0:
err_msg = err.decode("utf-8", errors="replace") if isinstance(err, bytes) else str(err)
raise OSError(f"modal write_file({sandbox_path}) exit={rc}, stderr={err_msg[:400]}")- exec: gather stdout/stderr reads with wait() (avoid backpressure stall); handle Modal's per-exec timeout signal explicitly. Verified on real Modal: a per-exec `timeout=` does NOT raise SandboxTimeoutError — wait() returns rc == -1. Now surfaced as a clear timeout (raise on check, sentinel stderr otherwise) instead of a generic non-zero exit. Kept the except branch as a defensive fallback for any path that does raise. - write_file: drain stderr alongside wait() so a chatty error can't stall the write, and include the captured stderr in the OSError message. - tests: +2 covering the real rc == -1 timeout path (36 total). E2E re-run on real Modal after these changes: 13/13. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…+ make_sandbox factory)
What
Adds a Modal backend (
ModalSandbox) alongside the existingE2BSandboxfor the agent-rolloutSandboxProtocol. Purely additive — the E2B path is byte-unchanged; Modal is opt-in viaVIME_AGENT_SANDBOX_BACKEND=modal.Stacked on #148 (
sync/slime-mega-D, which introducescoding_agent_rl). This is a vime-specific enhancement (slime ships E2B-only), not a slime port.Why
coding_agent_rltoday can only provision sandboxes through the cloud-onlyE2BSandbox. Modal gives an alternative provider many of us already have credentials for, with the same per-sample two-sandbox (work + eval) lifecycle.Changes (4 files)
vime/agent/sandbox.py— newModalSandbox(lazyimport modal) mirroringE2BSandbox's surface (__aenter__/__aexit__/exec/write_file/read_file/sandbox_id). E2B→Modal gaps resolved:user=→ emulated withrunuser -u <user>(env keys whitelisted through, matching E2Benvs=semantics)write_file(str | bytes | host Path)→mkdir -p && cat >with binary stdin streaming (2 MiB chunks), thenchowntouserImage.from_registry(tag)(+REGISTRY_USERNAME/REGISTRY_PASSWORDfromDOCKER_*for private registries)terminate(a leaked sandbox counts against the account's concurrent cap until its wall-clock timeout)examples/coding_agent_rl/sandbox.py—make_sandbox(image)factory; the work-sandbox and eval-sandbox call sites are now backend-agnostic.examples/coding_agent_rl/generate.py—ADAPTER_URL_OVERRIDE: lets a reverse tunnel supply a ready-made public adapter URL when the head has no directly routablehost:port; relaxes theVIME_HEAD_HOSTguard (raises only if neither is set).tests/test_agent_modal_sandbox.py— 34 unit tests, fakedmodal(no network, nomodal/e2bdependency).Reverse-network design
coding_agent_rlruns Claude Code inside the sandbox, dialing back to the head's in-process Anthropic adapter (ANTHROPIC_BASE_URL). Measured on real Modal sandboxes:block_network=False)So on a private cluster: expose the adapter's
SHIM_PORTviacloudflared tunneland pointADAPTER_URL_OVERRIDE(orVIME_HEAD_HOST) at the public URL.Test plan
pytest tests/test_agent_modal_sandbox.py→ 34 passed. Covers protocol conformance, env/kwarg config, image/app/create wiring, registry secret, exec root/runuser/env-whitelist, check-raise, output cap, timeout contract, write_file str/bytes/host-Path streaming + chown, read_file swallow, always-terminate, factory selection.buildpack-deps:bookworm, run locally — not in CI): 13/13 — exec root/egress, write_file str+read roundtrip, bytes sha256, 5 MiB host-Path sha256 (multi-chunk), runuser user, chown ownership, read-missing→"", check-raise, reverse dial-back proven on both ends (sandbox stdout + head log, xff = sandbox egress IP), terminate.pre-commit run(ruff/autoflake/isort/format) clean on all changed files.Not in scope / follow-ups
_spawn_claude_codecould later simplify to a long foregroundexec; kept as-is for parity in this PR.🤖 Generated with Claude Code