tests: Enable a subset of GSM8k upstream vllm tests - #846
Conversation
Signed-off-by: Joe Runde <joe@joerun.de>
Signed-off-by: Joe Runde <joe@joerun.de> # Conflicts: # pyproject.toml
Drop `tolerance` and `max_tokens` from all three Spyre gsm8k configs: both merely restated the upstream code defaults (0.08 and 256 in tests/evals/gsm8k/test_gsm8k_correctness.py), so they added config surface without changing behavior. Raise `num_questions` 16 -> 64 for a more stable accuracy estimate and update the models-spyre.txt header to match. Thresholds still carry the original N=16 baseline; CI re-validates them at N=64. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joe Runde <joe@joerun.de>
The GSM8K correctness evals rode the `upstream` marker, so they ran inside the upstream per-op suite. Each eval starts a full vLLM server and runs a batched eval (minutes per config), which doesn't belong in that partition. Carve them into a dedicated `quality` suite: - Makefile: `test-quality` / `test-quality-shard[-%]` targets, QUALITY_SHARDS=3 (one config per shard), added to tests-single-card; exclude gsm8k from test-upstream[-shard] via `and not gsm8k`. - sharding.py: register --quality-shards/-shard-id, add _apply_quality_shard (selects the `gsm8k` marker), and drop gsm8k from the upstream applier. - _test_matrix.yaml: 3 single-card quality shard jobs (regression/trunk only). - test_sharding.py: extend the Makefile<->matrix drift guards to `quality`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joe Runde <joe@joerun.de>
|
👋 Hi! Thank you for contributing. We also recommend installing prek and configuring it to check your code before every local commit. |
The three gsm8k_configs/*.yaml headers repeated an identical serving-rationale paragraph that also lives on the eval's upstream_tests.yaml entry; keep only the per-field facts a reviewer would get wrong (measured threshold, total-budget timeout) and drop the dead "see PR" pointers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Joe Runde <joe@joerun.de>
|
/claude-review |
The gsm8k accuracy gate runs an upstream vLLM suite ('gsm8k and upstream'),
so it correctly triggers a clone, but its Makefile targets are named
test-quality* rather than test-upstream*. The upstream-gate guard test
assumed the two were identical and failed. Recognise both prefixes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Joe Runde <joe@joerun.de>
|
/claude-review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # accuracy_threshold is the measured on-device baseline; num_questions is small to gate cheaply. | ||
| # request_timeout_seconds is a *total* per-request budget (incl. queue-wait), so it must exceed the whole batched run's wall-clock. | ||
| model_name: "Qwen/Qwen2.5-0.5B-Instruct" | ||
| accuracy_threshold: 0.3125 |
There was a problem hiding this comment.
On a single on-device run of test-quality-shard-2, Qwen2.5-0.5B measured 0.2812 — below this 0.3125 threshold. It passed only via the 0.08 tolerance (floor 0.2325). At 64 questions the binomial stderr is ~0.056, so 0.3125 overstates the actual measured baseline and the gate leans on the tolerance band rather than on the threshold. Consider setting accuracy_threshold to the value the hardware actually produces (~0.28) so the comment ("measured on-device baseline") holds and a ~3-question downward drift still trips the gate instead of being absorbed. I could only verify qwen on hardware; granite/mistral thresholds are unverified.
| # 1-card CI jobs, one config per shard. The plugin owns the partition | ||
| # (--quality-shards), balancing by recorded runtime when a durations file is | ||
| # present, else evenly. QUALITY_SHARDS is the single source of the count. | ||
| QUALITY_SHARDS ?= 3 |
There was a problem hiding this comment.
QUALITY_SHARDS is pinned at 3 and models-spyre.txt lists exactly 3 configs, so today each shard runs one. This suite is the one place where items == shards exactly: if a config is later removed from models-spyre.txt without lowering this count, a shard collects 0 items and pytest exits 5 (no tests collected), reddening that CI job with nothing pointing at the cause. The other sharded suites have items >> shards so they never hit this. A guard tying the count to the config-list length (or test_sharding.py asserting #configs >= QUALITY_SHARDS) would catch the drift.
| # parametrize markers. Instead, point the upstream conftest's --config-list-file at | ||
| # a Spyre-owned list; its own (later-running) pytest_generate_tests then parametrizes | ||
| # config_filename from our configs. tryfirst here guarantees we set it first. | ||
| if fc.config_list and "config_filename" in metafunc.fixturenames: |
There was a problem hiding this comment.
This config_list steering (plus FileConfig.config_list parsing) has no dedicated test. test_sharding.py covers the shard-count/matrix wiring, but nothing asserts that a config_list entry actually redirects config_filename onto the Spyre configs. A regression here — the upstream conftest renaming --config-list-file, or the tryfirst ordering breaking — would silently fall back to the upstream GPU config list and still collect "green" tests. I confirmed it works today by collecting the suite (3 Spyre configs, 1533 GPU defaults deselected); a collection-only test asserting those 3 ids would lock that in.
|
Reviewed the GSM8K accuracy-gate wiring end to end and exercised it on Spyre hardware. What I checked:
Raised 3 low-severity inline comments, no blockers:
The PR is well-constructed and functions correctly on hardware; the findings are calibration/coverage polish rather than defects. |
1 participant
Description
This PR just sets up an initial config for running a single one of vllm's upstream lm-eval suites (gsm8k) against a small set of models that we currently support. This is mostly just setting up a pattern that we could follow to implement more of these eval tests.
As is this is probably light enough to run on all of our PRs but if we enable hundreds of these tests we may want a different strategy for scheduling them, like nightly runs.
Related Issues
For #773
Test Plan
Checklist
bash format.sh)Signed-off-by:line (DCO compliance)