Skip to content

Commit dd9c759

Browse files
committed
Make ContainerEnvironment pluggable so non-Docker backends can run eval
The eval pipeline assumes Docker is the only container runtime: `Evaluator._new_env` constructs `ContainerEnvironment(...)` directly, and `_copy_file_from_container` shells `docker cp` by reaching into `env.executable` / `env.container_id`. That blocks anyone running ProgramBench inside an environment that doesn't support Docker-in-Docker (cloud sandboxes, gVisor, firecracker, most CI runners that share a single Docker daemon). They have to monkey-patch internals to swap the backend. This change introduces two Protocols (`Environment`, `ContainerBackend`) and threads an optional `backend` kwarg through `Evaluator.__init__`. The default is `DockerBackend()` — same behavior as today; the byte-identical test suite confirms it. Concrete changes: - `Environment` Protocol codifies the surface `Evaluator` already uses: `execute`, `copy_in`, `copy_in_tar`, `copy_out` (new), `commit`, `cleanup`. Plus `cwd` and `default_timeout` attributes that `_run_test_branch` reads directly. - `ContainerEnvironment.copy_out(container_path, *, timeout)` is new. It replaces the direct `docker cp` call that lived in `_copy_file_from_container`. Returns `(contents, command_string)` so the caller can log the wire command without knowing the backend's format. - `ContainerBackend` Protocol with `new_env(image, *, cwd, timeout, cpus, env, run_args)` and `remove_image(image_ref)`. - `DockerBackend` is the default implementation: wraps `ContainerEnvironment` and the module-level `remove_image` helper. Preserves the `executable` and base `run_args` plumbing. - `Evaluator.__init__` accepts `backend: ContainerBackend | None = None`, defaulting to `DockerBackend(executable=DOCKER_EXECUTABLE, run_args=DOCKER_RUN_ARGS)`. - `Evaluator._new_env` delegates to `self.backend.new_env(...)`. The baseline xdist hardening (PYTEST_ADDOPTS, serial_pytest, --init in run_args) is unchanged. - `Evaluator._copy_file_from_container` collapses from ~45 lines of subprocess plumbing to a single `env.copy_out(...)` call plus the same step-log entry shape it always wrote. Removed `tempfile` and `subprocess` imports from eval.py (now only needed in container.py). - `Evaluator.run`'s final `remove_image(committed_image, ...)` call becomes `self.backend.remove_image(committed_image)`. Tests: - `tests/test_pluggable_backend.py` adds 3 examples: * Building an Evaluator with a FakeBackend routes `_new_env` and `_copy_file_from_container` through the fake (no Docker shellouts). * No `backend=` kwarg → DockerBackend. * DockerBackend satisfies the ContainerBackend Protocol structurally. - All 28 existing tests still pass; 31/31 with the new ones. Motivating use case: saul-agent-infra runs the eval inside Daytona sandboxes which can't run Docker. Today we monkey-patch `programbench.container.ContainerEnvironment`, `Evaluator._copy_file_from_container`, and `Evaluator._restore_executable`. After this PR the first two go away entirely. (The third, swapping `mv` for `cp` to handle serial test branches without re-imaging, is a small separate fix worth its own discussion.)
1 parent b33e660 commit dd9c759

3 files changed

Lines changed: 253 additions & 50 deletions

File tree

‎src/programbench/container.py‎

Lines changed: 100 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,53 @@
66

77
import logging
88
import subprocess
9+
import tempfile
10+
import time
911
import uuid
1012
from pathlib import Path
11-
from typing import Any
13+
from typing import Any, Protocol
1214

1315
from programbench.constants import DOCKER_CP_TIMEOUT, DOCKER_RUN_TIMEOUT
1416

1517
log = logging.getLogger(__name__)
1618

1719

