fix(rescore): resolve paths from the workspace instead of a maintainer's home - #313
fix(rescore): resolve paths from the workspace instead of a maintainer's home#313vaibhavdabas16 wants to merge 2 commits into
Conversation
…r's home
clawbench-rescore is an installed console script and both docs/scoring.md
and eval/scoring.md advertise it as the way to reproduce leaderboard
numbers from public traces. It shipped two defaults pointing at one
machine's layout:
--sweep-root ~/work/ClawBench/claw-output/sweep
--models-yaml ~/work/ClawBench/models/models.yaml
Everywhere else that path does not exist, and find_run_dirs() reaches it
through Path.rglob(), which yields nothing for a missing directory rather
than raising. The tool printed "discovered 0 tasks" and exited 0 — a
silent no-op for exactly the audience the script exists to serve.
- Defaults are now WORKSPACE_ROOT / "test-output" (where the runner
actually writes runs, matching clawbench-batch --output-dir) and the
workspace-resolved MODELS_YAML.
- A --sweep-root or --only-batch that is not a directory, or a tree with
no run-meta.json anywhere under it, now prints a diagnostic naming the
offending path and exits 2.
- The judge model is resolved with the shared load_model_config() rather
than a private yaml.safe_load() + config.get("api_key"). rescore
previously rejected the api_keys list form that every other entry point
normalizes.
load_models_yaml() and load_model_config() move from
runner/run_support/config.py to utils/model_config.py, taking an optional
explicit path; config.py re-exports both, so its importers are unchanged.
The move is what makes them reusable here: run_support/config.py resolves
a container engine at import time and exits when neither Docker nor
Podman is installed, and rescore only reads finished runs — importing it
would have made a post-hoc scoring tool require a container runtime.
The argument parser is extracted into build_parser() so the shipped
defaults can be asserted in tests.
Fixes TIGER-AI-Lab#296.
Perry2004
left a comment
There was a problem hiding this comment.
At least one of the added test case is failing in CI. Please resolve this before further review, thanks.
https://github.com/TIGER-AI-Lab/ClawBench/actions/runs/32335738442/job/96648767983?pr=313
test_defaults_do_not_ship_a_maintainers_home_layout checked the substring "work/ClawBench" against the resolved default paths. GitHub Actions checks this repo out to /home/runner/work/ClawBench/ClawBench, so a correctly workspace-relative default contains that substring and the guard failed on the ubuntu and macos runners while passing on windows. The property worth guarding is that the defaults are not anchored to the invoking user's home directory, which the resolved paths cannot express from an arbitrary checkout location. Assert it against the source of build_parser() instead; test_defaults_are_workspace_relative already pins the resolved values exactly.
|
Pushed What failed
The guard was written as a substring check against the resolved default paths: for value in (args.sweep_root, args.models_yaml):
assert "work/ClawBench" not in value.as_posix()Actions checks this repo out to What changedThe property actually worth guarding is that the defaults are not anchored to the invoking user's home directory. That cannot be read off the resolved paths from an arbitrary checkout location, so it is now asserted against the source of src = inspect.getsource(rescore.build_parser)
assert "Path.home()" not in src
assert "expanduser" not in src
VerificationI reproduced the CI condition locally by forcing the workspace to a runner-shaped path via
The control matters: the old form fails under that path, so the reproduction is faithful rather than a vacuous pass. Full suite locally: 202 passed, 3 skipped. The one failure, Current stateThe workflow runs for the new head are sitting in |
Note for maintainers: this PR overlaps #314Separate from the CI fix above, and only relevant at merge time. This PR and #314 both touch
Each merges cleanly into So two things can happen, depending on merge order:
Either order works and neither is blocked; only the second one to merge needs the rebase. No preference from me — say which you would rather take first and I will rebase the other onto it promptly. One thing worth a maintainer's eye either way: after both land, |
What does this PR do?
Fixes #296 — all four asks in the issue.
clawbench-rescoreis an installed console script (pyproject.toml:29) thatdocs/scoring.mdandeval/scoring.mdboth advertise as the way to reproduce leaderboard numbers from public traces. It shipped one machine's layout as its defaults:find_run_dirs()reaches the root throughPath.rglob(), which yields nothing for a directory that does not exist rather than raising. So on any other machine the tool printeddiscovered 0 tasksand exited 0 — a silent no-op for precisely the audience the script exists to serve.--models-yamlto the workspace-resolved configMODELS_YAML--sweep-rootto where runs landWORKSPACE_ROOT / "test-output", matchingclawbench-batch --output-dir(batch.py:833)--sweep-root/--only-batchthat isn't a directory, or a tree with norun-meta.jsonunder it, prints a diagnostic naming the path and exits 2load_model_configyaml.safe_load()+config.get("api_key")Before and after, on a path that doesn't exist:
One structural change worth calling out
Ask 4 could not be satisfied by importing
run_support/config.py: that module runsENGINE = _detect_engine()at import time (config.py:90) andsys.exit(1)s when neither Docker nor Podman is onPATH.rescore.pyand both judges currently import none of it, soclawbench-rescoreruns fine on a host with no container runtime — which is the normal case for someone scoring published traces. Importingconfigto reuse the loader would have handed a post-hoc scoring tool a hard Docker dependency, making the portability problem worse.So
load_models_yaml()andload_model_config()move toutils/model_config.py— they only ever neededWORKSPACE_ROOTandyaml— and both gain an optional explicit path argument.run_support/config.pyre-exports both names (they were already in its__all__), so every existing importer,runner/run.pyincluded, is untouched. A test assertsrescorenever reaches forrun_support.configagain.Happy to split that move into its own commit or drop it in favour of a narrower fix if you'd rather keep the loaders where they are.
Also extracted the argument parser into
build_parser()so the shipped defaults are assertable from tests.Corpus
Host-side eval tooling; no task data involved.
Test plan
tests/test_rescore_cli.py(10 tests): both defaults resolve underWORKSPACE_ROOT; a regression guard asserting no shipped default containswork/ClawBench; exit code 2 with a useful message for a missing root, an existing-but-empty root, and a missing--only-batch;load_model_configaccepting both theapi_keyslist form and theapi_keyscalar form; the error naming the explicit--models-yamlpath; and a guard thatrescoredoes not import the container-probing module.Path.home() / "work/ClawBench/..."by reverting the file and re-reading them, and confirmedPath("/definitely/not/here").rglob("run-meta.json")returns[]rather than raising — the mechanism behind the silent no-op.import clawbench.eval.rescoresucceeds withshutil.whichstubbed to returnNonefor every command, i.e. with no container engine installed at all.202 passed, 3 skipped. The single failure,test_host_tasks.py::test_checked_task_json_files_parse_and_validate[v1-lite], reproduces identically on a cleanmainon this machine — thev1-litetask files are git symlinks (mode120000) that Windows checks out as text. Unrelated to this change.ruff checkandruff format --checkclean on all four files.Related issues
Fixes #296.
Noticed while in here, left out of scope:
batch.py:44-61andtui.py:110/:346each carry their own private copy ofMODELS_YAML+load_models_yaml. Now that there is one shared home for them, those two could be collapsed onto it in a follow-up if you want it.