[https://nvbugs/6473374][fix] Hoist the isinstance check out of the helper at the single call site in… - #16637
[https://nvbugs/6473374][fix] Hoist the isinstance check out of the helper at the single call site in…#16637trtllm-agent wants to merge 2 commits into
Conversation
|
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 (1)
💤 Files with no reviewable changes (1)
WalkthroughChangesMLA slicing behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/bot run --only-qa-verify test accuracy/test_llm_api_pytorch.py::TestDeepSeekV3Lite::test_nvfp4[moe_backend=CUTLASS-mtp_nextn=2-fp8kv=False-attention_dp=False-cuda_graph=False-overlap_scheduler=False-torch_compile=True] |
|
PR_Github #60527 [ run ] triggered by Bot. Commit: |
|
PR_Github #60527 [ run ] completed with state |
BowenFu
left a comment
There was a problem hiding this comment.
LGTM. The hoisted plain-tensor path (mla.py:1422) is byte-identical to the helper's non-Fp4 branch (mla.py:95); FP4 callers still route through the helper, and the change only moves the FX-graph call boundary. Waiver change is a removal. Codex-confirmed no other executable callers of the helper.
| # boundary otherwise shifts multi-stream sync-event insertion around the | ||
| # piecewise CUDA graph regions, producing an async CUDA IMA at sampler | ||
| # event synchronize on SM120 + torch_compile + MTP=2 (nvbugs/6473374). | ||
| if isinstance(hidden_states, Fp4QuantizedTensor): |
There was a problem hiding this comment.
The explanation does not make sense. The _slice_hidden_states_to_num_tokens already shortcut the non-FP4 tensor path. And the forward_impl is inside the MLA custom op and not visible for fx graph.
422d191 to
c9a9257
Compare
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part.
c9a9257 to
c5638a1
Compare
3098f1d to
c61dacb
Compare
|
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. |
brnguyen2
left a comment
There was a problem hiding this comment.
The code change is a no-op semantically — the helper's non-FP4 arm is already hidden_states[:num_tokens, ...], and Dynamo inlines plain Python functions, so "the function-call boundary changes FX node identity" needs evidence (a before/after graph dump or event ordering) rather than assertion. More concretely, the waiver removals are broader than the stated mechanism: several removed entries are torch_compile=False (and one cuda_graph=False) configs, which a torch.compile/piecewise-graph node-ordering explanation cannot account for. Please either show those configs passing on RTX PRO 6000, or keep their waivers and narrow this to the torch_compile=True cases.
| @@ -298,24 +298,16 @@ full:RTX_6000D/accuracy/test_llm_api_pytorch.py::TestQwen3_5_35B_A3B::test_fp8[e | |||
| full:RTX_6000D/accuracy/test_llm_api_pytorch.py::TestQwen3_5_4B::test_dflash SKIP (https://nvbugs/6273850) | |||
| full:RTX_6000D/accuracy/test_llm_api_pytorch.py::TestQwen3_5_4B::test_fp8 SKIP (https://nvbugs/6273850) | |||
| full:RTX_PRO_6000_Blackwell_Server_Edition/accuracy/test_disaggregated_serving.py::TestDeepSeekV3Lite::test_auto_dtype[mtp_nextn=2-overlap_scheduler=True] SKIP (https://nvbugs/6400067) | |||
There was a problem hiding this comment.
The removals below this line include configurations the stated root cause doesn't cover — e.g. test_bfloat16[mtp_nextn=2-attention_dp=False-cuda_graph=True-overlap_scheduler=False-torch_compile=False-...] and test_nvfp4[...-cuda_graph=False-overlap_scheduler=False-torch_compile=False]. If the IMA comes from torch.compile FX node ordering around piecewise CUDA graphs, the torch_compile=False cases were failing for some other reason and un-waiving them will just re-open the CI failure. Restrict the removal to configs you actually re-ran green.
pengbowang-nv
left a comment
There was a problem hiding this comment.
Review from attention perspective: the change to MLA is not meaningful and is a no-op.
c61dacb to
f26152e
Compare
|
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. |
…forward_impl slice helper On SM120 (RTX PRO 6000 Blackwell Server Edition), the fused NVFP4 kernel is disabled by RMSNorm's SM guard and no Fp4QuantizedTensor is ever produced. But MLA.forward_impl still calls _slice_hidden_states_to_num_tokens, whose outlined function-call boundary shifts multi-stream sync-event insertion around piecewise CUDA graph regions under torch.compile (enable_piecewise_cuda_graph=True, max_num_streams=3), producing an async CUDA IMA at spec sampler event synchronize on the mtp_nextn=2 + torch_compile=True config. Hoist the isinstance check out of the helper so the plain-tensor path takes an inline slice (matching the pre-501777ac89 FX-node shape) while the Fp4QuantizedTensor path still goes through the swizzled-SF-aware helper. No behavior change for the fused NVFP4 boundary fold on SM 10.x. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
Signed-off-by: handongl <handongl@nvidia.com>
f26152e to
8781a2d
Compare
Summary
hidden_states[:num_tokens, ...](restoring the pre-501777ac89 FX-node shape) while Fp4QuantizedTensor callers still go through the swizzled-SF-aware helper; also remove the now-fixed waiver.Test plan
Links
Dev Engineer Review
MLA.forward_impluses an inline slice for plain tensors.Fp4QuantizedTensorinputs still use_slice_hidden_states_to_num_tokens.tests/integration/test_lists/waives.txtremoves eight RTX PRO 6000 Blackwell waiver entries.QA Engineer Review
test-db/orqa/files were modified.mtp_nextn=2cases, and two DeepSeekV3Lite NVFP4 cases.