[https://nvbugs/6159132][fix] Differentiate the two paths via extra_acc_spec="tp_attn" when attention_dp=False - #13922
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:
WalkthroughCUDA graph cleanup now uses a shared safe-reset helper. Piecewise, decoder, and encoder capture paths restore mutable state, defer metadata publication, and reset partially created graphs when failures occur. New tests cover cleanup continuation, state restoration, and capture failure handling. ChangesCUDA Graph Capture Cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CUDAGraphRunner
participant ModelForward
participant safe_reset_cuda_graph
participant GraphMetadata
CUDAGraphRunner->>ModelForward: Run warmup and capture forwards
ModelForward-->>CUDAGraphRunner: Return outputs or raise an exception
CUDAGraphRunner->>CUDAGraphRunner: Restore KV-length and stream state
alt Capture or output processing fails
CUDAGraphRunner->>safe_reset_cuda_graph: Reset the partial graph
safe_reset_cuda_graph-->>CUDAGraphRunner: Log reset failure if needed
else Capture succeeds
CUDAGraphRunner->>GraphMetadata: Publish outputs and graph metadata
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[Repair Bot] Attempted automated rebase/maintenance, but could not complete it safely. Leaving the PR unchanged. Reason: The bot will try again on a later cycle. Manual rebase is also fine. |
ced631d to
10be260
Compare
10be260 to
d548a56
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. |
|
/bot run |
|
PR_Github #64800 [ run ] triggered by Bot. Commit: |
|
PR_Github #64800 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64933 [ run ] triggered by Bot. Commit: |
|
PR_Github #64933 [ run ] completed with state
|
4067f46 to
12ee846
Compare
|
/bot help |
GitHub Bot Help
Provide a user friendly way for developers to interact with a Jenkins server. Run See details below for each supported subcommand. Details
Launch build/test pipelines. All previously running jobs will be killed.
kill
Kill all running builds associated with pull request. skip
Skip testing for latest commit on pull request. reuse-pipeline
Reuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break. |
|
/bot run --add-multi-gpu-test |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py (1)
73-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify the
current_streamcall count assumption.
side_effect=[capture_stream, restored_stream]provides exactly two values. The capture path callstorch.cuda.current_stream()twice today. If a future change adds a third call inside the patched region, the test fails withStopIterationrather than a clear assertion. Considerside_effect=itertools.chain([capture_stream], itertools.repeat(restored_stream))to keep the failure message meaningful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py` around lines 73 - 100, Update test_capture_failure_resets_graph_before_entry_commit so the patched torch.cuda.current_stream side effect supplies capture_stream first and restored_stream for all subsequent calls, using an unbounded iterator instead of a fixed two-value list. Preserve the existing assertions and capture behavior while preventing unrelated StopIteration failures if additional current_stream calls are introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/_torch/executor/test_cuda_graph_cleanup.py`:
- Around line 183-209: Update
test_encoder_rejects_nested_output_without_orphaning_graph so capture reaches
nested-output validation: stub _stage_inputs or provide its complete required
inputs, including input_ids, seq_lens, and attention metadata containing
_seq_lens_cuda and _seq_lens. Patch torch.cuda.current_stream for CPU-only
execution, preserving the existing cleanup assertions.
---
Nitpick comments:
In `@tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py`:
- Around line 73-100: Update
test_capture_failure_resets_graph_before_entry_commit so the patched
torch.cuda.current_stream side effect supplies capture_stream first and
restored_stream for all subsequent calls, using an unbounded iterator instead of
a fixed two-value list. Preserve the existing assertions and capture behavior
while preventing unrelated StopIteration failures if additional current_stream
calls are introduced.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 44e555e1-38d9-43b1-ab02-7cfc2d5d850e
📒 Files selected for processing (5)
tensorrt_llm/_torch/compilation/piecewise_optimizer.pytensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytensorrt_llm/_torch/utils.pytests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.pytests/unittest/_torch/executor/test_cuda_graph_cleanup.py
|
PR_Github #64967 [ run ] triggered by Bot. Commit: |
12ee846 to
772a4bb
Compare
|
/bot run --add-multi-gpu-test |
|
|
|
/bot run --add-multi-gpu-test |
|
PR_Github #64973 [ run ] triggered by Bot. Commit: |
|
PR_Github #64967 [ run ] completed with state |
|
PR_Github #64973 [ run ] completed with state
|
|
The PR title/description describe a MiniMax-M2 GSM8K accuracy threshold fix, but the diff is CUDA graph capture/teardown hardening (nvbugs/6248648). Please update the title and body to match before merge. |
Head branch was pushed to by a user without write access
772a4bb to
8e7d5b4
Compare
…ion path The attention_dp=False variant of TestMiniMaxM2::test_4gpus uses the fused minimax_allreduce_rms_qk kernel for QK norm, which is numerically less precise than the per-rank RMSNorm path selected by attention_dp=True. The shared reference of 93.75 resulted in a threshold of 90.547 while the observed accuracy on the TP-sharded path is ~90.49, causing flaky failures. Differentiate the two paths via extra_acc_spec='tp_attn' and register a lower reference (92.0) for the TP-sharded path. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
8e7d5b4 to
be462ad
Compare
Dismissing, as I accidentally force pushed completely unrelated changes.
|
/bot run |
|
PR_Github #65080 [ run ] triggered by Bot. Commit: |
|
PR_Github #65080 [ run ] completed with state |
Summary
Test plan
Links