diff --git a/src/clawbench/eval/rescore.py b/src/clawbench/eval/rescore.py index 3df30a75..ce81ca38 100644 --- a/src/clawbench/eval/rescore.py +++ b/src/clawbench/eval/rescore.py @@ -36,7 +36,12 @@ from pathlib import Path from typing import Any -import yaml +from clawbench.utils.model_config import ( + MODELS_YAML, + ModelConfigError, + load_model_config, +) +from clawbench.utils.paths import WORKSPACE_ROOT JUDGE_FILE = {"strict": "judge.json", "lenient": "judge_llm.json"} @@ -196,14 +201,16 @@ def write_eval_results( print(f" eval_results written to {out_dir}/") -def main() -> int: +def build_parser() -> argparse.ArgumentParser: p = argparse.ArgumentParser( description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter ) p.add_argument( "--sweep-root", type=Path, - default=Path.home() / "work/ClawBench/claw-output/sweep", + default=WORKSPACE_ROOT / "test-output", + help="Directory tree to scan for completed runs " + "(default ./test-output, where the runner writes them).", ) p.add_argument( "--judge-model", @@ -214,7 +221,8 @@ def main() -> int: p.add_argument( "--models-yaml", type=Path, - default=Path.home() / "work/ClawBench/models/models.yaml", + default=MODELS_YAML, + help=f"Model definitions to resolve --judge-model against (default {MODELS_YAML}).", ) p.add_argument( "--rubric", @@ -242,19 +250,23 @@ def main() -> int: ) p.add_argument("--limit", type=int, default=0) p.add_argument("--only-batch", type=Path, default=None) - args = p.parse_args() + return p - cfg_all = yaml.safe_load(args.models_yaml.read_text()) - if args.judge_model not in cfg_all: - print( - f"ERROR: judge model {args.judge_model!r} not in {args.models_yaml}", - file=sys.stderr, - ) - return 2 - judge_cfg = dict(cfg_all[args.judge_model]) - if not judge_cfg.get("api_key"): - print(f"ERROR: judge {args.judge_model!r} has no api_key", file=sys.stderr) - return 2 + +def main() -> int: + args = build_parser().parse_args() + + # Shared loader: validates the model exists, checks required fields, and + # normalizes the api_key/api_keys pair. Re-parsing the YAML here used to + # reject the api_keys list form that every other entry point accepts. + try: + judge_cfg = load_model_config(args.judge_model, args.models_yaml) + except ModelConfigError as e: + # A bad --judge-model is a typo on the command line, caught before any + # scoring work starts. Report it like run.py does rather than letting + # the traceback out. + print(f"ERROR: {e}") + return 1 rubrics = [args.rubric] if args.rubric != "both" else ["lenient", "strict"] judge_funcs = {} @@ -267,11 +279,25 @@ def main() -> int: judge_funcs["lenient"] = judge_lenient - run_dirs = ( - find_run_dirs(args.only_batch) - if args.only_batch - else find_run_dirs(args.sweep_root) - ) + scan_root = args.only_batch or args.sweep_root + if not scan_root.is_dir(): + flag = "--only-batch" if args.only_batch else "--sweep-root" + print(f"ERROR: {flag} {scan_root} is not a directory.", file=sys.stderr) + print( + " Point it at a tree containing completed runs " + "(the runner writes them under ./test-output by default).", + file=sys.stderr, + ) + return 2 + + run_dirs = find_run_dirs(scan_root) + if not run_dirs: + print( + f"ERROR: no runs found under {scan_root} " + "(looked for run-meta.json at any depth).", + file=sys.stderr, + ) + return 2 pending = [] for rd in run_dirs: diff --git a/src/clawbench/runner/run_support/config.py b/src/clawbench/runner/run_support/config.py index 37dc6a99..a35991ff 100644 --- a/src/clawbench/runner/run_support/config.py +++ b/src/clawbench/runner/run_support/config.py @@ -5,14 +5,18 @@ import sys from pathlib import Path -import yaml - from clawbench.runner.run_support.harness_registry import ( HARNESS_REGISTRY, HARNESS_REGISTRY_YAML, HarnessRegistry, load_harness_registry, ) +from clawbench.utils.model_config import ( + MODELS_YAML, + ModelConfigError, + load_model_config, + load_models_yaml, +) from clawbench.utils.paths import ( ASSET_ROOT, WORKSPACE_ROOT, @@ -44,17 +48,6 @@ "resolve_test_case_path", ] - -class ModelConfigError(Exception): - """Raised when a model config in models/models.yaml is missing or invalid. - - A plain Exception (not SystemExit) so callers that load a model mid-run - (e.g. the judge stage, after the agent has already produced results) can - catch it and continue instead of the process dying before run-meta.json - is written. - """ - - HARNESSES = HARNESS_REGISTRY.harnesses DEFAULT_HARNESS = HARNESS_REGISTRY.default BASE_IMAGE = HARNESS_REGISTRY.base_image @@ -99,7 +92,6 @@ def _detect_engine() -> str: ENGINE = _detect_engine() -MODELS_YAML = WORKSPACE_ROOT / "models" / "models.yaml" def load_dotenv(path: Path) -> dict[str, str]: @@ -117,20 +109,6 @@ def load_dotenv(path: Path) -> dict[str, str]: return env -def load_models_yaml() -> dict: - """Load all model definitions from models/models.yaml. - - Raises ModelConfigError rather than exiting, for the same reason - load_model_config does: this runs inside the judge stage too, where a - SystemExit would escape the handlers and lose the run's metadata. - """ - if not MODELS_YAML.exists(): - raise ModelConfigError( - f"{MODELS_YAML} not found (copy models.example.yaml and fill in your keys)" - ) - return yaml.safe_load(MODELS_YAML.read_text()) or {} - - def load_runtime_env() -> dict[str, str]: """Load runtime credentials in increasing precedence order.""" env = load_dotenv(bundled_path(".env")) @@ -167,50 +145,3 @@ def resolve_task_file(path: Path) -> tuple[Path, Path, str]: if resolved.is_file(): return resolved.parent, resolved, resolved.stem return resolved, resolved / "task.json", resolved.name - - -def load_model_config(model: str) -> dict: - """Load a model config by name from models/models.yaml. - - The YAML key is the model name (passed as MODEL_NAME to the container). - """ - all_models = load_models_yaml() - if model not in all_models: - raise ModelConfigError( - f"model '{model}' not found in {MODELS_YAML}. " - f"Available models: {', '.join(sorted(all_models))}" - ) - - # Validate model name characters. Note: '/' and ':' are valid in - # vendor-prefixed ids like 'anthropic/claude-sonnet-4-6' or - # 'arcee-ai/trinity-large-preview:free' — they get sanitized to - # '--' before being used as path components. We only reject characters - # that could cause real trouble in shell/filesystem paths even after - # that sanitization. - bad = [c for c in ' \\*?"<>|' if c in model] - if bad: - raise ModelConfigError( - f"model name '{model}' contains illegal character(s): " - f"{' '.join(repr(c) for c in bad)}" - ) - - config = dict(all_models[model]) - config["model"] = model # the YAML key IS the model name - - required = ["base_url", "api_type"] - missing = [k for k in required if not config.get(k)] - if missing: - raise ModelConfigError( - f"required field(s) missing for model '{model}': {', '.join(missing)}" - ) - - # Normalize API keys: api_keys list wins, else wrap api_key into list. - if config.get("api_keys"): - config["api_key"] = config["api_keys"][0] - elif config.get("api_key"): - config["api_keys"] = [config["api_key"]] - - if not config.get("api_keys"): - raise ModelConfigError(f"no api_key or api_keys for model '{model}'") - - return config diff --git a/src/clawbench/utils/model_config.py b/src/clawbench/utils/model_config.py new file mode 100644 index 00000000..6322bfd6 --- /dev/null +++ b/src/clawbench/utils/model_config.py @@ -0,0 +1,92 @@ +"""Model definitions from models/models.yaml. + +Kept in `utils` rather than `runner.run_support.config` so that tools which +only score existing runs — `clawbench-rescore`, for one — can resolve a judge +model without importing the runner, which probes for a container engine at +import time and exits when Docker and Podman are both absent. +""" + +from pathlib import Path + +import yaml + +from clawbench.utils.paths import WORKSPACE_ROOT + +MODELS_YAML = WORKSPACE_ROOT / "models" / "models.yaml" + + +class ModelConfigError(Exception): + """Raised when a model config in models/models.yaml is missing or invalid. + + A plain Exception (not SystemExit) so callers that load a model mid-run + (e.g. the judge stage, after the agent has already produced results) can + catch it and continue instead of the process dying before run-meta.json + is written. + """ + + +def load_models_yaml(models_yaml: Path | None = None) -> dict: + """Load all model definitions from models/models.yaml. + + `models_yaml` overrides the workspace-resolved default, for callers that + expose an explicit --models-yaml flag. + + Raises ModelConfigError rather than exiting, for the same reason + load_model_config does: this runs inside the judge stage too, where a + SystemExit would escape the handlers and lose the run's metadata. + """ + path = models_yaml or MODELS_YAML + if not path.exists(): + raise ModelConfigError( + f"{path} not found (copy models.example.yaml and fill in your keys)" + ) + return yaml.safe_load(path.read_text()) or {} + + +def load_model_config(model: str, models_yaml: Path | None = None) -> dict: + """Load a model config by name from models/models.yaml. + + The YAML key is the model name (passed as MODEL_NAME to the container). + `models_yaml` overrides the workspace-resolved default. + """ + path = models_yaml or MODELS_YAML + all_models = load_models_yaml(models_yaml) + if model not in all_models: + raise ModelConfigError( + f"model '{model}' not found in {path}. " + f"Available models: {', '.join(sorted(all_models))}" + ) + + # Validate model name characters. Note: '/' and ':' are valid in + # vendor-prefixed ids like 'anthropic/claude-sonnet-4-6' or + # 'arcee-ai/trinity-large-preview:free' — they get sanitized to + # '--' before being used as path components. We only reject characters + # that could cause real trouble in shell/filesystem paths even after + # that sanitization. + bad = [c for c in ' \\*?"<>|' if c in model] + if bad: + raise ModelConfigError( + f"model name '{model}' contains illegal character(s): " + f"{' '.join(repr(c) for c in bad)}" + ) + + config = dict(all_models[model]) + config["model"] = model # the YAML key IS the model name + + required = ["base_url", "api_type"] + missing = [k for k in required if not config.get(k)] + if missing: + raise ModelConfigError( + f"required field(s) missing for model '{model}': {', '.join(missing)}" + ) + + # Normalize API keys: api_keys list wins, else wrap api_key into list. + if config.get("api_keys"): + config["api_key"] = config["api_keys"][0] + elif config.get("api_key"): + config["api_keys"] = [config["api_key"]] + + if not config.get("api_keys"): + raise ModelConfigError(f"no api_key or api_keys for model '{model}'") + + return config diff --git a/tests/test_rescore_cli.py b/tests/test_rescore_cli.py new file mode 100644 index 00000000..d995f140 --- /dev/null +++ b/tests/test_rescore_cli.py @@ -0,0 +1,231 @@ +"""rescore CLI portability: workspace-resolved defaults and loud failures.""" + +from __future__ import annotations + +import inspect +import sys +from pathlib import Path + +import pytest +import yaml + +from clawbench.eval import rescore +from clawbench.utils.model_config import ( + MODELS_YAML, + ModelConfigError, + load_model_config, +) +from clawbench.utils.paths import WORKSPACE_ROOT + + +@pytest.fixture +def stub_judge(monkeypatch: pytest.MonkeyPatch) -> None: + """Reach the scan-root check without needing real model credentials.""" + monkeypatch.setattr( + rescore, + "load_model_config", + lambda model, models_yaml=None: { + "model": model, + "base_url": "https://example.test", + "api_type": "openai-completions", + "api_key": "k", + "api_keys": ["k"], + }, + ) + + +# --- portable defaults (asks 1 and 2) ---------------------------------------- + + +def test_defaults_are_workspace_relative() -> None: + args = rescore.build_parser().parse_args([]) + + assert args.sweep_root == WORKSPACE_ROOT / "test-output" + assert args.models_yaml == MODELS_YAML + + +def test_defaults_do_not_ship_a_maintainers_home_layout() -> None: + """Regression guard against re-introducing ~/work/ClawBench/... defaults. + + Asserted against the source rather than the resolved paths: a checkout can + legitimately sit anywhere, including under a directory literally named + ``work/ClawBench`` (GitHub Actions checks this repo out to + ``/home/runner/work/ClawBench/ClawBench``), so only the absence of a + home-anchored default is meaningful. + """ + src = inspect.getsource(rescore.build_parser) + + assert "Path.home()" not in src + assert "expanduser" not in src + + +# --- loud failure instead of a silent no-op (ask 3) -------------------------- + + +def test_missing_sweep_root_errors_instead_of_exiting_zero( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + stub_judge: None, +) -> None: + missing = tmp_path / "nope" + monkeypatch.setattr( + sys, "argv", ["clawbench-rescore", "--sweep-root", str(missing)] + ) + + rc = rescore.main() + + assert rc == 2 + err = capsys.readouterr().err + assert "is not a directory" in err + assert str(missing) in err + + +def test_empty_sweep_root_errors_instead_of_exiting_zero( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + stub_judge: None, +) -> None: + """A real directory holding no runs is still a mis-pointed CLI.""" + empty = tmp_path / "test-output" + empty.mkdir() + monkeypatch.setattr(sys, "argv", ["clawbench-rescore", "--sweep-root", str(empty)]) + + rc = rescore.main() + + assert rc == 2 + assert "no runs found" in capsys.readouterr().err + + +def test_missing_only_batch_names_that_flag( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], + stub_judge: None, +) -> None: + monkeypatch.setattr( + sys, + "argv", + ["clawbench-rescore", "--only-batch", str(tmp_path / "batch-gone")], + ) + + rc = rescore.main() + + assert rc == 2 + assert "--only-batch" in capsys.readouterr().err + + +# --- shared model loader (ask 4) --------------------------------------------- + + +def _write_models(tmp_path: Path, entry: dict) -> Path: + path = tmp_path / "models.yaml" + path.write_text(yaml.safe_dump({"judge-model": entry}), encoding="utf-8") + return path + + +def test_load_model_config_accepts_the_api_keys_list_form(tmp_path: Path) -> None: + """rescore re-parsed the YAML itself and rejected the api_keys list form + that every other entry point accepts.""" + path = _write_models( + tmp_path, + { + "base_url": "https://example.test", + "api_type": "openai-completions", + "api_keys": ["first", "second"], + }, + ) + + cfg = load_model_config("judge-model", path) + + assert cfg["api_key"] == "first" + assert cfg["api_keys"] == ["first", "second"] + + +def test_load_model_config_still_accepts_the_api_key_scalar_form( + tmp_path: Path, +) -> None: + path = _write_models( + tmp_path, + { + "base_url": "https://example.test", + "api_type": "openai-completions", + "api_key": "only", + }, + ) + + cfg = load_model_config("judge-model", path) + + assert cfg["api_key"] == "only" + assert cfg["api_keys"] == ["only"] + + +def test_load_model_config_reports_the_explicit_path_on_a_bad_model( + tmp_path: Path, +) -> None: + """The error must name the file it actually read, not the workspace default. + + #314 made the loader raise ModelConfigError instead of exiting, so the + path now has to survive in the exception message rather than on stdout. + """ + path = _write_models( + tmp_path, + { + "base_url": "https://example.test", + "api_type": "openai-completions", + "api_key": "k", + }, + ) + + with pytest.raises(ModelConfigError) as excinfo: + load_model_config("not-there", path) + + assert str(path) in str(excinfo.value) + assert str(MODELS_YAML) not in str(excinfo.value) + + +def test_rescore_reports_a_bad_judge_model_instead_of_a_traceback( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """ModelConfigError escaping main() would replace the old one-line + ERROR with a traceback for what is only a command-line typo.""" + path = _write_models( + tmp_path, + { + "base_url": "https://example.test", + "api_type": "openai-completions", + "api_key": "k", + }, + ) + monkeypatch.setattr( + sys, + "argv", + ["clawbench-rescore", "--judge-model", "not-there", "--models-yaml", str(path)], + ) + + assert rescore.main() == 1 + assert "ERROR: model 'not-there' not found" in capsys.readouterr().out + + +def test_load_models_yaml_falls_back_to_the_workspace_default() -> None: + """Passing no path must keep resolving to MODELS_YAML for the runner.""" + from clawbench.utils import model_config + + sig = inspect.signature(model_config.load_models_yaml) + assert sig.parameters["models_yaml"].default is None + + +# --- the tool must not require a container engine ---------------------------- + + +def test_rescore_does_not_depend_on_the_container_probing_config_module() -> None: + """rescore only scores finished runs. run_support.config probes for a + container engine at import time and exits when none is installed, so + pulling it in would make the tool unusable on a host without Docker.""" + src = Path(rescore.__file__).read_text(encoding="utf-8") + + assert "run_support.config" not in src + assert "run_support import config" not in src diff --git a/tests/test_run_judge_stage.py b/tests/test_run_judge_stage.py index d911492f..3dd2fa9a 100644 --- a/tests/test_run_judge_stage.py +++ b/tests/test_run_judge_stage.py @@ -275,7 +275,13 @@ def test_missing_models_yaml_is_also_catchable( a SystemExit would escape the judge stage exactly as before.""" config = _import_run_module(monkeypatch) cfg_mod = sys.modules["clawbench.runner.run_support.config"] - monkeypatch.setattr(cfg_mod, "MODELS_YAML", tmp_path / "absent.yaml") + # The loaders read MODELS_YAML from the module that defines them, so the + # patch has to land there; run_support.config only re-exports the name. + monkeypatch.setattr( + sys.modules["clawbench.utils.model_config"], + "MODELS_YAML", + tmp_path / "absent.yaml", + ) with pytest.raises(config.ModelConfigError) as excinfo: cfg_mod.load_models_yaml()