Repository navigation
fix(dsv4): support R3 capture and MXFP4 online updates - #39392
Zhichenzzz merged 12 commits into
Conversation
1715a85 to
ecd86bc
Compare
e843e03 to
0d4f00c
Compare
0d4f00c to
23837cd
Compare
Before this PR, the Severity note (verified against the head source): today |
HJSang
left a comment
There was a problem hiding this comment.
LGTM — nice set of fixes, and the comments explaining why each invariant exists make this much easier to review than it could have been.
Things I checked independently and that hold up:
- The shuffle refactor is equivalent against FlashInfer 0.6.18:
shuffle_matrix_a(x, t) == x[get_shuffle_matrix_a_row_indices(x, t)]andshuffle_matrix_sf_a(x, t) == block_scale_interleave(x[get_shuffle_matrix_sf_a_row_indices(x, t)]), withnum_elts_per_sfdefaulting to 16. Folding[w1, w3] -> [w3, w1]into the gather asw3_w1_rows[perm[r]]reproduces the oldshuffle(reorder(w))ordering, andhalfis valid for the scales too since their row count matchesw13.shape[1]. supports_output_viewis a real per-backend attribute inflashinfer/moe_ep(modes/mega_layer.pyplus the cutedsl backends), so gating on it is a genuine fix rather than a guess — the DeepGEMM mega path doesn't advertise it.get_platform().is_sm100resolves todevice_capability_majors=[10], so sm_103 (B300/GB300) passes. Good, given where this was validated.- Every call site of the new
capture_routed_experts_if_allowed(allow_capture, ...)signature is updated, andHashTopKis a plainnn.Module, so theisinstancechain indisable_routed_experts_capture_for_draftis safe. - Merges cleanly onto current
sglang-miles(onlytopk.pyoverlaps the 12 commits since the base).
A few comments, none blocking.
1. Mxfp4FlashinferTrtllmMoEMethod has no restore_weights_before_loading
This is the one I'd most like addressed. process_weights_after_loading now permutes w13_weight / w2_weight in place, which makes it non-idempotent. restore_weight in model_loader/loader.py is the hook for exactly this ("Undo in-place quant packing so fresh weights can be loaded"), and Mxfp4MoEMethod.restore_weights_before_loading implements it for the Marlin path.
Without it, begin_weight_update -> a session that updates only non-MoE tensors -> end_weight_update runs postprocess_weight over already-permuted weights and permutes them a second time, silently. A full-model sync is unaffected, so this doesn't touch the validated recipe — but /begin_weight_update and /update_weights_from_tensor are public endpoints and the name list is caller-controlled. Applying the inverse permutation (argsort(w13_rows) / argsort(w2_rows)) in a restore hook would make the postprocess correct either way.
Related, in test_mxfp4_flashinfer_trtllm_hot_reload.py: the restore_weight(layer, torch.device("cuda")) call and the assertions after it are currently a no-op, since _apply_quant_method_hook does a hasattr check and skips the method. So the "loader hooks bracket every update session; neither may disturb the checkpoint-layout parameters" half of that test isn't actually exercised yet.
2. The *_shuffled split has two consequences worth a comment
- Memory: the fp32 checkpoint-layout scales used to be freed when they were replaced by the shuffled version. Now both layouts stay resident; the fp32 copy alone is roughly a quarter the size of
w13_weight. Necessary for reloadability, just worth stating. - EPLB:
DeepseekV2MoE.get_moe_weightspicks upw13_weight_scale_inv_shuffledonly becausecopy_or_rebind_paramregisters it as aParameterandfilter_moe_weight_param_global_expertadmits anything withshape[0] == num_local_experts. That's load-bearing — if these ever became buffers, a rebalance would update the checkpoint-layout scales and leave the kernel reading stale shuffled ones. A one-line comment pinning that would help.
3. apply() reads the method attribute, not the buffer
gemm1_clamp_limit=self._gemm1_clamp_limit_tensor — but the reason for register_buffer is that the layer's copy is what offload maintains. They agree today because _import_static_state restores in place, but nn.Module._apply (.to(), .cuda()) replaces _buffers[key] outright and would strand the method's reference. Reading layer._gemm1_clamp_limit_tensor matches what the FP8 side already does.
4. _can_fuse_padded_region: x.is_cuda also excludes XPU
On XPU both _is_cuda and _is_npu are False, so a tensor reaches this predicate and now falls back to eager even though Triton works there. The comment says "fall back to eager for host tensors"; not x.is_cpu matches that intent exactly and keeps ROCm and XPU on the fused path.
5. Smaller things
get_quant_method: after the SM100 raise, theis_sm90 or is_sm120branch is unreachable forflashinfer_trtllm_routed, so the comment above it now describes only theflashinfer_mxfp4arm. One check placed just before the TRT-LLM return would cover both backends and remove the dead branch — at the cost of changingflashinfer_mxfp4behaviour on unsupported arches, which may be a separate decision.apply():w13/w2are now unconditionally uint8 views, so... if w2.dtype == torch.uint8 else ...can't take theelse._update_weights_from_flattened_bucket'sreturn True, "Success"is now discarded by the caller._prepare_flashinfer_trtllm_activation_params:existing.fill_(float(value))would skip buildingtensoron the reload path. Alsoexisting is not None and tensor is Nonefalls into theregister_bufferbranch, which is the pointer-orphaning case the in-place branch exists to prevent — unreachable whilemoe_runner_configis fixed for the layer's lifetime, but the condition reads as though it were handled.torch.cuda.empty_cache()at the end ofprocess_weights_after_loadingnow runs per MoE layer on every weight-update session. With the in-place design there's far less to reclaim than before, so it may be worth restricting to the first build.disable_routed_experts_capture_for_draft:elif hasattr(module, "allow_routed_experts_capture")would coverHashTopKand any future router class without the lazy import and without anotherelifnext time.HashTopK.allow_routed_experts_capturecould be a class attribute next to the docstring with the same "draft-side blocks set this False" comment thatTopKConfigcarries; right now the HashTopK side has no explanation of what the flag is for.
6. CI hasn't run on this head
All the check failures are the same gate — Missing required label 'run-ci' — and every suite is skipping. Worth adding the label so the six new test files actually execute before this lands.
14a1fa7 to
6c94e57
Compare
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
…zation Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
d6d7096 to
ec8a0fb
Compare
|
@Zhichenzzz Addressed the flattened-bucket return-value finding in 091bc49: the internal helper now explicitly returns |
Signed-off-by: Bo Li <22713281+bobboli@users.noreply.github.com>
|
@HJSang Follow-up to your review: addressed in 0f0a367.
Validation: 9 B300 GPU cases, 7 HashTopK CPU tests and 5 FP8 ownership/lifecycle CPU tests passed; changed-file pre-commit passed. This is not a full GitHub CI pass. |
Replaces #38282, which GitHub auto-closed when its base branch was deleted and recreated.
Summary
Current rebase (2026-10-02)
Rebased the PR-only commits onto
sglang-milesat6c94e57f9ff5b84ba9746f5983ff13ce7e84c68b. Current head:0f0a367bad0d693ed8af044316111c25d919ed67.091bc49cf3: restore the capture helper's original API, keep device checks at launch sites, validate routed MXFP4 hardware before model construction, and make the flattened-bucket helper's exception-only return contract explicit.ec8a0fbf1c: integrate upstream expert padding and input prequantization with hot reload. Recognize signed packed weights when computing hidden size and clear padding before repeat shuffles. Adapt existing tests to the current loader/runtime-context interfaces.Experiment terminology
Review follow-up (2026-10-02)
0f0a367badadds inverse-shuffle restore for partial MXFP4 updates, live layer-buffer ownership for the clamp, XPU-compatible launch guards, in-place activation constant refresh, and first-build-only allocator cleanup. Empty/weight-only/scale-only sessions failed all six new cases before the restore fix and pass afterward.The source stack also passed an ARM/GB300 preflight: 69 Miles integration tests, 6 QAT GPU tests and 30 SGLang tests (7 HashTopK, 9 MXFP4 GPU, 5 FP8 ownership, 9 UE8M0). Checkpoint metadata, optimizer/scheduler/RNG state and rollout data-state were audited before the pending P2 resume. This preflight is not an end-to-end training pass.
GitHub CI remains gated on
run-ci. The PR author's attempt to add it was denied by GitHub permissions; a maintainer must add the label.Validation of the preceding rebase
git diff --checkpassed.The results below are historical, from earlier heads.
FP8/MXFP8 activation-state follow-up (2026-09-23)
Commit
104716dc91also fixes the TRT-LLM activation tensors introduced in the new base's FP8/MXFP8 path. These alpha/beta/clamp tensors were ordinary attributes, so the memory-saver static-state backup omitted them. Repeated postprocessing also replaced their storage even though CUDA graphs retained the old addresses. Register them as non-persistent buffers and copy into existing storage on reload.Evidence: an isolated reproduction using the actual preparation and static-state export/import functions left the old clamp at zero after simulated storage loss; re-preparation created a different pointer holding 10. The same function-level check passes after the fix (restored values, stable storage, no checkpoint entries). The existing runner-ownership test fixture now also covers this lifecycle.
The prior
23837cdabfsource passed 7 HashTopK tests and the GPU MXFP4 hot-reload test on GB300, but an end-to-end P1 run subsequently exhibited repetitive output and could not collect valid training groups. The activation-state defect is confirmed; attributing the complete rollout failure to it still requires a new RL run. Updated-source integration job completed successfully on GB300: 51 Miles integration tests, 6 GPU MXFP4 QAT tests, and 12 SGLang tests (HashTopK, MXFP4 hot reload, and FP8 runner ownership/activation-state lifecycle). Changed-file pre-commit passed withGITHUB_BASE_REF=sglang-miles. P1 smoke, with all other source/config pins unchanged, collected valid normal-mode samples and completed one valid optimizer step (ppo_kl=0, rollout/train log-prob absolute difference 0.02877). It then failed with host-memory OOM during trainer CPU offload; this is not a three-step end-to-end pass. The firstactor_traintimer was 703.9 seconds, not a steady-state performance measurement. P1 retry subsequently passed all three optimizer steps and exited successfully (1:03:47), with the same source pins and training settings and additional host-memory diagnostics.actor_train: 699.8 / 127.0 / 105.5 seconds;ppo_kl=0for all three steps; train/rollout log-prob absolute difference 0.02852 / 0.02860 / 0.02731. No host or CUDA OOM was observed in this run. This does not establish the earlier host-OOM root cause: the physical nodes and Ray-head/PP-stage placement differed. P2 and P3 both failed during the initial SGLang autotune forward, before online weight sync or training:ValueError: Unsupported trtllm-gen input tensor. The MXFP4 method passed signed checkpoint parameters directly to FlashInfer instead of zero-copy unsigned-byte views; the same issue exists in the pre-rebase PR head. The idle outer launchers were cancelled. SGLang commitf9cfbec2d2fixes that boundary with zero-copy uint8 views, preserving signed load parameters and storage. The new real-kernel case fails with the original exception before the fix; after the fix, all 13 local B300 SGLang tests pass (including real MXFP4 x MXFP8 forward and reload). ARM preflight completed successfully: 70 tests passed (51 Miles, 6 GPU QAT, 13 SGLang), exit0:0. P2 retry and P3 retry usedf9cfbec2d2and subsequently failed at initial FP32 export quantization, as detailed below. The completed P1 run above used SGLang104716dc91; it predates this MXFP4-only follow-up.FP32 export compatibility follow-up (2026-09-23)
P2 retry and P3 retry passed SGLang initialization but failed during initial online weight sync, before any optimizer step: the UE8M0 blockwise-FP8 helper rejected Bridge's FP32 export (
[2048, 4096]shared-expert weight). This is separate from the preceding packed-MXFP4 kernel boundary bug.SGLang commit
becddd92ece674066025b3e132768391b658214aaccepts BF16 or FP32 without changing the quantization math or rounding through BF16. Both new CPU regression cases fail before the fix. After it: 9 CPU tests and 13 related B300 GPU tests pass; the actual failing shape produces byte-identical FP8 weights and packed scales for BF16 weights versus their lossless FP32 exports. Changed-file pre-commit passes withGITHUB_BASE_REF=sglang-miles.ARM preflight completed (
0:0, 4:35): 79 tests passed (51 Miles, 6 GPU QAT, 22 SGLang).Completed direct-HF smoke, without Bridge #36
All three arms completed three valid optimizer steps, including repeated online weight updates and normal-mode rollout, and exited successfully. No host/CUDA OOM was observed in these successful runs; all nine logged
ppo_klvalues are zero, and loss/gradient norms are finite.Pins: Miles P1
946b00a41c, P2/P3f7c4529350; unmodified Bridgebridgebase2e09c234a; Megatron-LMf2fc0efcb. P1 used SGLang104716dc91; P2/P3 usedbecddd92ec, which adds the packed-MXFP4 boundary fix and FP32 UE8M0 input support. P1 was not rerun at the final SGLang head. All code was committed source; no runtime/site-packages patches or offline weight conversion.Setup: Preview, TE MXFP8 training, native/cuDNN attention, 8 x 4 GB300, normal mode, R3 enabled, 4K response cap, eval disabled, GPU optimizer with CPU trainer offload; independent experiment records. P3's routed-expert QAT hook installation was confirmed in the runtime log.
This establishes that Bridge #36 is not required for this tested recipe. It does not establish long-run stability, eval quality, QAT benefit, or the cause of the earlier host-OOM failure. First-step timings include warm-up; the subsequent 105.5–141.3 seconds are below the historical 185-second reference-attention figure but are not a controlled comparison with the historical 82.6-second cuDNN result. This is targeted validation, not a full CI pass.
CI States
Latest PR Test (Base): ❌ Run #37047037453
Latest PR Test (Extra): ❌ Run #37047036885
Latest PR Test (AMD ROCm 10): ❌ Run #37047037268