[https://nvbugs/6567403][fix] Revert #17010 to unblock DGX_H100 PyTorch-Others-1 stage timeout - #17352
Conversation
… stage timeout This reverts commit 60e7fca (NVIDIA#17010), which made DGX_H100-4_GPUs-PyTorch-Others-1 time out at 244-267 min instead of the usual 67-83 min. Evidence: - PostMerge on main flipped at this commit: build 2885 (43d6fa4) succeeded in 72 min, build 2886 (91fb443) failed at 262 min. - Intersecting the main bad range (43d6fa4..91fb443) with PR 17119 bad range (5dab9bf..e7e9da5) leaves exactly this commit. - Across 22 builds from 14 PRs plus main, containing this commit predicts the timeout: 10 SUCCESS / 0 FAILURE without it, 0 SUCCESS / 11 FAILURE with it. - The stage stalls in test_trtllm_bench_llmapi_launch, which otherwise runs in 118-125 s; it hangs until the session is killed or fails from mpirun. Keeping a strong reference to every captured CUDA graph output tensor for the runner lifetime pins one output buffer per graph key, so the capture-time storage can no longer return to the shared graph memory pool. The nvbugs/6567403 waives are removed too: the three MoE multi-GPU cases were only collateral damage and pass in 45-76 s once the stage completes normally. The nvbugs/6525011 waives removed by NVIDIA#17010 are restored by this revert. Signed-off-by: xxi <xxi@nvidia.com>
|
Please don't merge, this is a CI test |
|
/bot run --stage-list "DGX_H100-4_GPUs-PyTorch-Others-1" --add-multi-gpu-test --detailed-log |
WalkthroughThe change removes strong captured-output references from decoder and encoder CUDA graph runners. It also updates integration test waivers for B200, B300, and DGX_H100 platforms. ChangesCUDA graph output lifetime
Integration test waivers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/test_lists/waives.txt (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the NVIDIA copyright header.
tests/integration/test_lists/waives.txthas no NVIDIA copyright header. Add the repository-standard header and set the year to 2026 for this modified file.As per coding guidelines, add the NVIDIA copyright header to all new files and update the copyright year on modified files.
🤖 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/integration/test_lists/waives.txt` at line 1, Add the repository-standard NVIDIA copyright header to tests/integration/test_lists/waives.txt, using 2026 as the copyright year while preserving the existing waiver entry unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/integration/test_lists/waives.txt`:
- Line 1: Add the repository-standard NVIDIA copyright header to
tests/integration/test_lists/waives.txt, using 2026 as the copyright year while
preserving the existing waiver entry unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 10530326-fe14-4858-a675-bcbcd1c2f519
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.pytests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py
|
PR_Github #64277 [ run ] triggered by Bot. Commit: |
BowenFu
left a comment
BowenFu
left a comment
There was a problem hiding this comment.
The revert itself checks out and I'm not blocking on the mechanics — but I'm not the right first approver on the CUDA-graph output lifetime, so leaving this for someone who owns it.
Verified for the PR:
- Byte-for-byte inverse of #17010 in both runners (
CUDAGraphRunnerandEncoderCUDAGraphRunner): the dict init, the per-key assignment and theclear()ordering all come back out. No residual_graph_output_refsreference anywhere under_torch/pyexecutor/at this head, so nothing is left assuming a strong ref is held. - The two restored waives are exactly the two #17010 removed;
nvbugs/6525011has no other entries, so nothing extra gets silenced. - Removing the three
nvbugs/6567403lines is clean: this branch is based on merged #17344, so ordinary merge order intomainapplies without conflict.
The one thing worth deciding explicitly, since it's what the revert gives back: #17010 asserts in its comment that the weak ref lets capture-time output storage be reused while the graph is still replayable. That same pattern lives in tensorrt_llm/_torch/visual_gen/cuda_graph_runner.py:83-87, but with a caller-specific rationale — VisualGen keeps two graph keys' outputs live at once (true CFG combines the positive and negative Qwen-Image-Edit transformer outputs), which is what makes the aliasing reachable there. The pyexecutor runner has no caller that holds two keys' outputs simultaneously, so the same justification doesn't transfer, and what it does cost is one pinned output buffer per graph key for the runner's lifetime — which is the measured DGX-H100 regression.
Also worth knowing: #17354 merged at 08:52Z waiving test_trtllm_bench_llmapi_launch, so the stage is no longer blocked and this doesn't have to land as an emergency. But #17010 is on main, so the pinned-buffer cost is shipping to every 4-GPU CUDA-graph deployment, not just to CI — that's the argument for landing the revert rather than living on the waive.
|
|
||
| self.graphs[key] = graph | ||
| self._graph_output_refs[key] = output | ||
| graph_output = make_weak_ref(output) |
There was a problem hiding this comment.
This is what the revert restores: graph_outputs[key] goes back to weak-ref-only, with no owning copy retained.
The pinning that #17010 added was one output buffer per graph key held for the runner's lifetime — with a batch-size × draft-len key space that is not a small number of buffers, which is why a tp=2 mpirun CUDA-graph workload was the first thing to fall over.
If the hazard #17010 described is real for this runner (and not just for the VisualGen one it appears to be modelled on), the fix wants to be capture() returning the owning output rather than a lifetime-long strong-ref dict — otherwise this reopens nvbugs/6525011 with the memory cost simply traded back.
|
PR_Github #64277 [ run ] completed with state |
4 participants
Description
Reverts #17010 (commit
60e7fcaeaf), which regressed theDGX_H100-4_GPUs-PyTorch-Others-1stage from ~67-83 min to a 244-267 minwall-time kill. The stage has been failing for essentially every PR whose base
contains this commit since 2026-08-05 00:24 UTC, including PostMerge on
main.Evidence that #17010 is the cause
1. PostMerge on
mainflips exactly at this commit (PostMerge runsmaindirectly, with no PR code):
43d6fa410c91fb4433c550edd738172. Two independent bad ranges intersect in a single commit. The
mainbadrange
43d6fa410c..91fb4433c5(17 commits) intersected with PR #17119's badrange
5dab9bf760..e7e9da5148(20 commits) leaves exactly60e7fcaeaf.3. Containing the commit predicts the timeout, over 22 builds spanning 14
different PRs plus
main(ancestry checked via the GitHub compare API):60e7fcaeaf60e7fcaeafOne near-counterexample (PR #17130, 230 min) had
test_trtllm_bench_llmapi_launchpass in 123.5 s, so it is a different failure.
4. Within-PR control. PR #12733 ran 70 min SUCCESS on 08-05 01:58 with a base
that lacks the commit, then 250 min FAILURE on 08-06 01:16 after rebasing onto a
base that has it — same PR code, only the base changed.
5. The stall is in
test_trtllm_bench_llmapi_launch, which normally takes118-125 s. With the commit it either hangs until the pytest session is killed
(no test records uploaded for the whole stage) or fails with
subprocess.CalledProcessErrorfrommpirun.Why the change breaks it
#17010 added a strong reference to every captured CUDA graph's output tensor,
held for the runner's lifetime, where previously only a weak ref was kept. That
pins one output buffer per graph key, so the capture-time output storage can no
longer return to the shared graph memory pool.
test_trtllm_bench_llmapi_launchis a pytorch-backend, CUDA-graph,
mpiruntp=2 workload, which is where thepinned memory shows up first.
Reverting restores the original weak-ref-only behavior. The
nvbugs/6525011waives that #17010 removed are restored, so
TestLagunaXS::test_fp8is skippedagain on B200/B300 until a non-regressing fix for that bug lands.
Also removes the nvbugs/6567403 waives
The three MoE multi-GPU cases waived in #17344 were only collateral damage — the
stage was killed before they could finish, and they pass in 45-76 s whenever the
stage completes normally (e.g. PostMerge build 2885: 45.5 s / 68.4 s / 75.8 s).
They are un-waived here so this PR's CI actually exercises them and the fix can
be verified end to end.
Test Coverage
DGX_H100-4_GPUs-PyTorch-Others-1— the stage that regressed. It must return tothe ~67-83 min range with
test_trtllm_bench_llmapi_launchand all three MoEmulti-GPU cases passing.
PR Checklist
[JIRA/NVBUG/None][type] SummaryDev Engineer Review
nvbugs/6525011waives forTestLagunaXS::test_fp8on B200 and B300.nvbugs/6567403waives for three DGX_H100 configurable MoE multi-GPU cases.QA Engineer Review
tests/integration/test_lists/waives.txt.TestLagunaXS::test_fp8waives for the B200 and B300 suites undernvbugs/6525011.nvbugs/6567403.