[https://nvbugs/6478707][fix] detect v2 inside remove_functionalize_inner, resolve each mutates_args via… - #16629
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 skipped from review as they are similar to previous changes (1)
WalkthroughThe MLA custom operation’s inplace metadata now records ChangesMLA inplace metadata
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Torch 2.12 AOT-autograd emits auto_functionalized_v2 HOP nodes for mutable-arg ops even when inductor's enable_auto_functionalized_v2 is False (that flag only affects inductor lowering). In v2 nodes, mutable tensors live in _all_bases and each mutable arg has its own _<name>_base_index kwarg; direct kwargs like 'output' are absent. Previously remove_copy_for_mutates_args never detected v2 nodes at the call site, so kwargs["output"] raised KeyError. The v2 branch itself keyed into _all_bases by mutates_args positional index (k-1) rather than by the _<name>_base_index kwarg, which is unrelated to op-arg position. Detect auto_functionalized_v2 inside the inner helper and, for each mutable arg, resolve its base tensor via _<arg>_base_index into _all_bases, mapping a missing/None index to None so optional mutable args are preserved. Also register the two additional mutable args (dsv4_output, dsv4_output_sf) for mla_custom_op_inplace in inplace_info() so the reconstructed op call carries them. This unblocks TestDeepSeekV3Lite::test_nvfp4_4gpus with torch_compile under moe_backend=TRTLLM on B200. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
d7dfd7f to
613e5eb
Compare
|
@JunyiXu-nv #16629 may have fixed this issue, should we unwaive these tests? |
|
NVBug 6478707 is closed as Bug - Fixed. The linked bug appears resolved elsewhere or for a reason that does not prove this PR is redundant. This PR should be judged on its own merits; repair-bot is not auto-closing it. |
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
Indices match the op's declared mutates_args=("output", "dsv4_output", "dsv4_output_sf") at tensorrt_llm/_torch/modules/mla.py:161, so the mapping is right.
Two things:
- The description doesn't match the diff. The
auto_functionalized_v2detection and_all_basesresolution it claims are already on main (commit e96c330, PR #16410), and nvbugs/6478707 is already closed as fixed by that PR. This diff is only the twoinplace_infoentries — please retitle/rewrite so the change stands on its own (registering the DSv4 mutable args). - No test covers the new entries; see the inline note.
| "mla_custom_op_inplace": { | ||
| 1: "output" | ||
| 1: "output", | ||
| 2: "dsv4_output", |
There was a problem hiding this comment.
Please add a case to tests/unittest/_torch/compilation/test_remove_copy_pass.py covering the DSv4-enabled graph — _all_bases=(output, dsv4_output, dsv4_output_sf) with all three _<arg>_base_index set and getitem users at 1/2/3. The existing tests only exercise the all-None variant, so index 2/3 here are untested; under auto_functionalized_v2 the getitem index is a position in _all_bases, not in the declared arg list, and a wrong entry silently rewires a getitem to the wrong buffer rather than failing loudly.
Summary
_all_bases_<arg>_base_indexinto_all_bases(None-safe), and register the two additional mla_custom_op_inplace mutable args (dsv4_output, dsv4_output_sf) in inplace_infoTest plan
Links
Dev Engineer Review
remove_copy_passhandling forauto_functionalized_v2nodes to resolve mutable arguments through_all_basesusing_<arg>_base_index, includingNonehandling.dsv4_outputanddsv4_output_sfas mutable outputs formla_custom_op_inplace.KeyErrorfailures caused by v2 nodes storing mutable arguments indirectly.QA Engineer Review
No test changes.