[None][fix] Restore DSv4 NVFP4 routed swiglu_limit on TRTLLM-Gen - #18239
Open
Barry-Delaney wants to merge 4 commits into
Open
[None][fix] Restore DSv4 NVFP4 routed swiglu_limit on TRTLLM-Gen#18239Barry-Delaney wants to merge 4 commits into
Barry-Delaney wants to merge 4 commits into
Conversation
Barry-Delaney
requested review from
Wanli-Jiang,
leslie-fang25,
sunnyqgg and
yechank-nvidia
August 26, 2026 07:31
xxi-nv
approved these changes
Aug 26, 2026
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. WalkthroughThe changes enable per-expert SwiGLU limit tensor construction for TRTLLM-Gen NVFP4 and move NVFP4 parameter normalization into shared quantization processing. TRTLLM-Gen FP8 block-scale remains scalar-only. ChangesDeepSeek V4 MoE quantized processing
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to This localized change restores NVFP4 DeepSeek-V4 routing behavior, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the issue and solution and identifies pre-merge CI as testing. It does not include the full PR Checklist and uses a Testing heading instead of Test Coverage, but the core required information is present.
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Barry-Delaney
force-pushed
the
fix/nvfp4-swiglu-limit-base
branch
from
August 26, 2026 07:38
cc12adc to
80a5ec4
Compare
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #69369 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #69369 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Barry-Delaney
force-pushed
the
fix/nvfp4-swiglu-limit-base
branch
from
August 27, 2026 02:12
80a5ec4 to
c4f265a
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Collaborator
|
PR_Github #69594 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #69594 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #69650 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #69650 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #69667 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #69667 [ run ] completed with state
|
Barry-Delaney
force-pushed
the
fix/nvfp4-swiglu-limit-base
branch
from
August 27, 2026 15:52
c4f265a to
22188cc
Compare
Collaborator
Author
|
/bot run --disable-fail-fast |
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Collaborator
|
PR_Github #69757 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #69757 [ run ] completed with state
|
leslie-fang25
approved these changes
Aug 28, 2026
…DIA#14673) Signed-off-by: Barry Kang <43644113+Barry-Delaney@users.noreply.github.com>
Signed-off-by: Barry Kang <43644113+Barry-Delaney@users.noreply.github.com>
Barry-Delaney
force-pushed
the
fix/nvfp4-swiglu-limit-base
branch
from
August 28, 2026 03:56
22188cc to
8c8f0c1
Compare
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #69871 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #69871 [ run ] completed with state
|
brnguyen2
approved these changes
Aug 28, 2026
brnguyen2
left a comment
brnguyen2
left a comment
Collaborator
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The mechanism checks out: TRTLLMGenFusedMoE._get_quant_method returns NVFP4TRTLLMGenFusedMoEBaseMethod for plain SwiGLU NVFP4 (no swiglu_alpha, not SiTu, activation not Relu2/Silu) — that's the DeepSeek-V4 routed path — and the pre-dequant-domain division lived only in the padded NVFP4TRTLLMGenFusedMoEMethod subclass, so the limit reached the cubin in the wrong domain. Moving it to the base is the right place.
Two asks:
- The guard being removed was justified by measured near-zero accuracy. Please attach before/after accuracy numbers for DSv4 NVFP4 on TRTLLM-Gen (the config that motivated the original guard), not just "pre-merge CI" — CI has no test that exercises this combination. Also, the release notes attached to the PR say pre-merge CI failed; worth confirming that's resolved or unrelated before merge.
- This is a fix for a real accuracy regression that a model split silently dropped; it should carry an NVBug in the title tag rather than
[None], so the same loss is traceable next time.
Open
1 task
Collaborator
|
#18409 depends on this PR. |
xxi-nv
added a commit
to xxi-nv/TensorRT-LLM
that referenced
this pull request
Aug 31, 2026
…checking exact classes The MoE factory decided what a backend could accept by naming it. Fifteen `moe_cls is <Class>` / `moe_cls in [...]` / `isinstance` gates across `create_moe`, `moe_scheduler` and two model files spelled capability as class identity, so a subclass of a supported backend was silently ineligible, and the same eligibility rule lived in the factory, the constructor and a late runtime error, free to disagree. It already did: MEGAMOE_DEEPGEMM reached SiTU only because `activation="situ"` and `activation_type` were two parameters that contradicted each other -- the factory built its problem from the unset `activation_type` default (Swiglu), passed the gate, then handed the constructor `"situ"` -- while the identical MEGAMOE_CUTEDSL request raised. Each backend now declares what it accepts and the factory reads the declaration: - `MoEActivationSupport` (new `activation.py`) states which `ActivationType`s a backend implements and, per constant, the shape its kernel boundary requires (`PER_EXPERT_TENSOR` / `UNIFORM_SCALAR` / `UNSUPPORTED`). The bias and swiglu-parameter allow-lists are gone. - Selection reads that declaration in one place. `MoEProblem` carries `activation_constants` -- which registers the caller actually fills, a question the kind alone cannot answer, since clamped and unclamped SwiGLU share one `ActivationType` but not one set of eligible backends -- and `_reject_unsupported_activation` declines a candidate whose declaration cannot carry them. Previously such a mismatch raised from the adapter at construction, past the point where another candidate could still be chosen. - `MoEStaticCapability.supports_eplb` replaces `supported_load_balancer_backends`, the same shape of list one layer down. `CuteDslB12xFusedMoE` now rejects `eplb_enabled` in `can_implement` instead of failing at construction. - `MoEImplBase.try_fused_route_quant` replaces the `isinstance(moe.backend, TRTLLMGenFusedMoE)` branch in `moe_scheduler`, so a backend opts into the fused route+quant path by overriding it. The hook drops the model name the method carried while it lived on one class, because the contract is generic -- return what `routing_method.apply` and `quantize_input` would have produced, or `None` -- and only the implementation is specialized: the op it calls hardcodes 896 experts, top-16, hidden 3584 and 64 tokens, so the `TRTLLMGenFusedMoE` override still says Kimi K3. The second `isinstance` in that same branch went the other way, to `input_requirement.routing_scales_dtype`. Both replaced a class name, but one asked which dtype the backend reads, which is data a declaration can carry, and this one asks whether a faster path exists, which is behavior only the backend can answer. - Kimi K3's explicit-backend requests pass `allow_backend_degradation=False` instead of asserting on the resolved class, so a silent substitution fails loudly rather than running a different kernel. The `moe_cls in (...)` branches that remain in `create_moe` select which kwargs each constructor takes. They are signature dispatch, not eligibility, and are the factory's job. A declaration is only worth reading if no backend inherits one it never made. `CuteDslFusedMoE`, `DeepGemmFusedMoE` and `MarlinFusedMoE` derived from `CutlassFusedMoE` for constructor reuse while overriding both entry points, so each silently inherited its parent's capability object -- including `supports_expert_bias=True`, which none of them implements and which this change would otherwise have turned from a dormant inconsistency into a selection input. They now derive from `MoEImplBase` and share the constructor through `apply_moe_impl_construction_state`, and every impl declares `capabilities`, `input_requirement` and `activation_support` itself, so no declared value travels by inheritance. `CuteDslB12xFusedMoE` keeps its `CutlassFusedMoE` parent, the one case where that is a real dependency rather than reuse: `_route_to_cutlass` sends every NVFP4 prefill chunk through `CutlassFusedMoE.quantize_input` / `run_moe`, which read the whole Cutlass execution state. This also closes TRTLLM-15649, because the exact-class gates were only needed while one activation was described by several parameters at once: - One carrier. `MoEActivation` (`SwigluActivation`, `SiTuActivation`, ...) holds type, alpha, beta and clamp together, and is what the layer hands to the factory and to selection. `trtllm_gen_activation_type/alpha/beta` and MegaMoE's `activation` string are deleted; `ActType_TrtllmGen` becomes a C++ ABI encoding rather than a second user-facing enum. `build_moe_problem` and `resolve_moe_impl` take only the carrier: their `activation_type` parameter had no caller left, since the kind is derivable and the constants are not. - One clamp, at every layer. `swiglu_limit` (per-expert tensor) and `swiglu_limit_scalar` merge into a single `clamp`, and the materialized view keeps one field too: which ABI a backend gets is already pinned by its declaration, so `materialize_activation_params` renders the shape and no backend chooses between two slots. - Kind-neutral names above the ABI. The installed slots are `act_alpha` / `act_beta` / `act_clamp`, because SiTU fills the same registers the C++ boundary spells `swiglu_*` and every non-SwiGLU backend was renaming it back on arrival. The op-call sites keep each kernel's own schema names. The MegaMoE CuteDSL op, whose signature we own, is renamed to match; DeepGEMM's `situ_beta` stays because `_import_deep_gemm` gates on the installed signature accepting exactly that. - SiTU travels as `ActivationType.SiTu` with alpha/beta, collapsing MegaMoE's two translation tables into the declaration. Gates that meant "FC1 is gated" and were written `== ActivationType.Swiglu` now say `is_gated_activation`, which stops them from rejecting SiTU. The slots are installed once per writer of an input the materialization reads, under the rule that whoever writes that input last re-installs. `apply_moe_impl_construction_state` establishes the invariant for an impl built directly, which unit tests and microbenchmarks do and which never sees the wrapper. `ConfigurableMoE.create_weights` re-installs after applying the layerwise quant config, since `TRTLLMGenFusedMoE` narrows its clamp to a uniform scalar on the FP8 block-scale path and would otherwise keep a tensor clamp that kernel silently ignores; being the last write before `create_weights`, it is also the value every reader in the wrapper path sees. The EPLB slot sync and DWDP's `fixup_moe_backends` re-install because they rewrite `expert_size_per_partition`, which sizes a per-expert constant. `MoEActivationSupport.limit_when_absent` exists because CuteDSL's epilogue always applies the clamp functor: at that boundary "no clamp" is `+inf`, not `None`. A constructor fixup cannot express it, because `ConfigurableMoE` re-installs the activation slots after syncing `expert_size_per_partition` and would overwrite it. `tests/unittest/_torch/moe/test_fused_moe.py` drops the 15 tests that were already permanently skipped as covered by `test_moe_backend.py` and `test_moe_module.py`, together with the six helpers only those tests called -- `run_fused_moe_nvfp4` alone was 240 lines -- and the module-level cloudpickle / `MPI.pickle` bootstrap, which existed so the deleted `MPIPoolExecutor` cases could pickle test functions across ranks. The file goes 2639 -> 466 lines. The three live tests and the two reference classes they use are byte-identical, and no test list or CI stage referenced any deleted name. Tested on GB300 (aga) and B300 at an earlier revision of this change: `test_moe_backend` 363 passed, `test_moe_module` single_gpu 267 / multi_gpu 438 / multi_gpu_eplb 18, `test_kimi_k3_situ_moe` 64 passed + 1 xfailed. multi_gpu gives byte-identical counts on both clusters. Accuracy on B300: `TestNemotronV3Nano::test_fp8`, `TestGPTOSS::test_w4_1gpu`, `TestDeepSeekV4Flash::test_nvfp4_4gpus_static_eplb` and `TestMiniMaxM3::test_nvfp4[use_msa=False]` all pass. Re-verified on GB300 (aga) after rebasing onto origin/main, which is the run that covers the selection-time activation gate, the two re-install points, the single-field clamp and the reparent: `test_moe_backend` 371 + 8 passed, `test_moe_module` single_gpu 278 / multi_gpu 470 / multi_gpu_eplb 18 / DEEPGEMM 14 / CUTEDSL-NVFP4 18 / DENSEGEMM 5, `test_kimi_k3_situ_moe` 69 passed + 1 xfailed, `test_configurable_moe` 2 passed, and the trimmed `test_fused_moe` 9 passed + 64 skipped. Every count is identical to the pre-rebase run of the same matrix, which is the signal that neither the reparent nor the upstream span the rebase pulled in -- the relocation of `_torch/modules/fused_moe` to `_torch/moe/fused_moe` among it -- changed anything observable. The 21 architectural gates in that job all pass, including `fc31_normalization_in_base_class` and `dsv4_swiglu_limit_war_removed`, which are what the two carried commits from NVIDIA#18239 exist to satisfy. `test_configurable_moe` goes 2 failed -> 2 passed relative to the pre-reparent run: its wrapper fixture builds a `ConfigurableMoE` through `__new__` and never set `ep_size`, which `_reject_non_divisible_ep_backend` reads. The accuracy cases run on B300 rather than aga and are covered by the run above. Signed-off-by: xxi <xxi@nvidia.com>
xxi-nv
added a commit
to xxi-nv/TensorRT-LLM
that referenced
this pull request
Aug 31, 2026
…checking exact classes The MoE factory decided what a backend could accept by naming it. Fifteen `moe_cls is <Class>` / `moe_cls in [...]` / `isinstance` gates across `create_moe`, `moe_scheduler` and two model files spelled capability as class identity, so a subclass of a supported backend was silently ineligible, and the same eligibility rule lived in the factory, the constructor and a late runtime error, free to disagree. It already did: MEGAMOE_DEEPGEMM reached SiTU only because `activation="situ"` and `activation_type` were two parameters that contradicted each other -- the factory built its problem from the unset `activation_type` default (Swiglu), passed the gate, then handed the constructor `"situ"` -- while the identical MEGAMOE_CUTEDSL request raised. Each backend now declares what it accepts and the factory reads the declaration: - `MoEActivationSupport` (new `activation.py`) states which `ActivationType`s a backend implements and, per constant, the shape its kernel boundary requires (`PER_EXPERT_TENSOR` / `UNIFORM_SCALAR` / `UNSUPPORTED`). The bias and swiglu-parameter allow-lists are gone. - Selection reads that declaration in one place. `MoEProblem` carries `activation_constants` -- which registers the caller actually fills, a question the kind alone cannot answer, since clamped and unclamped SwiGLU share one `ActivationType` but not one set of eligible backends -- and `_reject_unsupported_activation` declines a candidate whose declaration cannot carry them. Previously such a mismatch raised from the adapter at construction, past the point where another candidate could still be chosen. - `MoEStaticCapability.supports_eplb` replaces `supported_load_balancer_backends`, the same shape of list one layer down. `CuteDslB12xFusedMoE` now rejects `eplb_enabled` in `can_implement` instead of failing at construction. - `MoEImplBase.try_fused_route_quant` replaces the `isinstance(moe.backend, TRTLLMGenFusedMoE)` branch in `moe_scheduler`, so a backend opts into the fused route+quant path by overriding it. The hook drops the model name the method carried while it lived on one class, because the contract is generic -- return what `routing_method.apply` and `quantize_input` would have produced, or `None` -- and only the implementation is specialized: the op it calls hardcodes 896 experts, top-16, hidden 3584 and 64 tokens, so the `TRTLLMGenFusedMoE` override still says Kimi K3. The second `isinstance` in that same branch went the other way, to `input_requirement.routing_scales_dtype`. Both replaced a class name, but one asked which dtype the backend reads, which is data a declaration can carry, and this one asks whether a faster path exists, which is behavior only the backend can answer. - Kimi K3's explicit-backend requests pass `allow_backend_degradation=False` instead of asserting on the resolved class, so a silent substitution fails loudly rather than running a different kernel. The `moe_cls in (...)` branches that remain in `create_moe` select which kwargs each constructor takes. They are signature dispatch, not eligibility, and are the factory's job. A declaration is only worth reading if no backend inherits one it never made. `CuteDslFusedMoE`, `DeepGemmFusedMoE` and `MarlinFusedMoE` derived from `CutlassFusedMoE` for constructor reuse while overriding both entry points, so each silently inherited its parent's capability object -- including `supports_expert_bias=True`, which none of them implements and which this change would otherwise have turned from a dormant inconsistency into a selection input. They now derive from `MoEImplBase` and share the constructor through `apply_moe_impl_construction_state`, and every impl declares `capabilities`, `input_requirement` and `activation_support` itself, so no declared value travels by inheritance. `CuteDslB12xFusedMoE` keeps its `CutlassFusedMoE` parent, the one case where that is a real dependency rather than reuse: `_route_to_cutlass` sends every NVFP4 prefill chunk through `CutlassFusedMoE.quantize_input` / `run_moe`, which read the whole Cutlass execution state. This also closes TRTLLM-15649, because the exact-class gates were only needed while one activation was described by several parameters at once: - One carrier. `MoEActivation` (`SwigluActivation`, `SiTuActivation`, ...) holds type, alpha, beta and clamp together, and is what the layer hands to the factory and to selection. `trtllm_gen_activation_type/alpha/beta` and MegaMoE's `activation` string are deleted; `ActType_TrtllmGen` becomes a C++ ABI encoding rather than a second user-facing enum. `build_moe_problem` and `resolve_moe_impl` take only the carrier: their `activation_type` parameter had no caller left, since the kind is derivable and the constants are not. - One clamp, at every layer. `swiglu_limit` (per-expert tensor) and `swiglu_limit_scalar` merge into a single `clamp`, and the materialized view keeps one field too: which ABI a backend gets is already pinned by its declaration, so `materialize_activation_params` renders the shape and no backend chooses between two slots. - Kind-neutral names above the ABI. The installed slots are `act_alpha` / `act_beta` / `act_clamp`, because SiTU fills the same registers the C++ boundary spells `swiglu_*` and every non-SwiGLU backend was renaming it back on arrival. The op-call sites keep each kernel's own schema names. The MegaMoE CuteDSL op, whose signature we own, is renamed to match; DeepGEMM's `situ_beta` stays because `_import_deep_gemm` gates on the installed signature accepting exactly that. - SiTU travels as `ActivationType.SiTu` with alpha/beta, collapsing MegaMoE's two translation tables into the declaration. Gates that meant "FC1 is gated" and were written `== ActivationType.Swiglu` now say `is_gated_activation`, which stops them from rejecting SiTU. The slots are installed once per writer of an input the materialization reads, under the rule that whoever writes that input last re-installs. `apply_moe_impl_construction_state` establishes the invariant for an impl built directly, which unit tests and microbenchmarks do and which never sees the wrapper. `ConfigurableMoE.create_weights` re-installs after applying the layerwise quant config, since `TRTLLMGenFusedMoE` narrows its clamp to a uniform scalar on the FP8 block-scale path and would otherwise keep a tensor clamp that kernel silently ignores; being the last write before `create_weights`, it is also the value every reader in the wrapper path sees. The EPLB slot sync and DWDP's `fixup_moe_backends` re-install because they rewrite `expert_size_per_partition`, which sizes a per-expert constant. `MoEActivationSupport.limit_when_absent` exists because CuteDSL's epilogue always applies the clamp functor: at that boundary "no clamp" is `+inf`, not `None`. A constructor fixup cannot express it, because `ConfigurableMoE` re-installs the activation slots after syncing `expert_size_per_partition` and would overwrite it. `tests/unittest/_torch/moe/test_fused_moe.py` drops the 15 tests that were already permanently skipped as covered by `test_moe_backend.py` and `test_moe_module.py`, together with the six helpers only those tests called -- `run_fused_moe_nvfp4` alone was 240 lines -- and the module-level cloudpickle / `MPI.pickle` bootstrap, which existed so the deleted `MPIPoolExecutor` cases could pickle test functions across ranks. The file goes 2639 -> 466 lines. The three live tests and the two reference classes they use are byte-identical, and no test list or CI stage referenced any deleted name. Tested on GB300 (aga) and B300 at an earlier revision of this change: `test_moe_backend` 363 passed, `test_moe_module` single_gpu 267 / multi_gpu 438 / multi_gpu_eplb 18, `test_kimi_k3_situ_moe` 64 passed + 1 xfailed. multi_gpu gives byte-identical counts on both clusters. Accuracy on B300: `TestNemotronV3Nano::test_fp8`, `TestGPTOSS::test_w4_1gpu`, `TestDeepSeekV4Flash::test_nvfp4_4gpus_static_eplb` and `TestMiniMaxM3::test_nvfp4[use_msa=False]` all pass. Re-verified on GB300 (aga) after rebasing onto origin/main, which is the run that covers the selection-time activation gate, the two re-install points, the single-field clamp and the reparent: `test_moe_backend` 371 + 8 passed, `test_moe_module` single_gpu 278 / multi_gpu 470 / multi_gpu_eplb 18 / DEEPGEMM 14 / CUTEDSL-NVFP4 18 / DENSEGEMM 5, `test_kimi_k3_situ_moe` 69 passed + 1 xfailed, `test_configurable_moe` 2 passed, and the trimmed `test_fused_moe` 9 passed + 64 skipped. Every count is identical to the pre-rebase run of the same matrix, which is the signal that neither the reparent nor the upstream span the rebase pulled in -- the relocation of `_torch/modules/fused_moe` to `_torch/moe/fused_moe` among it -- changed anything observable. The 21 architectural gates in that job all pass, including `fc31_normalization_in_base_class` and `dsv4_swiglu_limit_war_removed`, which are what the two carried commits from NVIDIA#18239 exist to satisfy. `test_configurable_moe` goes 2 failed -> 2 passed relative to the pre-reparent run: its wrapper fixture builds a `ConfigurableMoE` through `__new__` and never set `ep_size`, which `_reject_non_divisible_ep_backend` reads. The accuracy cases run on B300 rather than aga and are covered by the run above. Signed-off-by: xxi <xxi@nvidia.com>
Resolves the conflict introduced by NVIDIA#17952, which relocated tensorrt_llm/_torch/modules/fused_moe/ to tensorrt_llm/_torch/moe/fused_moe/. That turns this branch's quantization.py hunks into modify-vs-delete. Git resolves it locally via directory-rename detection, but the merge-base..main diff spans 10217 files, past the default diff.renameLimit, so GitHub skips rename detection and reports the PR as unmergeable. Merged rather than rebased so the branch needs no force-push. Signed-off-by: xxi <xxi@nvidia.com>
Addresses review feedback on NVIDIA#18239. process_weights_after_loading in NVFP4TRTLLMGenFusedMoEBaseMethod divided w3_w1_bias / w2_bias by fc31_alpha / fc2_alpha whenever those attributes were not None. Hoisting the block up from NVFP4TRTLLMGenFusedMoEMethod widened it to W4A8NVFP4FP8TRTLLMGenFusedMoEMethod, which auto-creates all-zero bias tensors that its kernel (fp8_fp4_block_scale_moe_runner) never receives. Dividing those by an alpha of zero produces NaN. The same method's _shuffle_all_experts already gates the bias on module.bias, so the two halves of one method disagreed about what "has a bias" means. Gate the division on module.bias instead. On every NVFP4 path the two conditions are equivalent, because create_weights registers the bias parameters only under `if module.bias` and sets them to None otherwise; behavior therefore changes only for the auto-created zero tensors, which are now left alone. Also add CPU-only regression tests pinning what DeepseekV4MoE hands to create_moe: NVFP4 keeps the per-expert swiglu_limit tensor (sized by local slots), while the TRTLLM-Gen FP8 path receives the scalar only. No test covered this, which is how the model split silently dropped the tensor. Signed-off-by: xxi <xxi@nvidia.com>
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Collaborator
|
Addressed both review comments (thanks @brnguyen2) directly on this branch, and 0. The conflict was a path conflict, not a content one. #17952 moved I also checked the one thing a merge like this can silently get wrong: whether 1. Bias gate (
Two notes on the risk assessment, for the record: the specific trigger in the 2. Regression tests (
Each test also asserts the backend the layer actually resolved to, so a I reverse-verified each of the three so none of them can be vacuously green. |
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
6 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.
Description
Cherry-picks #14673 onto main after the original fix landed on feat/deepseek_v4 but was not carried over with the model split.
Testing
Dev Engineer Review
swiglu_limitsupport for TRTLLM-Gen NVFP4.swiglu_beta, andswiglu_limitnormalization intoNVFP4TRTLLMGenFusedMoEBaseMethod.QA Engineer Review
No test changes.