diff --git a/CHANGELOG.md b/CHANGELOG.md index ec9a25f2..26fdcc1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/) and this project adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Changed +- Container-engine detection is now lazy: `run_support.config.engine()` probes PATH on first use instead of at import time, so importing the runner modules no longer requires Docker or Podman. `config.ENGINE` still resolves but is deprecated. +- `runner/batch.py` now uses the shared `config.engine()` instead of its own copy of the PATH probe. ## [0.10.0] - 2026-08-30 ### Added diff --git a/src/clawbench/runner/batch.py b/src/clawbench/runner/batch.py index 8d3637e5..fce544c5 100644 --- a/src/clawbench/runner/batch.py +++ b/src/clawbench/runner/batch.py @@ -7,7 +7,6 @@ import json import os import re -import shutil import signal import sys import time @@ -17,24 +16,14 @@ import yaml +from clawbench.runner.run_support.config import engine from clawbench.utils.paths import ASSET_ROOT, WORKSPACE_ROOT, ensure_workspace_templates -def detect_engine() -> str: - env = os.environ.get("CONTAINER_ENGINE", "").strip().lower() - if env: - if env not in ("docker", "podman"): - print(f"ERROR: CONTAINER_ENGINE must be 'docker' or 'podman', got '{env}'") - sys.exit(1) - if not shutil.which(env): - print(f"ERROR: CONTAINER_ENGINE={env} but '{env}' not found on PATH") - sys.exit(1) - return env - for cmd in ("docker", "podman"): - if shutil.which(cmd): - return cmd - print("ERROR: Neither 'docker' nor 'podman' found on PATH") - sys.exit(1) +# Re-exported under its historical name. The body used to be a third copy of +# the same PATH probe; it is config.engine() now that importing config no +# longer costs a container-engine probe. +detect_engine = engine # --------------------------------------------------------------------------- @@ -636,10 +625,10 @@ async def async_main(args: argparse.Namespace) -> int: # Build image once — reuse run.py's spinner/progress helper so first-time # builds show a clear banner and live step counter instead of a wall of # apt/npm output. - engine = detect_engine() + resolved_engine = detect_engine() # Ensure child run.py processes (and the imported helper below) use the # same engine as we just detected. - os.environ["CONTAINER_ENGINE"] = engine + os.environ["CONTAINER_ENGINE"] = resolved_engine from clawbench.runner import run as _run_mod _run_mod.docker_build(args.harness) diff --git a/src/clawbench/runner/run.py b/src/clawbench/runner/run.py index 4c9bf3d3..eb65c47c 100644 --- a/src/clawbench/runner/run.py +++ b/src/clawbench/runner/run.py @@ -17,11 +17,11 @@ from clawbench.runner.run_support.config import ( BASE_IMAGE, DEFAULT_HARNESS, - ENGINE, HARNESSES, IMAGE, WORKSPACE_ROOT, ModelConfigError, + engine, harness_image, load_model_config, load_runtime_env, @@ -70,7 +70,6 @@ __all__ = [ "BASE_IMAGE", "DEFAULT_HARNESS", - "ENGINE", "HARNESSES", "IMAGE", "docker_build", @@ -510,7 +509,7 @@ def handle_sigterm(sig, frame): def handle_sigint(sig, frame): print("\nCtrl+C received, stopping container gracefully...") subprocess.run( - [ENGINE, "stop", "-t", "20", container], capture_output=True + [engine(), "stop", "-t", "20", container], capture_output=True ) signal.signal(signal.SIGINT, handle_sigint) diff --git a/src/clawbench/runner/run_support/config.py b/src/clawbench/runner/run_support/config.py index 37dc6a99..cf134ed5 100644 --- a/src/clawbench/runner/run_support/config.py +++ b/src/clawbench/runner/run_support/config.py @@ -1,8 +1,10 @@ """Configuration and path helpers for single ClawBench runs.""" +import functools import os import shutil import sys +import warnings from pathlib import Path import yaml @@ -24,7 +26,6 @@ "ASSET_ROOT", "BASE_IMAGE", "DEFAULT_HARNESS", - "ENGINE", "HARNESS_REGISTRY", "HARNESS_REGISTRY_YAML", "HARNESSES", @@ -33,6 +34,7 @@ "WORKSPACE_ROOT", "HarnessRegistry", "ModelConfigError", + "engine", "harness_image", "load_dotenv", "load_harness_registry", @@ -74,14 +76,21 @@ def harness_image(harness: str) -> str: IMAGE = harness_image(DEFAULT_HARNESS) -def _detect_engine() -> str: - # Help output is host-only and should work on machines that have not - # installed Docker/Podman yet. Actual run paths still call this without - # help flags and fail fast if no engine is available. - if any(arg in {"-h", "--help"} for arg in sys.argv[1:]): - env = os.environ.get("CONTAINER_ENGINE", "").strip().lower() - return env if env in ("docker", "podman") else "docker" +@functools.lru_cache(maxsize=1) +def engine() -> str: + """Return the container engine to shell out to, probing PATH on first call. + Resolution is deliberately lazy: importing this module must not require a + container runtime. Plenty of code that imports it never starts a container + (rescore, the Harbor adapter, batch's job planning, `--help` output), and + an import-time probe that ends in sys.exit gives those callers no way to + degrade. The probe happens here, on the first call that actually needs an + engine, where the failure is attributable to the operation that needs it. + + Cached because every container command re-asks, and shutil.which walks + PATH each time. Call engine.cache_clear() if CONTAINER_ENGINE changes + within a process (batch does, after it resolves the engine for children). + """ env = os.environ.get("CONTAINER_ENGINE", "").strip().lower() if env: if env not in ("docker", "podman"): @@ -98,7 +107,25 @@ def _detect_engine() -> str: sys.exit(1) -ENGINE = _detect_engine() +def __getattr__(name: str) -> str: + """Deprecated back-compat shim for the old module-level ENGINE constant. + + Kept for one release for out-of-tree callers; in-tree code calls engine(). + Because this only fires on attribute access, `from ... import config` stays + free of the probe, which is the point of the change. + """ + if name == "ENGINE": + warnings.warn( + "config.ENGINE is deprecated; call config.engine() instead. " + "The constant probed for a container engine at import time, which " + "made importing this module fail on hosts without one.", + DeprecationWarning, + stacklevel=2, + ) + return engine() + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + MODELS_YAML = WORKSPACE_ROOT / "models" / "models.yaml" diff --git a/src/clawbench/runner/run_support/docker.py b/src/clawbench/runner/run_support/docker.py index 904e3c2b..09efbe60 100644 --- a/src/clawbench/runner/run_support/docker.py +++ b/src/clawbench/runner/run_support/docker.py @@ -18,10 +18,10 @@ from clawbench.runner.run_support.config import ( BASE_IMAGE, DEFAULT_HARNESS, - ENGINE, HARNESSES, HARNESS_REGISTRY, IMAGE, + engine, harness_image, ) from clawbench.runner.run_support.usage import ( @@ -66,7 +66,7 @@ def run(cmd: list[str], **kwargs): # type: ignore[no-untyped-def] def image_exists(ref: str = IMAGE) -> bool: return ( subprocess.run( - [ENGINE, "image", "inspect", ref], + [engine(), "image", "inspect", ref], capture_output=True, ).returncode == 0 @@ -76,7 +76,7 @@ def image_exists(ref: str = IMAGE) -> bool: def image_id(ref: str) -> str | None: try: r = subprocess.run( - [ENGINE, "image", "inspect", ref, "--format", "{{.Id}}"], + [engine(), "image", "inspect", ref, "--format", "{{.Id}}"], capture_output=True, text=True, timeout=10, @@ -92,7 +92,7 @@ def image_id(ref: str) -> str | None: def container_engine_version() -> str | None: try: r = subprocess.run( - [ENGINE, "--version"], + [engine(), "--version"], capture_output=True, text=True, timeout=10, @@ -182,7 +182,15 @@ def _looks_like_stale_cache(output_lines: list[str]) -> bool: def _build_one(dockerfile: Path, tag: str) -> None: """Run one container build with a stale-cache retry.""" - cmd = [ENGINE, "build", "-f", str(dockerfile), "-t", tag, str(DOCKER_CONTEXT_ROOT)] + cmd = [ + engine(), + "build", + "-f", + str(dockerfile), + "-t", + tag, + str(DOCKER_CONTEXT_ROOT), + ] rc, last_line, output_lines = _run_build(cmd) if rc != 0 and _looks_like_stale_cache(output_lines): @@ -197,7 +205,7 @@ def _build_one(dockerfile: Path, tag: str) -> None: ) console.print() cmd_nc = [ - ENGINE, + engine(), "build", "--no-cache", "-f", @@ -246,7 +254,7 @@ def fix_data_ownership(data_dir: Path) -> None: """Fix root-owned copied data on Linux + rootful Docker.""" if sys.platform != "linux": return - if ENGINE != "docker": + if engine() != "docker": return if not data_dir.exists(): return @@ -266,7 +274,7 @@ def fix_data_ownership(data_dir: Path) -> None: print(f" Fixing ownership of {data_dir} (rootful Docker -> host UID)") subprocess.run( [ - ENGINE, + engine(), "run", "--rm", "-v", @@ -284,7 +292,7 @@ def fix_data_ownership(data_dir: Path) -> None: def _network_flags() -> list[str]: """Force slirp4netns on podman to avoid host-network port collisions.""" - if ENGINE == "podman": + if engine() == "podman": return ["--network=slirp4netns"] return [] @@ -292,7 +300,7 @@ def _network_flags() -> list[str]: def _proxy_env_flags() -> list[str]: """Forward host proxy env vars into the container.""" host_gw = ( - "host.containers.internal" if ENGINE == "podman" else "host.docker.internal" + "host.containers.internal" if engine() == "podman" else "host.docker.internal" ) flags: list[str] = [] has_proxy = False @@ -331,7 +339,7 @@ def docker_run_human( recording_mode: str = "x11", ) -> None: cmd = [ - ENGINE, + engine(), "run", "-d", "--name", @@ -376,7 +384,7 @@ def docker_run( recording_mode: str = "x11", ) -> None: env_flags = [ - ENGINE, + engine(), "run", "-d", "--name", @@ -497,7 +505,7 @@ def _container_usage_summary( try: r = subprocess.run( [ - ENGINE, + engine(), "exec", name, "sh", @@ -527,7 +535,7 @@ def docker_wait( """Block until the container exits, showing a live status line.""" start = time.time() proc = subprocess.Popen( - [ENGINE, "wait", name], stdout=subprocess.PIPE, stderr=subprocess.PIPE + [engine(), "wait", name], stdout=subprocess.PIPE, stderr=subprocess.PIPE ) last_actions = 0 usage_summary: dict | None = None @@ -542,7 +550,7 @@ def docker_wait( mins, secs = divmod(elapsed, 60) try: r = subprocess.run( - [ENGINE, "exec", name, "wc", "-l", "/data/actions.jsonl"], + [engine(), "exec", name, "wc", "-l", "/data/actions.jsonl"], capture_output=True, text=True, timeout=30, @@ -586,13 +594,13 @@ def docker_wait( def docker_copy(name: str, output_dir: Path) -> None: - run([ENGINE, "cp", f"{name}:/data", str(output_dir / "data")]) + run([engine(), "cp", f"{name}:/data", str(output_dir / "data")]) (output_dir / "data" / ".stop-requested").unlink(missing_ok=True) def docker_logs(name: str) -> None: - subprocess.run([ENGINE, "logs", "--tail", "40", name]) + subprocess.run([engine(), "logs", "--tail", "40", name]) def docker_rm(name: str) -> None: - subprocess.run([ENGINE, "rm", "-f", name], capture_output=True) + subprocess.run([engine(), "rm", "-f", name], capture_output=True) diff --git a/src/clawbench/runner/run_support/metadata.py b/src/clawbench/runner/run_support/metadata.py index 25df585d..c7f44522 100644 --- a/src/clawbench/runner/run_support/metadata.py +++ b/src/clawbench/runner/run_support/metadata.py @@ -12,8 +12,8 @@ from clawbench.runner.run_support.config import ( ASSET_ROOT, BASE_IMAGE, - ENGINE, WORKSPACE_ROOT, + engine, harness_image, ) from clawbench.runner.run_support.docker import container_engine_version, image_id @@ -118,7 +118,7 @@ def _sanitized_model_config(model_cfg: dict | None) -> dict[str, Any] | None: def _runtime_meta(harness: str) -> dict[str, Any]: harness_ref = None if harness == "human" else harness_image(harness) return { - "container_engine": ENGINE, + "container_engine": engine(), "container_engine_source": ( "env" if os.environ.get("CONTAINER_ENGINE") else "auto" ), diff --git a/tests/test_cli_entrypoints.py b/tests/test_cli_entrypoints.py index 54bb5583..fdeef64a 100644 --- a/tests/test_cli_entrypoints.py +++ b/tests/test_cli_entrypoints.py @@ -26,8 +26,11 @@ def test_module_help_does_not_require_container_runtime(module: str) -> None: """Help output should work even when docker/podman are unavailable. The subprocess monkeypatches ``shutil.which`` before running the module so - import-time container-engine probes see a host with no container runtime. - This keeps the test fully Python-based and cross-platform. + any container-engine probe sees a host with no container runtime. This + keeps the test fully Python-based and cross-platform. ``--help`` used to + pass only because ``_detect_engine`` sniffed ``sys.argv`` for a help flag + and returned a guess; it now passes because argparse exits before anything + asks for an engine. """ code = textwrap.dedent( diff --git a/tests/test_container_engine_resolution.py b/tests/test_container_engine_resolution.py new file mode 100644 index 00000000..a4047494 --- /dev/null +++ b/tests/test_container_engine_resolution.py @@ -0,0 +1,158 @@ +"""Container-engine resolution is lazy: importing a module must not probe PATH.""" + +from __future__ import annotations + +import importlib +import os +import shutil +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +SRC_ROOT = Path(__file__).resolve().parents[1] / "src" + + +@pytest.fixture(autouse=True) +def _clear_engine_cache(): + """engine() memoizes; don't leak a faked result into the next test.""" + from clawbench.runner.run_support import config + + config.engine.cache_clear() + yield + config.engine.cache_clear() + + +# Every module that reaches the engine, plus the ones that only import +# something that does. batch is in the list because its job planning, --dry-run +# and --resume paths are useful on a host with no container runtime at all. +ENGINE_DEPENDENT_MODULES = ( + "clawbench.runner.run_support.config", + "clawbench.runner.run_support.docker", + "clawbench.runner.run_support.metadata", + "clawbench.runner.run", + "clawbench.runner.batch", + "clawbench.eval.rescore", +) + + +def _import_in_subprocess( + module: str, *, engine_on_path: bool +) -> subprocess.CompletedProcess: + """Import ``module`` in a fresh interpreter, optionally hiding docker/podman.""" + + code = textwrap.dedent( + """ + import importlib + import shutil + import sys + + module, hide = sys.argv[1], sys.argv[2] == "hide" + if hide: + shutil.which = lambda _cmd: None + importlib.import_module(module) + print("imported") + """ + ) + env = os.environ.copy() + env["PYTHONPATH"] = str(SRC_ROOT) + env.pop("CONTAINER_ENGINE", None) + return subprocess.run( + [sys.executable, "-c", code, module, "hide" if not engine_on_path else "show"], + capture_output=True, + env=env, + text=True, + timeout=60, + ) + + +@pytest.mark.parametrize("module", ENGINE_DEPENDENT_MODULES) +def test_import_succeeds_with_no_container_engine_on_path(module: str) -> None: + """The regression this guards: ENGINE = _detect_engine() at module scope. + + That probe ended in a bare sys.exit(1), so importing any of these modules + killed the interpreter on a host without docker or podman -- not an + exception a caller could catch and degrade on. + """ + result = _import_in_subprocess(module, engine_on_path=False) + assert result.returncode == 0, result.stdout + result.stderr + assert "imported" in result.stdout + + +def test_engine_still_fails_fast_when_actually_needed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Laziness moves the failure, it does not remove it.""" + from clawbench.runner.run_support import config + + monkeypatch.delenv("CONTAINER_ENGINE", raising=False) + monkeypatch.setattr(shutil, "which", lambda _cmd: None) + config.engine.cache_clear() + + with pytest.raises(SystemExit) as excinfo: + config.engine() + + assert excinfo.value.code == 1 + + +@pytest.mark.parametrize("env_value", ["podman", "docker"]) +def test_container_engine_env_var_wins( + monkeypatch: pytest.MonkeyPatch, env_value: str +) -> None: + from clawbench.runner.run_support import config + + monkeypatch.setenv("CONTAINER_ENGINE", env_value) + monkeypatch.setattr(shutil, "which", lambda cmd: cmd) + config.engine.cache_clear() + + assert config.engine() == env_value + + +def test_container_engine_env_var_is_validated(monkeypatch: pytest.MonkeyPatch) -> None: + from clawbench.runner.run_support import config + + monkeypatch.setenv("CONTAINER_ENGINE", "containerd") + config.engine.cache_clear() + + with pytest.raises(SystemExit) as excinfo: + config.engine() + + assert excinfo.value.code == 1 + + +def test_engine_probes_path_once(monkeypatch: pytest.MonkeyPatch) -> None: + """Every container command re-asks; shutil.which walks PATH each time.""" + from clawbench.runner.run_support import config + + calls: list[str] = [] + + monkeypatch.delenv("CONTAINER_ENGINE", raising=False) + monkeypatch.setattr(shutil, "which", lambda cmd: calls.append(cmd) or cmd) + config.engine.cache_clear() + + assert config.engine() == "docker" + assert config.engine() == "docker" + assert calls == ["docker"] + + +def test_engine_constant_still_resolves_but_warns( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """config.ENGINE is kept for one release for out-of-tree callers.""" + config = importlib.import_module("clawbench.runner.run_support.config") + + monkeypatch.delenv("CONTAINER_ENGINE", raising=False) + monkeypatch.setattr(shutil, "which", lambda cmd: cmd) + config.engine.cache_clear() + + with pytest.deprecated_call(): + assert config.ENGINE == "docker" + + +def test_unknown_config_attribute_still_raises() -> None: + config = importlib.import_module("clawbench.runner.run_support.config") + + with pytest.raises(AttributeError): + config.NOT_A_REAL_SETTING diff --git a/tests/test_mock_container_runtime.py b/tests/test_mock_container_runtime.py index 236e15ef..ada3ba3f 100644 --- a/tests/test_mock_container_runtime.py +++ b/tests/test_mock_container_runtime.py @@ -3,9 +3,7 @@ from __future__ import annotations import importlib -import shutil import subprocess -import sys from dataclasses import dataclass, field from pathlib import Path from types import ModuleType @@ -13,21 +11,18 @@ import pytest -def _import_docker_helpers(monkeypatch: pytest.MonkeyPatch) -> ModuleType: - """Import docker helpers with a fake docker binary visible on PATH.""" +def _import_docker_helpers( + monkeypatch: pytest.MonkeyPatch, engine: str = "docker" +) -> ModuleType: + """Import docker helpers with the container engine pinned to ``engine``. - for module_name in ( - "clawbench.runner.run_support.docker", - "clawbench.runner.run_support.config", - ): - sys.modules.pop(module_name, None) - monkeypatch.delenv("CONTAINER_ENGINE", raising=False) - monkeypatch.setattr( - shutil, - "which", - lambda cmd: str(Path("mock-bin") / cmd) if cmd == "docker" else None, - ) - return importlib.import_module("clawbench.runner.run_support.docker") + Importing the module is free of any engine probe, so the test only has to + say which engine the helpers should build commands for. + """ + + docker = importlib.import_module("clawbench.runner.run_support.docker") + monkeypatch.setattr(docker, "engine", lambda: engine) + return docker @dataclass @@ -223,13 +218,12 @@ def test_docker_run_human_uses_podman_network_flags_with_mock_runtime( monkeypatch: pytest.MonkeyPatch, tmp_path: Path, ) -> None: - docker = _import_docker_helpers(monkeypatch) + docker = _import_docker_helpers(monkeypatch, engine="podman") commands: list[list[str]] = [] schema_path = tmp_path / "eval-schema.json" personal_info_dir = tmp_path / "my-info" schema_path.write_text("{}") personal_info_dir.mkdir() - monkeypatch.setattr(docker, "ENGINE", "podman") monkeypatch.setattr(docker, "run", lambda cmd: commands.append(cmd)) docker.docker_run_human( diff --git a/tests/test_model_api_preflight.py b/tests/test_model_api_preflight.py index 79eae71b..67b9fffd 100644 --- a/tests/test_model_api_preflight.py +++ b/tests/test_model_api_preflight.py @@ -2,8 +2,6 @@ import importlib import json -import shutil -import sys import urllib.error import urllib.parse from email.message import Message @@ -237,20 +235,9 @@ def fake_urlopen(req: Any, timeout: int) -> FakeResponse: def _import_run_module(monkeypatch: pytest.MonkeyPatch) -> ModuleType: - for module_name in ( - "clawbench.runner.run", - "clawbench.runner.run_support.metadata", - "clawbench.runner.run_support.docker", - "clawbench.runner.run_support.config", - ): - sys.modules.pop(module_name, None) - monkeypatch.delenv("CONTAINER_ENGINE", raising=False) - monkeypatch.setattr( - shutil, - "which", - lambda cmd: str(Path("mock-bin") / cmd) if cmd == "docker" else None, - ) - return importlib.import_module("clawbench.runner.run") + run_mod = importlib.import_module("clawbench.runner.run") + monkeypatch.setattr(run_mod, "engine", lambda: "docker") + return run_mod def test_run_stops_on_preflight_failure_before_docker_build( diff --git a/tests/test_results_and_metadata.py b/tests/test_results_and_metadata.py index cc946054..f407ed76 100644 --- a/tests/test_results_and_metadata.py +++ b/tests/test_results_and_metadata.py @@ -5,8 +5,6 @@ import argparse import importlib import json -import shutil -import sys from pathlib import Path import pytest @@ -170,14 +168,8 @@ def test_remove_transient_usage_artifact(tmp_path: Path) -> None: def test_run_metadata_redacts_model_and_judge_secrets( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: - for module_name in ( - "clawbench.runner.run_support.metadata", - "clawbench.runner.run_support.docker", - "clawbench.runner.run_support.config", - ): - sys.modules.pop(module_name, None) - monkeypatch.setattr(shutil, "which", lambda cmd: cmd) metadata = importlib.import_module("clawbench.runner.run_support.metadata") + monkeypatch.setattr(metadata, "engine", lambda: "docker") monkeypatch.setattr(metadata, "container_engine_version", lambda: "docker fake") monkeypatch.setattr(metadata, "image_id", lambda _ref: "sha256:fake")