Repository navigation
Conversation
cf8ef21 to
a30d34a
Compare
Kh4L
left a comment
Kh4L
left a comment
There was a problem hiding this comment.
Self-review (/review-pr-team), per the contributor self-review policy
Scoped to this PR's own increment (be5015649..8f6003694 as reviewed — the mxfp8 commit plus its ruff-format fix). The two layers below have their own posted self-reviews and were not re-reviewed.
The headline: the bf16 default path is a strict no-op and is safe, but the mxfp8 path could not complete setup() on any config in this repo, and on the model this work targets it quantized the MoE router. Four findings are fixed in a30d34a; six are left inline for your judgement because each needs a design decision I did not want to make unilaterally on someone else's design.
Fixed in a30d34a
- MoE routers were being quantized.
SKIP_WEIGHT_SUBSTRINGSheld"router"and"mlp.gate.", which between them miss most router spellings. A top-k router is a discrete argmax, so quantization error flips expert selection — a rollout/trainer policy divergence, the one failure an RL run can't absorb. Widened to.gate.+shared_expert_gate., coveringmlp.gate.(Qwen3-MoE, DeepSeek),block_sparse_moe.gate.(Mixtral),mixer.gate.(NemotronH/nanov3) and Qwen2-MoE's separate gate, while leavinggate_proj/gate_up_proj/shared_expert.gate_projquantized. The repo already draws this line inDEFAULT_NVFP4_IGNOREand the nanov3 recipe'sreal_quant_ignore. CONVERTER_VERSION→ 5, mandatory given (1)._quantization_fingerprinthashes the config knobs but not the module-level selection constants, so changing the skip list without bumping it silently reuses a cached checkpoint whose router is already quantized — while the refit, now skipping it, sends BF16 into a slot the engine loaded as MXFP8. See the inline comment on the fingerprint for the durable fix.convert_mxfp8requiredmodel.safetensors.index.json. That's every shipped SGLang recipe: all five run Qwen2.5-Math-1.5B-Instruct, which ships a singlemodel.safetensorsand no index._checkpoint_weight_namesin the same module already handled both layouts; the converter now takes the same branch.test_quantized_refit_requires_megatron_backendasserted nothing — only the two non-raising cases. Neutering the guard to a barereturnleft all 17 tests green. Plusruff checkfailures (two unused imports, three import-ordering) that the earlier ruff-format commit didn't cover butlint-checkwould have caught, and a 2025 copyright header on two files added in a 2026 commit.
Process disclosure
The adversarial pass ran but was unreliable to reach; several rounds produced nothing before it reported. It ultimately delivered, and it earned its place — it overturned my own fix. My first version of finding (1) enumerated block_sparse_moe.gate. and shared_expert_gate., which still left backbone.layers.N.mixer.gate.weight quantized — the router of NVIDIA-Nemotron-3-Nano-30B-A3B, the model this work targets. It caught that by fetching the real checkpoint's safetensors header and running should_quantize against the actual tensor names and shapes. The committed fix is its .gate. widening, not mine.
It also corrected the review in three other directions worth recording: it disputed a confidence-90 finding of chained .get() at grpo.py:1402 as a style note on code matching pre-existing house style exactly (dropped); it downgraded a coverage claim after showing three independent hand-rolled measurements disagreed on the denominator (so no percentage appears below); and it found one agent's cited test artifact did not exist, so that agent's numbers are not cited here.
Environment caveats
No GPU and no sglang/flashinfer installed, so nothing involving SGLang's or flashinfer's runtime behaviour was executed by anyone. Findings are marked traced-vs-executed inline. pytest could not run the repo suite (uv.lock is Linux-only on this machine); the new test file's assertions were verified by executing the real should_quantize against them, and the file will collect normally in CI, where transformers is a core dependency.
What would take this from "unexercised" to "shippable"
The PR body is candid that no mxfp8 run exists. In rough order of cost:
- One smoke run — GRPO + Megatron + SGLang +
scheme: mxfp8, 2–3 steps, on a sharded model. This alone exercises everything fixed above. - Round-trip correctness — per-token logprob agreement between the mxfp8 SGLang rollout and the Megatron policy on a fixed prompt batch. The existing sglang recipes already gate on
median(token_mult_prob_error) < 1.1, so the same number under mxfp8 vs the bf16 baseline is the natural A/B with no new tooling. - ≥3 refits, not 1, with the importance-sampling ratio logged per step — drift from quantizing during refit only shows after several updates.
- Throughput and memory A/B, bf16 vs mxfp8, same model/TP/batch. Without this there is no stated benefit.
- A run with
num_layers_at_start_in_bf16 > 0where the emittedconfig.jsonis inspected — the cheapest possible check, and it would have caught the layer-band issue flagged inline.
No docs/ page accompanies seven new user-facing config keys. docs/fp8.md is the natural home — it already documents the vLLM FP8 story including the equivalent layer-band knobs.
Generated by Claude Code
fdd2c03 to
84713d6
Compare
84713d6 to
f60ca44
Compare
github-actions
Bot
added
the
Documentation
f60ca44 to
3c88ad3
Compare
|
/ok to test 3c88ad3 |
3c88ad3 to
2e2bffe
Compare
Kh4L
added
the
CI:L2
|
/ok to test 2e2bffe |
|
/ok to test b7a6731 |
|
/ok to test 532df8f |
532df8f to
bfa0eaf
Compare
|
/ok to test bfa0eaf |
|
/ok to test |
@Kh4L, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test 54e9be4 |
|
/ok to test f777ea0 |
|
Current head |
f777ea0 to
7f9f520
Compare
|
/ok to test 7f9f520 |
Enable scheme=mxfp8 under policy.generation.sglang_cfg.quantization: - mxfp8_setup.py: offline HF->MXFP8 checkpoint conversion with cache fingerprinting (ensure_mxfp8_checkpoint), run before SGLang boot - mxfp8_quantization_core.py: quantize_mxfp8 (flashinfer), dynamic skip-substring policy, scale-key helpers shared by offline conversion and online refit - MegatronSGLangHfWeightIterator: quantize eligible tensors during the AutoBridge walk, emitting (weight, weight_scale_inv) pairs sized into post-transformation buckets - wire target_precision/sglang_quantization_cfg through the megatron refit paths; SglangQuantizationConfig type + exemplar YAML block Signed-off-by: zhihaow6 <zhihaow6@illinois.edu> (cherry picked from commit d0cf391)
Same gate as the equivalent commit lower in this stack: the mxfp8 quantization modules and their tests fail `ruff format --check` under the version pinned in .pre-commit-config.yaml (v0.9.9), which breaks lint-check and therefore the whole pipeline. Signed-off-by: Serge Panev <spanev@nvidia.com>
…ts, pass ruff Four self-review findings on the mxfp8 increment. `SKIP_WEIGHT_SUBSTRINGS` held "router" and "mlp.gate.", which between them miss most MoE router spellings. A top-k router is a discrete argmax, so quantization error flips expert selection outright -- a rollout/trainer policy divergence, which is the one failure an RL run cannot absorb. Widened to `.gate.` plus `shared_expert_gate.`, which covers the class rather than the families we happened to notice: `mlp.gate.` (Qwen3-MoE, DeepSeek), `block_sparse_moe.gate.` (Mixtral), `mixer.gate.` (NemotronH/nanov3) and Qwen2-MoE's separate `shared_expert_gate`. The dots are load-bearing -- `gate_proj`, `gate_up_proj` and `shared_expert.gate_proj` are ordinary MLP weights and stay quantized. Worth stating why the narrow version of this fix was wrong: it enumerated `block_sparse_moe.gate.` and `shared_expert_gate.`, which left `backbone.layers.N.mixer.gate.weight` -- the router of `NVIDIA-Nemotron-3-Nano-30B-A3B`, the model this work targets -- still quantized. Verified against that checkpoint's real tensor names and shapes, and the repo already draws the same line in `DEFAULT_NVFP4_IGNORE` and the nanov3 recipe's `real_quant_ignore`. `CONVERTER_VERSION` goes to 5 because of that skip-list change. The cache directory hashes the config knobs via `_quantization_fingerprint`, but not the module-level selection constants, so without the bump a checkpoint converted under the old list would be reused with its router already quantized -- while the refit, now skipping it, sends BF16 into a slot the engine loaded as MXFP8. `convert_mxfp8` opened `model.safetensors.index.json` unconditionally, so any checkpoint whose BF16 weights fit in one file died with a bare FileNotFoundError. That is every shipped SGLang recipe: all five run Qwen2.5-Math-1.5B-Instruct, which ships a single `model.safetensors` and no index. `_checkpoint_weight_names` in the same module already handles both layouts; the converter now takes the same branch and derives its shard list from the directory when there is no index. `test_quantized_refit_requires_megatron_backend` called only the two non-raising cases, so the branch the guard exists for was never exercised -- neutering the function to a bare return left the whole suite green. The rejecting case is now its own test. Also: `ruff check` was failing on the increment (unused `Iterable` and `pytest` imports, three import-ordering violations). The earlier `chore(sglang): satisfy the repo-pinned ruff formatter` commit only covered `ruff-format`, so `lint-check` -- which the whole pipeline `needs` -- would still have failed. And two of the three new modules carried a 2025 copyright header in a 2026 commit. Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
7f9f520 to
5babc80
Compare
|
/ok to test 5babc80 |
Signed-off-by: Serge Panev <spanev@nvidia.com>
Signed-off-by: Serge Panev <spanev@nvidia.com>
|
/ok to test 321e188 |
Original work: #3188 by @xiuhu17. Re-cut at his request; the original authorship and sign-offs are preserved.
Add MXFP8 SGLang rollouts with Megatron weight refit. Startup and refit share precision exclusions and weight/scale grouping; MoE router weights remain BF16. Startup requires a local checkpoint produced by the explicit offline GPU converter.
#3613 is merged. This PR contains the MXFP8 increment against main; #3615 builds on it.
Current main refresh
Head
321e18808b8b249ef2ae15f015a51262fdfbdddbincorporates maincfd9de9902bde17e93d6f7514b0789b907606a13. The merge is clean. The quantization core, checkpoint-selection and grouping modules are unchanged from the previous PR head. Dependency files and submodule pins match main.Historical GPU evidence: HSG job
6588661completed four Nemotron-3-Nano optimizer steps and four MXFP8 broadcast refits on three nodes / 12 GPUs, with decode CUDA graphs and the expected 2688-to-2816 hidden-size padding. That run predates this main refresh and does not validate the current head.Fresh CI is requested for the current commit. Maintainer review remains required.