20+
class Environment(Protocol):
21+
"""The minimal surface ``Evaluator`` needs from a per-task container.
22+
23+
Implementations don't have to inherit from this — duck-typed protocols
24+
are fine — but you do have to provide every method below. ``cwd`` and
25+
``default_timeout`` are read directly by ``Evaluator`` so they need to
26+
be regular attributes.
27+
"""
28+
29+
cwd: str
30+
default_timeout: int
31+
32+
def execute(self, command: str, *, timeout: int | None = None) -> dict[str, Any]: ...
33+
def copy_in(self, local_path: Path, container_path: str) -> None: ...
34+
def copy_in_tar(self, tar_path: Path, container_path: str) -> None: ...
35+
def copy_out(self, container_path: str, *, timeout: int = 60) -> tuple[str, str]: ...
36+
def commit(self, image_ref: str) -> str: ...
37+
def cleanup(self) -> None: ...
38+
39+
40+
class ContainerBackend(Protocol):
41+
"""Factory that produces ``Environment`` instances for the Evaluator.
42+
43+
The Evaluator asks the backend for one ``Environment`` per phase
44+
(compile, then one per test branch). The default implementation
45+
(:class:`DockerBackend`) returns :class:`ContainerEnvironment` —
46+
i.e., real Docker containers. Custom backends can swap in
47+
non-Docker isolation (firecracker VMs, gVisor sandboxes, no
48+
isolation at all if you trust the input, etc.).
49+
"""
50+
51+
def new_env(self, image: str, *, cwd: str, timeout: int, cpus: int,
52+
env: dict[str, str] | None, run_args: list[str] | None) -> Environment: ...
53+
def remove_image(self, image_ref: str) -> None: ...
54+
55+
1856
class ContainerEnvironment:
1957
"""Manage a long-running container for command execution and file injection."""
2058

@@ -116,6 +154,32 @@ def copy_in(self, local_path: Path, container_path: str) -> None:
116154
if result.returncode != 0:
117155
raise RuntimeError(f"docker cp failed: {result.stderr.strip()}")
118156

157+
def copy_out(self, container_path: str, *, timeout: int = 60) -> tuple[str, str]:
158+
"""Read a file out of the container via ``docker cp``.
159+
160+
Returns ``(contents, command_string)``. The command string is
161+
included so callers (notably :meth:`Evaluator._copy_file_from_container`)
162+
can record it in their step logs without having to know the
163+
backend's wire format. Raises ``RuntimeError`` with the
164+
underlying stderr (or ``"... timed out after Ns"``) on failure.
165+
166+
Bypasses bash so login-shell stderr (``mesg: ttyname failed`` etc.)
167+
can't pollute the bytes the way ``cat <file>`` would.
168+
"""
169+
host_tmp = Path(tempfile.mkstemp(suffix=Path(container_path).suffix or ".out")[1])
170+
cmd_list = [self.executable, "cp", f"{self.container_id}:{container_path}", str(host_tmp)]
171+
cmd_str = " ".join(cmd_list)
172+
try:
173+
try:
174+
cp = subprocess.run(cmd_list, capture_output=True, text=True, timeout=timeout)
175+
except subprocess.TimeoutExpired:
176+
raise RuntimeError(f"docker cp timed out after {timeout}s")
177+
if cp.returncode != 0:
178+
raise RuntimeError((cp.stdout + cp.stderr).strip())
179+
return host_tmp.read_text(), cmd_str
180+
finally:
181+
host_tmp.unlink(missing_ok=True)
182+
119183
def copy_in_tar(self, tar_path: Path, container_path: str) -> None:
120184
"""Stream an on-disk tar(.gz) into the container via the container's tar.
121185
@@ -206,3 +270,38 @@ def remove_image(image_ref: str, *, executable: str = "docker") -> None:
206270
)
207271
except Exception:
208272
pass
273+
274+
275+
class DockerBackend:
276+
"""The default ``ContainerBackend``: spawns real Docker containers.
277+
278+
Drop-in for any callsite that previously instantiated
279+
:class:`ContainerEnvironment` directly — same args, same behavior.
280+
"""
281+
282+
def __init__(self, *, executable: str = "docker", run_args: list[str] | None = None):
283+
self.executable = executable
284+
self.default_run_args = list(run_args or [])
285+
286+
def new_env(
287+
self,
288+
image: str,
289+
*,
290+
cwd: str = "/",
291+
timeout: int = 30,
292+
cpus: int = 10,
293+
env: dict[str, str] | None = None,
294+
run_args: list[str] | None = None,
295+
) -> ContainerEnvironment:
296+
return ContainerEnvironment(
297+
image=image,
298+
cwd=cwd,
299+
executable=self.executable,
300+
timeout=timeout,
301+
cpus=cpus,
302+
env=env,
303+
run_args=list(self.default_run_args) + list(run_args or []),
304+
)
305+
306+
def remove_image(self, image_ref: str) -> None:
307+
remove_image(image_ref, executable=self.executable)

‎src/programbench/eval/eval.py‎

Lines changed: 34 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@
2020
"""
2121

