Skip to content

[https://nvbugs/6248648][fix] Wrap warmup+capture in try/except to graph.reset() the orphan then re-raise… - #14914

Open
tensorrt-cicd wants to merge 2 commits into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6248648
Open

[https://nvbugs/6248648][fix] Wrap warmup+capture in try/except to graph.reset() the orphan then re-raise…#14914
tensorrt-cicd wants to merge 2 commits into
NVIDIA:mainfrom
tensorrt-cicd:repair-bot-bug6248648

Conversation

@tensorrt-cicd

@tensorrt-cicd tensorrt-cicd commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Root cause: A capture-time CUDA error orphans the local CUDAGraph (never stored in self.graphs); GC later runs ~CUDAGraph() after the generator state is gone, so unregister_graph throws in the destructor and aborts the process, masking the real error.
  • Fix: Wrap warmup+capture in try/except to graph.reset() the orphan then re-raise the original error; also guard per-graph reset in clear() so one failing teardown doesn't abort the rest.
  • Automated fix generated by repair-bot

Test plan

  • Verify fix on the same GPU type as the original failure
  • Check for regressions in related tests

Links

Dev Engineer Review

  • CUDA graph warmup and capture reset partially created graphs before re-raising the original exception.
  • Graph metadata is committed only after successful capture.
  • Cleanup continues when an individual graph reset raises RuntimeError.
  • Speculative-decoding state is restored after capture failures.
  • _safe_reset_cuda_graph logs reset failures instead of propagating them.
  • clear and clear_cuda_graphs now return None.
  • No configuration or test-list files changed.
  • The changes address the reported orphaned-graph failure mode. No additional correctness or API issues are evident.

QA Engineer Review

Added test functions:

  • test_clear_continues_after_reset_failure
  • test_capture_failure_resets_graph_before_entry_commit
  • test_decoder_clear_continues_after_reset_failure
  • test_decoder_warmup_failure_restores_state_without_publishing_metadata
  • test_decoder_capture_failure_resets_graph_without_publishing_metadata
  • test_encoder_rejects_nested_output_without_orphaning_graph

These are test-code changes outside tests/integration/test_lists/. No test-db/ or qa/ coverage entries were changed. Coverage status is unavailable. Verdict: needs follow-up.

@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

CUDA graph capture now restores temporary state and resets partial graphs on failure. Cleanup uses guarded resets that log errors and continue. Graph metadata is committed only after successful capture. New tests cover decoder, encoder, and piecewise runner cleanup paths.

Changes

CUDA Graph Error Resilience

Layer / File(s) Summary
Capture failure recovery
tensorrt_llm/_torch/utils.py, tensorrt_llm/_torch/compilation/piecewise_optimizer.py, tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py
Capture paths reset partial graphs, restore decoder state, reject unsupported nested encoder outputs, and publish graph metadata only after successful capture.
Cleanup reset handling
tensorrt_llm/_torch/utils.py, tensorrt_llm/_torch/compilation/piecewise_optimizer.py, tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py
Cleanup uses best-effort graph resets, logs reset failures, continues processing remaining graphs, and adds None return annotations.
Cleanup and capture validation
tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py, tests/unittest/_torch/executor/test_cuda_graph_cleanup.py
Tests verify reset failures, state restoration, capture failure cleanup, metadata isolation, and encoder output validation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: hnover-nv

Sequence Diagram(s)

sequenceDiagram
  participant CUDAGraphRunner
  participant PiecewiseRunner
  participant CUDA Graph
  CUDAGraphRunner->>CUDA Graph: capture decoder or encoder execution
  PiecewiseRunner->>CUDA Graph: capture piecewise execution
  CUDA Graph-->>CUDAGraphRunner: return capture result
  CUDA Graph-->>PiecewiseRunner: return capture result
  CUDAGraphRunner->>CUDA Graph: reset partial graph on failure
  PiecewiseRunner->>CUDA Graph: reset partial graph on failure
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.03% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the bug fix and summarizes orphaned CUDA graph cleanup during warmup and capture.
Description check ✅ Passed The description explains the root cause, fix, test coverage, and bug reference, but it omits the repository checklist.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch repair-bot-bug6248648
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (1)

565-573: ⚡ Quick win

Log exception details for debugging.

While the broad Exception catch is justified here for defensive teardown (and the comment explains this well), losing the exception details makes debugging harder when resets do fail. Include the exception in the log.

♻️ Suggested improvement
     `@staticmethod`
     def _safe_reset_graph(graph: torch.cuda.CUDAGraph, context: str):
         # graph.reset() can raise (e.g. a stale CUDA generator state inside
         # ~CUDAGraph()); swallow it so one failing reset cannot abort the rest
         # of the teardown or mask an earlier, more relevant error.
         try:
             graph.reset()
-        except Exception:
-            logger.warning("Failed to reset CUDA graph %s.", context)
+        except Exception as e:
+            logger.warning("Failed to reset CUDA graph %s: %s", context, e)
🤖 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 `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py` around lines 565 - 573,
The _safe_reset_graph function swallows exceptions without details; modify the
except block in _safe_reset_graph(graph: torch.cuda.CUDAGraph, context: str) to
capture the exception (e.g., except Exception as exc:) and include the exception
information in the warning log (for example by passing exc or exc_info to
logger.warning or including exc in the message) so failures to reset the CUDA
graph log the exception details for debugging.
🤖 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.

Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py`:
- Around line 565-573: The _safe_reset_graph function swallows exceptions
without details; modify the except block in _safe_reset_graph(graph:
torch.cuda.CUDAGraph, context: str) to capture the exception (e.g., except
Exception as exc:) and include the exception information in the warning log (for
example by passing exc or exc_info to logger.warning or including exc in the
message) so failures to reset the CUDA graph log the exception details for
debugging.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c461e243-7204-4aa9-adcb-1602ba1fc8bb

📥 Commits

Reviewing files that changed from the base of the PR and between a163d74 and b99805e.

📒 Files selected for processing (1)
  • tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py

try:
graph.reset()
except Exception:
logger.warning("Failed to reset CUDA graph %s.", context)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact exception is not logged

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

for _ in range(self.WARMUP_STEPS):
_setup_spec_decoding_and_forward(key, forward_fn,
capture_inputs)
if postprocess_fn is not None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postprocess_fn is pulled inside the loop, is that intended?

@2ez4bz 2ez4bz Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is intentional and predates this PR. It was added in a49cfb3e68f for NVBug 5516666 so speculative-decoding input state is postprocessed / restored after each warmup forward, before the next iteration. This PR preserves that behavior while ensuring it also runs on failure and outside the graph-capture context.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (1)

861-869: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a narrower exception contract for _safe_reset_graph.

Line 868 catches Exception. This hides unrelated programming errors during teardown and triggers Ruff BLE001. Catch the specific exception type or types raised by torch.cuda.CUDAGraph.reset() for the supported PyTorch versions. If a broad catch is required for destructor safety, document that invariant. Add -> None to _safe_reset_graph.

PyTorch documents CUDAGraph.reset() as deleting the graph held by the instance. (docs.pytorch.org) Verify the repository’s supported PyTorch versions before selecting the exception type.

As per coding guidelines, **/*.py: “Catch specific exceptions instead of using broad or bare except: handlers” and “Annotate every function.”

🤖 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 `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py` around lines 861 - 869,
Update the static method _safe_reset_graph to declare -> None and replace the
broad Exception handler with the specific exception type(s) that
CUDAGraph.reset() raises across the repository’s supported PyTorch versions; if
destructor safety requires retaining a broad catch, document that invariant
directly in the method. Keep the existing warning and teardown behavior
unchanged.

Sources: Coding guidelines, MCP tools, Linters/SAST tools

🤖 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 `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py`:
- Around line 535-551: Make CUDA graph creation transactional across warmup and
capture: move the warmup loop and capture/postprocess operations into one
exception handler, and on any failure safely reset the local graph, remove the
pending self.graph_metadata[key], and restore saved_kv_lens_cuda without masking
the original exception. Defer publishing graph metadata until self.graphs[key]
has been successfully committed, and add regression coverage for both warmup and
capture failures.

---

Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py`:
- Around line 861-869: Update the static method _safe_reset_graph to declare ->
None and replace the broad Exception handler with the specific exception type(s)
that CUDAGraph.reset() raises across the repository’s supported PyTorch
versions; if destructor safety requires retaining a broad catch, document that
invariant directly in the method. Keep the existing warning and teardown
behavior unchanged.
🪄 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: 96604267-3204-43b4-826b-834128f09181

📥 Commits

Reviewing files that changed from the base of the PR and between 5533f66 and d327421.

📒 Files selected for processing (4)
  • tensorrt_llm/_torch/auto_deploy/compile/backends/torch_cudagraph.py
  • tensorrt_llm/_torch/auto_deploy/compile/piecewise_runner.py
  • tensorrt_llm/_torch/auto_deploy/shim/ad_executor.py
  • tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py

Comment thread tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py Outdated
@trtllm-agent
trtllm-agent force-pushed the repair-bot-bug6248648 branch from d327421 to 63b8c8f Compare August 6, 2026 15:27
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (1)

531-547: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Make capture setup transactional.

The try starts after warmup. A failure in Lines 519-525 leaves self.graph_metadata[key] published and can leave saved_kv_lens_cuda unrestored. A later lookup can return metadata for a graph that was never committed.

Wrap warmup, capture, and postprocessing in one failure path. Restore capture-scoped state without replacing the original exception. Remove or defer graph_metadata[key] until self.graphs[key] and self.graph_outputs[key] are committed.

🤖 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 `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py` around lines 531 - 547,
Make the capture flow transactional by extending the existing try/except around
graph creation to include warmup and setup operations before the current try
boundary, including the code that publishes graph_metadata[key]. On any failure,
restore saved_kv_lens_cuda and other capture-scoped state without masking the
original exception, safely reset the partially captured graph, and avoid
publishing graph_metadata[key] until self.graphs[key] and
self.graph_outputs[key] are committed.
🤖 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 `@tensorrt_llm/_torch/auto_deploy/compile/backends/torch_cudagraph.py`:
- Around line 71-74: Replace broad Exception handlers with RuntimeError handlers
in the CUDA graph reset cleanup blocks at
tensorrt_llm/_torch/auto_deploy/compile/backends/torch_cudagraph.py:71-74,
tensorrt_llm/_torch/auto_deploy/compile/piecewise_runner.py:418-425,
tensorrt_llm/_torch/auto_deploy/shim/ad_executor.py:557-560, and
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py:861-864; preserve the
existing warning behavior for reset failures.

---

Duplicate comments:
In `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py`:
- Around line 531-547: Make the capture flow transactional by extending the
existing try/except around graph creation to include warmup and setup operations
before the current try boundary, including the code that publishes
graph_metadata[key]. On any failure, restore saved_kv_lens_cuda and other
capture-scoped state without masking the original exception, safely reset the
partially captured graph, and avoid publishing graph_metadata[key] until
self.graphs[key] and self.graph_outputs[key] are committed.
🪄 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: 3e800bf3-df0f-434e-bee0-57bfb9beccd9

📥 Commits

Reviewing files that changed from the base of the PR and between 1745a6e and 63b8c8f.

📒 Files selected for processing (4)
  • tensorrt_llm/_torch/auto_deploy/compile/backends/torch_cudagraph.py
  • tensorrt_llm/_torch/auto_deploy/compile/piecewise_runner.py
  • tensorrt_llm/_torch/auto_deploy/shim/ad_executor.py
  • tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py

Comment thread tensorrt_llm/_torch/auto_deploy/compile/backends/torch_cudagraph.py Outdated
@hnover-nv

Copy link
Copy Markdown
Collaborator

AutoDeploy is deprecated and being removed in September. If this change isn't needed for correctness / compatabilty with the non-AD change then probably better to skip it. Otherwise I can take a final pass and approve it when the usage in main TRT-LLM is approved.

# does not abort cleanup of the remaining graphs.
for graph in self.graphs.values():
graph.reset()
self._safe_reset_graph(graph, "during cleanup")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EncoderCUDAGraphRunner in this same file has the identical unguarded pattern — capture() at ~1223 and clear() at ~1316 — and the TypeError raised at line 1239 after a successful capture orphans the graph deterministically. PiecewiseRunner in _torch/compilation/piecewise_optimizer.py is the same (capture ~233, clear_cuda_graphs ~186). Given AutoDeploy is being removed in September, could the AD changes be dropped in favor of covering these two mainline paths instead?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I dropped the AD changes and moved the coverage to the active pytorch paths.

EncoderCUDAGraphRunner now resets any graph that fails before commit (including the post-capture nested-output TypeError) and uses best-effort cleanup.

PiecewiseRunner now does the same for capture and clear. I added focused regression tests for both paths.

"""
try:
graph.reset()
except Exception:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on @achartier's note on the other site: the swallowed exception is dropped at all four sites. Please bind it (except Exception as e:logger.warning("... %s", e), or exc_info=True) — losing the reset error reintroduces exactly the "teardown failure masks the real problem" behavior this PR is fixing.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The shared reset helper now logs the caught exception (Failed to reset CUDA graph ...: <error>) while preserving the original failure/continuing teardown.

# registered to the state"), masking the real capture-time
# error.
try:
graph.reset()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reimplements the _safe_reset_cuda_graph() helper already added in backends/torch_cudagraph.py (same package), with the explanatory comment duplicated near-verbatim. ad_executor.py is a third copy. Please import and reuse the single helper.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AD changes (and their duplicate helpers) were removed. The retained pytorch paths share one internal _safe_reset_cuda_graph helper.

key, forward_fn, capture_inputs)
if postprocess_fn is not None:
postprocess_fn(capture_inputs)
_restore_spec_decode_capture_state(attn_metadata,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try also covers postprocess_fn and _restore_spec_decode_capture_state, which run outside the graph context. If either raises the capture actually succeeded, so the "after a capture failure" comment is misleading — and _restore_spec_decode_capture_state is then skipped, leaving attn_metadata in its capture state. Either narrow the try to the with block, or move the restore into a finally.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. State restoration now runs after every forward via a scoped context manager, outside torch.cuda.graph. Postprocessing is also outside the graph context. Any failure before graph commit resets the local graph, and a restoration failure no longer masks an active forward / postprocess exception.

"""Releases all captured graphs and the associated memory pool."""
# Reset each graph independently so a failure tearing down one graph
# does not abort cleanup of the remaining graphs.
for graph in self.graphs.values():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clear() half is unit-testable without a GPU: inject a fake graph whose reset() raises into self.graphs, then assert the remaining graphs still get reset and all the dicts end up cleared. Worth adding — this resilience property will otherwise silently regress.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

-padding_size]

@staticmethod
def _safe_reset_graph(graph: torch.cuda.CUDAGraph, context: str):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing -> None, unlike its twin _safe_reset_cuda_graph added in torch_cudagraph.py.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@2ez4bz
2ez4bz force-pushed the repair-bot-bug6248648 branch from 63b8c8f to 77351d1 Compare August 8, 2026 19:56
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (4)
tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (2)

516-530: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Detect the in-flight exception from the yield, not from sys.exc_info().

sys.exc_info() returns the innermost exception that is being handled in the current thread. If a caller invokes capture() from inside an except block, active_error is not None even when the forward pass succeeded. A real restore failure is then downgraded to a warning and hidden. Catch the exception around the yield instead. That makes the decision local and deterministic.

♻️ Proposed refactor
         `@contextlib.contextmanager`
         def _restore_state_after_forward() -> Iterator[None]:
+            active_error = None
             try:
                 yield
-            finally:
-                active_error = sys.exc_info()[1]
+            except BaseException as forward_error:
+                active_error = forward_error
+                raise
+            finally:
                 try:
                     _restore_spec_decode_capture_state(attn_metadata,
                                                        saved_kv_lens_cuda)
                 except RuntimeError as restore_error:
                     if active_error is None:
                         raise
                     logger.warning(
                         "Failed to restore speculative-decoding state after "
                         "CUDA graph forward failure: %s", restore_error)
🤖 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 `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py` around lines 516 - 530,
Update _restore_state_after_forward to track whether the context-managed yield
itself raises by catching the yield exception locally, rather than deriving
active_error from sys.exc_info(). Preserve re-raising restore failures when
forward succeeds, while logging restore failures only when the forward pass is
already failing.

1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove the copyright header in this directory.

Files under tensorrt_llm/_torch/pyexecutor/ intentionally omit the NVIDIA copyright header. Drop lines 1-2 to keep this directory consistent.

Based on learnings: "Do not add NVIDIA copyright headers to Python files under tensorrt_llm/_torch/pyexecutor/".

🤖 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 `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py` around lines 1 - 3,
Remove the SPDX copyright and license header lines from cuda_graph_runner.py so
Python files under tensorrt_llm/_torch/pyexecutor/ consistently omit NVIDIA
copyright headers.

Source: Learnings

tests/unittest/_torch/executor/test_cuda_graph_cleanup.py (1)

156-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert the encoder and decoder capture paths also leave memory_pool untouched.

The test confirms graphs, graph_outputs, and graph_metadata stay empty. self.memory_pool is the fourth field committed at Line 569 in cuda_graph_runner.py. Add an assertion for it so a future regression that assigns the pool before the commit is caught.

🤖 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/executor/test_cuda_graph_cleanup.py` around lines 156 -
178, Extend
test_decoder_capture_failure_resets_graph_without_publishing_metadata to assert
runner.memory_pool remains unchanged after capture fails, alongside the existing
graphs, graph_outputs, and graph_metadata assertions. Add equivalent memory_pool
assertions to the encoder and decoder capture failure tests so both capture
paths verify no pool is published.
tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py (1)

73-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for extra_attrs["global_stream"] after a capture failure.

The test patches get_model_extra_attrs to return {} and never inspects it. The failure path in piecewise_optimizer.py skips the global_stream restore, and this test cannot detect that. Assert the dictionary contents after the failure so the restore behavior is pinned.

💚 Proposed test change
+    extra_attrs = {"global_stream": object()}
+    eager_stream = object()
     with (
         piecewise_cuda_graph(True),
         capture_piecewise_cuda_graph(True),
         patch("torch.cuda.CUDAGraph", return_value=graph),
         patch("torch.cuda.graph", return_value=nullcontext()),
-        patch("torch.cuda.current_stream", return_value=object()),
+        patch("torch.cuda.current_stream", return_value=eager_stream),
         patch(
             "tensorrt_llm._torch.compilation.piecewise_optimizer.get_model_extra_attrs",
-            return_value={},
+            return_value=extra_attrs,
         ),
         pytest.raises(ValueError, match="capture failed"),
     ):
         runner()
 
     assert graph.reset_count == 1
     assert runner.entries[1].cuda_graph is None
+    assert extra_attrs["global_stream"] is eager_stream
🤖 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 - 96, Update
test_capture_failure_resets_graph_before_entry_commit to retain the patched
get_model_extra_attrs dictionary, then assert that extra_attrs["global_stream"]
is restored after runner() raises ValueError. Keep the existing graph reset and
cuda_graph assertions unchanged.
🤖 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 `@tensorrt_llm/_torch/compilation/piecewise_optimizer.py`:
- Around line 240-262: Update the graph-capture exception handling around
`entry.callable(*args)` so `extra_attrs["global_stream"]` is restored in a
`finally` block, regardless of whether capture succeeds or raises. Keep
`_safe_reset_cuda_graph` and exception propagation in the existing `except`
path, while ensuring the success path still proceeds with the restored stream.

In `@tensorrt_llm/_torch/utils.py`:
- Around line 391-396: Preformat warning messages because tensorrt_llm.logger
joins arguments instead of applying printf-style interpolation: update
_safe_reset_cuda_graph in tensorrt_llm/_torch/utils.py (391-396) and the warning
call in tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py (528-530) to pass a
single f-string argument; update
tests/unittest/_torch/executor/test_cuda_graph_cleanup.py (124-124) to assert
the rendered message via warning.call_args.args[0] rather than the placeholder
argument.

---

Nitpick comments:
In `@tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py`:
- Around line 516-530: Update _restore_state_after_forward to track whether the
context-managed yield itself raises by catching the yield exception locally,
rather than deriving active_error from sys.exc_info(). Preserve re-raising
restore failures when forward succeeds, while logging restore failures only when
the forward pass is already failing.
- Around line 1-3: Remove the SPDX copyright and license header lines from
cuda_graph_runner.py so Python files under tensorrt_llm/_torch/pyexecutor/
consistently omit NVIDIA copyright headers.

In `@tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py`:
- Around line 73-96: Update
test_capture_failure_resets_graph_before_entry_commit to retain the patched
get_model_extra_attrs dictionary, then assert that extra_attrs["global_stream"]
is restored after runner() raises ValueError. Keep the existing graph reset and
cuda_graph assertions unchanged.

In `@tests/unittest/_torch/executor/test_cuda_graph_cleanup.py`:
- Around line 156-178: Extend
test_decoder_capture_failure_resets_graph_without_publishing_metadata to assert
runner.memory_pool remains unchanged after capture fails, alongside the existing
graphs, graph_outputs, and graph_metadata assertions. Add equivalent memory_pool
assertions to the encoder and decoder capture failure tests so both capture
paths verify no pool is published.
🪄 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: 32e4a977-5df4-495a-93d6-6a3bb4041bbe

📥 Commits

Reviewing files that changed from the base of the PR and between 3d1596b and 77351d1.

📒 Files selected for processing (5)
  • tensorrt_llm/_torch/compilation/piecewise_optimizer.py
  • tensorrt_llm/_torch/pyexecutor/cuda_graph_runner.py
  • tensorrt_llm/_torch/utils.py
  • tests/unittest/_torch/compilation/test_piecewise_cuda_graph_cleanup.py
  • tests/unittest/_torch/executor/test_cuda_graph_cleanup.py

Comment thread tensorrt_llm/_torch/compilation/piecewise_optimizer.py
Comment thread tensorrt_llm/_torch/utils.py Outdated
@2ez4bz
2ez4bz force-pushed the repair-bot-bug6248648 branch from 77351d1 to 25de6d6 Compare August 9, 2026 03:21
…uctor abort

Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
@2ez4bz
2ez4bz force-pushed the repair-bot-bug6248648 branch 2 times, most recently from 4f292ed to 43af684 Compare August 9, 2026 03:26
@2ez4bz

2ez4bz commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator Author

PR_Github #64826 [ run ] triggered by Bot. Commit: 43af684 Link to invocation

Signed-off-by: William Zhang <williamz@nvidia.com>
@2ez4bz
2ez4bz force-pushed the repair-bot-bug6248648 branch from 43af684 to 5aadd73 Compare August 9, 2026 03:55
@2ez4bz

2ez4bz commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

/bot run

@2ez4bz
2ez4bz enabled auto-merge (squash) August 9, 2026 03:59
@tensorrt-cicd

Copy link
Copy Markdown
Collaborator Author

PR_Github #64826 [ run ] completed with state SUCCESS. Commit: 43af684
/LLM/main/L0_MergeRequest_PR pipeline #52668 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants