fix(#1312): hold the NemotronH device MoE router pair resident instead of re-uploading it per token - #1348
Open
localai-bot wants to merge 6 commits into
Open
fix(#1312): hold the NemotronH device MoE router pair resident instead of re-uploading it per token#1348localai-bot wants to merge 6 commits into
localai-bot wants to merge 6 commits into
Conversation
…y forward, and counts it `NemotronHMoeBlockDevice` calls `UploadOwned` for `mixer.gate.weight` and for `mixer.gate.e_score_correction_bias` on every forward, and `UploadOwned` ends in `d.b.Synchronize(d.q)`. That is not a formality on this backend: `cuda_backend.cu:110` implements `Synchronize` as `cudaStreamSynchronize`, a host-blocking drain of the whole stream. The released checkpoint has 23 MoE layers, so a decode token pays 46 host->device copies and 46 pipeline drains for two tensors that do not change after load. This commit is the spec and the RED. No product code changes, so the new case FAILS, and it fails for the reason it exists: forwards 2 and 3 each upload the gate and the bias again, and the steady-state host-sync count is 4 rather than 2. A numeric gate cannot see this defect at all, which is why the existing equivalence case stayed green while it was true. The observable is a count, so the new case installs a DELEGATING backend over the real CUDA one -- the arm calls real Marlin kernels, so a fake backend could not run the block -- and counts `Copy` BY SOURCE HOST POINTER and `Synchronize` per forward. Counting by pointer rather than by byte width means no unrelated 4096-byte transfer can be read as the router's. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
…d of re-uploading it per token `Synchronize` is a real drain, so this was serialisation and not only bandwidth. `src/vt/cuda/cuda_backend.cu:110` implements it as `cudaStreamSynchronize(AsStream(q))`, which blocks the host thread until the whole stream has completed. The issue's own refutation clause -- "refuted if Synchronize is a no-op on this backend" -- therefore fails. Only Vulkan makes it a batch flush (`vulkan_backend.cpp:110`) and only the CPU base implementation is the no-op; neither runs this arm. `mixer.gate.weight` and `mixer.gate.e_score_correction_bias` now live in two slots on `NemotronHMoeMarlinResident`, filled on first use and read in place after that. The seam is the one this function already uses for the Marlin arena, and the one `dense_attn_block.h:177` states for every other resident weight in the tree; no new caching mechanism is introduced. At 23 MoE layers the router stops costing 46 host->device copies and 46 full pipeline drains per decode token. The refusal keeps its polarity, which is the part worth reviewing. `UploadOwned`'s four `VT_CHECK`s are now `RequireOwned`, and `ResidentOwned` runs them BEFORE the residency test, so a mis-shaped or absent router weight still refuses by name on every forward rather than on the first one only. The first-use copy keeps its drain: it is one per layer per process, and removing it is a separate argument. `DenseMarlinE1` no longer uploads `std::vector<float> ones(M)`. That buffer was handed to `DBuf`'s constructor, whose copy is async, and the vector then died at the end of the function with the copy possibly in flight -- the use-after-free shape `UploadAs:183-189` documents, uncovered. It is fixed by removal rather than by a drain: the kernel dereferences `topk_weights_ptr` only under `if (mul_topk_weights)` (`marlin_template.h:518` is the only load, `:1879` the only read) and this call site passes `false`, so the tensor is allocated and left unwritten. What remains of that function -- a dense GEMV routed through the grouped-MoE alignment machinery -- is #1341 and is unmeasured, so it is filed rather than guessed at. Corrected in flow, because it sits directly above the changed lines and a reviewer would read it: the lazy-arena comment claimed this block "has no production caller at all". A2-P landed the paged fold, so `ForwardNemotronHForCausalLM` -> `NemotronHPagedForward` (`nemotron_h_registry.cpp:200-202`) -> here is one. The conclusion still stands on its other clause, and the comment now says which. #1346. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
Measured on `dgx:gpu0` (GB10, sm_121a), one `rc run` lease, one build tree for both arms, with `marlin-nvfp4`, `cutlass-nvfp4`, `cutlass-fp8` and `fa2` all ENABLED for `[121a]` and the Triton AOT kernels resolving to `sm_121a`, so the stack is the production one. Per MoE block per forward, steady state: gate uploads 1 -> 0, bias uploads 1 -> 0, host drains charged to the router 2 -> 0. At 23 MoE layers that is 46 copies and 46 drains per decode token, to zero. The answer did not move: worst relative deviation 0 over 128 elements, twice, element count asserted against the geometry. The A3 end-to-end token gate reads `96/96 over 3 prompt(s) (full rows=3, short rows=0, mode=decode)`, `STRICT PASS`, through `include/vllm.h` alone. That is the first green sm_121a leg; `nemotron-h-a2p-paged-forward.md` section 10 records it as the one that remained, and reconciling that record is A2-P's rather than this row's, so it is reported rather than edited. The Outcome also records what was REJECTED, because it is the part a later reader needs. No speed claim is made. The gate decodes 32 tokens in 335.87 s, so 46 drains at the tree's own ~66 us figure is of order 3 ms, roughly 0.03% of a step. #1312 is worth fixing because it is free and because an unsynchronised pageable upload is a correctness hazard, not because it was the decode gap. The next traceable step is the 23 Mamba2 blocks still computing their FP8 projections on the host every token. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
`origin/main` moved nine commits while this row held a GB10 lease. Merged here rather than at push time so the gate below runs on the tree that actually lands. No product overlap: main touched no `nemotron_h*` file, so `src/vllm/model_executor/models/nemotron_h_device.cpp` and its test merge untouched. `.agents/issue-index.md` was the one shared file, and the union driver's auto-merge was DISCARDED rather than trusted, because the index is a keyed record and AGENTS.md says an automatic three-way merge of one is never accepted. Rebuilt instead as main's complete 448-line version with this branch's three rows re-appended, then verified: main's file is a byte-exact PREFIX of the result (`cmp` rc 0), 430 keys on main and 433 after, zero duplicate keys, zero main keys lost, and the three added keys are exactly #1312, #1341 and #1346. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
localai-bot
marked this pull request as ready for review
August 19, 2026 16:35
GitHub does not run the `merge=union` driver `.gitattributes:7` sets, so a branch touching `.agents/issue-index.md` re-conflicts whenever main appends a row. Resolved locally where the driver runs: the union auto-merge was DISCARDED, main's file taken wholesale, only rows whose KEY main lacks re-appended, and main's file asserted a strict PREFIX with zero duplicate keys. Any keyed-table collision kept BOTH sides, main's first, so no row is dropped. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
GitHub does not run the `merge=union` driver `.gitattributes:7` sets, so a branch touching `.agents/issue-index.md` re-conflicts whenever main appends a row. Resolved locally where the driver runs: the union auto-merge was DISCARDED, main's file taken wholesale, only rows whose KEY main lacks re-appended, and main's file asserted a strict PREFIX with zero duplicate keys. Any keyed-table collision kept BOTH sides, main's first, so no row is dropped. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [claude-code]
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.
Closes #1312. Files #1341 and #1346, both fixed-in-flow or owned by this spec.
The question the issue asked first, answered first
Synchronizeis a real drain on this backend, not a no-op.src/vt/cuda/cuda_backend.cu:110-112implements it asCheck(cudaStreamSynchronize(AsStream(q)), "cudaStreamSynchronize"), which blocks the calling host thread until every operation previously submitted to that stream has completed. The issue's refutation clause -- "refuted if the sync count is already near zero, i.e. ifSynchronizeis a no-op on this backend" -- therefore fails, and #1312 is serialisation as well as bandwidth. The no-op is theBackendbase implementation (include/vt/backend.h:42), which suits the synchronous CPU backend; Vulkan overrides it with a batch flush (src/vt/vulkan/vulkan_backend.cpp:110). Neither runs this arm.What was wrong
NemotronHMoeBlockDevicecalledUploadOwnedformixer.gate.weightand formixer.gate.e_score_correction_biason every forward, andUploadOwnedended ind.b.Synchronize(d.q). At the released checkpoint's 23 MoE layers a decode token paid 46 host->device copies and 46 full pipeline drains for two tensors that do not change after load. vLLM has no counterpart to mirror, because it holds the router as annn.Parameterthat already lives on the device.What changed
Both tensors now sit in two slots on
NemotronHMoeMarlinResident, filled on first use and read in place after that. The seam is the one this same function already uses for the 16.5 GB Marlin arena, and the oneinclude/vllm/model_executor/models/dense_attn_block.h:177states for every other resident weight in the tree. No new caching mechanism is introduced.The refusal keeps its polarity, and that is the part to review.
UploadOwned's fourVT_CHECKs are nowRequireOwned, andResidentOwnedruns them BEFORE the residency test. A mis-shaped or absent router weight still refuses by name on every forward rather than on the first one only. The first-use copy keeps its drain: one per layer per process is not what this row is about, and removing it is a separate argument.DenseMarlinE1stops uploadingstd::vector<float> ones(M). That vector was handed toDBuf's constructor, whose copy is async, and then died at the end of the function with the copy possibly still in flight -- the use-after-free shapeUploadAsdocuments at:183-189, uncovered. It is fixed by removal, not by adding a drain: the kernel dereferencestopk_weights_ptronly underif (mul_topk_weights)(marlin_template.h:518is the only load,:1879the only read,:1797and:1880-1884the other branches on the same flag), and this call site passesfalse. The tensor is allocated and left unwritten.One stale comment corrected in flow (#1346), because it sits directly above the changed lines. The lazy-arena justification claimed this block "has no production caller at all (
ForwardNemotronHForCausalLMstill routes to the host reference)". A2-P landed the paged fold, soForwardNemotronHForCausalLM->NemotronHPagedForward(nemotron_h_registry.cpp:200-202) -> here is one, and the cited line numbers point at an unrelated refusal. The conclusion still stands on its other clause and the comment now says which.Not fixed, filed instead: #1341.
DenseMarlinE1still routes a dense GEMV through the grouped-MoE alignment machinery. Nobody has measured what that costs atM = 1, and the obvious alternative is reached through an accessor A2-Q2a deliberately avoids on account of #984, so it is not a one-line swap. It is filed rather than left unremarked.Evidence
All of it from ONE
rc runlease ondgx:gpu0(job75f0121b-ed8a-42cc-a639-e00bfefafb9c, 2026-08-19), one build tree, RED and GREEN from the same configure.The box and the stack, re-derived in the container rather than inherited.
rc-worker-4b8lj, Ubuntu 24.04.4, aarch64, 20 cores, 119 GB, NVIDIA GB10, GPU at 0% on entry.nvccrelease 13.0, V13.0.88 (asserted 13.x, not matched by a wildcard). CUTLASS v4.5.0 ate406c186f. Configure:-G Ninja -DCMAKE_BUILD_TYPE=Release -DVLLM_CPP_CUDA=ON -DVLLM_CPP_CUDA_ARCHITECTURES=121a -DVLLM_CPP_TRITON=ON -DVLLM_CPP_CUTLASS_DIR=...,CONFIGURE_RC=0. The feature lines say this is the production stack and not a degraded one:The three
DISABLEDlines arecutlass-nvfp4-sm100,scaledmm-c3x-sm90andscaledmm-c3x-sm100, each "no requested arch in [121a] provides it". They are other architectures' features, not a lost lever on this one.The RED, at
9d55ef92fThat one case alone,
RED_TC_EXIT=1,1 | 0 passed | 1 failed | 2 skipped,18 | 13 passed | 5 failed:The GREEN, at
e551bfa17, same build treeThat one case alone,
GREEN_TC_EXIT=0,1 | 1 passed | 0 failed | 2 skipped,18 | 18 passed | 0 failed:The deviation is 0 over an element count asserted against the geometry,
REQUIRE( n == T * H ),128 == 128, on both comparisons. The answer did not move.Counts per MoE block per forward, before and after
The two that remain are
UploadAsof the activation andDownloadF32of the result, both artefacts of the test seam.NemotronHPagedForwardkeeps the carry on the device and has neither. At the released checkpoint's 23 MoE layers, per decode token: 46 copies and 46 drains, to 0 and 0.The mutation
Put the per-forward upload back in a scratch copy, one line,
slot = nullptr;immediately before the residency test. This is the check that the case detects the defect rather than merely passing beside it:git diff --statreports the insertion, so the mutation applied; the build returned 0, so the red is the test's and not the compiler's; and the tree restored to an emptygit diffand an emptygit status --porcelainafterwards.The A3 end-to-end token gate, at GREEN, on the released checkpoint
Golden revision
29f2d1746d8f41e316523194b19018707749b1b1, three entries of width 32, against oraclevllm=0.23.1rc1.dev1511+g555967922. Throughinclude/vllm.halone.No speed claim is made, and here is why one would be wrong
No A/B was run, so nothing here is a throughput result. What the numbers do settle is that #1312 was never going to be the decode gap. The gate above decodes 32 tokens in 335.87 s, which is 10.50 s per token. The tree's own figure for a
cudaStreamSynchronizeis ~66 us (deepseek_v4.cpp:203, and that is a DeepSeek-V4 comment, not a measurement taken on this arm), so 46 drains is of order 3 ms, about 0.03% of the step. Removing them is correct and it is free, but it cannot be what a 6.31% decode occupancy is made of.That is not a ceiling and nothing here says the gap is closed. The next traceable step is the one this arm makes obvious rather than the one this row owns: the 23 Mamba2 blocks still compute their FP8 projections on the HOST every decode token, which is #940 / #1289 / #1311, and that is where a 10.50 s token has to be attributed before anything else on this model is worth measuring.
The local gate, and the two reds that are not this branch's
scripts/agent-ready.pyon the merged tree reports two failures, and both were checkedagainst
origin/mainrather than assumed:test_check_gate_commandsfails identically atorigin/main601b576c6: 44 tests,8 failures, and the failing-test sets on the two trees
diffclean. It arrived withmain's own ENG-CUDAGRAPH-BREAK commit, which made
ENG-CUDAGRAPH-BREAKrunnable whileRUNNABLE_BASELINEstill expects it absent. Inherited, subtracted, not chased.test_cpu_x86_llamacpp_floorfails the way#618 records it: the contended leg exits
4 (
NO_QUIET_WINDOW) instead of 2 (GIVING_UP), printingbusy=104% load=25.20at abox loadavg of 27.31. Environmental, and this change touches no CPU path.
Also inherited and worth naming so it is not read as this branch's:
#1371, where main's
369ea7fd4head_size % 8 == 0FA guard turnstest_nemotron_h_paged_forwardred.Earlier in this row a preflight run reported ten record suites red; every one of them was a
tempfile.mkdtempfailure with the root filesystem at 100%, and all passed bare once spacereturned. That is now indexed as
#1353 on main.
How this is tested
A numeric gate cannot see this defect: the answer is identical either way, which is why the existing equivalence case stayed green while it was true. The observable is a count, taken over more than one forward, at
T = 1, which is the decode width the claim is about.The new case installs a DELEGATING
vt::Backendover the real CUDA one -- the arm calls real Marlin kernels, so a backend that owned its own memory could not run the block and the counts would be counts of a simulation -- and countsCopyBY SOURCE HOST POINTER andSynchronizeper forward. Counting by pointer rather than by byte width means no unrelated 4096-byte transfer can be read as the router's. It asserts that forward 1 uploads each tensor once, that forwards 2 and 3 upload neither, that the steady-state drain count is 2 with each of the two named, and that all three forwards returned the same answer over an element count asserted against the geometry.The synthetic NVFP4 fixture is the only operand that reaches this arm without a 21 GiB checkpoint, and its entry point is a host-in/host-out seam. Production reachability of the changed lines is carried by the A3 end-to-end gate, which enters through
include/vllm.halone. Both are run. Neither is sufficient alone.FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5 [claude-code]