Repository navigation
test(bellwether): read the tree bellwether unpack writes, checked against sets.toml - #2834
Conversation
…nst sets.toml smg-project/bellwether#31 stores a benchmark set as <set>.jsonl.zst in Git LFS, and makes the plain tree `bellwether unpack` writes the root consumers read. Pointed at fixtures/, as this test's doc said, the jsonl filter skipped every compressed set without a word: with Qwen3-8B's BFCL sets recorded, the run read 63 of 6,124 cases and passed. The doc and the skip notice now point at the unpacked tree, and: - a compressed set or a Git LFS pointer under the root fails the run with the unpack command; - the cases read from each render and parse set must be the cases the model's sets.toml lists; a listed set the tree lacks, a set file the table does not list, or sets with no sets.toml beside them fail the run; - each case is compared as its line is read, not after a model's whole directory is read, and the run prints each set with its count, the differences and a tally per model, not a line per case; - a model whose tokenizer does not load no longer ends the run: it is listed when the run fails at the end, after the other models are compared. Unit tests on small fixture trees cover the refusal, the comparison with sets.toml, a tree without sets.toml and a model that does not load. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
…t sets.toml The render parity test of the chat request path gets the tokenizer fixture test's changes, for the render sets it replays. It points at the tree `bellwether unpack` writes and refuses compressed sets and Git LFS pointers. It holds the cases read from each render set to the model's sets.toml, and compares each case as its line is read. It prints the differences with a tally per model, and lists a model whose tokenizer does not load at the end instead of stopping there. With Qwen3-8B's BFCL sets recorded, it read 47 of 3,688 render cases from fixtures/ and passed; it now fails there with the unpack command, and reads all 3,688 from the unpacked tree. The tokenizer download is now written beside its name and renamed into place, as the tokenizer fixture test does, so an interrupted write no longer leaves a short file that the next run trusts. Signed-off-by: Alex McC <319643551+hello-alexmcc@users.noreply.github.com>
github-actions
Bot
added
tokenizer
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 SummarySummary by CodeRabbit
WalkthroughBoth Bellwether test suites now validate unpacked JSONL fixture trees, stream cases during comparison, and report fixture or tokenizer-loading failures after processing available models. The render parity test also stages downloaded tokenizer files before moving them to their destination. ChangesBellwether fixture validation and comparison
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Other Merge Risk: ⚪ Minimal · up to This change only affects the opt-in Bellwether fixture tests. They now read the unpacked fixture tree, reject packed files and Git LFS pointer files, and report tokenizer-load failures after the remaining models have been compared. No production code paths change, and no blocking issues were found, so the change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| tally: &mut Tally, | ||
| known: &BTreeMap<&str, &str>, | ||
| ) { | ||
| self.seen.insert(fixture.id.clone()); |
There was a problem hiding this comment.
🟡 Nit: This file and crates/tokenizer/tests/bellwether_fixtures.rs share most of their code, but this record drops the insert result. The tokenizer version asserts it ("{id}: recorded more than once"). Now that each model reads several set files and each set's line count is checked against sets.toml, an id that appears in two sets (e.g. a BFCL set and common) passes here without a word. When that happens:
tally.casescounts both lines, butseenkeeps one, so the per-model tally and the closingN cases matchdisagree.differences.insertoverwrites the first occurrence's reason with the second's.
Asserting it as the tokenizer test does would keep the two in step:
| self.seen.insert(fixture.id.clone()); | |
| assert!( | |
| self.seen.insert(fixture.id.clone()), | |
| "{}: recorded more than once", | |
| fixture.id | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| provenance: Value, | ||
| } | ||
|
|
||
| /// One `[<kind>.<set>]` table of `<slug>/sets.toml`, which `bellwether record` |
There was a problem hiding this comment.
🟡 Nit: This new doc correctly says <slug>/sets.toml, but the unchanged struct docs above it still name bellwether's own tree: Manifest (line 73, fixtures/<slug>/manifest.toml), RenderCase (line 82) and ParseCase (line 100). The same goes for Manifest (line 131) and Fixture (line 139) in model_gateway/tests/bellwether_render_parity.rs. The module docs and assertion messages already dropped the fixtures/ prefix. The root is now the unpacked tree (fixtures-plain/ by default), and bellwether's fixtures/ is exactly the directory these tests refuse, so <slug>/manifest.toml and <slug>/render/<set>.jsonl would keep those docs from pointing readers at the wrong tree.
These tests at scale: 80 checkpoint groupsI ran this branch's two tests over a local bellwether recording: 80 checkpoint groups (#54's group primaries) and the corpus of every importer branch, sampled where sources are large. The fixtures were unpacked with
Tokenizer test: 5,173,149 cases, 34,863 differ, all known
Render-parity test: 2,281,750 cases, 2,065,153 match, 216,597 differ
Two new defects came out of this, each with a minimal Qwen3-8B fixture that reproduces it. #2836 is the larger: it touches 55 of 71 groups through BFCL multi-turn's Found while running it
|
2 participants
Description
Problem
bellwether #31 stores each benchmark set as
<set>.jsonl.zstin Git LFS, and makes the plain treebellwether unpackwrites the root that consumers read. smg's two bellwether tests still point atfixtures/. Their.jsonlfilter skips every compressed set without a word. With Qwen3-8B's BFCL sets recorded:Solution
Both tests read the unpacked tree, which #2822's workflow writes with
bellwether unpack, and hold what they read to each model'ssets.toml:sets.tomllists. Each of these fails the run:sets.tomlbeside them.Changes
crates/tokenizer/tests/bellwether_fixtures.rs: the unpacked tree, thesets.tomlchecks, per-line comparison, and the tally.model_gateway/tests/bellwether_render_parity.rs: the same for the render sets it replays, plus the atomic tokenizer download.Test Plan
On eb42d3a..92c1e1c, on main 200ed03, with a target directory of its own and exit codes captured unpiped:
cargo +nightly fmt --all -- --check: 0.cargo +1.98.0 clippy -p llm-tokenizer -p smg --tests -- -D warnings: 0.BELLWETHER_FIXTURES=<bellwether main, unpacked> cargo +1.98.0 test -p llm-tokenizer -p smg --test 'bellwether_*' -- --nocapture: 0, with 5 passed in each suite:sets_without_a_sets_toml_stop_the_runcompressed_sets_and_lfs_pointers_are_found_under_the_rootthe_cases_read_must_be_the_cases_sets_toml_listsa_model_whose_tokenizer_does_not_load_fails_the_run_after_the_othersencode_and_incremental_decode_match_the_reference/render_fixtures_match_the_reference_byte_for_byteChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses