Half the tie-stability device sweep is a duplicate: VT_MOE_ROUTER_WARP does not change the dispatch above E = 256
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 423
- Forks
- 53
- Avg merge
- 20h 26m
- Merged PRs (30d)
- 310
Description
Row: MODEL-MM-QWEN4-EXP
The gap
tests/vt/test_moe_router_tie_stability.cpp (landed by #2595) runs its device
sweep twice, once with VT_MOE_ROUTER_WARP pinned "1" and once pinned "0",
across E in {256, 512, 1024}. At E = 256 those are two different kernels. At
E = 512 and E = 1024 they are the same kernel launched twice:
MoeRouterWarpValuesPerThread(src/vt/cuda/moe_router_warp.h:96-98) returns
0for anyEoutside{32, 64, 128, 256}.LaunchRouterWarp(src/vt/cuda/cuda_moe.cu:564) returnsfalseonvpt == 0
before touching the tensors, soLaunchRouter(:599-609) falls through to
MoeRouterTopKKernel<Tin,false>whatever the env flag says.
What is and is not affected
The counts are honest. 108 rows, 4652 assertions and the mutant's 104
failures are all real assertions that really executed, and the closed-form
expectation is re-checked each time. Nothing is inflated in the sense of being
uncounted.
The COVERAGE at E > 256 is half what the doubled numbers imply. Of the 108
rows in case 1, 72 are at E in {512, 1024} and 36 of those are byte-identical
repeats of the other 36. In the mutation table the per-cell figure 12 is
2 arms x 3 h x 2 dtypes, so 6 of each 12 are the same disagreement counted a
second time. A reader who takes "both VT_MOE_ROUTER_WARP arms" as two
independent structures at the geometry qwen4_exp actually routes reads twice
the coverage that exists.
A comment saying so is added to the file by #2595. This issue owns the fix.
What closing this means
One of:
- Restrict the arm loop to the geometries where the flag changes the dispatch
(E = 256here), and say in the file thatE > 256has one kernel; or - Keep both arms and assert the redundancy instead of implying independence —
e.g.REQUIRE(MoeRouterWarpValuesPerThread(e) == 0)atE > 256, which
turns the duplicate into a checked property of the dispatch rather than an
unstated one; or - Widen
MoeRouterWarpValuesPerThreadso the warp kernel really does admit
E = 512, which is a kernel change with its own derivation obligation
(moe_router_warp.hexcludesE > 256on purpose, because the seed loops at
cuda_moe.cu:71,83accumulate several experts per thread in ascending order).
(3) is a product change and needs its own spec. (1) or (2) is a test repair and
needs GPU time to re-measure the counts every record cites, which is why it is
not folded into #2595.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with tests/vt/test_moe_router_tie_stability.cpp and trace its arm loop through LaunchRouter in src/vt/cuda/cuda_moe.cu and MoeRouterWarpValuesPerThread in src/vt/cuda/moe_router_warp.h. Choose the test-only repair described in the issue, then rerun the GPU sweep and verify that the recorded rows, assertions, and mutation counts reflect independent coverage or explicitly checked redundancy.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- backend, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100