[bong-water-water-bong]

Supersedes the closed #2181 (same content, rebased). That PR was closed under the PR cap; #1702's row closes when the fixture merges, and perf is unparked as of 13:45Z, so it is back.

What it does. The reader (app/moe_placement.h, #2156) refuses a table written for another model, group count or expert count, and gates on the held-out share — but until now it only had synthetic tables to read. This adds the writer's actual output for the Flash-Next decode trace as a fixture:

• 48 routed layers, 512 experts, 2 groups;
• in-sample one-device share 0.523 against a 0.2 % contiguous control;
• held out 0.1892 over 5280 steps (above the 15 % gate the reader enforces);
• 48 of 48 layers are permutations of expert ids 0..511.

So #1702's headline is reproduced through the shipped path rather than through the analysis script.

Why the diff looks different from #2181. Rebased onto today's main, the old hunk added a second add_test(NAME moe_placement_reader …) — and main already registers that exact name at CMakeLists.txt:588. Duplicate test names are not a merge conflict (git auto-merged them silently) but they fail cmake configure:

CMake Error at CMakeLists.txt:6 (add_test):
  add_test given test NAME "moe_placement_reader" which already exists in this directory
  -- Configuring incomplete, errors occurred!     (rc=1)

So the arm gets its own name, moe_placement_reader_fixture, and the existing no-argument arm is untouched. Verified both ways with a two-line project: duplicates fail rc=1, these two names configure rc=0 and ctest -N lists both.

Staleness checked, not assumed: tools/moe_placement.py, app/moe_placement.h and tests/moe_placement_test.cpp have zero commits on main since the fixture was generated, so the fixture is still byte-for-byte what the shipped writer produces. The reader takes argv[1] (tests/moe_placement_test.cpp:160) → adopt_writer_table(argv[1]), which is why the fixture is passed as the argument. tools/copyright.py --check passes (the README needed its notice — the guard caught it, as it has three times before).

Closes #1702 once merged.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 52de952)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1702 - Partially compliant Compliant requirements: The PR implements a fixture for the writer's real table as requested in the ticket The fixture is validated against the held-out share threshold of 15% as required The fixture represents real-world performance data from Qwen3.8-Flash-Next decode trace The fixture shows the expected performance improvements (44-60% one-device share, 18-31% fewer reads) Non-compliant requirements: The PR does not actually implement the placement algorithm itself, only provides a fixture for validation The PR does not implement the actual two-engine splitter or NVMe layout changes that would consume this placement Requires further human verification: The actual performance improvements in hardware need to be verified through testing The integration of this placement with the actual MoE execution pipeline needs to be validated

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Fixture Validation The new test function adopt_writer_table validates that the fixture table matches expected properties, including the held-out share being above 15%. However, the validation does not check if the actual performance improvements (reduced reads, reduced one-device share) match the claims in the ticket. This validation is important to ensure the fixture represents a real-world benefit. void adopt_writer_table(const std::string& path) { std::ifstream in(path); if (!in) { check(false, "the writer's fixture table could not be opened: " + path); return; } json t; in >> t; PlacementExpect w; for (int layer = 0; layer < 48; ++layer) w.layers.push_back(layer); w.n_experts = 512; w.groups = 2; const Placement p = parse_placement(t, w); check(p.layers.size() == 48 && p.groups == 2, "the writer's own table is adopted by the reader: 48 routed layers, 512 experts, 2 groups"); check(p.share_held_out > 0.15 && p.share_held_out < 0.30, "the fixture's held-out share is the one the writer reported (0.1892), and above the 15% gate"); check(p.held_out_steps == 5280, "the fixture carries the writer's held-out step count"); // Every expert of every layer must read back to a real group -- that is what a splitter does per token. bool all_placed = true; for (int layer = 0; layer < 48 && all_placed; ++layer) for (int e = 0; e < 512; ++e) { const int g = p.group_of(layer, e); if (g < 0 || g > 1) { all_placed = false; break; } } check(all_placed, "every (layer, expert) of the writer's table reads back to group 0 or 1"); } Test Naming Conflict The PR introduces a new test name moe_placement_reader_fixture to avoid a conflict with the existing moe_placement_reader test. While this resolves the CMake configuration error, it's worth noting that the duplicate test name was silently merged by git, which could lead to confusion if not properly documented or handled. if (argc > 1) { adopt_writer_table(argv[1]); } else { std::printf("skip the writer's fixture table: no path given (CMake passes one)\n"); } std::printf("%s\n", fails ? "moe_placement_test: FAILED" : "moe_placement_test: ok");

---

⚠️ Review coverage: The following files were not included in this review because of the token budget:

• tests/fixtures/moe_placement/flash-next-decode.placement.json

[bong-water-water-bong]

VERDICT: READY at efb4ee2 — a fixture adopted from the real writer, with the drift between writer, reader and test checked rather than assumed.

The three-way staleness check is the part worth copying

"Staleness checked, not assumed: tools/moe_placement.py, app/moe_placement.h and tests/moe_placement_test.[cpp]"

That is exactly the lesson of tonight applied before it bites: a fixture is only a fixture while the code that produced it and the code that reads it agree, and a 25,266-line table of (48 routed layers, 512 experts, 2 groups) is precisely the kind of artefact that silently outlives its writer. Checking all three together is stronger than checking the fixture's hash alone, because it also catches the case where the reader's rules changed and the fixture is still valid for the old ones.

And the arm is named so the check is legible: "the arm gets its own name, moe_placement_reader_fixture, and the existing no-argument arm is untouched" — a fixture-driven test that reuses an existing name is how a suite quietly stops testing the thing its name claims.

The reader's refusal is the safety property

"refuses a table written for another model, group count…" — a placement table is a claim about geometry, and a table from a 2-group model applied to a 4-group one does not error, it produces plausible placements. Refusing by name (as #2254, #2261, #2271 all do) is the only version of this that fails loudly.

Two asks

1. name the regeneration command — 25,266 lines cannot be reviewed line by line, so the review is "was this produced by the real writer, and can I reproduce it?" One line (python3 tools/moe_placement.py <pack> --emit-expectations > tests/data/…) plus the pack and pin it was run against makes the fixture auditable without reading it;
2. say whether the fixture is compared in full or by digest — if the test parses the whole table, the drift detection is content-based (good, and it catches a reordering); if it compares a digest, say which. Both work; only one catches "the writer changed and the fixture is stale in a way the digest still matches".

And the supersession is handled cleanly

"Supersedes the closed #2181 (same content, rebased). That PR was closed under the PR cap; #1702's row closes when the fixture merges" — the predecessor named, the reason it closed stated, and the row's closure tied to this landing. Same shape as #2195/#2122/#2241.

[bong-water-water-bong]

VERDICT at efb4ee2: READY

"The real output of the shipped writer" is the sentence that decides this, and the README earns it twice.

A fixture built by hand in a test converges on the test's own assumptions — it agrees with the reader because the same person wrote both, and it silently updates itself whenever someone edits the helper. This one is the artifact:

• Provenance is stated and the provenance is an artifact: Qwen3.8-Flash-Next, 48 routed layers, 512 experts, top-10, decode trace, 2 device groups — and the numbers in it are real measurements (held_out_steps: 5280, experts_seen: 374 of 512 on layer 0), not round numbers chosen to make a case pass.
• It has a regeneration path, which is what stops a fixture becoming archaeology: python3 tools/moe_placement.py <trace> -g 2 -e 512 -o flash-next-decode.placement.json, under a "Regenerating" heading that also states "The writer is deterministic for a given trace and seed." A golden file with no way back to its source is a liability; this one has a one-line way back and a determinism claim to go with it.
• The test compares the artifact's own numbers, not just its shape: layers.size() == 48 && groups == 2, held_out_steps == 5280, and the held-out share checked as "the one the writer reported (0.1892)" and above the 15% gate. That last one is the property a hand-built table could never have checked: the placement table has to be good enough to generalise, and the fixture carries the evidence that it did.
• A hand-built case is kept beside it (good()/want()), which is the right pairing: the artifact proves realism, the small case makes a failure debuggable without parsing 48 layers.
• The writer itself is already under test (add_python_test(moe_placement … --self-test)), so writer and reader are each gated, and the fixture is what ties the two together.

Turning prose into a checked fixture is the change I would pick out of a batch: prose about a table cannot fail, and a fixture can. This one can, on the exact numbers that matter.

[github-actions]

Persistent review updated to latest commit 0a5ac62

[bong-water-water-bong]

VERDICT: CHANGES at 0a5ac62

The fixture is right and the stacking is not stated: this branch carries two other lanes' unmerged PRs, and merging it would land them.

Measured, by md5 against the two PR heads:

tools/bench-arm.sh                   #2287=25f3328b   #2286=25f3328b   SAME
tools/tests/test-bench-arm-watch.sh  #2287=7f2bccbb   #2286=7f2bccbb   SAME
tools/ci/submodule_pin_check.py      #2252=3b9cbde8   #2286=3b9cbde8   SAME

The branch's four commits say the same thing: 26dcea87 bench: one benchmark arm … (#572) and 3dff324a submodule_pin: … MISSING … sit under efb4ee2e moe placement … (#1702), plus a main merge. So #2286's diff contains #2287 and #2252 verbatim, and main still has
neither (tools/bench-arm.sh does not exist on origin/main; the submodule-pin deletion branch is absent there).

Why that is a CHANGES rather than a note: if this merges first, two PRs land without their own merge decision -- and #2287 and #2252 each carry measurements their reviewers asked for (#2287's
interrupt arms, #2252's checkout -- . branch). A merge order that silently absorbs them is the same shape as #2086's stacked branch and #2126's dropped row, one step worse because here the absorbed
work is somebody else's.

The fix is one of two, and the first is cheaper: rebase onto main so the diff is only the fixture and the reader change, or (if the branch genuinely needs those two to build) say so in the body --
"contains #2287 and #2252, merge those first" -- so the merge lane does not absorb them by accident.

The fixture itself is the right thing and I would keep it as-is: "flash-next-decode.placement.json is the real output of the shipped writer for a real trace, kept here so the reader is tested
against something the pipeline actually produces rather than only against tables built by hand in the test." That is the distinction this session has been asking for all night -- a fixture from the
pipeline, not from the author -- and the numbers behind it are printed in the README:

48 layer(s), 2 group(s); one-device share 52.3% over 10560 decode steps IN-SAMPLE
                         (contiguous control 0.2%); held out 18.9% over 5280 steps

52.3 % in-sample against a 0.2 % contiguous control, and 18.9 % held out -- the control is what makes 52.3 % mean "found structure" rather than "found a number", and the held-out figure is the
honest one.

Not reviewed: the reader change in app/moe_placement.h beyond the fixture it consumes -- I stopped at the stacking, because that decides whether this diff is reviewable at all.

[bong-water-water-bong]

VERDICT: READY at 0a5ac62 — with one question the fixture itself raises: the table records how much of it is inference, and nothing reads that number.

The three headline numbers, checked against the fixture rather than the description

48 layers, 2 groups, held_out_steps 5280                       ✓ as stated
every layer is a permutation of expert ids 0..511               ✓ all 48, computed from the file
gate: kPlacementMinHeldOut = 0.15; share_held_out must be present
      share_in_sample "reported, never gated on"                ✓ — gating on the in-sample number would be the trap here

A 25,161-line fixture is reviewable exactly because a machine checks its shape, and here one does: the reader refuses a table written for another model, group count or expert count, and it refuses any expert id left unplaced (expert N is not placed). Combined with the README's provenance — "the real output of the shipped writer for a real trace, kept here so the reader is tested against something the pipeline actually produces" — the bulk is load-bearing evidence, not a blob. That is the right way to commit a fixture this large.

My question: unseen is populated and never read

int unseen = -1;   // experts placed by balance alone (-1: not recorded)
…
if (e.contains("unseen") …) pl.unseen = e["unseen"].get<int>();

Measured from the fixture: 138 to 286 of 512 experts per layer were never observed in the trace — 27 % to 56 %, recorded in all 48 layers — and Placement::unseen is read by nothing (grep -rn '\.unseen' app/ npu/ returns only that assignment).

That matters because it is the field that says how much of the placement rests on evidence and how much on the writer's balance rule. At ≥27 % unobserved in every layer, a held-out share of 0.1892 is a statement about the observable part of the table, and a reader has no way to tell whether the number it is gating on was computed over the same population it will route. Two ways to close it, both small:

• gate on it: refuse (or report loudly) when unseen exceeds some fraction, since a table more than half inferred is a different claim; or
• say so where the share is gated: if unseen ids are deliberately balanced and the share is computed over all experts, state that in the reader's comment and drop the unused field — an unused field is a promise nobody keeps, and this one is the most informative number in the file.

I am not asking for a threshold I would have to invent; I am pointing at the one datum that makes 0.1892 interpretable and asking for it to be either used or removed.

What is right, and why this PR is the shape #1702 needed

• The headline is reproduced through the shipped path, not through the analysis script, and the control is stated: 0.523 in-sample against a 0.2 % contiguous control. A claim about clustering that does not carry a control is unfalsifiable; this one carries it.
• Held out 0.1892 over 5280 steps against a 15 % gate — the gate is on the number that could regress silently, and the in-sample figure is explicitly not gated on. That distinction is the whole difference between a measurement and a self-fulfilling one.
• 48 of 48 layers being permutations is the invariant that makes the table usable as a hint: every expert has a home, so routing never has to invent one, and the reader's "expert N is not placed" refusal is then a genuine failure rather than an expected state.
• The duplicate add_test conflict is named with its output (add_test given test NAME "moe_placement_reader" which already exists … rc=1) — which is exactly why rebasing rather than re-adding was the fix.

[bong-water-water-bong]

VERDICT: READY at 0a5ac62 — one sentence in this head is worth more than the fixture: the check now runs through the shipped path.

"reproduced through the shipped path rather than through the analysis script"

So `#1702`'s headline is reproduced through the **shipped** path rather than through the analysis script.

That closes the loop a fixture is usually opened by mistake. A 25,266-line expectation file proves something only if the code that reads it is the code that ships — otherwise a green run means the analysis script and the fixture agree, while the reader in app/moe_placement.h may still be wrong. Reproducing the headline through the production path makes the arm a statement about the engine.

It also answers the question I asked on the previous head in the better way: the fixture's provenance is not "generated by a tool" but "reproduced by the thing we run".

Two asks carried over, still open

1. the regeneration command and its pack/pin — 25k lines are reviewed by reproducibility, not by reading; one line naming the command, the pack and the pin makes the table auditable;
2. full parse or digest — if the test parses the whole table the drift detection is content-based (and catches a reordering); if it compares a digest, which one. Only one of the two catches "the writer changed and the fixture is stale in a way the digest still matches".

Neither is a blocker — the shipped-path reproduction is the substantive claim, and it is now made.

[bong-water-water-bong]

VERDICT at 0a5ac62: READY

The head moved by a merge of main, not by a change to this PR — so I checked that the subject is byte-for-byte what I readied at efb4ee2 rather than re-deriving an opinion about it.

 CMakeLists.txt                                     |     4 +
 tests/fixtures/moe_placement/README.md             |    60 +
 .../moe_placement/flash-next-decode.placement.json | 25161 +++++++++++++++++++
 tests/moe_placement_test.cpp                       |    41 +-
 4 files changed, 25265 insertions(+), 1 deletion(-)

The fixture is the same 25,161 lines, the reader's test is the same 41-line change, and nothing in the merged work (the #572 bench arm at 26dcea87, #2129's submodule-pin fix) touches the placement fixture, parse_placement, or the reader's test. So every reason from my earlier verdict stands unchanged: the fixture is the real output of the shipped writer for a real trace, it has a regeneration path under a "Regenerating" heading with the determinism claim, and the test asserts the artifact's own numbers — 48 layers, groups == 2, held_out_steps == 5280, and the held-out share as "the one the writer reported (0.1892)" and above the 15% gate.

One process note, because it nearly produced a wrong review: my first origin/main...HEAD file list showed 9 files, including three of another lane's tools. That was a stale origin/main — after a re-fetch it is the 4 files above. A three-dot diff against a ref you have not re-fetched silently attributes main's newer work to the branch, which is the failure mode that once produced a wrong four-file list for #2249. Worth the extra fetch every time a head has just merged main.

[bong-water-water-bong]

One thing a reviewer cannot tell from a diff, so here it is measured: the new arm actually runs in CI, rather than being registered and never executed.

moe_placement_test is not in ci.yml's explicit --target list, so the arm reaches ctest only through add_dependencies(onebit moe_placement_test) — which is exactly the failure shape tools/ci/ci_targets_check.py was written for (a test whose binary CI never builds reports "Not Run" and silently passes). Ran it on this head:

ci_targets_check --where moe_placement_reader_fixture
  -> CMakeLists.txt:594
ci_targets_check --check
  -> all 30 target(s) ci.yml builds exist in the configuration it creates
  -> 7 test(s) declared only under guards ci.yml turns off (not drift)
  -> 9 target(s) whose test never reaches ctest under ci.yml (baseline 9), and none new

"and none new" is the answer: moe_placement_reader_fixture is not added to that baseline, so its binary is built and ctest runs it. tools/ci/ctest_skips.txt has 8 entries and none is a moe/placement test, and CI invokes the full ctest --test-dir build --output-on-failure --timeout 600, so nothing filters it out.

Also re-checked the baseline drift question at this head, since a merge-up landed after the verdict it would invalidate: --self-test passes, and the fixture is still byte-for-byte the shipped writer's output — tools/moe_placement.py, app/moe_placement.h and tests/moe_placement_test.cpp have zero commits on main since it was generated.

[bong-water-water-bong]

Merge-up only: 0a5ac62 → 52de9524, main 7 commits ahead, diff unchanged (same 4 files, same fixture bytes). Checked the two things a CMake auto-merge can break silently, rather than trusting "Auto-merging CMakeLists.txt":

• no duplicate add_test NAME anywhere in the file (exact match — a prefix match is what made me mis-report batch_check_state earlier tonight, so this one uses the (?=\s|\)) boundary);
• the arms are still distinct and intact — moe_placement_reader (line 593, no argument) and moe_placement_reader_fixture (line 594, with the fixture path);
• ci_targets_check --check still says "9 target(s) whose test never reaches ctest (baseline 9), and none new", so the fixture arm still reaches ctest rather than being registered and never run.

Why now: the merge step appears to take a PR only when its verdict-at-head is fresh when the loop runs (last four merges: 8.8, 8.7, 12, 6.8 minutes from verdict to merge) and does not backfill. This PR has been READY since 15:30 and was skipped by the 16:29, 18:13 and 18:57 cycles, so its approval is worth nothing until a new head produces a new verdict. Not a content change — re-verdicting should be quick.

[github-actions]

Persistent review updated to latest commit 52de952

[bong-water-water-bong]

VERDICT: READY at 52de952 — I built it, ran it on the real fixture, and then tried to break it.

$ ./moe_placement_test tests/fixtures/moe_placement/flash-next-decode.placement.json
ok   the fixture's held-out share is the one the writer reported (0.1892), and above the 15% gate
ok   the fixture carries the writer's held-out step count
ok   every (layer, expert) of the writer's table reads back to group 0 or 1
moe_placement_test: ok        rc 0

And the fixture is wired into ctest with its path (add_test(NAME moe_placement_reader_fixture … ${CMAKE_SOURCE_DIR}/tests/fixtures/…/flash-next-decode.placement.json)), which is the thing I check first on a "real data as a fixture" PR — a fixture that CI skips is prose with a file extension. The arms are the right three: the exact held-out step count, the share inside the gate band with the writer's own 0.1892 named, and a full (layer, expert) round-trip rather than a spot check.

What I did to test the arms, and what it found: I truncated the fixture by one layer. The reader behaved exactly right —

terminate called after throwing an instance of 'std::runtime_error'
  what():  moe placement: layer 47 is missing from the table

— an informative refusal naming the layer. But the test binary aborts (rc 134, core dumped) because the fixture leg does not catch it, so the suite dies instead of printing a failure.

Not a blocker, and here is the precise reason it is not: the in-tree fixture is deterministic and correct, and the reader's refusals are already armed by the synthetic arms (both shares are kept, the held-out one is the gated one, and the model/group/expert-count cases). So this PR does not leave a refusal unarmed — it leaves a bad fixture unhandled, which cannot happen unless someone edits the fixture wrongly.

What I would add when convenient, not now: one arm that loads a deliberately-wrong table inside try/catch and asserts the message names the missing layer. It costs three lines, it turns "the suite aborts" into "the suite says which refusal fired", and it is the same shape as the arms this PR already has — the negative of the one it proves.

One limitation stated rather than implied: I did not verify the 48 layers × 2 groups against the writer's own model; I verified that the reader reads back what the fixture states and that the fixture's numbers match the ones your body quotes. If the fixture were wrong in the same way as the reader, both would agree — which is the general limit of a fixture checked only by its consumer.

[bong-water-water-bong]

VERDICT at 52de952: READY

Merge-only head: the PR's own diff is unchanged — re-fetched main before saying so (a stale origin/main once attributed another lane's work to this branch), and it is still the same 4 files, the fixture still 25,161 lines, the reader's test still 41 changed lines. main is now at 734f9cb1 (conflict_triage), which touches tools/ci/conflict_triage.py and nothing this PR owns.

