MODEL-NEMOTRON-H W2 land-prep: the non-gated relu² MoE expert, re-merged onto main and fully re-gated (#517) - #586
Closed
localai-bot wants to merge 11 commits into
Closed
MODEL-NEMOTRON-H W2 land-prep: the non-gated relu² MoE expert, re-merged onto main and fully re-gated (#517)#586localai-bot wants to merge 11 commits into
localai-bot wants to merge 11 commits into
Conversation
…M, not a merged pair (#517) Every grouped-MoE path in this tree is SwiGLU-shaped: a merged gate+up pair with a silu(gate)*up epilogue. NemotronH's expert has no gate half at all -- `ckpt_names=("up_proj", "down_proj", "")` (nemotron_h.py:220 @ 555967922), the empty third entry being the absent gate -- so the expert is h = up_proj(x); h = relu(h)^2; y = down_proj(h) SEAM VERDICT: this is NOT a new merged pair and gets no `MergedGemmGroup` descriptor. `MergedGemmGroup` describes N GEMMs SHARING operand A collapsed into one launch; with N == 1 there is nothing to merge and no launch to save, so an arity-1 descriptor would name a fusion that does not exist. `MlpGateUpMethodBase` is likewise a merged [2I,H] gate_up seam with no pair to hold. The arm is therefore the EXISTING grouped projection plus the activation we did not have -- exactly the shape the gated bf16 archs had before their pair was folded (kMoeGroupedGemmBf16 + kMoeSiluMul). The reasoning is recorded next to the seam it excludes, in merged_gemm.h. No parallel MoE path was added. up : kMoeGroupedGemmBf16 | kMoeGroupedGemmNvfp4Marlin (W4A16 g16) act : kMoeRelu2 <- the only new kernel down : kMoeGroupedGemmBf16 | kMoeGroupedGemmNvfp4Marlin (W4A16 g16) comb : kMoeCombine(..., routed_scale) vt::MoeRelu2 (OpId::kMoeRelu2, appended before kCount; CPU + CUDA) mirrors ReLUSquaredActivation (layers/activation.py:609-628) as the fused-MoE path reaches it: activation_without_mul("relu2") -> MoEActivation.RELU2_NO_MUL (layers/fused_moe/activation.py:33,98) -> `F.relu(input, inplace=True); torch.square(input, out=output)`. The DTYPE ORDER is the mirrored part, not an implementation detail: upstream's kernel (csrc/libtorch_stable/ activation_kernels.cu:673-678) widens to f32, clamps at zero in f32, squares in f32 and rounds ONCE on the store. No new f32 buffer is introduced -- the op reads and writes the caller's dtype and only its arithmetic is f32. routed_scaling_factor goes on the OUTPUT (apply_routed_scale_to_output=True, nemotron_h.py:234), so vt::MoeCombine gained a trailing `routed_scale` (default 1.0f -- every landed caller stays byte-identical, proven by a memcmp test) that multiplies the routed sum BEFORE the shared term is added. That is literally moe_runner.py:389-406 (`fused_output *= routed_scaling_factor`, `shared_output` untouched) then :722-725 (`shared_output + fused_output`). Upstream forces the ROUTER's factor to 1.0 in exactly this case (layer.py:291-300), which is the opposite polarity from Laguna, which folds the same factor into the router weights by linearity (laguna_ops.h:48). group_size=16 NVFP4 -- the spec's named risk -- is SUPPORTED, not emulated: MoeMarlinArgs already defaults to group_size=16 / mxfp4=false, and cuda_moe_marlin.cu:7,115-129 consumes exactly that (group_blocks=1, s_type=kFE4M3fn, num_groups=size_k/group_size); 32 is reachable only via the MXFP4 branch. A test pins the default so a later widening cannot silently re-point these experts. RED first: the new test failed to build on `vt::MoeRelu2 is not a member of vt` and `too many arguments to vt::MoeCombine`. Green after: focused 10/10 cases, 71/71 assertions, Status SUCCESS; clean-tree Release -Werror rebuild 395/395 ctest; Debug arm green on the MoE + op-parity suites (Release is NDEBUG). Mutations executed and caught (restored and re-proven green after each): relu instead of relu^2 (5 cases red), silu instead of relu^2 (5 red), the square narrowed through bf16 before the store (2 red -- the bf16-in/f32-out arm is what sees it; a bf16-out-only test absorbs it), routed scale dropped (2 red), routed scale applied to the combined output including the shared term (2 red), routed scale folded into the router LOGITS (3 cases / 498 assertions red in test_ops_moe_router_grouped), NVFP4 group_size default moved to 32 (1 red). OWED, not claimed: this worktree has no GPU (nvcc absent), so the CUDA arms -- kMoeRelu2 on kCUDA and kMoeGroupedGemmNvfp4Marlin on the real g16 tensors -- are compiled-and-reviewed only. The spec's W2 note records them as owed to a GB10 run. W3/W4 (loader, model file) still own wiring this into NemotronH itself. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
… landed) `main` gained the Mamba2 SSD W1 kernels at `47960a009` (#496), which append three ops to the same `OpId` enum W2 appends `kMoeRelu2` to. Exactly one conflict, and it is mechanical. Resolved by UNION with main's entries FIRST: kMamba2ChunkScan, kMamba2StateUpdate, kRmsNormGatedGroup, <- landed kMoeRelu2, <- this branch kCount Order matters and this is the safe direction: main's three ids are already built against, so putting them first leaves every existing op's id unchanged and gives `kMoeRelu2` the next free slot. The reverse order would silently shift three landed ops. Nothing indexes the enum positionally -- `OpId::kCount` is a sentinel used only to size the provider table (`op_provider.cpp:40`) and registration is by explicit `OpId`, so the union is safe beyond the id-stability argument. FOLLOWING_AGENTS_PROTOCOL Refs #517, #496. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…survived green (#517) Repairs the six findings from the fresh review of `row/MODEL-NEMOTRON-H-W2` @ `e2d68404`. The verdict was PASS; finding 1 is the one that mattered, and it is on exactly the error class the spec names as its top risk. FINDING 1 (the real one). `vt::MoeCombine` scales the ASSEMBLED routed sum: `acc = routed_scale * sum_j w[j]*eo[j]`. The alternative -- folding the factor into each router weight, `acc += (routed_scale*w[j])*eo[j]` -- is equal in exact arithmetic and a DIFFERENT f32 value, because it rounds K times inside the reduction instead of once at the end. Upstream scales the finished tensor (`moe_runner.py:402-406`, `fused_output *= routed_scaling_factor`). That fold SURVIVED the landed suite green -- 10/10 cases, 71/71 assertions -- because every landed case compared with a tolerance and the two forms differ only in the last ulps. It is also the single most likely W4 mistake: Laguna performs exactly that fold (`laguna_ops.h:48`), legally, because Laguna passes no `shared`. Now pinned bitwise, on decimal-grid data whose f32 products carry full mantissas so the orderings actually separate (rows differ by 10 and 4 ULP). A `REQUIRE` asserts the data separates the two forms BEFORE the CHECKs assert which one we implement, so the green cannot be vacuous. The comparison is exact and portable because the project pins `-ffp-contract=off` for every C++ TU (`CMakeLists.txt:42-56`) -- no FMA contraction can make reference and kernel disagree. FINDING 2. The spec claimed the CUDA arms were "compiled-and-reviewed"; they were never compiled by the implementer, whose own handoff says so. Replaced with what is now true and WHO ran it: the fresh REVIEWER compiled and GPU-verified them on dgx.casa (GB10, nvcc 13.0.88) from a `git archive` of `e2d68404` -- Release CUDA build exit 0, 671/671 targets, zero warnings, `cuda_moe.cu.o` under `-Werror=all-warnings`; a reviewer-authored GPU parity test proved `MoeRelu2` CUDA == CPU bit-for-bit over 4097 elements in all four dtype arms, that CUDA `routed_scale` scales the routed sum only, and that the `1.0f` default is byte-identical to the landed 4-arg call across all 8 dtype combinations. Still OWED, and now recorded as owed: `kMoeGroupedGemmNvfp4Marlin` on the real g16 tensors, and the end-to-end MoE block on GB10. FINDING 3. Spec §7 `## Now` said implementation "not started" and "dispatch implementers for W1 and W2" in the same file whose §6a documents W2 as built. Updated to the real state. FINDING 4. Anchor drift, re-verified against the pinned oracle rather than taken on faith -- and the check found one MORE than the review reported: - `apply_routed_scale_to_output=True` :246 -> :234 (spec §2) - `routed_scaling_factor` :232 -> :233 (test header) - `MoEActivation.RELU2_NO_MUL` :33 -> :34 (NOT in the review) - `_maybe_apply_routed_scale_to_output` :389-406 -> :390-407, branch :402-406 `:220`, `:227`, `:98`, `:609-628`, `:672-678`, `:722-725` and `layer.py:291-300` re-checked and correct; left alone. FINDING 5. Upstream's fp16 arm (`:403-406`, divide `shared_output` instead) is genuinely unreachable -- `MoeCombine` gates `out.dtype` through `IsOutFloat` (`ops.cpp:22`), which admits f32/bf16 only. Recorded next to the op AND pinned by a test, so "unreachable" cannot quietly become false: widening `IsOutFloat` to admit `kF16` now REDs instead of silently making an unmirrored upstream branch reachable. FINDING 6. The case named ".../device contract violation" never exercised the device `VT_CHECK`. It does now, in both operand positions. Runnable without a GPU: the wrapper validates devices before `GetOp` dispatches, so a tensor merely LABELLED kCUDA is rejected host-side and never dereferenced. MUTATIONS (Release, each restored and md5-verified afterwards). All 10 RED: M1 relu, square dropped RED 5 cases / 27 assertions M2 silu, the gated family's activation RED 5 / 35 M3 square narrowed through bf16 RED 2 / 20 M4 routed_scale dropped RED 3 / 32 M5 routed_scale on routed + shared RED 2 / 29 M6 routed_scale folded into each weight RED 1 / 4 <- was GREEN before M7 routed scale folded into router logits RED 3 / 498 (router suite) M8 NVFP4 group_size default 16 -> 32 RED 1 / 1 M9 MoeRelu2 device VT_CHECK dropped RED 1 / 2 M10 IsOutFloat widened to admit kF16 RED 1 / 2 M7 does not red the NemotronH file by design: this path forces the router factor to 1.0 (`layer.py:291-300`), so the router's own suite is where that defect is visible. GATES. Clean Release `-Werror`: exit 0, 1207/1207 targets, zero warnings; full `ctest` 401 tests, 400 pass + 1 skip, `test_engine_core_proc` failed under `-j 8` and passes serially (known parallel flake). Debug arm (NDEBUG absent, asserts live): exit 0, 1207/1207, zero warnings, full `ctest` 401/401 pass. `test_ops_moe_nongated_relu2` 10 cases/71 assertions -> 12 cases/81 assertions. `agent-preflight.sh --staged` exit 0. `test_cpu_x86_llamacpp_floor` first failed with `NO_QUIET_WINDOW after 30s (busy=141%)` -- this box was running ~10 concurrent `cc1plus` from other sessions. `scripts/` and `tests/scripts/` are byte-identical to `origin/main` on this branch, so the gate's inputs are untouched by it; re-run on a quiet box (load 5.18) it is 10/10 OK. Contention, not a defect. No CUDA on this host, so the CUDA edits here are comment-only and the CUDA arms remain covered by the reviewer's GB10 run recorded in §6a. FOLLOWING_AGENTS_PROTOCOL Refs #517. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…bility (#517) `main` gained §5a and §5b in `af8170154` while this repair branch was gating: the pinned oracle now demonstrably LOADS AND RUNS Nemotron-3.5-Lightning on GB10, with three greedy goldens committed, and the GGUF k-quant arm is recorded as OWED and tracked as W7. The merge took both sections cleanly, but left §7 `## Now` saying "the oracle smoke run is still queued behind the GPU lock" -- false as of `af8170154`, and the exact defect finding 3 of this repair was raised to remove. Fixing the same class twice in one branch would be a poor look; leaving it would be worse. Per the record rule, main's version was taken wholesale and only the scoped §7 sentence reapplied. §7 now states oracle gateability is CLOSED and carries forward three open items rather than two: the two OWED GPU items in §6a (`kMoeGroupedGemmNvfp4Marlin` on the real g16 tensors, the end-to-end MoE block on GB10) and the OWED GGUF arm (W7, §5b). No lifecycle state changes; the row stays `INVENTORIED`, so this owes no `STATUS`/`BENCHMARKS` write. FOLLOWING_AGENTS_PROTOCOL Refs #517. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
… HfSnapshot before it is declared (#546) `origin/main` @ `8b00f79f2` does not compile. `tests/parity/hf_snapshot.h` defines `parity::Nemotron35LightningSnapshot()` at `:51-57`, which calls `HfSnapshot(...)` at `:52`, but `HfSnapshot` is not declared until `:62`. Introduced by `af8170154` (#517), which inserted the new helper ABOVE `HfSnapshot` instead of below it. Every other snapshot helper in the file sits after the `HfSnapshot` definition, so this is a placement slip, not a design question -- the fix restores the file's own convention. Found while re-gating `row/MODEL-NEMOTRON-H-W2-FIX` after merging `main`. It is NOT this branch's defect, and the check that proves it is one line: the header is byte-identical to `origin/main` here, and it fails to compile ON ITS OWN, with no build system involved: printf '#include "parity/hf_snapshot.h"\nint main(){return 0;}\n' > /tmp/h.cpp g++ -std=c++20 -I tests -fsyntax-only /tmp/h.cpp tests/parity/hf_snapshot.h:52:10: error: 'HfSnapshot' was not declared in this scope 52 | return HfSnapshot("models--nvidia--NVIDIA-Nemotron-3.5-Lightning-30B-A3B-NVFP4", | ^~~~~~~~~~ In a normal configure it surfaces as a failed TU rather than as a header problem, which is what makes it easy to misattribute to whichever branch hits it first: FAILED: tests/CMakeFiles/test_qwen36_weights.dir/vllm/test_qwen36_weights.cpp.o Every target including `parity/hf_snapshot.h` is affected; a full `cmake --build` stops at ~413/1207. Fixed by MOVING the helper and its comment block below the `HfSnapshot` definition. Pure relocation -- 13 insertions, 13 deletions, no semantic change, no revision string or env-override spelling touched. After it, a clean Release `-Werror` build is exit 0, 1207/1207 targets, zero warnings, and full `ctest` is 401/401. SCOPE NOTE, flagged for the reviewer: `tests/parity/hf_snapshot.h` is outside the authority this repair task was given. It is repaired here rather than deferred because AGENTS.md requires a bug found mid-flow to be filed AND fixed in the same flow, and because no gate on this branch -- or any other -- can be run while `main` does not build. Issue #546 was filed before the fix. A reviewer who would rather see this land on its own row should say so and it will be split out. FOLLOWING_AGENTS_PROTOCOL Fixes #546. Refs #517. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…he same parity header (#517) Brings `row/MODEL-NEMOTRON-H-W2-FIX` @ `2b96eaf74` up to `origin/main` @ `fafa16f0f` so the W2 work can go to a fresh re-review on a current tree. ONE conflict: `tests/parity/hf_snapshot.h`. Both sides fix the SAME defect -- `Nemotron35LightningSnapshot()` calling `HfSnapshot` before its declaration, so every TU including the header failed to compile. This branch fixed it at `2b96eaf74` (#546); `main` fixed it independently at `fafa16f0f` (#546/#551, landed as #556). Resolved by taking **main's version wholesale** (`git checkout origin/main -- tests/parity/hf_snapshot.h`), not by keeping both relocations: `git diff origin/main -- tests/parity/hf_snapshot.h` is empty. The two fixes were the same 13-line relocation and differed only in one comment word ("Qwen pins above" vs "below"); main's wording is what stands. Proven by COMPILING, not by a checker sweep -- `scripts/agent-preflight.sh` does not compile the parity TUs, which is exactly how the original breakage reached main: printf '#include "parity/hf_snapshot.h"\nint main(){return 0;}\n' > h.cpp g++ -std=c++20 -I tests -fsyntax-only h.cpp # exit 0 plus a clean Release `-Werror` build of `test_hf_snapshot_pinning`. `.agents/specs/nemotron-h-model.md` is a keyed record and was merged by hand, per AGENTS.md. `main`'s spec is unchanged since `8b00f79f2`, which this branch had already merged, so main's version is carried whole and only this branch's scoped W2 regions differ. Verified section by section (md5 per `##` block): sections 0, 1, 3, 4 (incl. the W7 row), 5, 5a, 5b, 6, 8 and the preamble are BYTE-IDENTICAL to main; the only differences are the branch's own W2 edits -- the §2 `apply_routed_scale_to_output` anchor corrected to `nemotron_h.py:234` (factor `:233`), the new §6a W2 note, and the §7 reconcile. `main`'s other commit (`d861819bb`, DSA top-k) touches `src/vt/cuda/cuda_deepseek_v4.cu` and its test only. Nothing in this merge touches `src/vt/cuda/cuda_moe.cu` or any header it includes, so the W2 CUDA arm compiled and GPU-verified by the fresh reviewer at `e2d68404` is unchanged. FOLLOWING_AGENTS_PROTOCOL Refs #517. Refs #546. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…pec re-merged by hand (#517) Second re-merge of this land-prep: `origin/main` moved from `fafa16f0f` to `72e661ae4` while the W2 gates were running, and one of the two new commits is `1bc5ef82c` (#561), the W1 land-prep for THIS SAME ROW. It edits the same keyed record, exactly as the task anticipated. `.agents/specs/nemotron-h-model.md` was therefore merged BY HAND, per AGENTS.md ("take the target branch version wholesale and reapply your scoped edit; verify unrelated keys byte-for-byte. Never accept an automatic three-way merge of a keyed record"). Git's automatic resolution was DISCARDED -- `git checkout origin/main -- <spec>` first, then the three W2 regions re-applied with uniqueness-asserted anchors. Verified section by section (md5 per `##` block), 13 blocks: BYTE-IDENTICAL to main : preamble, 0, 1, 3, 4, 5, 5a, 5b, 6, 8 differ (W2's own) : 2 (one table row), 6a (new), 7 (Now) §4 now carries main's "W1 progress -- the resolver has LANDED" subsection and the W7 row; §5a (oracle gateability) and §5b (owed GGUF arm) are main's, unchanged. §6a is byte-identical to this branch's own W2 note. §7 is the one place both rows speak, and it is reconciled rather than overwritten: W1 has LANDED at `1bc5ef82c`, W2 is this branch in re-review. The only W2 edit to §2 is the routed-scale anchor the repair pass re-verified against the pin: `apply_routed_scale_to_output=True` at `nemotron_h.py:234`, factor `:233` (main still carried the pre-repair `:246`). Everything else auto-merged with no conflict, and the resulting delta vs `origin/main` is exactly this branch's eight W2 files. `tests/CMakeLists.txt` carries BOTH registrations (`test_modelopt_mixed_precision*` from W1 at :67-80, `test_ops_moe_nongated_relu2` at :1165), and `tests/parity/hf_snapshot.h` is byte-identical to main. No file in `src/vt/cuda/cuda_moe.cu`'s include closure moved -- it includes only `vt/ops.h` plus CUDA/std headers, and `vt/ops.h` is untouched by this merge -- so the CUDA arm the fresh reviewer compiled and GPU-verified on GB10 is unchanged. FOLLOWING_AGENTS_PROTOCOL Refs #517. Refs #561. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…o, spec re-merged by hand again (#517) Third re-merge of this land-prep. `origin/main` moved from `72e661ae4` to `51ec6bed5` while the re-gate was running, and one of the two new commits is `c6b240edd` (#576) -- W3 of THIS SAME ROW, which rewrote 349 lines of the same keyed record. `.agents/specs/nemotron-h-model.md` was merged BY HAND again, per AGENTS.md. Git's automatic resolution was DISCARDED: `git checkout origin/main -- <spec>` first, then the three W2 regions re-applied with uniqueness-asserted anchors. Verified block by block (md5 per `##` heading), **16 blocks: 13 BYTE-IDENTICAL to main, 3 differ and all three are W2's own**: identical : preamble, 0, 1, 3, 4, 5, 5a, 5b, 5c, 5d, 5e, 6, 8 W2's own : 2 (one table row), 6a (new), 7 (Now) That carries main's W3 sections §5c/§5d/§5e whole, §4's three W1 subsections whole, and §5a/§5b whole. §7 is the one place all three Ws speak: reconciled to "W1 and W3 have LANDED, W2 is in re-review", not overwritten. The only W2 edit to §2 is the routed-scale anchor the repair pass re-verified against the pin: `apply_routed_scale_to_output=True` at `nemotron_h.py:234`, factor `:233` (main still carried the pre-repair `:246`). Everything else auto-merged clean, and the delta vs `origin/main` is exactly this branch's eight W2 files. `tests/CMakeLists.txt` carries both W3's `test_nemotron_h_scaffold` (:365) and W2's `test_ops_moe_nongated_relu2` (:1187). `src/vt/cuda/cuda_moe.cu` includes only `vt/ops.h` plus CUDA/std headers, and neither it nor `vt/ops.h` is touched by this merge, so the CUDA arm the fresh reviewer compiled and GPU-verified on GB10 at `e2d68404` is unchanged. FOLLOWING_AGENTS_PROTOCOL Refs #517. Refs #576. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…pin gate and the intake record (#517) Fourth re-merge of this land-prep; `origin/main` `51ec6bed5` -> `751325460`. Both new commits are clean auto-merges and neither collides with W2: - `751325460` (#579) re-pins the Nemotron checkpoint by CONTENT in `tests/parity/hf_snapshot.h` and `tests/parity/test_hf_snapshot_pinning.cpp`. That is the same file this branch resolved in its FIRST merge, so it is re-checked rather than assumed: `git diff origin/main -- tests/parity/hf_snapshot.h` is EMPTY, i.e. main's version stands whole, and the branch contributes nothing to it. - `ce7071122` (#580) is a records-only intake placement. `.agents/specs/nemotron-h-model.md` is unchanged on main across this pair, so the hand-merged version from the previous commit still holds; re-verified block by block anyway -- 16 blocks, 13 BYTE-IDENTICAL to main, and the 3 that differ are W2's own (§2's one table row, the new §6a, §7 `Now`). The delta vs `origin/main` remains exactly this branch's eight W2 files, and `src/vt/cuda/cuda_moe.cu` and `vt/ops.h` are untouched by this merge, so the GPU-verified CUDA arm is still unchanged. FOLLOWING_AGENTS_PROTOCOL Refs #517. Refs #579. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…ver correction (#517) Fifth re-merge; `origin/main` `751325460` -> `65625f34a` (#582), a records-only commit that rewrites §2 of the SAME keyed record this branch also edits in §2. Source and tests are untouched by it, so no gate is invalidated. `.agents/specs/nemotron-h-model.md` was merged BY HAND again per AGENTS.md: `git checkout origin/main -- <spec>` first, discarding git's automatic resolution, then the three W2 regions re-applied with uniqueness-asserted anchors. Verified block by block (md5 per `##` heading) -- **16 blocks, 13 BYTE-IDENTICAL to main, 3 differ and all three are W2's own**: §2's single table row (`apply_routed_scale_to_output=True` at `nemotron_h.py:234`, factor `:233`), the new §6a, and §7 `Now`. Main's rewritten §2 PROSE is carried whole -- the W2 edit is one row of the anchor table above it and does not touch the resolver narrative #582 corrected. Delta vs `origin/main` remains exactly this branch's eight W2 files. FOLLOWING_AGENTS_PROTOCOL Refs #517. Refs #582. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
localai-bot
pushed a commit
that referenced
this pull request
Aug 13, 2026
… record and comment (#517) Fresh review of PR #586 (`row/MODEL-NEMOTRON-H-W2-LAND` @ `f6a7f8709`) returned PASS -- the code is correct and the previously-unreviewed repair delta changes zero executable lines. Its four findings were record and comment accuracy. Each was re-verified against the file or the measurement before being changed; none was taken on the reviewer's word. F3 -- the `:33` -> `:34` anchor fix had landed in 1 of 3 places. At the pinned oracle (`5559679229bc`, `vllm/model_executor/layers/fused_moe/activation.py`, verified byte-identical to the pin with `git diff 5559679229bc -- <file>`), `:34` is `RELU2_NO_MUL` and `:33` is `GELU_TANH_NO_MUL`. Fixed at `include/vt/ops.h:1669` and in §6a of the spec; `:98` (`activation_without_mul`) and `:184` (`apply_moe_activation`'s branch) were re-checked and are correct, and `tests/vt/test_ops_moe_nongated_relu2.cpp:12` already carried the right numbers, so it is unchanged. F4 -- §6a's M7 evidence did not reproduce. Recorded `RED 3 / 498`; re-measured here as **3 cases / 525 assertions** failing out of 14 / 941, agreeing with the review's and the operator's independent reproductions. Third identical measurement, so the count is stable and 498 was a transcription slip, not drift: `tests/vt/test_ops_moe_router_grouped.cpp` and the grouped-router kernel have not moved since `e2d68404`. Mutation: fold `routed_scaling_factor` into the logits fed to sigmoid/softmax (`cpu_ops.cpp:2325-2339`) and disable the post-renormalize weight scale (`:2430-2434`); tree restored and md5-verified (`cd409b9465c00834be373cf3ecfb4c1d`), rebuilt, back to 14/941 SUCCESS. F5 -- §4 contradicted §6a. §4's W2 row still described the arm as going "through `MlpGateUpMethodBase` / `vt::MergedGemmGroup`" -- the plan §6a correctly refutes, since a non-gated expert has no pair to merge and an arity-1 descriptor would name a fusion that does not exist. §4's Content cell now records that the planned routing was refuted during implementation and points at §6a as the authority; its Gate column was already satisfied and is untouched. F6 -- an overstated bit-identity comment. `cuda_moe.cu` claimed the combine "stays bit-for-bit equal" between CPU and CUDA. Nothing sets nvcc `--fmad=false`: `CMakeLists.txt` pins `-ffp-contract=off` for CXX (`:55`), HIP (`:393`) and OBJCXX (`:467`) only, and `:41-56` carves CUDA out deliberately ("GPU parity tests compare GPU-vs-GPU"). Device code may therefore contract the `acc += w * Load(...)` reduction into an FMA where the host may not -- this project has already observed nvcc doing exactly that (`.agents/benchmark-record.md:532`). The comment is narrowed to what holds: the `routed_scale` step is one standalone multiply on the finished accumulator with nothing to contract into, and `MoeRelu2` is a compare-select plus one multiply, so both are bit-identical by construction; the reduction is not covered. The flag gap itself is repo-wide and belongs to **#591**, not to this row; the pre-existing `cuda_moe.cu:467` claim about that same reduction is main's line and is left for #591. Also corrected in the record: four of this branch's land-prep merge messages inferred "so the CUDA arm GPU-verified at `e2d68404` is unchanged" from "`vt/ops.h` is untouched by this merge". Each per-merge half is true (`git diff --shortstat <merge>^1 <merge> -- include/vt/ops.h` is empty for `57e4301489`, `40908db153`, `6ac8eed85e`, `721f44d38d`, `f6a7f87090`) but the conclusion is false: since `e2d68404`, `include/vt/ops.h` has moved **+180/-1** and `src/vt/cuda/cuda_moe.cu` **+7/-1**, the header movement being `d3cd442e6`, the Mamba2 SSD W1 merge, which touched `ops.h` by +179 lines against its first parent. §6a now records that, and names the PR's `cuda-fat-build` CI job as the standing evidence for the CUDA arm at this head. No dgx recompile was attempted: `dgx.casa` is unreachable. EXECUTABLE-LINE DIFF IS EMPTY. `git diff -U0 -- include src`, filtered of comment and blank lines, produces nothing; the whole delta is comments and the spec. The ten mutations therefore cover unchanged code and were not re-run, except M7, which is the finding. Gates (CPU box, no GPU; `df -h` 67G avail throughout, so no silent `No space left on device`): - clean-tree Release rebuild from an empty build dir: exit 0, **414 targets, ZERO warnings** (`-Werror` on by default per `cmake/CompilerWarnings.cmake`). - `test_ops_moe_nongated_relu2` 12 cases / 81 assertions, Status SUCCESS. - `test_ops_moe_router_grouped` 14 / 941, Status SUCCESS. - `test_hf_snapshot_pinning` 11 / 42, Status SUCCESS. - Full `ctest -j4`: **100% tests passed, 0 failed out of 404** (skipped: `test_modelopt_mixed_precision_checkpoint`, `test_voxtral_e2e` -- neither has its asset here). Re-run on the clean rebuild, same result. - `scripts/agent-preflight.sh --staged`: every gate ok except `commit-trailers` on `656b04dba7fd`, which is the pre-existing merge-commit trailer shape this branch inherits and which the squash-merge resolves (review finding F2, the operator's landing decision, deliberately not repaired here). FOLLOWING_AGENTS_PROTOCOL Refs #517. Refs #591. Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
Collaborator
Author
|
Superseded by #593 — same content, standalone off current |
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
2 participants
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.
Land-prep for W2 of
.agents/specs/nemotron-h-model.md— the non-gatedrelu²MoE expert. Issue #517. The underlying work already passed a freshreview at
e2d68404(PASS, CUDA arm compiled and GPU-verified bit-for-bitagainst CPU on
dgx.casa), anddd7a6477drepaired that review's six findings.This branch is the re-merge and re-gate only — no redesign, no new design
decisions.
What is in the diff
Exactly eight files, all W2's own:
The re-merges
origin/mainmoved five times under this branch during the re-gate,including three commits on this same row (#561 W1, #576 W3, #582 §2), so there
are five merge commits. Two things needed care:
tests/parity/hf_snapshot.h. The branch tip2b96eaf74fixed the sameNemotron35LightningSnapshot()-before-HfSnapshotbreakage that main fixedindependently at
fafa16f0f(#556). Resolved by taking main's versionwholesale, not by keeping both relocations.
git diff origin/main -- tests/parity/hf_snapshot.his empty. Proven by compiling, because
agent-preflight.shdoes not compilethe parity TUs — that gap is how the original breakage reached main:
from
8b00f79f2still fails on the same command, so the check is armed)test_hf_snapshot_pinningbuilt and run in both arms: 11 cases / 42assertions
Status: SUCCESS.agents/specs/nemotron-h-model.mdis a keyed record and was merged BY HANDevery time, per AGENTS.md — git's automatic resolution discarded,
git checkout origin/main -- <spec>first, then the three W2 regions re-appliedwith uniqueness-asserted anchors. Verified block by block (md5 per
##heading): 16 blocks, 13 byte-identical to main, 3 differ and all three are
W2's own — §2's single anchor row, the new §6a, and §7
Now. Main's §4 (threeW1 subsections), §5a, §5b and §5c/§5d/§5e (W3) are carried whole.
Gates — all re-run on the final merged tree
Disk is recorded next to each number because this box hit 100% twice today and
an ENOSPC build leaves the previous binary in place, which then prints
Status: SUCCESSover code that never compiled.-Werrorctest(Release)ctest(Debug)test_ops_moe_nongated_relu2test_ops_moe_router_groupedtest_hf_snapshot_pinningagent-preflight.sh --stagedcommit-trailers(below)test_engine_core_procandtest_async_llmeach failed once underctest -j8and passed serially and under
-j4— the known parallel starvation flake, nota defect.
Mutations — 10/10 caught, run three times
Every mutation is applied with a uniqueness-asserted anchor, rebuilt, run, then
restored with an md5 assert. Identical counts on the Release arm, the Debug arm
(asserts live), and the final merged tree:
relu(square dropped)silu(gated family's activation)test_ops_moe_router_groupedMoeMarlinArgs group_size16 → 32MoeRelu2deviceVT_CHECKdroppedIsOutFloatwidened tokF16M6 is the one the repair exists for: it survived green at
e2d68404becauseevery landed case compared with a tolerance. It is now pinned bitwise.
M7's "by design" claim was cross-checked rather than asserted: the same logits
fold measured against the NemotronH suite is 12/12 SUCCESS, so it is
genuinely invisible there (this path forces the router factor to 1.0,
layer.py:291-300) and visible only in the router's own suite.CUDA arm: no recompile needed, with evidence
src/vt/cuda/cuda_moe.cuincludes onlycuda_bf16.h,cuda_runtime.h,<stdexcept>,<string>andvt/ops.h. Across all five merges, neithercuda_moe.cunorvt/ops.hwas touched by anything coming from main(
git diff <pre-merge> <post-merge> --name-onlyshows no hit). Since thereviewer's GB10 compile at
e2d68404, this branch's only change tocuda_moe.cuis a comment block (git diff e2d68404c HEAD -- src/vt/cuda/cuda_moe.cuis 6 added comment lines and 1 changed comment line). No codegen input moved.
One gate is RED, and it is inherited — needs a decision
commit-trailersfails on656b04dba, an inherited merge commit from therepair branch whose message is git's default
Merge remote-tracking branch ...with no
FOLLOWING_AGENTS_PROTOCOLand no trailers. The checker exempts nothing,merge commits included.
It was invisible until now: the preflight trailer gate is guarded by
git merge-base --is-ancestor origin/main HEAD(agent-preflight.sh:218-224),so on a branch behind main it never runs. Bringing this branch up to date is
what first exposed it.
It cannot be repaired without rewriting a published commit's message and
replacing the remote branch, which this task explicitly forbade, so it is left
for a decision rather than done unilaterally. Two ways out:
656b04dba, and its own preflightstays green. Nothing else needed.
filter-branch --msg-filtertouching only thatone commit's message (trailer text already drafted), then replacing the
branch ref.
A local
git merge --no-ffwould carry656b04dbainto main's history and turnthis gate red for everyone, so that route needs option 2 first.
FOLLOWING_AGENTS_PROTOCOL
Refs #517.