Skip to content

fix(runner): resolve the container engine on use, not on import - #339

Open
vaibhavdabas16 wants to merge 1 commit into
TIGER-AI-Lab:mainfrom
vaibhavdabas16:fix/lazy-container-engine
Open

fix(runner): resolve the container engine on use, not on import#339
vaibhavdabas16 wants to merge 1 commit into
TIGER-AI-Lab:mainfrom
vaibhavdabas16:fix/lazy-container-engine

Conversation

@vaibhavdabas16

@vaibhavdabas16 vaibhavdabas16 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Closes #315.

run_support/config.py ran engine detection as a side effect of import:

ENGINE = _detect_engine()

and _detect_engine ends in a bare sys.exit(1) when neither docker nor podman is on PATH. So importing config — or anything that transitively imports it — killed the interpreter on a host with no container runtime. Not an exception a caller could catch and degrade on.

This makes resolution lazy and cached (config.engine()), and calls it at the 16 sites that actually shell out to a container.

Answering @Perry2004's question on the issue

The core entrypoints, such as clawbench-run and clawbench-batch require the container engine and fail fast since they depend on it. Irrelevant scripts like adaptors and rescores are not requiring the engine.

That is exactly right, and it is the reason to make the change rather than an argument against it — the entrypoints that need an engine still fail fast, and this is what the current design costs the ones that don't. Measured on main, not argued from principle:

$ python -c "import shutil; shutil.which = lambda _c: None; \
    import clawbench.runner.run_support.config"
ERROR: Neither 'podman' nor 'docker' found on PATH        # exit 1

$ python -c "... import clawbench.runner.batch"           # imports fine
$ python -c "... import clawbench.eval.rescore"           # imports fine

batch and rescore import fine only because they were kept away from config. That avoidance has a price already paid in the tree:

  • eval/rescore.py grew its own yaml.safe_load of models.yaml rather than reuse load_model_config — the divergence reported in rescore: public reproducibility CLI defaults to a maintainer's home paths and silently no-ops elsewhere #296.
  • runner/batch.py carried a third copy of the PATH probe (config._detect_engine, batch.detect_engine, and tui._engine_from_env_or_path are the same logic three times, and batch's even had a different error string). This PR deletes batch's copy: detect_engine = engine.
  • Three tests reloaded modules and monkeypatched shutil.which purely to survive the import. They now pin module.engine directly.

So the change does not weaken fail-fast for clawbench-run/clawbench-batch; it stops the probe from being a tax on every module that merely sits in the same import graph.

What changed

Module Before After
run_support/config.py ENGINE = _detect_engine() at module scope @lru_cache engine()
run_support/docker.py 13 uses of ENGINE engine()
runner/run.py 2 engine()
run_support/metadata.py 1 engine()
runner/batch.py own copy of the probe detect_engine = engine

Two things fall out that the issue did not ask for but are direct consequences:

  1. _detect_engine sniffed sys.argv for -h/--help and returned a guessed engine so help output would work without Docker. With lazy resolution nothing asks for an engine before argparse exits, so the special case is gone. test_module_help_does_not_require_container_runtime still passes — now for the real reason instead of the workaround.
  2. Caching. Every container command re-asked, and shutil.which walks PATH each time. engine.cache_clear() is documented for the one process that changes CONTAINER_ENGINE mid-run (batch, when it pins the engine for child processes).

config.ENGINE is kept for one release as a module __getattr__ shim that warns and returns engine(), as the issue suggested. Nothing in this repo reads it any more. Because it only fires on attribute access, importing the module stays free of the probe.

Verification

213 passed, 4 skipped — unchanged from main on this machine. (test_host_tasks.py::…[v1-lite] fails on main too: the v1-lite symlinks check out as text files on Windows. Unrelated, not touched.)

13 new tests in tests/test_container_engine_resolution.py. The core one imports each of the six affected modules in a fresh interpreter with shutil.which stubbed to None. Against the pre-change source, config, docker, metadata and run all die at import; batch and rescore pass only by avoidance, and now pass while importing config directly — which is the property worth guarding.

The rest cover: engine() still exits 1 when no engine exists at call time, CONTAINER_ENGINE still wins and is still validated, PATH is probed once, the deprecated ENGINE warns, and an unknown attribute still raises AttributeError rather than being swallowed by the shim.

Merge notes

Touches runner/batch.py, as does #340 (different functions — imports and detect_engine here, print_run_stats/write_summary_json there). CHANGELOG.md will conflict textually with any other open PR of mine; trivial to resolve either way.

run_support/config.py ran engine detection at module scope:

    ENGINE = _detect_engine()

and _detect_engine ends in a bare sys.exit(1) when neither docker nor
podman is on PATH. Importing config -- or anything that transitively
imports it, which is docker.py, metadata.py and run.py -- therefore
terminated the interpreter on a host with no container runtime. Not an
exception a caller could catch and degrade on.

Replace the constant with a cached engine() function and call it at the
16 sites that actually shell out to a container. The probe now happens on
first use, where the failure belongs to the operation that needs an
engine, and importing the module is free of side effects.

Three follow-on cleanups fall out of that:

- batch.py's detect_engine() was a third copy of the same PATH probe,
  kept only because importing config was expensive. It is now an alias
  for config.engine().
- _detect_engine sniffed sys.argv for -h/--help and returned a guessed
  engine so help output would work without Docker. With lazy resolution
  nothing asks for an engine before argparse exits, so the special case
  is gone.
- Three tests reloaded modules and monkeypatched shutil.which purely to
  survive the import probe. They now pin module.engine directly.

config.ENGINE is kept for one release as a module __getattr__ shim that
warns and returns engine(); nothing in this repo reads it any more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

run_support/config.py resolves a container engine at import time, so any module importing it inherits a hard Docker dependency

2 participants