test: add a MoE fixture, and fix a flaky assertion in the fixture suite - #144
Merged
Conversation
Two things, both found by running the suite rather than reasoning about it. **A MoE shape.** No MoE model appears anywhere in the CI matrix, so the path that stacks per-expert `mlp.experts.N.*` tensors into `switch_mlp` during sanitize was untested at any level. This fixture is a Qwen3-style MoE with per-expert tensors, an expert count nested under `text_config`, and a decoy count under `vision_config` that detection has to ignore. 230 KB. It does *not* reproduce #112, and the docstring says so. #112 was `num_local_experts` being the only spelling checked, so a config declaring `num_experts` was called dense, `--stream-experts` was dropped, and a 397B model was materialised whole until the OS killed it. But `modelTypeImpliesMoE` treats any model_type containing "moe" as MoE whatever the keys say, so `qwen3_moe` is caught by that fallback regardless. Verified rather than assumed: reverting detection to the single-key top-level form leaves this fixture still enabling streaming. Reproducing #112 end to end needs a MoE architecture whose model_type lacks "moe" — deepseek_v3, with `n_routed_experts` — and its MLA attention makes that a larger fixture. ModelProfilerMoEDetectionTests covers the profiler directly meanwhile. The suite does assert that `--stream-experts` is honoured for this fixture, which is the closest end-to-end check available: a model classified dense has the flag silently dropped, and silence was what turned #112 into an OOM kill instead of an error message. **A flaky assertion.** The existing check required completion_tokens >= 1. With random weights and the default temperature, sampling varies run to run: on one run kv-shared-absent drew EOS immediately, returned zero tokens, and failed — having passed minutes earlier on the same code. The assertion now pins temperature to 0 and checks prompt_tokens plus a well-formed response, which is what "the checkpoint loaded and a forward pass ran" actually means; token count never was. Three consecutive runs: 6 passed, 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
solderzzc
force-pushed
the
test/moe-fixture-and-flake-fix
branch
from
August 14, 2026 06:19
1aae41f to
bbcd3b0
Compare
solderzzc
added a commit
that referenced
this pull request
Aug 14, 2026
… faults (#145) Review follow-ups on #143/#144. All three findings were in code I wrote. **The MoE fixture tested nothing it claimed.** It used `qwen3_moe`, whose Swift configuration decodes from the root of config.json, so the expert count had to be at the top level; the nested copy underneath was never reached, because findExpertCounts is breadth-first and returns on the first hit. Both advertised properties — the nesting, and the `vision_config` decoy that "must not be mistaken" — were dead weight. Deleting the nested walk entirely would not have failed it. Rebuilt on `gemma4`, whose Gemma4Configuration decodes `text_config` and nothing else, so the count exists only one level down and the decoy is genuinely reached. `gemma4` also contains no "moe", so modelTypeImpliesMoE cannot rescue it — which is what made the qwen3_moe version untestable. Red-green verified, the check the previous version could not pass: reverting detection to the pre-#114 top-level single-key form fails the new assertion, and restoring it passes. The fixture now reproduces #112. The assertion also moved to the right gate. There are two: a config-level MoE check, and a model-level StreamableMoE conformance check. gemma4 passes the first and legitimately declines the second, so asserting "streaming enabled" would have tested the wrong thing. It now asserts only that detection did not reject. Incidentally covers the fused-expert remap — real gemma4 checkpoints ship `experts.gate_up_proj` as one tensor that sanitize splits into `switch_glu.gate_proj`/`up_proj`. A wrong split axis is a silent numerical fault. **Two harness faults.** cleanup() killed the server without waiting, and the readiness loop probed health before checking liveness. If a teardown outlived the 1s sleep, the next fixture would fail to bind, and its first probe would be answered by the previous server — assertions then run against the wrong checkpoint and report a false pass, not a flake. cleanup now waits, and liveness is checked first. The generator wrote into existing directories without clearing them, so files a builder stopped emitting survived and the fixture kept testing a shape the source no longer described. Each fixture's own directory is now cleared first — scoped to one known directory, which is also what keeps regeneration away from siblings like tests/fixtures/omni, whose assets belong to test-omni.sh. Suite: 6 passed, 0 failed, twice consecutively. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Follow-up to #143. Two things, both found by running the suite rather than reasoning about it.
1. A MoE shape
No MoE model appears anywhere in the CI matrix, so the sanitize path that stacks per-expert
mlp.experts.N.*tensors intoswitch_mlpwas untested at any level. This fixture is a Qwen3-style MoE with per-expert tensors, an expert count nested undertext_config, and a decoy count undervision_configthat detection must ignore. 230 KB.It does not reproduce #112 — and I checked rather than assumed
I built this intending to close #112's shape, then red-checked it the way the b674 fixture was checked, and it failed to reproduce the bug.
#112 was
num_local_expertsbeing the only spelling checked, so a config declaringnum_expertswas called dense,--stream-expertswas silently dropped, and a 397B model was materialised whole until the OS killed it. ButmodelTypeImpliesMoEtreats any model_type containing"moe"as MoE regardless of config keys — soqwen3_moeis caught by that fallback no matter what the spelling is.Verified by reverting detection to the single-key top-level form: this fixture still enables streaming.
Reproducing #112 end to end needs a MoE architecture whose model_type lacks
"moe"—deepseek_v3, withn_routed_experts. Its MLA attention makes that a substantially larger fixture, so it stays a follow-up;ModelProfilerMoEDetectionTestscovers the profiler function directly meanwhile. The docstring records all of this so the next person doesn't re-derive it.The suite does assert
--stream-expertsis honoured for this fixture, which is the closest end-to-end check available — a model classified dense has the flag dropped silently, and that silence is what turned #112 into an OOM kill rather than an error message.2. A flaky assertion — mine, from #143
The check required
completion_tokens >= 1. With random weights and the default temperature, sampling varies run to run. On one runkv-shared-absentdrew EOS immediately, returned zero tokens, and failed — having passed minutes earlier on identical code:That would have been an intermittent red check for everyone. The assertion now pins
temperature: 0and checksprompt_tokensplus a well-formed response — which is what "the checkpoint loaded and a forward pass ran" actually means. Token count never was the thing being proved.Three consecutive runs after the fix: 6 passed, 0 failed each time.
Net
🤖 Generated with Claude Code