Repository navigation
Conversation
… global
`ModelOptNvFp4FusedMoEMethod.apply` resolves the MoE backend twice from two
different sources. `create_weights` caches it as `self._moe_runner_backend`, and
`apply` reads that cache for its error message and for the marlin/megamoe checks,
but the flashinfer dispatch guards use properties that re-read the *global*
`get_moe_runner_backend()`.
Under speculative decoding those disagree. `--speculative-moe-runner-backend`
makes the global the draft model's backend while the target layer's cache is
still the one the user asked for, so every flashinfer branch is skipped and the
method falls through to:
NotImplementedError: Unsupported moe_runner_backend for NVFP4 MoE:
MoeRunnerBackend.FLASHINFER_CUTLASS. Use --moe-runner-backend
flashinfer_cutlass instead.
which names the backend already in use, because the message prints the cache
while the guard read the global.
Reported in sgl-project#38795. Reproduces on nvidia/GLM-5.3-Flash-NVFP4, 8xB300, TP8, with
`--moe-runner-backend flashinfer_cutlass --speculative-adaptive`, at adaptive
CUDA-graph capture (`build_adaptive_runtime_state`). Fixed-step EAGLE with the
same backend is unaffected, which is why this only shows up with adaptive.
Point the three flashinfer guards at the local `moe_runner_backend` the function
already computes, matching the marlin and megamoe checks a few lines above. The
branch taken and the backend reported are now the same value by construction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Could a maintainer add the The diff is 3 lines in Happy to rebase if that helps. |
| # tuple). Defer per-attribute access to the branches that actually | ||
| # consume them. | ||
| activation = self.moe_runner_config.activation | ||
| # Resolved once in create_weights and cached here. Dispatch MUST use this, not |
There was a problem hiding this comment.
Could you make the comment less verbose? Thanks
There was a problem hiding this comment.
Shortened to one line in 3338742, thanks.
Review nit: trim the explanation to one line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nvpohanh
left a comment
There was a problem hiding this comment.
[by Codex] Posting two inline review findings from the revalidated dry-run review.
|
|
||
| # FlashInfer TRTLLM FP4 path | ||
| if self.enable_flashinfer_trtllm_moe and hasattr(layer, "g1_scale_c"): | ||
| if moe_runner_backend.is_flashinfer_trtllm() and hasattr(layer, "g1_scale_c"): |
There was a problem hiding this comment.
[by Codex] Severity: functional | Confidence: High
Issue
This guard no longer recognizes flashinfer_trtllm_routed. ModelOptNvFp4FusedMoEMethod.__init__ deliberately enables the same weight-preparation path for both regular and routed TRT-LLM, and the runner registers FP4 support for the routed backend. With a cached FLASHINFER_TRTLLM_ROUTED backend, this new condition is false, every later backend condition is also false, and apply() raises NotImplementedError.
Fix
Keep using the cached backend, but also accept moe_runner_backend.is_flashinfer_trtllm_routed(). Add a regression case that verifies routed TRT-LLM selects this quant-info branch.
There was a problem hiding this comment.
Fixed in 45ae650: the guard accepts is_flashinfer_trtllm_routed() too, with a CPU regression test for the routed dispatch.
|
|
||
| # FlashInfer TRTLLM FP4 path | ||
| if self.enable_flashinfer_trtllm_moe and hasattr(layer, "g1_scale_c"): | ||
| if moe_runner_backend.is_flashinfer_trtllm() and hasattr(layer, "g1_scale_c"): |
There was a problem hiding this comment.
[by Codex] Severity: style | Confidence: High
Issue
The modified guard retains hasattr, which is not allowed in this code path. It also turns a missing required TRT-LLM preparation field into a misleading unsupported-backend error.
Fix
Declare and initialize g1_scale_c as part of the TRT-LLM layer/setup structure, then select the branch only from the explicit backend predicates and access layer.g1_scale_c directly.
There was a problem hiding this comment.
Dropped the hasattr in 45ae650: the branch selects on the backend predicates only and reads layer.g1_scale_c directly; a layer that skipped the TRT-LLM prep now fails on the missing field.
…ttr guard The cached-backend guard replaced enable_flashinfer_trtllm_moe, which covered both flashinfer_trtllm and flashinfer_trtllm_routed, with is_flashinfer_trtllm() alone. A layer whose cached backend is the routed one then fell through every branch and apply() raised NotImplementedError. The guard now accepts both, as __init__ and the weight prep already do. The hasattr(layer, "g1_scale_c") check goes too. The TRT-LLM weight prep (align_fp4_moe_weights_for_flashinfer_trtllm) always registers g1_scale_c, so the branch is selected on the backend predicates only and reads layer.g1_scale_c directly; a layer that skipped the prep now fails on the missing field instead of reporting the backend as unsupported. Adds a CPU unit test that pins the dispatch: routed and regular TRT-LLM select FlashInferTrtllmFp4MoeQuantInfo, the cached backend wins over the live global, and a missing prep names g1_scale_c. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks, both findings were right. Fixed in 45ae650. Routed TRT-LLM:
Regression test: |
|
/tag-and-rerun-ci |
|
/tag-and-rerun-ci |
|
Thanks for the rerun. On "PR Test Base" it retried only the failed jobs ( |
|
/rerun-failed-ci |
|
Status after the rerun, in case it helps: Base (attempt 2): all 17 Extra: the gate exits by design — MLX / ROCm: both are currently failing on So the two things outstanding are a Base run where the |
|
/rerun-failed-ci |
|
The two red B200 shards aren't test failures — both died in That SHA is the old Everything that picked up a runner before ~12:30Z passed, including the on-device Blackwell coverage this PR cares about:
Also, when you get a chance: could you add |
|
this small change doesn't need extra-ci. You can ignore the extra-ci failures. |
|
/rerun-failed-ci |
|
/rerun-test test/registered/unit/layers/quantization/test_modelopt_nvfp4_moe_dispatch.py |
|
Results for 🚀 |
Motivation
Fixes #38795.
ModelOptNvFp4FusedMoEMethod.applyresolves the MoE backend twice from two different sources, and under speculative decoding they disagree.create_weightscaches it:applyreads that cache for the error message and for the marlin/megamoe checks:but the flashinfer dispatch guards re-read the live global instead:
With
--speculative-moe-runner-backend, the global is the draft model's backend while the target layer's cache is still the user's choice. Every flashinfer branch is skipped and the method falls through to:The message names the backend already in use, because it prints the cache while the guard read the global.
Repro
nvidia/GLM-5.3-Flash-NVFP4, 8×B300 (sm_103), TP8/EP8:Dies at
build_adaptive_runtime_state->decode_cuda_graph_runner->run_moe_core->modelopt_quant.apply. Dropping--speculative-adaptive(fixed-step EAGLE, everything else identical) boots healthy and serves, which is why this only surfaces on the adaptive path.Modifications
Point the three flashinfer guards at the local
moe_runner_backendthe function already computes, matching theis_marlin()andis_flashinfer_megamoe()checks a few lines above:The branch taken and the backend reported are now the same value by construction. A comment at the local records the invariant so the global does not creep back in.
The properties are left in place; they have other callers outside this method.
Checklist
main).CI States
Latest PR Test (Base): ✅ Run #35291501780
Latest PR Test (Extra): ✅ Run #35360849765
Latest PR Test (AMD ROCm 10): ❌ Run #35291501721