[bong-water-water-bong]

Refs #1702. The writer (tools/moe_placement.py) and the reader (app/moe_placement.h, #2156) are both tested — but never against each other. Every reader arm builds its table by hand in C++ (two layers, hand-written ids), so nothing proves that a table the pipeline actually produces is one a consumer can adopt. A schema typo or a renamed key would keep both suites green and fail in the field, which is precisely the silent failure the reader exists to prevent.

What it adds

The writer's real output for a real trace, plus an arm that loads it through the shipped reader:

Qwen3.8-Flash-Next, 48 routed layers, 512 experts, 2 groups
one-device share 52.3 % in-sample (contiguous control 0.2 %), held out 18.9 % over 5280 steps
48 of 48 layers are permutations of expert ids 0..511 · share_held_out 0.1892 (≥ the 15 % gate)

The arm asserts adoption, the held-out share the writer reported, the step count, and that every (layer, expert) reads back to group 0 or 1 — which is what a splitter does per token. The path comes from the command line and add_test passes it, so this runs in CI rather than being a fixture nothing reads; with no path the test skips and says so.

It is a regression fixture, not a placement recommendation

The share is a property of the trace, not the model: the issue's own table spans 44–60 % across three traces and two models. Regenerate with tools/moe_placement.py; a fixture that drops below 15 % held out is refused by the reader, which is the check this fixture exercises.

Verified

Built the test standalone (g++ -std=c++17 -I app -I <nlohmann>) and ran it with the fixture:

ok  the writer's own table is adopted by the reader: 48 routed layers, 512 experts, 2 groups
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

Review notes

• 247 KB / 25 k lines of fixture. That is the writer's one-id-per-line JSON for 48×512 experts; a table that covers all routed layers cannot be much smaller. If that is too large for the repo, the alternative is a small committed trace plus a generation step in CI, at the cost of the test no longer exercising the artefact a consumer receives.
• The C++ side is +41/−2 across tests/moe_placement_test.cpp and one add_test line; the rest is data.
• What remains for moe: co-activation expert placement pays for the HRX+NPU split (44-60 % of tokens on one device) and for the NVMe layout (18-31 % fewer reads), but not on top of prefill dedup #1702 is not this: the two-engine splitter that consumes such a table, which needs a simultaneous HRX+NPU window. This PR only proves the pipeline between the two halves that already exist.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1702 - Partially compliant Compliant requirements: The PR adds a test that validates the writer's output can be correctly read by the shipped reader The test fixture flash-next-decode.placement.json is a real output from the writer tool The test ensures that the placement table is correctly parsed and validated by the reader The test verifies that the held-out share is above the 15% threshold as required by the reader The test ensures that every expert in every layer reads back to a valid group (0 or 1) Non-compliant requirements: The PR does not implement the actual placement logic for the HRX+NPU split or NVMe layout (this is a test-only change) The PR does not implement the two-engine splitter or HRX streamer functionality that would consume the placement Requires further human verification: The actual performance improvements (44-60% one-device share and 18-31% fewer reads) need to be verified in hardware The integration of the placement with the actual MoE execution pipeline needs to be validated

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Test Coverage for Writer-Reader Integration The test adopt_writer_table verifies that the writer's output can be correctly parsed by the reader, which is important for ensuring compatibility between the two components. However, the test only checks basic properties of the placement table (layer count, group count, held-out share, and expert group assignments) and does not validate the actual co-activation optimization benefits described in the ticket. 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"); } Fixture Documentation Clarity The README explains that the fixture is a regression test and not a placement recommendation, which is correct. However, it could be more explicit about the specific trace used and how the 18.9% held-out share was measured to ensure reproducibility. # moe_placement fixtures `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. ## What it is Qwen3.8-Flash-Next (48 routed layers, 512 experts, top-10) decode trace, 2 device groups: ``` $ python3 tools/moe_placement.py <trace> -g 2 -e 512 -o flash-next-decode.placement.json moe_placement: 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 ``` | property | value | |---|---| | schema | `1bit/moe-placement/1` | | groups | 2 | | layers | 48, keyed by decimal layer number | | per layer | every expert id `0..511` exactly once (48 of 48 layers are permutations) | | `share_in_sample` | 0.523 | | `share_held_out` | **0.1892** (above the reader's 15 % floor) | | `held_out_steps` | 5280 | | per-layer `unseen` | 138 of 512 experts never fire in a 220-step trace — informational; unseen experts are still assigned a group, which is why the permutation rule holds | ## Why it is a fixture and not a shipped placement The numbers are a property of **this trace**, not of the model: a different prompt mix moves the one-device share (the issue's table shows 44–60 % across three traces and two models). So this is a regression fixture for the writer→reader path, not a recommendation to place a model this way. ## Regenerating The writer is deterministic for a given trace and seed; regenerate with the command above. A new fixture must keep `share_held_out >= 0.15` or the reader refuses it, which is the check the fixture exists to exercise. ## See also - `tools/moe_placement.py` — the writer (CI-registered via `add_python_test(moe_placement … --self-test)`). - `app/moe_placement.h` — the reader, and the only authority on whether a table is usable. - `docs/moe-streaming.md` — what consumes a table, and why the two-engine splitter is the next step.

