NVIDIA / NVIDIA/cuopt

cuda_graph_t discards CUDA error codes, obscuring capture failures

Open
#1,758 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

awaiting response bug improvement
Dominant language
Cuda
Stars
1k
Forks
233
Avg merge
4d 4h
Merged PRs (30d)
95

Description

Summary

cuopt::routing::detail::cuda_graph_t (cpp/src/routing/cuda_graph.cuh) discards the return code of every CUDA call it makes:

void start_capture(rmm::cuda_stream_view stream)
{
  cudaStreamBeginCapture(stream, cudaStreamCaptureModeThreadLocal);
  capture_started = true;
}

void end_capture(rmm::cuda_stream_view stream)
{
  ...
  cudaStreamEndCapture(stream, &graph);
  ...
  cudaGraphExecUpdate(instance, graph, &errorNode, &updateResult);
  ...
  cudaGraphInstantiate(&instance, graph);
  cudaGraphDestroy(graph);
}

void launch_graph(rmm::cuda_stream_view stream) { cudaGraphLaunch(instance, stream); }

When a capture is invalidated mid-flight, cudaStreamEndCapture returns an error and sets graph to nullptr. That null graph is then passed to cudaGraphInstantiate, and instance is later launched, all unchecked.

Why this matters

This actively obstructed the diagnosis of #1748. The real failure was a single cudaErrorStreamCaptureUnsupported at one line inside a capture region. What CI showed instead was a cascade of downstream cudaErrorStreamCaptureInvalidated errors from unrelated call sites, because the first error was swallowed and execution continued on a null graph.

The failure was found in the end, but the logs pointed away from the cause rather than at it.

Prior attempt and why it was reverted

#1753 initially added RAFT_CUDA_TRY around these calls. That was scope creep on a CI hotfix and was reverted in favour of keeping that PR to the actual fix (see the review discussion there).

The attempt also surfaced two things worth carrying into any future change — both were bugs introduced by adding the checks naively, and both are traps for whoever does this properly:

  1. capture_started must be cleared before the error propagates. RAFT_CUDA_TRY(cudaStreamEndCapture(...)) throws before the flag is reset, leaving a reused wrapper with stale capture state. Store the code, clear the flag, then throw.

  2. A failed cudaGraphInstantiate leaks the captured graph. Throwing skips the cudaGraphDestroy(graph) at the end of end_capture. Destroy before propagating, or use a scope guard.

cpp/src/utilities/manual_cuda_graph.cuh already handles both correctly and is the reference for the ordering.

Open questions for whoever picks this up

  • cudaGraphExecUpdate. Its failure is a normal, expected path — the code falls back to re-instantiation. But the current code cannot distinguish "update failed, re-instantiate" from a genuine error, since it only inspects updateResult and never the API return code. cudaErrorGraphExecUpdateFailure should feed the existing fallback; anything else is probably a real error.
  • Should launch_graph verify graph_created? Raised in review on #1753: if instantiation failed, instance may be unset or destroyed.
  • Testing. There is currently no coverage of these paths, and exercising a failed cudaGraphInstantiate needs CUDA fault injection, which cpp/tests does not have. Note also that cpp/tests/routing/local_search_cand_test.cu is not registered in cpp/tests/routing/CMakeLists.txt.

Related

  • #1748 — the CUDA 13 capture failures this obscured
  • #1753 — the fix, where the error checks were reverted out
  • rapidsai/rmm#2518 — the upstream cause

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in cpp/src/routing/cuda_graph.cuh and compare its capture-state and graph cleanup ordering with cpp/src/utilities/manual_cuda_graph.cuh. Review the CUDA return-code handling, the cudaGraphExecUpdate fallback, and the existing routing tests; done means failures propagate without stale capture state or leaked graphs, with coverage added where the test infrastructure permits.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
hpc, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.