Make a family a kind of graph, and stop paying for the DSL to decline - #502
Conversation
Four changes, all narrowing an over-broad promise: 1. _GEMM_CLOSURE no longer lists RESHAPE. closed_under is a PROMISE that the family serves every node type in it, and nothing in gemm/frost consumes a RESHAPE node. With it there, `matmul -> reshape` matched, only the matmul was compiled, and execute failed with "the variant pack is missing buffers for ['mm::C']" -- with no backend fallback, because the python engine had already claimed the graph. It now declines at build with the honest "no engine proposed a plan". Plain matmul routing is unchanged. 2. cu_seq_len_q / cu_seq_len_kv become a FACT (has_cu_seq_len) judged by a Capabilities row, instead of poisoning SdpaGraphFacts.invalid. `invalid` means malformed-for-everyone; putting a not-implemented-here feature there would also bar the engine that eventually implements prefix sums. Net eligibility is unchanged: no row sets cu_seq_len=True. 3. EngineRow -> EngineFamily. A family is the unit everything is scoped to: node-type envelope, id block, arch range, maturity gate, facts vocabulary. Splitting one (fp8 SDPA out of SDPA) costs one more entry and nothing elsewhere. Adds the id-block disjointness check that was assumed but never tested -- a shared id would make the engine an autotune result names ambiguous. 4. Facts hang off the graph, scoped per family, and no caller asks for them. validate() walks the manifest, finds the families that claim this graph, runs each one's declared analyzer once and attaches the record; a graph no family claims carries no payload. Engines read that record back instead of parsing again. Facts are family-scoped by construction -- an SDPA fact means nothing to a GEMM engine -- so this is a mapping, never a union record every family would have to widen. The record is keyed by the analyzer callable itself, so the ranking (which resolves it from EngineFamily.analyzer before any engine module is imported) and the engine (which passes the callable it already imports) reach ONE record with no family-name string to keep in sync. That is the drift the backend's SDPA heuristics have, where the feature vector and the engine's own view of the graph are extracted separately. Both SDPA families go through it. The bwd family was reading the same module-private weakref cache as fwd, so this is not removing a duplicate parse -- it is keeping the sharing after the cache moved onto the graph, and making it reachable by the ranking rather than only by engines. That weakref cache is gone and analyze() is now pure. Attaching at validate() exposed a live staleness hole: build_operation_graph -> _sync_ir_shapes_from_backend rewrites IR dim/stride with the backend's inference (channels-last conv and friends), and facts describe layout. The node count is unchanged, so nothing would have caught it; the records are now dropped and re-attached there. Lazy evaluation had hidden this by always running after the sync. Verified on Blackwell (sm100): test_engine_router 62, sdpa graph-analyzer 66.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe router now classifies graphs by engine family, finalizes backend layouts, freezes graphs, and attaches analyzer facts before ranking. SDPA analysis uses cuDNN data types and cached facts. SDPA and FROST exports load lazily. ChangesEngine family routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Graph
participant BackendLayout
participant Manifest
participant Analyzer
participant Router
Graph->>BackendLayout: finalize layout
BackendLayout->>Graph: freeze graph or record decline
Graph->>Manifest: resolve family
Manifest->>Analyzer: resolve and analyze family graph
Analyzer->>Graph: attach cached facts
Graph->>Router: generate ranked plan
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The manifest had two different things wearing one name: three entries owned a
100-wide id block and returned several engines, five owned a single id and
returned one. GDN and KDA graphs matched two entries each, so "which family
serves this graph" had no single answer.
A family is now a KIND OF GRAPH -- roughly what the backend calls an
operation-graph mode, at a granularity of our choosing -- and every graph
belongs to exactly one or to none:
gdn {GDN, GDN_BWD} GdnFrostEngine + GdnCuTileEngine one block
kda {KDA, KDA_BWD} KdaFrostEngine + KdaCuTileEngine one block
gdn2, frost_gemm, frost_sdpa_fwd, frost_sdpa_bwd
Classification is a lookup (_FAMILY_OF_NODE), not N families each declaring a
claim that then has to be proven disjoint: a function returns one value, so
"two families claimed this graph" is not a case that can arise and is not an
invariant anyone has to test. A graph naming two families (a matmul and an sdpa
together) belongs to neither and goes to the backend.
Each family reserves FAMILY_BLOCK ids and never opens a second block, so
engine_id alone identifies the family. Ids are pre-release (engine_ids.py), so
the blocks are re-cut here; the earlier claim that fp8 SDPA could not be split
out because its ids had shipped was simply wrong.
closed_under is deleted. It existed to reject a graph without paying for an
import, and it duplicated a judgment the engine has to make anyway -- which is
how it came to promise RESHAPE support that nothing implemented. The real
defect was in the gemm analyzer: _node_to_recorded_op returned None for an
unrecognized node and the caller SKIPPED it, so any unhandled node type
silently compiled a subgraph and execute then demanded buffers the caller never
bound. It declines now.
That deletion is only safe because declining no longer costs an import. Support
checks were dragging the CuTe DSL in: cudnn/sdpa/__init__ eagerly imported .bwd
and .fwd, and fwd/engines.py -- which holds Capabilities and mismatch, both pure
data -- imported api_dsl at module level to bind EngineSpec.lower. The three
package inits are lazy (PEP 562, the pattern cudnn/__init__ already used) and
the adapter resolves at build time:
import cudnn.sdpa.graph_analyzer 1059 ms, +381 modules -> 9.4 ms, +2
support-check module +1387 modules -> +7
Facts and capabilities now speak cudnn.data_type instead of torch.dtype. Facts
are what every engine of a family reads, so expressing them in one framework's
types would make dispatch require that framework; torch appears only where a
torch tensor is actually allocated (graph_analyzer.to_torch_dtype).
Finally, planning does finalize -> freeze -> analyze in that order, and the
finalize runs the SAME way whether or not an out-of-tree engine was registered.
Lowering to C++ and reflecting layout back is how the graph learns the strides
it will execute with -- a property of the graph, not of whichever engine serves
it. Splitting those paths is what made the frozen snapshot unenforceable: the
registered-engine path lowered later, inside the Router, and
_sync_ir_shapes_from_backend writes through object.__setattr__ specifically to
bypass the freeze. _facts_for() memoizes only a frozen graph, so there is no
invalidation rule left to get wrong.
heuristics_sort still does not read facts. That is deliberate and now says so:
the seam is in place so that writing a real policy -- order a family's engines
on its facts, then merge against the backend on predicted time -- does not mean
re-plumbing the graph first.
Verified on Blackwell sm100: test_engine_router 64, sdpa graph-analyzer 66,
sdpa/frost + gemm/frost 4500 passed / 2119 skipped at -n 32, identical to the
counts before this change. linear_attention fails 286 tests here and on
pristine gh/develop alike (CUDA graph capture), untouched by this.
|
Earlier comments on this PR were deleted rather than left to mislead: they described the pre-rescope design, and one claim in them is now backwards — I wrote that fp8 SDPA could not be split into its own family because its ids had shipped. Two things from those comments are worth keeping, both about measuring these suites: These suites randomize shapes from an unseeded
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
python/cudnn/_pygraph.py (2)
1013-1037: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
_attach_factsdocstring with the single-family lookup.The docstring says the graph "finds its own families" and "hangs one record per family off itself".
manifest.family_for()returns at most one family, so the code attaches at most one record. The plural wording will mislead a reader into expecting multi-family attachment.📝 Proposed doc fix
- Part of planning, not something a caller invokes: the graph finds its - own families through the manifest and hangs one record per family off - itself, as an optional payload. A graph no family claims carries none. + Part of planning, not something a caller invokes: the graph finds its + own family through the manifest and hangs that family's record off + itself, as an optional payload. A graph no family claims carries none.🤖 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/_pygraph.py` around lines 1013 - 1037, The _attach_facts docstring describes multi-family discovery and attachment, but manifest.family_for() returns only one family. Update the docstring to use singular wording throughout, stating that the graph finds its family and attaches at most one family-scoped record, while preserving the existing no-family behavior.
1002-1011: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the backend-decline rollback into one helper.
Lines 1004-1011 and lines 1129-1142 record the same decline and roll back the same five fields. The two sites must stay in step; if one gains a field and the other does not, a half-lowered graph survives and a later
build_operation_graph()walks into the descriptor that already failed. Extract a_record_backend_decline(exc)helper and call it from both.♻️ Proposed refactor
+ def _record_backend_decline(self, exc: Exception) -> None: + """Record a backend decline and roll back any partial lowering. + + A half-lowered graph makes a later build_operation_graph() walk into + the descriptor that just failed.""" + self._backend_declined = exc + self._lowered_graph = None + self._cpp_tensors.clear() + self._cpp_bog_done = False + self._cpp_plans_created = False + self._backend_entries = [] + def _finalize_backend_layout(self) -> None:Then in
_finalize_backend_layout:except (cudnn.cudnnGraphNotSupportedError, RuntimeError, ImportError, AttributeError) as exc: _LOG.warning("backend could not build this graph, treating as a decline: %s", exc) - self._backend_declined = exc - self._lowered_graph = None - self._cpp_tensors.clear() - self._cpp_bog_done = False - self._cpp_plans_created = False - self._backend_entries = [] + self._record_backend_decline(exc)🤖 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/_pygraph.py` around lines 1002 - 1011, Extract the duplicated backend-decline state reset into a `_record_backend_decline(exc)` helper, including logging the exception, storing `_backend_declined`, clearing `_lowered_graph` and `_cpp_tensors`, resetting `_cpp_bog_done` and `_cpp_plans_created`, and emptying `_backend_entries`. Replace the rollback blocks in the `_lower_backend_graph()` exception path and `_finalize_backend_layout` with calls to this helper so both paths remain synchronized.test/python/test_engine_router.py (1)
877-878: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPoint the probe family at a real factory to avoid a cached failure.
"unused_factory"does not exist in this module.manifest.instantiate()therefore raisesAttributeError, logs a WARNING with a full traceback on every passing run, and caches[]under engine id_OOT + 900in the module-level_INSTANCES.monkeypatchrestoresMANIFESTbut not_INSTANCES, so that entry outlives both tests. A later test that reuses id_OOT + 900with a working factory would receive the cached empty list instead.Define a module-level factory that returns
[]and name it here.♻️ Proposed test fix
+def _probe_factory(): + """No engines: the probe family exists to carry an analyzer, not a kernel.""" + return [] + + def _probe_analyzer(graph):Then at both construction sites:
- family = manifest.EngineFamily(_OOT + 900, "probe_family", __name__, "unused_factory", analyzer=(__name__, "_probe_analyzer")) + family = manifest.EngineFamily(_OOT + 900, "probe_family", __name__, "_probe_factory", analyzer=(__name__, "_probe_analyzer"))Also applies to: 920-921
🤖 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_engine_router.py` around lines 877 - 878, Define a module-level probe factory in test_engine_router.py that returns an empty list, then replace the invalid "unused_factory" value with that factory’s name in both EngineFamily construction sites for IDs _OOT + 900 and _OOT + 920/921. Keep the existing analyzer configuration 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 `@python/cudnn/_pygraph.py`:
- Line 151: Update the inline comment on the _facts field to describe that it
stores analyzer records only and reference the _facts_for() accessor instead of
facts(). Do not change the _facts data structure or surrounding logic.
In `@python/cudnn/engines/__init__.py`:
- Line 28: Restore the EngineRow compatibility export in cudnn.engines,
preserving its legacy constructor and behavior through a deprecated adapter;
update the package exports such as __all__ and imports alongside MANIFEST and
EngineFamily so existing cudnn.engines.EngineRow imports continue working.
In `@python/cudnn/engines/manifest.py`:
- Around line 24-32: Update the routing documentation near the stage
descriptions and validate() comments to reflect lookup classification rather
than removed anchors/closed_under matching, state that lookup returns at most
one family (or none), and describe analyzer attachment as occurring during
planning after the graph is frozen. Ensure the comments no longer claim multiple
family claims compete or that validate() resolves analyzers.
In `@python/cudnn/sdpa/graph_analyzer.py`:
- Around line 545-556: Update the stale analyzer lifecycle documentation: in
python/cudnn/sdpa/graph_analyzer.py lines 545-556, describe planning followed by
_freeze() and _attach_facts() instead of validate(); make the same
planning-once-per-frozen-graph correction in lines 10-13. In
python/cudnn/sdpa/fwd/engines.py lines 482-483 and
python/cudnn/sdpa/bwd/engines.py lines 225-226, replace references to the record
validate() attached with planning-attached wording; no behavioral changes are
needed.
- Around line 46-51: Update to_torch_dtype to validate that dt exists in
_TORCH_FROM_CUDNN before indexing it, and raise the project’s typed
decline/unsupported error with the unmapped dtype included when it does not.
Preserve the existing torch dtype conversion for mapped values so
tensor_desc_from_ir can decline unsupported O or Stats dtypes instead of
propagating KeyError.
In `@test/python/test_engine_router.py`:
- Around line 799-943: Add the pytest.mark.L0 decorator to each of the seven new
test functions in this diff: test_classification_is_a_partition,
test_a_graph_spanning_two_families_belongs_to_neither,
test_family_id_blocks_are_disjoint, test_declared_analyzers_are_importable,
test_planning_attaches_facts_without_anyone_asking,
test_ranking_and_engine_read_the_same_record, and
test_facts_are_recomputed_when_the_graph_grows.
---
Nitpick comments:
In `@python/cudnn/_pygraph.py`:
- Around line 1013-1037: The _attach_facts docstring describes multi-family
discovery and attachment, but manifest.family_for() returns only one family.
Update the docstring to use singular wording throughout, stating that the graph
finds its family and attaches at most one family-scoped record, while preserving
the existing no-family behavior.
- Around line 1002-1011: Extract the duplicated backend-decline state reset into
a `_record_backend_decline(exc)` helper, including logging the exception,
storing `_backend_declined`, clearing `_lowered_graph` and `_cpp_tensors`,
resetting `_cpp_bog_done` and `_cpp_plans_created`, and emptying
`_backend_entries`. Replace the rollback blocks in the `_lower_backend_graph()`
exception path and `_finalize_backend_layout` with calls to this helper so both
paths remain synchronized.
In `@test/python/test_engine_router.py`:
- Around line 877-878: Define a module-level probe factory in
test_engine_router.py that returns an empty list, then replace the invalid
"unused_factory" value with that factory’s name in both EngineFamily
construction sites for IDs _OOT + 900 and _OOT + 920/921. Keep the existing
analyzer configuration 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: b929af2b-03e8-4345-b5a1-b2bf04d718a7
📒 Files selected for processing (20)
python/cudnn/_pygraph.pypython/cudnn/engines/__init__.pypython/cudnn/engines/engine_ids.pypython/cudnn/engines/heuristics.pypython/cudnn/engines/manifest.pypython/cudnn/gemm/frost/graph_analyzer.pypython/cudnn/linear_attention/__init__.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/sdpa/__init__.pypython/cudnn/sdpa/bwd/__init__.pypython/cudnn/sdpa/bwd/engines.pypython/cudnn/sdpa/fwd/__init__.pypython/cudnn/sdpa/fwd/engines.pypython/cudnn/sdpa/graph_analyzer.pytest/python/sdpa/frost/test_sdpa_graph_analyzer.pytest/python/test_engine_router.py
Facts no longer read torch.cuda. Compute capability and SM count come from
cudnn.create_device_properties() -- the backend's OWN device descriptor, the
same object the C++ deviceless-AoT path serializes and replays. graph_analyzer
now imports no torch at module level at all; torch appears only where a torch
tensor is actually allocated. This is also the step that makes a deviceless
python engine possible: facts computed from a serialized descriptor need no
live device, whereas torch.cuda.current_device() required one.
frost/buffers.py gains current_device_id() with the same two-probe shape as
current_sm(): a missing cuda-python must not look like a missing GPU.
Review fixes:
- _ATTACHABLE was defined, unused, and its comment described behaviour that
did not exist. Deleted.
- The three lazy __init__ files returned only __all__ from __dir__(), hiding
every normal module attribute including __name__. Union with globals() now.
- linear_attention/frost/__init__ eagerly imported the GDN, KDA and GDN2
engines, so importing one family's engine pulled its neighbours -- which
defeated the per-engine ImportError tolerance the family factories exist to
provide. Lazy now.
- _facts_for() keyed on f"{module}.{qualname}", contradicting its own
docstring and letting a reloaded same-named callable collect another
analyzer's record. Keyed on the callable itself.
- frost/README.md still described backend-first ranking, including the
pseudocode; the implementation has been python-first since the seam landed.
Verified on Blackwell sm100: test_engine_router 64, sdpa graph-analyzer 66,
sdpa/frost + gemm/frost 4500 passed / 2119 skipped at -n 32 -- the same counts
as before this series began.
…t boundaries
Engine ids were assigned in four places: two _ID_OFFSETS tables (sdpa fwd and
bwd) and class attributes on the gemm and linear-attention engines. The manifest
only VALIDATED containment after the fact, so "two engines share a slot" was
possible in a way "two families overlap blocks" was not.
The manifest now assigns. Each family lists its engines as slots:
slots={"sdpa_fwd_prefill_sm100_d128": EngineSlot(0, opt_in=True), ...}
and instantiate() hands the factory {name: engine_id}. Engines carry no id of
their own, so one cannot claim a number it was not given -- the error stops
being caught and starts being unrepresentable. The whole python id space reads
out of one file instead of being reconstructed from four.
opt_in moves with it, from per-family to per-ENGINE. Maturity is a property of
one implementation: the half-precision SDPA engines can now graduate while the
fp8 engine is still maturing, which one flag per family made impossible. It
stays in the manifest rather than on the engine class because the whole point
of the gate is to know what to offer WITHOUT importing the engine.
register_backend() is now only what its docstring always claimed. In-tree
engines never registered -- the manifest discovers them -- but eleven tests
still called it, and an "the in-tree owner may register itself" exemption
existed to keep that working. Every one of those calls was redundant (the
cuTile-declines test already asserts against the ranked plan list by name), so
they are gone, and with them the exemption, its test, and the namespace-
containment patch this series had added to keep it alive. The check is now one
rule: ids below OUT_OF_TREE_ID_BASE are rejected.
Two tests replace what was runtime luck:
- test_every_engine_spec_has_a_manifest_slot: an engine added without a slot
would silently never be built. Checked both ways, plus slot uniqueness and
range, on CPU.
- test_import_boundaries.py: what each dispatch stage may drag in. The graph
API must not require a framework, and deciding whether an engine COULD serve
a graph must not import the machinery that would serve it -- deleting
closed_under is only safe while that holds. Each check runs in a FRESH
interpreter (a module imported by the test process would make an in-process
assertion pass for the wrong reason) and measures the DELTA against an empty
one (nvidia_cutlass_dsl is injected at startup by a .pth, so an absolute
check blames us for what the interpreter did first).
Making that pass took one more step: fwd/engines.py and bwd/engines.py still
imported torch at module level for lowering helpers that share the file with
Capabilities and mismatch(). Deferred, so the support-check modules now pull
110 modules instead of 1103, and neither torch nor cutlass.
Verified on Blackwell sm100: test_engine_router 64 + test_import_boundaries 5,
sdpa graph-analyzer 66, sdpa/frost + gemm/frost 4568 passed / 2119 skipped
at -n 32.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_engine_router.py`:
- Around line 909-911: Apply Black formatting to both EngineFamily fixture
constructors in test/python/test_engine_router.py at lines 909-911 and 954-956,
wrapping their arguments across multiple lines while preserving the existing
values and behavior.
In `@test/python/test_import_boundaries.py`:
- Line 36: Rename the ambiguous loop variable in the stdout line-selection
expression to a descriptive name such as line, updating its references
consistently so Ruff E741 passes.
- Around line 33-35: Update the probe execution handling around subprocess.run
so a non-zero return code fails the test instead of calling pytest.skip. Keep
skipping only for an unavailable probe interpreter if that case is explicitly
distinguishable, and ensure failures from the probe code, including import
errors, are surfaced with stderr details.
🪄 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: ba1c1344-c04f-4a81-8754-0dafdf00e8ab
📒 Files selected for processing (22)
python/cudnn/_pygraph.pypython/cudnn/engines/manifest.pypython/cudnn/gemm/frost/engine.pypython/cudnn/linear_attention/__init__.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/sdpa/bwd/engine.pypython/cudnn/sdpa/bwd/engines.pypython/cudnn/sdpa/fwd/engine.pypython/cudnn/sdpa/fwd/engines.pytest/python/gemm/frost/test_frontend_integration.pytest/python/linear_attention/frost/test_gdn2_bprop_kernel.pytest/python/linear_attention/frost/test_gdn2_prefill_kernel.pytest/python/linear_attention/frost/test_gdn_bprop_kernel.pytest/python/linear_attention/frost/test_gdn_prefill_kernel.pytest/python/linear_attention/frost/test_kda_bprop_kernel.pytest/python/linear_attention/frost/test_kda_prefill_kernel.pytest/python/test_engine_router.pytest/python/test_import_boundaries.py
💤 Files with no reviewable changes (12)
- test/python/linear_attention/frost/test_gdn2_bprop_kernel.py
- python/cudnn/linear_attention/frost/gdn_engine.py
- python/cudnn/linear_attention/frost/kda_engine.py
- test/python/linear_attention/frost/test_gdn_prefill_kernel.py
- python/cudnn/linear_attention/frost/gdn2_engine.py
- python/cudnn/linear_attention/cutile/kda_engine.py
- python/cudnn/linear_attention/cutile/gdn_engine.py
- test/python/linear_attention/frost/test_gdn2_prefill_kernel.py
- test/python/linear_attention/frost/test_kda_prefill_kernel.py
- python/cudnn/_pygraph.py
- test/python/linear_attention/frost/test_kda_bprop_kernel.py
- test/python/linear_attention/frost/test_gdn_bprop_kernel.py
🚧 Files skipped from review as they are similar to previous changes (4)
- python/cudnn/linear_attention/init.py
- python/cudnn/sdpa/bwd/engines.py
- python/cudnn/sdpa/fwd/engines.py
- python/cudnn/engines/manifest.py
| family = manifest.EngineFamily( | ||
| _OOT + 900, "probe_family", __name__, "unused_factory", slots={"probe": manifest.EngineSlot(0)}, analyzer=(__name__, "_probe_analyzer") | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Format the EngineFamily fixture constructors with Black.
These constructors exceed the configured 160-character line length.
test/python/test_engine_router.py#L909-L911: Wrap theEngineFamilyarguments with Black formatting.test/python/test_engine_router.py#L954-L956: Apply the same formatting to the duplicate fixture constructor.
📍 Affects 1 file
test/python/test_engine_router.py#L909-L911(this comment)test/python/test_engine_router.py#L954-L956
🤖 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_engine_router.py` around lines 909 - 911, Apply Black
formatting to both EngineFamily fixture constructors in
test/python/test_engine_router.py at lines 909-911 and 954-956, wrapping their
arguments across multiple lines while preserving the existing values and
behavior.
Source: Coding guidelines
to_torch_dtype() indexed its map directly, so an unmapped cudnn.data_type raised KeyError deep in lowering. Only Q's dtype is capability-checked, and tensor_desc_from_ir() runs on every bound tensor, so O / Stats / a side output can still carry one. It declines now. Two docstrings still named validate() as the site that runs the analyzer; planning does, after the freeze.
|
Thanks — verified each against HEAD rather than the commit it was filed on, since three were already addressed by later commits in the series. Fixed (
Already fixed before the review landed — these were filed against
Not doing: the
More to the point, a compatibility adapter could not preserve the old behaviour even if we wanted it to. The PR description records this under API impact. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@python/cudnn/sdpa/graph_analyzer.py`:
- Around line 11-14: Update the module docstring wording near the SDPA family
names to use the grammatically complete phrase “callable for the SDPA family
names,” without changing the surrounding documentation.
🪄 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: 2d9b674c-7180-4b3b-9460-f4a73a995d60
📒 Files selected for processing (2)
python/cudnn/engines/manifest.pypython/cudnn/sdpa/graph_analyzer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cudnn/engines/manifest.py
| callable the SDPA family names in ``engines/manifest.py``; PLANNING runs it once | ||
| per graph -- after the backend's layout inference has landed and the graph is | ||
| frozen -- and attaches the record, so the ranking and the engine share it rather | ||
| than each parsing the graph. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the module docstring grammar.
The phrase callable the SDPA family names is incomplete. Rewrite it so the relationship is explicit, for example by changing it to callable for the SDPA family names.
Proposed fix
-callable the SDPA family names in ``engines/manifest.py``; PLANNING runs it once
+callable for the SDPA family names in ``engines/manifest.py``; PLANNING runs it once📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| callable the SDPA family names in ``engines/manifest.py``; PLANNING runs it once | |
| per graph -- after the backend's layout inference has landed and the graph is | |
| frozen -- and attaches the record, so the ranking and the engine share it rather | |
| than each parsing the graph. | |
| callable for the SDPA family names in ``engines/manifest.py``; PLANNING runs it once | |
| per graph -- after the backend's layout inference has landed and the graph is | |
| frozen -- and attaches the record, so the ranking and the engine share it rather | |
| than each parsing the graph. |
🤖 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/sdpa/graph_analyzer.py` around lines 11 - 14, Update the module
docstring wording near the SDPA family names to use the grammatically complete
phrase “callable for the SDPA family names,” without changing the surrounding
documentation.
family_for() took an `sm` and folded availability into its answer, so None meant either "not that kind of graph" or "no engine for it here" and a caller could not tell which. What kind of graph something is cannot depend on the machine: classification is now pure, and EngineFamily.offered_ids(sm) answers availability separately. A matmul graph is a gemm graph on a host with no gemm engine at all. _FAMILY_OF_NODE becomes _ANCHOR_NODE_TO_FAMILY. The old name read as "every node maps to a family", when the table holds only the node types that NAME one -- POINTWISE, REDUCTION, a type added tomorrow are absent on purpose and ignored, which is why `matmul + pointwise` is a gemm graph. Whether a family can serve the WHOLE graph stays its analyzer's judgment; a coarser copy of that here is what closed_under was. The comment now says so at the table rather than leaving it to be discovered. test_classification_does_not_depend_on_the_machine pins it: flipping the opt-in flag or asking about another arch changes what is OFFERED and not what the graph IS. Verified on Blackwell sm100: test_engine_router 65 + test_import_boundaries 5, sdpa/frost + gemm/frost 4500 passed / 2119 skipped at -n 32.
Capabilities.arches was an exact set -- frozenset({(10, 0), (10, 3)}) -- checked
with `facts.device_cc not in capabilities.arches`. An sm100 kernel runs on the
whole sm100 line, so enumerating the members that exist today silently declines
the ones that ship later: Rubin (sm107) and Thor (sm110) are meant to reuse
these kernels and both were excluded. It is a range now, inclusive, encoded
major*10 + minor as engines/manifest.py already did, and the decline message
says "requires SM100-119" instead of listing device families.
The manifest's own sm_lo/sm_hi is deleted, for the reason closed_under was:
a coarser duplicate of a judgment the engine has to make anyway, which is a
second thing to maintain and a place to lie. It already lied, and in exactly
the way that matters here -- frost_gemm capped at SM103 while kernel_registry
declares PIPELINE_ARCH_RANGES["sm100"] = ((100, 120),) with the comment "sm100
templates use only family-portable Blackwell instructions"; frost_sdpa_bwd
capped at SM121 against a Capabilities row of 120-129. On Rubin the gemm family
would not have been offered at all, so its engine never got asked.
Deciding an engine is wrong for a device is now said once, by the engine.
Dropping the manifest copy costs one module import before a decline, which the
laziness work already made cheap: measured on an sm90 host, the SDPA family
instantiates its seven engines and they decline, pulling no CuTe DSL
(test_import_boundaries.py holds that). current_sm() leaves the dispatch path
entirely -- engines_for(graph) and _attach_facts no longer probe the device,
so there is one less way for device state to reach classification.
The SDPA test suites had five hand-copied _is_sm100 gates, every one pinned to
exactly (10, 0), so all five skipped on sm103 while the engines they test serve
the line; only the MXFP8 file had it right. conftest.py now carries one
requires_blackwell / requires_blackwell_geforce / requires_dsl, aligned with
what the engines declare rather than re-derived per file.
Verified on Blackwell sm100: test_engine_router 65 + test_import_boundaries 5,
sdpa/frost + gemm/frost 4500 passed / 2119 skipped at -n 32.
…dded Eight test files each defined their own arch and DSL gates -- five copies of _is_sm100 all pinned to exactly (10, 0), and _dsl_deps_available / _dsl_available / _require_dsl for one `import cutlass`. They now come from frost_test_utils, matching the convention gemm_test_utils already set, so the gate is stated once and against what the engines declare. Comments trimmed to the load-bearing facts; the measurements and the history behind them are in the commit messages of this series where they belong. One of them had already gone stale -- EngineFamily's docstring still listed "the arch range" among what a family owns, one commit after that was deleted.
Deferring the DSL import past check_support moved a failure from discovery to
execution, and the two stages do not catch the same things. Without the cutedsl
extra installed, check_support passed (it reads Capabilities and facts only),
then build_plan raised ImportError -- which is in neither the engine's except
list nor decline_types(), so build_plans() propagated it instead of walking on.
The backend was in the same ranked list and never got its turn. Before this
series the DSL was imported at module scope, instantiate() caught the
ImportError, and the family simply vanished; the laziness work broke that
without replacing it.
Two answers, in the right order:
- The engine now DECLINES at check_support, using probes that do not execute
the module: importlib.util.find_spec("cutlass") (7 ms, 30 modules) and
importlib.metadata.version (5 ms, 2), against ~4.3 s and 1410 modules for the
real import. A plan that cannot be lowered no longer enters the ranked list
at all.
- ImportError joins decline_types() and the engines' build-time except clause,
as the second line rather than the only one.
The CuTe primitives these engines lower through need 4.7.0, so the version is
part of the same check -- an older DSL fails during codegen naming a missing
attribute rather than a version. Two wrinkles that cost a first attempt:
pyproject declares nvidia-cutlass-dsl while this box had
nvidia-cutlass-dsl-internal, so a single-name lookup finds nothing on one of
the two; and internal RCs number themselves independently ("0.3.0+2026..."),
so the public floor cannot judge them -- my first version declined the very
build every test here had been passing on. Unparsable, absent, and internal all
count as not-too-old: refusing on a string we failed to read would reject a
machine that works.
Verified on the PUBLIC wheel, which nothing had run against before: swapped
nvidia-cutlass-dsl-internal 0.3.0 for nvidia-cutlass-dsl 4.7.0 and got
4500 passed / 2119 skipped on sm100, identical to the internal RC.
The cutedsl extra asked for >=4.5.0 while the primitives these engines lower through need 4.7.0, so pip would install a version support checks then decline. CUTEDSL_MIN_VERSION and this bound are the same number now.
|
Latest round — two fixed, two not applicable. Fixed
Not applicable
Also worth flagging, since it came out of a question about this PR rather than from review: deferring the DSL import past |
Deferring torch put `import torch` inside one branch of the execute path while a later, independent branch also used it: an fp8 graph takes the second without the first, so `carver.take(facts.b, torch.int32)` raised UnboundLocalError. 30 tests in test_mhas_v2 failed on it. Only that suite could see it. sdpa/frost pins an engine and exercises the kernel directly; test_mhas_v2 goes through routing, which is what reaches synth_kv_padding. The other three deferred imports are unconditional at function scope and were checked. test_mhas_v2 on sm100: 2178 passed / 707 skipped, and FROST serves 413/3201 graphs across four engines (d128 182, d256 122, d128_fp8 55, d192_d128 54) -- so family consolidation, manifest-assigned ids, the arch range and the DSL laziness leave routing intact.
| [project.optional-dependencies] | ||
| cutedsl = [ | ||
| "nvidia-cutlass-dsl[cu13]>=4.5.0", | ||
| "nvidia-cutlass-dsl[cu13]>=4.7.0", # the CuTe primitives these engines lower through |
There was a problem hiding this comment.
No — you're right, and it's reverted in the next push.
Pinning the extra to >=4.7.0 would make cudnn-frontend incompatible with anything holding the DSL back: quack-kernels pins ==4.6.0, and vLLM and friends carry their own constraints. The extra stays >=4.5.0.
The 4.7.0 floor is real — it's what the CuTe primitives these engines lower through need — but it belongs at runtime, not in the dependency:
- engine:
check_support()declines when the installed DSL is older, so the graph goes to the backend and only those engines are lost. - tests: the same predicate, so an older DSL makes the suite skip rather than fail.
Both read CUTEDSL_MIN_VERSION from cudnn/frost/buffers.py, so there's one number rather than two that can drift.
The probe doesn't cost anything either — importlib.util.find_spec (7 ms) and importlib.metadata.version (5 ms), neither of which executes the module, against ~4.3 s and 1410 modules for a real import cutlass.
Pinning the cutedsl extra to >=4.7.0 (the previous commit) would make cudnn-frontend incompatible with anything holding the DSL back -- quack-kernels pins ==4.6.0, and vLLM and friends carry their own constraints. Reverted to >=4.5.0, with the reason at the pin rather than in a commit nobody will find. Reviewer flagged the same thing. The 4.7.0 floor is real, so it lives at runtime where it costs only the engines that need it: check_support() declines an older DSL and the graph goes to the backend. The test gate now uses the same predicate and the same constant, so an older DSL makes the SDPA suite SKIP rather than fail -- it had checked presence only, which would have run the tests and let them fail for a reason the suite already knew. sm100: sdpa/frost + gemm/frost + test_mhas_v2 = 6678 passed / 2826 skipped, FROST serving 413/3201 graphs.
|
/bot run |
|
@cudnn-ci-bot run frost |
|
🚀 Running mirror pipeline Branch: cudnn-gh/pr-502-9c4fa64 |
Anerudhan
left a comment
There was a problem hiding this comment.
Few things:
- Can you check-in a design doc so it is easier for agents in future..
- Can you update your agents to create labels for your PR..
- Use the PR template for license.
Aside on the code:
- I am still vague about how the stream/current_device works. Is it a fallback if user does not provide the device?.
docs/python_graph_and_execution_backends.md described the shape this PR replaces, so a reader following it would have looked for anchors and closed_under that no longer exist. Reviewer asked for a design doc; this one has been checked in since NVIDIA#336 and just needed to catch up. Four sections added -- what the manifest decides and what it deliberately leaves to the engine, how facts are attached and why after the freeze, how handle/stream/device actually resolve, and which imports each dispatch stage may pay for. Existing sections corrected where the PR moved them: ImportError joins the decline types, engines no longer declare their own id, register_backend is out-of-tree only. The stream question specifically: _resolve_stream is a fallback for having NO handle, not for a handle whose stream could not be read -- that raises, since running on the wrong stream is a correctness bug rather than a degradation.
|
All four — thanks. Design doc. There is one, Four sections added: what the manifest decides and what it deliberately leaves to the engine; how facts are attached and why after the freeze; how handle/stream/device resolve; and which imports each dispatch stage may pay for. Existing sections corrected where this PR moved them. Labels. Added: PR template. My fault — I wrote the body from scratch and dropped the checklist. Restored, license included. Stream / current_device. Three separate things, and the short answer to your question is: it's a fallback for having no handle, not for a handle we failed to read.
That last one has a second payoff worth flagging: facts computed from a serialisable descriptor need no live device, which is the precondition for a python engine to participate in deviceless AoT at all. It cannot today. Relatedly, #506 (separate PR, off |
Before submitting
pre-commit runand committed any formatting changes.cat-cleanup,mod-frontend,mod-frost,orig-nv-eng.Affected area
Python API or bindings
Summary
Cleanup of the python engine dispatch that landed with #476. Each change deletes a declaration that duplicated a judgment made elsewhere — and every one of those duplicates was already wrong.
An sm100 kernel now says it serves the sm100 line
Capabilities.archeswas an exact set,frozenset({(10, 0), (10, 3)}), checked withdevice_cc not in arches. Rubin (sm107) and Thor (sm110) are meant to reuse these kernels and both were excluded. It is a range now —SM100-119— and the decline message says so instead of listing device families.The manifest carried a second, narrower copy, which is deleted:
frost_gemmPIPELINE_ARCH_RANGES["sm100"] = ((100, 120),), commented "family-portable Blackwell instructions"frost_sdpa_bwdCapabilities120-129On Rubin the gemm family would not have been offered at all, so its engine never got asked. Whether an engine suits a device is said once now, by the engine.
A family is a kind of graph, not a group of engines
Eight manifest entries were two different things wearing one name: three owned a 100-wide id block and returned several engines, five owned a single id and returned one. GDN and KDA had two entries each declaring identical anchors, so those graphs were claimed by two families.
Six families now, and classification is a lookup (
_ANCHOR_NODE_TO_FAMILY) rather than N families each declaring a claim that then has to be proven disjoint.family_for(graph)is a pure property of the graph — nosm, no environment — because what kind of graph something is cannot depend on which machine is asking. Availability is a separate question.The manifest assigns engine ids
Ids were assigned in four places (two
_ID_OFFSETStables, two class attributes) and the manifest only validated containment afterwards.instantiate()hands each factory the ids its engines are to use; engines declare none.opt_inmoves with them, per engine — the half-precision SDPA engines can graduate while the fp8 one matures.register_backend()is now only the out-of-tree escape hatch its docstring always claimed. Eleven tests called it on in-tree engines, all redundant since the manifest discovers them; with them go the "in-tree owner may register itself" exemption and its test.closed_underis deleted, and the RESHAPE bug is fixed at its sourceIt promised gemm served RESHAPE when nothing did, so
matmul -> reshapewas claimed and then failed inexecutedemanding a buffer the caller never bound. The real defect was in the gemm analyzer:_node_to_recorded_opreturnedNonefor an unrecognized node and the caller skipped it, so any unhandled node type silently compiled a subgraph. It declines now.Deciding costs no framework
That deletion is only safe because declining stopped costing an import. Three package
__init__s are lazy (PEP 562) and the DSL adapter resolves at build time:import cudnn.sdpa.graph_analyzerFacts and capabilities speak
cudnn.data_typerather thantorch.dtype, and the device comes fromcudnn.create_device_properties()— the backend's own descriptor, the same object the C++ deviceless-AoT path serializes.graph_analyzerimports no torch at all now.test_import_boundaries.pyholds the line, in a fresh interpreter, measuring the delta against an empty one.A missing or too-old CuTe DSL is a decline at check_support, via probes that never execute the module (
find_spec7 ms,importlib.metadata5 ms, against ~4.3 s for the real import).ImportErroralso joinsdecline_types()as a second line of defence — without it, deferring the import had turned "extra not installed" from "the family is absent" into an exception that skipped past the backend sitting in the same ranked list.Why
Every declaration this deletes duplicated a judgment made elsewhere, and every one of the duplicates was already wrong:
closed_underpromised RESHAPE support nothing implemented, the manifest's arch range cappedfrost_gemmat SM103 against templates declaring[100, 120), andCapabilities.archesenumerated device families rather than the line, excluding Rubin and Thor. The shared shape is allow-by-default over an open set — a coarse copy that says yes where the real check would say no.Related issues
Follows #476 (FROST engines). Related to #506, which fixes a CUDA-graph-capture bug in
buffers.probe()that this investigation surfaced.API and compatibility impact
None for callers.
EngineFamily, the manifest,_attach_facts/_facts_forare internal; lazy exports (from cudnn.sdpa import SdpafwdSm100D256) are preserved by PEP 562. Python engine ids are renumbered, whichengine_ids.pydocuments as permitted while pre-release — no API could return one beforecreate_execution_plan()learned about them.EngineRowis gone rather than deprecated: it arrived with #476 two days before this PR, has never been in a release, and carriedanchors/closed_under, both deleted by design — a shim preserving them would be a new place to lie.pyproject's cutedsl extra stays at>=4.5.0. The engines need 4.7.0, but pinning that here would make cudnn-frontend incompatible with anything holding the DSL back —quack-kernelspins==4.6.0. The floor lives at runtime instead, where it costs only the engines that need it:check_support()declines an older DSL and the graph goes to the backend, and the test gate uses the same predicate so the suite skips rather than fails.Behavioural:
matmul -> reshapedeclines at build instead of failing insideexecute. That path could not have worked before.Testing
Blackwell sm100:
The 4500/2119 split is identical to the counts before this series began.
test_mhas_v2matters separately: it is the only suite that goes through routing rather than pinning an engine and exercising the kernel, and it reports which engine served each graph. FROST serves 413/3201 graphs across four engines (d128 182, d256 122, d128_fp8 55, d192_d128 54), so consolidating families, moving id assignment into the manifest, widening the arch declaration and deferring the DSL leave routing intact.It also caught a bug the other suites structurally could not: deferring
torchhad put the import inside one branch of the execute path while a later, independent branch used it too, so an fp8 graph hitUnboundLocalError. Thirty tests.sdpa/froststayed green throughout because it never reaches that path.On the public wheel, which nothing had been verified against — this box ships
nvidia-cutlass-dsl-internalwhilepyprojectdeclaresnvidia-cutlass-dsl. Swapped internal 0.3.0 for public 4.7.0: 4500 passed / 2119 skipped, identical.Across architectures, which matters because deleting the manifest's arch filter opened a path sm100 cannot exercise — the engines are now imported and must decline themselves:
While there: twelve gemm registry tests had no arch gate at all and fail on any non-sm100 host on develop today; they now carry
requires_sm100. The SDPA suite's five hand-copied_is_sm100gates, each pinned to exactly(10, 0)and so skipping on sm103, are one marker aligned with what the engines declare.test/python/linear_attentionfails 286 / passes 67 here — the same 286/67/1769 on pristinegh/develop, verified in a separate worktree. CUDA graph capture, untouched by this PR.Design doc
docs/python_graph_and_execution_backends.mdis updated in this PR — it has been checked in since #336 but described the shape this replaces. Four sections added: what the manifest decides and what it leaves to the engine, how facts are attached and why after the freeze, how handle/stream/device resolve, and which imports each dispatch stage may pay for.Known gaps
create_execution_plan(engine_id, knobs), the explicit replay entry point, does not go through finalize/freeze/attach; onlycreate_execution_plans()does.key()can retry backend lowering after facts were attached._freeze()is shallow:MappingProxyType(dict(node.params))freezes the mapping, not list-valued params.register_backend()/set_router()still gate on_planning_done, so an engine registered afterwards is ignored by the cached candidate set.heuristics_sortstill does not read facts. Deliberate, and its docstring says what will: order a family's engines on its facts, then merge against the backend on predicted time.