2222
import logging
23-
import subprocess
24-
import tempfile
2523
import threading
2624
import time
2725
import uuid
@@ -40,7 +38,7 @@
4038
DOCKER_RUN_ARGS,
4139
WORKSPACE_DIR,
4240
)
43-
from programbench.container import ContainerEnvironment, remove_image
41+
from programbench.container import ContainerBackend, ContainerEnvironment, DockerBackend
4442
from programbench.exceptions import EmptyTestResultError, EvalStepError, XmlParseError
4543

4644
log = logging.getLogger(__name__)
@@ -295,6 +293,7 @@ def __init__(
295293
docker_cpus: int = DOCKER_CPUS,
296294
branch_workers: int = 1,
297295
branch_retries: int = 1,
296+
backend: ContainerBackend | None = None,
298297
):
299298
self.image_name = image_name
300299
self.solution_branch = solution_branch
@@ -310,6 +309,7 @@ def __init__(
310309
self.docker_cpus = docker_cpus
311310
self.branch_workers = max(1, branch_workers)
312311
self.branch_retries = max(0, branch_retries)
312+
self.backend = backend or DockerBackend(executable=DOCKER_EXECUTABLE, run_args=list(DOCKER_RUN_ARGS))
313313
self._has_rerunfailures = False
314314
self._log_lock = threading.Lock()
315315
self._from_existing = from_existing
@@ -394,52 +394,38 @@ def _copy_file_from_container(
394394
step_name: str,
395395
timeout: int = 60,
396396
) -> str:
397-
"""Copy a file out of the container via ``docker cp`` and return its contents.
397+
"""Copy a file out of the container and return its contents.
398398
399-
Bypasses bash so login-shell stderr (``mesg: ttyname failed`` etc.) can't
400-
pollute the bytes the way ``cat <file>`` would. Logs to ``log_buf`` with
401-
the same shape as ``_run_step``; on success the entry's ``output`` holds
402-
the file contents so ``from_existing`` replay keeps working.
399+
Delegates to ``env.copy_out``; the backend decides how to fetch the
400+
bytes (Docker's ``docker cp``, a non-Docker backend's filesystem
401+
read, etc.). Logs to ``log_buf`` with the same shape as
402+
``_run_step``; on success the entry's ``output`` holds the file
403+
contents so ``from_existing`` replay keeps working.
403404
"""
404-
host_tmp = Path(tempfile.mkstemp(suffix=Path(container_path).suffix or ".out")[1])
405-
cmd_list = [env.executable, "cp", f"{env.container_id}:{container_path}", str(host_tmp)]
406-
cmd_str = " ".join(cmd_list)
407-
log.debug("Running step: %s", cmd_str)
408405
t0 = time.monotonic()
409406
try:
410-
try:
411-
cp = subprocess.run(cmd_list, capture_output=True, text=True, timeout=timeout)
412-
rc = cp.returncode
413-
err = (cp.stdout + cp.stderr).strip()
414-
except subprocess.TimeoutExpired:
415-
rc, err = -1, f"docker cp timed out after {timeout}s"
407+
contents, cmd_str = env.copy_out(container_path, timeout=timeout)
408+
except RuntimeError as exc:
416409
wall_time = time.monotonic() - t0
417-
if rc != 0:
418-
log_buf.append(
419-
{
420-
"step": step_name,
421-
"command": cmd_str,
422-
"wall_time": wall_time,
423-
"output": err,
424-
"returncode": rc,
425-
"exception_info": "",
426-
}
427-
)
428-
raise EvalStepError(f"{step_name}_failed", err)
429-
contents = host_tmp.read_text()
430-
log_buf.append(
431-
{
432-
"step": step_name,
433-
"command": cmd_str,
434-
"wall_time": wall_time,
435-
"output": contents,
436-
"returncode": 0,
437-
"exception_info": "",
438-
}
439-
)
440-
return contents
441-
finally:
442-
host_tmp.unlink(missing_ok=True)
410+
log_buf.append({
411+
"step": step_name,
412+
"command": f"copy_out({container_path})",
413+
"wall_time": wall_time,
414+
"output": str(exc),
415+
"returncode": -1,
416+
"exception_info": "",
417+
})
418+
raise EvalStepError(f"{step_name}_failed", str(exc))
419+
wall_time = time.monotonic() - t0
420+
log_buf.append({
421+
"step": step_name,
422+
"command": cmd_str,
423+
"wall_time": wall_time,
424+
"output": contents,
425+
"returncode": 0,
426+
"exception_info": "",
427+
})
428+
return contents
443429

444430
def _new_env(self, image: str, *, serial_pytest: bool = False) -> ContainerEnvironment:
445431
# Baseline xdist hardening that always works (xdist ships with
@@ -456,14 +442,13 @@ def _new_env(self, image: str, *, serial_pytest: bool = False) -> ContainerEnvir
456442
env = {"PYTEST_ADDOPTS": addopts}
457443
if serial_pytest:
458444
env["PYTEST_XDIST_AUTO_NUM_WORKERS"] = "1"
459-
return ContainerEnvironment(
460-
image=image,
445+
return self.backend.new_env(
446+
image,
461447
cwd=WORKSPACE_DIR,
462-
executable=DOCKER_EXECUTABLE,
463448
timeout=600,
464449
cpus=self.docker_cpus,
465450
env=env,
466-
run_args=[*DOCKER_RUN_ARGS, "--init"],
451+
run_args=["--init"],
467452
)
468453

469454
def _remove_hashed_files(self, env: ContainerEnvironment, log_buf: list[dict]) -> None:
@@ -839,7 +824,7 @@ def run(self) -> EvaluationResult:
839824
if compile_env is not None:
840825
compile_env.cleanup()
841826
if committed_image is not None:
842-
remove_image(committed_image, executable=DOCKER_EXECUTABLE)
827+
self.backend.remove_image(committed_image)
843828

844829

845830
def parse_test_results(results_xml: str, branch: str = "") -> EvaluationResult:

‎tests/test_pluggable_backend.py‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
# Copyright (c) Meta Platforms, Inc. and affiliates.
2+
# All rights reserved.
3+
#
4+
# This source code is licensed under the license found in the
5+
# LICENSE file in the root directory of this source tree.
6+
7+
"""Tests for the pluggable ContainerBackend / Environment abstraction.
8+
9+
The point of this PR is that downstream callers shouldn't have to monkey-
10+
patch internals to run the eval against a non-Docker isolation primitive
11+
(e.g. Daytona sandboxes, gVisor, firecracker, or just a bare host shell).
12+
These tests exercise the substitution: build a fake in-memory backend,
13+
pass it to the Evaluator, and verify the eval calls the fake's methods
14+
instead of shelling Docker.
15+
"""
16+
17+
from pathlib import Path
18+
19+
from programbench.container import ContainerBackend, DockerBackend
20+
from programbench.eval.eval import Evaluator
21+
22+
23+
class FakeEnv:
24+
"""Records every method call so the test can assert the surface."""
25+
26+
def __init__(self, *, image, cwd, timeout, cpus, env, run_args):
27+
self.image = image
28+
self.cwd = cwd
29+
self.default_timeout = timeout
30+
self.cpus = cpus
31+
self.env = env
32+
self.run_args = run_args
33+
self.calls: list[tuple] = []
34+
self.canned_copy_out: dict[str, str] = {}
35+
36+
def execute(self, command, *, timeout=None):
37+
self.calls.append(("execute", command))
38+
return {"output": "", "returncode": 0, "exception_info": ""}
39+
40+
def copy_in(self, local_path, container_path):
41+
self.calls.append(("copy_in", str(local_path), container_path))
42+
43+
def copy_in_tar(self, tar_path, container_path):
44+
self.calls.append(("copy_in_tar", str(tar_path), container_path))
45+
46+
def copy_out(self, container_path, *, timeout=60):
47+
self.calls.append(("copy_out", container_path))
48+
contents = self.canned_copy_out.get(container_path, "")
49+
return contents, f"fake-cp {container_path}"
50+
51+
def commit(self, image_ref):
52+
self.calls.append(("commit", image_ref))
53+
return image_ref
54+
55+
def cleanup(self):
56+
self.calls.append(("cleanup",))
57+
58+
59+
class FakeBackend:
60+
"""Hands out FakeEnv instances and records image removals."""
61+
62+
def __init__(self):
63+
self.envs: list[FakeEnv] = []
64+
self.removed: list[str] = []
65+
66+
def new_env(self, image, *, cwd, timeout, cpus, env, run_args):
67+
env_obj = FakeEnv(image=image, cwd=cwd, timeout=timeout,
68+
cpus=cpus, env=env, run_args=run_args)
69+
self.envs.append(env_obj)
70+
return env_obj
71+
72+
def remove_image(self, image_ref):
73+
self.removed.append(image_ref)
74+
75+
76+
def test_evaluator_uses_injected_backend():
77+
"""Constructing an Evaluator with a custom backend routes all container
78+
operations through it — _new_env calls backend.new_env, and
79+
_copy_file_from_container calls env.copy_out."""
80+
backend = FakeBackend()
81+
e = Evaluator(
82+
tests_branches=["abc123"],
83+
image_name="example/example.deadbeef",
84+
backend=backend,
85+
)
86+
87+
env = e._new_env("example/example.deadbeef:task")
88+
assert isinstance(env, FakeEnv)
89+
assert env.image == "example/example.deadbeef:task"
90+
assert env.cwd # WORKSPACE_DIR
91+
assert len(backend.envs) == 1
92+
93+
log_buf: list[dict] = []
94+
env.canned_copy_out["/workspace/eval/results.xml"] = "<xml/>"
95+
contents = e._copy_file_from_container(
96+
env=env, log_buf=log_buf,
97+
container_path="/workspace/eval/results.xml",
98+
step_name="results_read",
99+
)
100+
assert contents == "<xml/>"
101+
assert ("copy_out", "/workspace/eval/results.xml") in env.calls
102+
# Step was logged with the env's reported command-string, not "docker cp".
103+
assert log_buf[0]["step"] == "results_read"
104+
assert log_buf[0]["command"].startswith("fake-cp ")
105+
106+
107+
def test_evaluator_default_backend_is_docker():
108+
"""No `backend=` kwarg → DockerBackend (preserves pre-PR behavior)."""
109+
e = Evaluator(tests_branches=[], image_name="foo")
110+
assert isinstance(e.backend, DockerBackend)
111+
112+
113+
def test_docker_backend_satisfies_protocol():
114+
"""DockerBackend implements the ContainerBackend protocol surface."""
115+
backend: ContainerBackend = DockerBackend()
116+
# Protocol membership is structural — this assignment compiles only
117+
# because DockerBackend has the required methods. Smoke-touch them.
118+
assert callable(backend.new_env)
119+
assert callable(backend.remove_image)

0 commit comments

Comments
 (0)