samples: skip unsupported test case, remove invalid cudaGraphDestroy - #734
Conversation
Blackwell + cuDNN<9.18 + deterministic SDPA backward is a combination explicitly marked as unsupported at include/cudnn_frontend/node/scaled_dot_product_flash_attention.h:1385.
… sample In "Cuda graphs with matmul add", `cudnn_cuda_graph_new` is obtained via `cudaGraphChildGraphNodeGetGraph(cudnn_node_in_main_graph, &cudnn_cuda_graph_new)`. Per the CUDA Runtime API documentation for `cudaGraphChildGraphNodeGetGraph`: "This call does not clone the graph. Changes to the graph will be reflected in the node, and the node retains ownership of the graph." Destroying `main_cuda_graph` with `cudaGraphDestroy(main_cuda_graph)` destroys the parent graph and all embedded child graphs owned by its nodes. Calling `cudaGraphDestroy(cudnn_cuda_graph_new)` afterwards attempts to destroy an already-destroyed graph handle, returning cudaErrorInvalidValue and leaving a sticky CUDA runtime error in the process device context that corrupts subsequent tests executing on that context in monolithic test runners like Catch2. Remove the invalid `cudaGraphDestroy(cudnn_cuda_graph_new)` call and wrap destruction calls in `CUDA_CHECK`.
|
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 (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe changes add CUDA status checks during graph cleanup, remove redundant cuDNN graph destruction, and skip unsupported deterministic SDPA backward execution on Blackwell with older cuDNN versions. ChangesCUDA graph cleanup
SDPA backward compatibility
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized sample change skips an unsupported test case and removes an invalid CUDA graph destruction call; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Full details: Description checkExplanation The description covers the affected area, summary, rationale, related issues, compatibility impact, and testing commands. The Milestone and Projects fields are not addressed, and test results are not reported, but the description is otherwise complete and relevant. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks @dextero for the PR. |
Before submitting
pre-commit runand committed any formatting changes.cat-*, one or moremod-*, and oneorig-*(see label list).cat-bugfix mod-frontend orig-external (The UI doesn't appear to let me attach labels)
Affected area
Summary
cudaGraphDestroy(cudnn_cuda_graph_new)incudagraphssample. As per documentation ofcudaGraphChildGraphNodeGetGraphit's owned by the parentmain_cuda_graphand gets destroyed whenever the main one is.Why
This makes the samples tests pass on older cuDNN.
Related issues
None
API and compatibility impact
None
Testing
Summary by CodeRabbit