Repository navigation
feat(bench): hold the comparison preconditions in one tool - #1732
Merged
Merged
Conversation
Fifty scripts under `scripts/` produce benchmark CSVs and none consumes them. Every comparison during the 2026-09 campaign was written inline, so a precondition checked in one place was absent in the next, and six published figures were wrong in exactly that way: a 0.35x cross-host deficit that paired rows from different commits, a 61% parity figure that paired an mlxcel VLM row with an mlx-lm text baseline, a 298% margin that divided a re-swept runtime by a two-day-old reference, a 117% README entry that put a text value in a VLM row, a pair count that differed between two hosts because the roster policy lived in whoever wrote the join, and a before-and-after table that did not apply the prompt-length gate its sibling comparison did. `compare_bench_csv.py` holds all of them. It refuses a VLM row paired with a text one, refuses differing `mlxcel_commit` values unless the version change is the measurement, refuses a reference taken more than a day apart, drops pairs whose `prompt_tokens` disagree beyond a tolerance, and excludes non-generative checkpoints from a generation roster. Names resolve through the catalog's `;`-separated `aliases`, since splitting that on `,` loses thirteen keys. Every drop is reported with its reason, because a pair count alone hides what it left out and one of the six wrong figures was a pair count. It also warns when a baseline row is superseded by a newer reading elsewhere for the same host and harness. That is the shape that produced the 0.35x: a sweep file held a pre-fix number while a newer per-model CSV beside it held the current one, and comparing the two sweeps is correct and still misses it. Validated by replaying all six. The two commit-mixing cases refuse outright, the harness mismatch exits 2, the roster disagreement resolves to 47 pairs on both hosts, and the corrected margins reproduce at 105% on M5 Max and 108% on M1 Ultra. The superseded warning was checked in both directions: silent for a baseline nothing supersedes, and naming seventeen rows when the 2026-09-04 sweep is compared past the 2026-09-06 one.
`bench_decode.sh` and `bench_mlxlm.py` were given a discovery guard after each of them enumerated an empty store, wrote a header-only CSV and exited 0. Three shell harnesses still had the same shape and none could even be pointed elsewhere: `bench_all_models.sh`, `bench_longprompt.sh` and `bench_na_attention.sh` all hardcoded `MODELS_DIR="./models"`, which holds checkpoints directly on one benchmark host and only container directories on the other. All three now take `MODELS_DIR` from the environment and share `scripts/lib/store_guard.sh`, which reports the root and the checkpoint count on every run and refuses before anything is written. A checkpoint is a directory holding `config.json`, the predicate the sweeps already use for `SKIP:not_a_checkpoint`. `bench_longprompt.sh` takes a named roster rather than enumerating, so it gets the other variant, and it needed it: three of its five default models (`llama-3.1-8b-4bit`, `qwen2.5-7b-4bit`, `mixtral-8x7b-4bit`) were renamed by the 2026-09-09 store consolidation, so the default sweep had been skipping them one at a time while still producing a CSV. The names are corrected and `require_named_checkpoints` now says which of a roster is missing, refusing only when none resolves, since running a subset on purpose is ordinary. Verified by exit code read without a pipe, because reading it through one returns the status of `sed`: container-only root exits 1 in both `all`-mode scripts, the real store passes at 211 checkpoints, an all-missing roster exits 1, and a partial roster warns and proceeds.
inureyes
added a commit
that referenced
this pull request
Sep 9, 2026
) The guards added in #1732 sat after the point where each script creates its output, which is the defect they were modelled on and which #1729 had already fixed once in `bench_decode.sh`. `bench_na_attention.sh` truncates `$OUTPUT` and writes a header before reaching its guard; `bench_longprompt.sh` runs `: > "$OUTPUT"` thirty lines before its own; `bench_all_models.sh` takes `$1` as the log name and starts writing to it immediately, so a stray argument created a file named after it. Running the guards during review produced exactly that: a file named `all` and an empty `na_attention_m5max_2026-09-10.csv`, both from invocations the guard then refused. All three now run the check before the output path is chosen, so nothing has been created or truncated when they refuse. The test is the one that would have caught it: an existing file named with `--output` has to survive a refused run. All three exit 1 with the file byte-identical, and no stray output is left behind. Checking only that the guard fires is what let this through the first time, because a guard that fires after the damage still fires.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six figures published during the 2026-09 VLM campaign were wrong, none of them because of a code defect. Each came from a comparison written inline for that one occasion, so a precondition enforced in one place was missing in the next. This adds the tool that holds all of them, and fixes the three sweeps that could still measure nothing and say it went fine.
The comparison tool
scripts/compare_bench_csv.py. Fifty scripts underscripts/produce benchmark CSVs and nothing consumed them, which is why every join was hand-written.It refuses a VLM row paired with a text one, refuses two different
mlxcel_commitvalues unless the version change is what is being measured, refuses a reference runtime measured more than a day apart, drops pairs whoseprompt_tokensdisagree beyond a tolerance, and excludes embedders and rerankers from a generation roster. Names resolve throughdocs/model-catalog.tsv, whosealiasescolumn is;-separated; splitting it on,loses thirteen keys, two of which were that campaign's control checkpoints. Every drop is reported with its reason, because a pair count alone hides what it left out and one of the six wrong figures was a pair count.It also warns when a baseline row has been superseded by a newer reading in another CSV for the same host and harness. That is the shape that produced the worst of the six: a sweep file held a pre-fix number while a newer per-model CSV beside it held the current one, and comparing the two sweeps is correct and still misses it.
Validated by replaying all six. The two commit-mixing cases refuse outright, the harness mismatch exits 2, the roster disagreement resolves to 47 pairs on both hosts, and the corrected margins reproduce at 105% on M5 Max and 108% on M1 Ultra. The superseded warning was checked in both directions, silent where nothing supersedes and naming seventeen rows where something does.
The three unguarded sweeps
bench_decode.shandbench_mlxlm.pywere guarded when each of them enumerated an empty store and exited 0.bench_all_models.sh,bench_longprompt.shandbench_na_attention.shhad the same shape and hardcoded their store root, so they could not even be pointed at the other host's layout. All three now readMODELS_DIRand sharescripts/lib/store_guard.sh, which prints the root and checkpoint count on every run and refuses before anything is written.bench_longprompt.shnames a roster instead of enumerating, and three of its five defaults (llama-3.1-8b-4bit,qwen2.5-7b-4bit,mixtral-8x7b-4bit) were renamed by the 2026-09-09 store consolidation. The default sweep had been skipping them one at a time and still producing a CSV. Corrected, with a guard that names what is missing and refuses only when nothing resolves.What was checked and found sound
bench_embeddings.pywas re-checked for the column shift fixed earlier; all threewriterowcalls are 21 fields against a 21-field header. A checker that reported otherwise was itself wrong, which is worth saying because it was the first result.Validation
Exit codes read without a pipe, since reading one through
sedreturns the status ofsedrather than the script. Container-only root exits 1 in bothall-mode scripts, the real store passes at 211 checkpoints, an all-missing roster exits 1, and a partial roster warns and proceeds.bash -nclean on all four shell files.