Four graph-API bugs, each with the regression test it needed - #546
Open
YangXu1990uiuc wants to merge 4 commits into
Open
Four graph-API bugs, each with the regression test it needed#546YangXu1990uiuc wants to merge 4 commits into
YangXu1990uiuc wants to merge 4 commits into
Conversation
`self.__workspace = False` inside `class Graph` writes `_Graph__workspace`, but `__exit__` probed for it with `hasattr(self, "__workspace")` — a plain string literal, which Python does not name-mangle. The probe was therefore always False, and `__exit__` unconditionally overwrote the sentinel with a fresh `torch.empty(get_workspace_size())`. Two silent consequences. A caller who asked to own the workspace still paid an allocation per Graph that nothing ever read — hundreds of MB for a large SDPA graph. And the "Need to specify workspace to execute graph" guard in execute() became unreachable, so a caller who set workspace_alloc=False and then forgot to pass workspace= ran on the buffer the wrapper had allocated behind their back rather than being told. Give __workspace a class-level default the way __handle already has one, and test the sentinel by identity. The kwarg and the broken probe arrived in the same commit (v1.15, 2025-10), so it has never worked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cudnn.experimental` raised RecursionError. The lazy hook fetched it with `from . import experimental`, which is exactly the form the eight-line comment six lines earlier documents as recursive: the fromlist machinery calls hasattr(cudnn, "experimental"), the name is not in __dict__ yet, so __getattr__ re-enters and the cycle never terminates. The `ops` branch right above it already avoids this with importlib.import_module. The two branches now share one body, so a third lazy submodule cannot pick the wrong form. Test every name __getattr__ special-cases, in a subprocess, since the failure only reproduces on a cold module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two docstrings cited ``cudnn/frost/dispatch.py`` as the home of the workspace and stream contracts. That file has never existed on this branch: it lives on the gitlab side, and when FROST was cherry-picked over in NVIDIA#476 its responsibilities were absorbed by ``cudnn/engines/`` while the prose came along unchanged. Cite engines/base.py, which is where the contract is. Five engines reported an ImportError from a probe as "requires the Cutlass DSL" / "requires cuda.bindings". The probe raises for anything missing anywhere in the import chain, not just its target, so the message asserts a cause it did not establish. Debugging one of these today cost an import hook to discover the real answer: a stray /tmp/cuda.py shadowing the cuda package, reported as "requires the Cutlass DSL: No module named 'vllm'" — two contradictory halves, neither pointing at the file. Say which import failed and let the original exception name the missing module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build_plans() skips a plan whose build raises one of decline_types() and moves on to the next entry, ultimately to the cuDNN backend. Anything else propagates and takes down the whole walk, so a graph the backend could have served fails outright. Seven probes reported "no CUDA device is visible" and "the driver did not report MaxSharedMemoryPerBlockOptin / L2CacheSize" as RuntimeError. Both are declines — this engine cannot serve the graph, another entry can — and neither is a decline type nor caught by the engines' own build_plan handlers, which take (NotImplementedError, ValueError). So a driver that does not answer a property query aborted planning rather than falling back. Left as-is on purpose: cuDeviceGet/cudaGetDevice failures (a real driver error), and the ValueErrors for a device ordinal that does not exist or a non-CUDA buffer (caller mistakes). Those should not be swallowed by a fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe PR standardizes lazy imports, changes unsupported CUDA probes to use decline exceptions, improves import failure diagnostics, and fixes graph workspace state handling. Tests cover import boundaries, decline behavior, and workspace allocation. ChangesRuntime contracts
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@cudnn-ci-bot run frost |
|
🚀 Running mirror pipeline Branch: cudnn-gh/pr-546-00841a0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/cudnn/linear_attention/cutile/gdn_engine.py (1)
128-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChain the original
ImportErrorin both cuTile engines.Both handlers report the import failure but omit explicit exception chaining. Add
from eat both sites to preserve the original cause and satisfy Ruff B904.
python/cudnn/linear_attention/cutile/gdn_engine.py#L128-L128: appendfrom eto theNotImplementedErrorraise.python/cudnn/linear_attention/cutile/kda_engine.py#L140-L140: appendfrom eto theNotImplementedErrorraise.🤖 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 `@python/cudnn/linear_attention/cutile/gdn_engine.py` at line 128, Chain the original ImportError when raising NotImplementedError in both cuTile engine handlers: update python/cudnn/linear_attention/cutile/gdn_engine.py lines 128-128 and python/cudnn/linear_attention/cutile/kda_engine.py lines 140-140 to use explicit exception chaining with from e.Source: Linters/SAST tools
test/python/test_wrapper_graph.py (1)
37-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd tensor-dictionary coverage and backend gating.
Graph.__call_with_tensor_dicthas the sameworkspace_alloc=Falseguard, but no fluentcudnn.Graphtest covers it. Add omitted-workspace and supplied-workspace cases, then compareout_dict["Y"]with the matmul reference. Gate both tests for cuDNN 9.12.0 or newer becauseGraphrejects older backends.🤖 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 `@test/python/test_wrapper_graph.py` around lines 37 - 57, Add cudnn.Graph tensor-dictionary tests covering workspace_alloc=False with both omitted and supplied workspace, invoking Graph.__call_with_tensor_dict and comparing out_dict["Y"] against the matmul reference. Gate both tests to cuDNN 9.12.0 or newer, matching the existing backend-version test utilities.
🤖 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 `@test/python/test_wrapper_graph.py`:
- Around line 12-26: Add a pytest.mark.skipif decorator to the fluent graph test
helper or its containing test scope, using cudnn.backend_version() < 91200 as
the condition and a clear reason. Apply it to the tests exercising
_matmul_graph, without relying on the shared CUDA assertion for skipping.
---
Nitpick comments:
In `@python/cudnn/linear_attention/cutile/gdn_engine.py`:
- Line 128: Chain the original ImportError when raising NotImplementedError in
both cuTile engine handlers: update
python/cudnn/linear_attention/cutile/gdn_engine.py lines 128-128 and
python/cudnn/linear_attention/cutile/kda_engine.py lines 140-140 to use explicit
exception chaining with from e.
In `@test/python/test_wrapper_graph.py`:
- Around line 37-57: Add cudnn.Graph tensor-dictionary tests covering
workspace_alloc=False with both omitted and supplied workspace, invoking
Graph.__call_with_tensor_dict and comparing out_dict["Y"] against the matmul
reference. Gate both tests to cuDNN 9.12.0 or newer, matching the existing
backend-version test utilities.
🪄 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: b2dad5e5-72e5-4f05-a570-fc83df54deb7
📒 Files selected for processing (14)
python/cudnn/__init__.pypython/cudnn/frost/device.pypython/cudnn/frost/workspace.pypython/cudnn/gemm/frost/compiler.pypython/cudnn/gemm/frost/tile_config.pypython/cudnn/linear_attention/cutile/gdn_engine.pypython/cudnn/linear_attention/cutile/kda_engine.pypython/cudnn/linear_attention/frost/gdn2_engine.pypython/cudnn/linear_attention/frost/gdn_engine.pypython/cudnn/linear_attention/frost/kda_engine.pypython/cudnn/wrapper.pytest/python/test_decline_types.pytest/python/test_import_boundaries.pytest/python/test_wrapper_graph.py
Comment on lines
+12
to
+26
| def _matmul_graph(**kwargs): | ||
| """A 64x64 half matmul through the fluent wrapper.""" | ||
| with cudnn.Graph( | ||
| handle="auto", | ||
| io_data_type=cudnn.data_type.HALF, | ||
| compute_data_type=cudnn.data_type.FLOAT, | ||
| inputs=["X", "W"], | ||
| outputs=["Y"], | ||
| **kwargs, | ||
| ) as graph: | ||
| X = graph.tensor(name="X", dim=[1, 64, 64], stride=[64 * 64, 64, 1]) | ||
| W = graph.tensor(name="W", dim=[1, 64, 64], stride=[64 * 64, 64, 1]) | ||
| Y = graph.matmul(name="mm", A=X, B=W) | ||
| Y.set_output(True).set_name("Y") | ||
| return graph |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'torch\.cuda\.(is_available|get_device_capability)|cudnn\.backend_version|pytest\.mark\.skipif|pytestmark|support' \
test/python || trueRepository: NVIDIA/cudnn-frontend
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- target file ---'
cat -n test/python/test_wrapper_graph.py
echo '--- conftest support and markers ---'
rg -n -C 5 \
'backend_version|cuda\.is_available|get_device_capability|skipif|pytestmark|L0|Graph|cudnn_version|version' \
test/python/conftest.py test/conftest.py 2>/dev/null || true
echo '--- graph test references and fixtures ---'
rg -n -C 4 \
'test_wrapper_graph|_matmul_graph|tensor_dict|execute.*tensor|backend_version|Graph\(' \
test/python -g '*.py' | head -n 500Repository: NVIDIA/cudnn-frontend
Length of output: 38965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- conftest imports and session hooks ---'
sed -n '1,180p' test/python/conftest.py
echo '--- Graph implementation and version checks ---'
rg -n -C 6 \
'class Graph|backend_version|9\.12|91200|get_device_capability|compute_capability|workspace_alloc' \
python/cudnn test/python -g '*.py' | head -n 800
echo '--- test marker configuration ---'
rg -n -C 4 \
'pytest\.ini|markers|L0|L1|L2|L3|L4' \
pytest.ini pyproject.toml setup.cfg test/python -g '*.ini' -g '*.toml' -g '*.cfg' -g '*.py' | head -n 300Repository: NVIDIA/cudnn-frontend
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate wrapper files ---'
fd -i 'wrapper|graph' . -t f | head -n 200
echo '--- Graph definitions and constraints ---'
rg -n -C 8 \
'class Graph|workspace_alloc|backend_version|9\.12|91200|matmul' \
python test -g '*.py' -g '*.cpp' -g '*.h' -g '*.hpp' | head -n 1000
echo '--- marker definitions ---'
find . -maxdepth 3 \( -name 'pytest.ini' -o -name 'pyproject.toml' -o -name 'setup.cfg' \) -print
rg -n -C 4 'markers|L0|L1|L2|L3|L4' \
pytest.ini pyproject.toml setup.cfg 2>/dev/null || trueRepository: NVIDIA/cudnn-frontend
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- wrapper outline ---'
ast-grep outline python/cudnn/wrapper.py
echo '--- Graph implementation ---'
rg -n -C 12 \
'^class Graph|def __init__|def __enter__|def __exit__|workspace_alloc|backend_version|9\.12|91200|compute_capability' \
python/cudnn/wrapper.py
echo '--- exact version references ---'
rg -n -C 3 '9\.12|91200|9\.11|Graph.*require|requires.*Graph' \
python test docs include -g '*.py' -g '*.md' -g '*.h' -g '*.hpp' -g '*.cpp' | head -n 300
echo '--- L0 configuration and nearby fast tests ---'
find . -maxdepth 3 \( -name 'pytest.ini' -o -name 'pyproject.toml' -o -name 'setup.cfg' \) -print
rg -n -C 3 'L0|level|fast' test/python/README* README.md pytest.ini pyproject.toml setup.cfg 2>/dev/null | head -n 300Repository: NVIDIA/cudnn-frontend
Length of output: 15908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pytest levels ---'
cat -n test/python/pytest.ini | sed -n '1,140p'
echo '--- native graph support checks ---'
sed -n '390,455p' test/python/test_graph_native.py
echo '--- architecture gates for comparable graph matmul tests ---'
rg -n -C 5 \
'Graph\(|pygraph\(|matmul\(|get_device_capability|requires SM|compute capability' \
test/python/test_graph_native.py test/python/test_matmul_bias_relu.py test/python/test_wrapper_graph.py
echo '--- read-only invariant probe ---'
python3 - <<'PY'
from pathlib import Path
import ast
target = Path("test/python/test_wrapper_graph.py").read_text()
wrapper = Path("python/cudnn/wrapper.py").read_text()
tree = ast.parse(target)
marks = [
node for node in ast.walk(tree)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Attribute)
and isinstance(node.func.value, ast.Attribute)
and isinstance(node.func.value.value, ast.Name)
and node.func.value.value.id == "pytest"
and node.func.value.attr == "mark"
]
print(f"target_has_skipif={any(getattr(node.func, 'attr', '') == 'skipif' for node in marks)}")
print(f"target_has_architecture_check={'get_device_capability' in target}")
print(f"target_has_backend_check={'backend_version' in target}")
print("wrapper_requires_91200=" + str("cudnn.backend_version() < 91200" in wrapper))
print("target_declares_L0=" + str("`@pytest.mark.L0`" in target))
PYRepository: NVIDIA/cudnn-frontend
Length of output: 43090
Skip fluent graph tests on cuDNN versions below 9.12.0.
cudnn.Graph raises RuntimeError when cudnn.backend_version() < 91200. Add a pytest.mark.skipif for this condition. The shared CUDA assertion does not provide a skip.
🤖 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 `@test/python/test_wrapper_graph.py` around lines 12 - 26, Add a
pytest.mark.skipif decorator to the fluent graph test helper or its containing
test scope, using cudnn.backend_version() < 91200 as the condition and a clear
reason. Apply it to the tests exercising _matmul_graph, without relying on the
shared CUDA assertion for skipping.
Source: Coding guidelines
Collaborator
|
No labels/conflicts/CI etc. Moving to 1.29 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four independent defects on the public Python graph API. Each is reproducible today, each gets a test that fails without its fix, and none depends on the others.
cudnn.Graph(workspace_alloc=False)has never workedself.__workspace = Falseinsideclass Graphwrites_Graph__workspace, but__exit__probed for it withhasattr(self, "__workspace")— a plain string literal, which Python does not name-mangle. The probe was always False, so__exit__overwrote the sentinel with a freshtorch.empty(get_workspace_size())every time.A caller who asked to own the workspace still paid an allocation per Graph that nothing read — hundreds of MB for a large SDPA graph — and the "Need to specify workspace to execute graph" guard became unreachable, so forgetting
workspace=ran silently on the wrapper's hidden buffer. The kwarg and the broken probe arrived in the same commit (v1.15, 2025-10).cudnn.experimentalraises RecursionErrorThe lazy hook fetched it with
from . import experimental, the exact form the eight-line comment six lines above documents as recursive. Theopsbranch already avoids it withimportlib.import_module; the two branches now share one body so a third submodule cannot pick the wrong form.An environment probe aborted the plan walk instead of declining
build_plans()skips a plan whose build raises one ofdecline_types()and moves to the next entry, ultimately to the cuDNN backend; anything else propagates and takes down the walk. Seven probes reported "no CUDA device is visible" and "the driver did not report MaxSharedMemoryPerBlockOptin / L2CacheSize" asRuntimeError, which is neither a decline type nor caught by the engines' ownbuild_planhandlers. A driver that does not answer a property query therefore failed planning outright instead of falling back.Left as
RuntimeError/ValueErroron purpose: real driver errors, and the caller mistakes (a device ordinal that does not exist, a non-CUDA buffer).Diagnostics pointing at the wrong thing
Two docstrings cited
cudnn/frost/dispatch.pyas the home of the workspace and stream contracts. That file has never existed on this branch — it lives on the gitlab side, and when FROST was cherry-picked over in #476 its responsibilities were absorbed bycudnn/engines/while the prose came along unchanged.Five engines reported an
ImportErrorfrom a probe as "requires the Cutlass DSL" / "requires cuda.bindings". The probe raises for anything missing anywhere in the import chain, not just its target. Debugging one of these cost an import hook to find the real cause — a stray/tmp/cuda.pyshadowing thecudapackage, reported as "requires the Cutlass DSL: No module named 'vllm'", two contradictory halves neither of which pointed at the file.Tests:
test_wrapper_graph.py(new),test_decline_types.py(new),test_import_boundaries.py(extended). Each verified to fail with its fix stashed.Summary by CodeRabbit
Bug Fixes
Documentation
Tests