---

⚠️ 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 at 1688086: CHANGES

cl-1077, the author of the reader (app/moe_placement.h, #2156). I ran it at this head, and the fixture is what it says it is. But the test does not catch the failure its own comment names.

What checks out. Standalone build of tests/moe_placement_test.cpp against this head's reader, run with the fixture: moe_placement_test: ok, rc 0, all four new arms pass. I also read the fixture independently: schema 1bit/moe-placement/1, 2 groups, 48 layers, 48 of 48 layers are permutations of 0..511, experts 512 on every layer, share_held_out 0.1892, held_out_steps 5280, share_in_sample 0.523.

What does not. The new arm's comment gives its reason as "a schema typo or a renamed key would leave both suites green and fail in the field". The fixture is a frozen copy of past writer output, so a writer that renames a key still leaves both suites green. I checked that on a copy of this head's tools/moe_placement.py, with "share_held_out" renamed to "share_heldout" (1 occurrence):

moe_placement self-test: 0 failure(s)        writer self-test with the key renamed: rc=0
moe_placement_test: ok                       reader test with the frozen fixture:    rc=0

So the arm proves that the reader adopts one table the writer once wrote. That is worth keeping. But it is not a writer→reader contract test: the writer can drift and nothing turns red.

The ask (small): produce the table at test time with the shipped writer, and hand it to the reader:

• commit the trace, or a small slice of it. The README says the writer is deterministic for a given trace, but the trace itself is not in the PR, so nobody else can regenerate the fixture. A few layers is enough; the reader's checks do not depend on 48.
• add a ctest step that runs tools/moe_placement.py <trace> -g 2 -e 512 -o ${CMAKE_CURRENT_BINARY_DIR}/placement.json (FIXTURES_SETUP), and make moe_placement_reader read that file (FIXTURES_REQUIRED).

A key rename in the writer then fails the reader test, and the frozen JSON can go. That also removes 25 k lines from the repo.

Two small ones:

• README, per-layer unseen: "138 of 512 experts never fire" is true of layer 0 only. Across the 48 layers it is 138 to 286.
• in >> t throws on a truncated or invalid file, so the test aborts instead of printing a FAIL line. Wrapping it the way load_placement() does would keep the failure readable.

[bong-water-water-bong]

Closing under the 16:45Z lane cap (at most 2 open PRs per lane), not on merit — this is the documentation half of #1702, whose consumer the orchestrator parked with #572/#679.

The branch and commit are preserved (1bit/1702-writer-table-fixture, 1688086c): the writer table fixture (tests/fixtures/moe_placement/flash-next-decode.placement.json), the README with its regeneration command, and the adoption arm that reads the writer's real output through the shipped reader (4/4 checks passing locally, moe_placement_test: ok). Nothing is lost — it is one gh pr reopen away whenever #1702 unparks or the cap lifts.

Ordering I applied, so the choice is visible: I kept #2204 (the three-class narrowing enumeration — durable tooling that serves #1962/#2127/#2186/#2167 and outlives any single row) and #2207 (the five-line census entry that closes #2171's reported breach). This was the only one of the three attached to a parked row, so it is the one that waits.