[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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review. Walkthrough
ChangesMLA slicing behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR restores the intended plain-tensor slicing path while preserving the specialized path for Fp4QuantizedTensor inputs; no actionable merge-blocking risk remains beyond normal checks and review. 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. |
f26152e to
8781a2d
Compare
…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>
8781a2d to
a8b4a3c
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. |
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.waives.txtremoves seven RTX PRO 6000 Blackwell waiver entries.QA Engineer Review
test-db/orqa/files were modified.mtp_nextn=2cases and two DeepSeekV3Lite NVFP4mtp_nextn=2cases.