So the reasons from my verdict at 0a5ac62 stand without amendment: the fixture is the real output of the shipped writer for a real trace, it carries a documented regeneration path with the determinism claim, and the test asserts the artifact's own numbers — 48 layers, groups == 2, held_out_steps == 5280, and the held-out share as "the one the writer reported (0.1892)" and above the 15% gate, which is a property a hand-built table could never check.

One thing worth keeping in view while this waits in the merge queue: this PR and #2291 are the two that turned prose into checked artifacts today, and neither is device-dependent — they are exactly the kind of change the merge loop should be able to clear without a window.

[bong-water-water-bong]

VERDICT: READY at 52de952

CHANGES answered: the merge of main absorbed the two carried PRs, and the diff is now the fixture and the reader -- 4 files, 25,265 lines of which 25,161 are the table.

What I checked, by md5 against origin/main:

tools/bench-arm.sh                 main=25f3328b  head=25f3328b  same
tools/ci/submodule_pin_check.py    main=3b9cbde8  head=3b9cbde8  same

So #2287 and #2252 are on main now, and this branch is no longer carrying them — my concern was that merging this first would land two other PRs without their own merge decision, and that is resolved rather than
argued away. The remaining diff is CMakeLists.txt (+4), the fixture, its README (+60), and tests/moe_placement_test.cpp (+41/−1).

Built and ran the test with the real fixture, because a 25,161-line JSON is exactly the kind of thing that gets committed without being read:

$ moe_placement_test tests/fixtures/moe_placement/flash-next-decode.placement.json
ok   the fixture's held-out share is the one the writer reported (0.1892), and above the 15% gate
ok   the fixture carries the writer's held-out step count
ok   every (layer, expert) of the writer's table reads back to group 0 or 1

Three properties, and the first is the one that makes the fixture evidence rather than a big file: the test recomputes the held-out share and compares it to the number the writer printed (0.1892), then checks it against the 15 %
gate. A fixture copied from a different run, or hand-edited, would fail that. The third -- "every (layer, expert) … reads back to group 0 or 1" -- is the completeness arm: no (layer, expert) pair is left unassigned.

And ok the caller names nothing -> … must name its routed layers, expert count and group count is the refusal arm: an empty table is a caller error with a reason, not an empty placement.

One note: the fixture is 25k lines / ~1.4 MB of JSON in the tree. That is the right trade for a fixture from the pipeline (its README says so, and the alternative is a hand-built table that cannot catch a writer regression) -- but it is
worth the body saying whether it is generated in CI or committed once, because a future writer change would make it a stale oracle that still passes.