Repository navigation
fix(hipdnn): make the stage 2 mining tools work on real corpora - #12941
yassinsolim wants to merge 9 commits into
Conversation
…aph features An fp32 SDPA graph aborted the whole --graphs source with 'unknown dtype spelling'. Recognised out-of-family dtypes (fp32, fp64) are now recorded as excluded with a reason in <out stem>.excluded.json and counted; an unrecognised spelling still fails. Operand UIDs are read from the node's inputs/outputs as hipDNN graphs bind them, so a bound sink is mined as use_sinks=true. Each graph shape carries _graph_features: the operand layouts (BSHD/BHSD from strides) and every bound feature the request has no field for (varlen seq_len, paging, attn_mask, sinks, dropout, stats). Features take part in shape identity, so a BHSD or varlen graph no longer merges into its dense twin.
…cline boundary
A ValueError from the spec factory is the dispatcher's own decline: the
gfx942 and gfx950 dense candidates' support() return it as (False, msg).
dispatch_parity now reports it as [refused] under --report-gaps instead of
aborting the corpus; request construction failures and any other factory
exception stay operational (exit 2).
Mined graphs outside the profile's new graph_contract (layouts, admitted
features) are reported as [out_of_contract], since the request cannot carry
them. reconcile_applicability skips them in the comparison, accepts a
merged multi-source corpus (a reused graph name only fails when a --declines
key uses it; provenance paths are accepted as keys) and warns on an empty
source instead of failing.
dispatch_parity canonicalises runtime-shape fields by default when the
profile maps runtime_param_fields to canonical values: fields the spec's own
runtime_param_fields lists are set to those values and identical specs
collapse into one catalog entry. --per-shape keeps the old one-kernel-per-shape
output. The gfx950 profile now uses the mapping form (B1/Sq512/Skv512) and
declares graph_contract {layouts: [BSHD], features: []}.
… variant's factory dense_spec_for_request pins the auto variant (256/64 tile), so a shape only the bm128 sibling variant serves (Sq=128, Skv=256, non-causal) was refused, and reconcile reported 12 such corpus shapes as unreconciled although the library's dispatcher and the shipped bm128 catalog tiles serve them. dispatch_parity gains dispatch.spec_attribute, naming the attribute of a dispatcher result that holds the spec, and the gfx950 profile now uses dispatch.attention.dispatch_attention. On the real corpus the other 785 servable shapes resolve to the identical spec; the 12 become servable.
…shape position
Kernel names encoded every scalar spec field plus a running index _vN, so
re-mining the corpus in another order renamed kernels and churned the
descriptors and the kpack. The compact form then needed one opaque tag
knob set per kernel (141 on the gfx942 run).
Names are now the slug plus prefix{value} for each of the profile's
metadata fields (the catalog key), and the kernel list is sorted by name,
so a reordered corpus emits an identical config. Kernels the key cannot
tell apart get a digest of their spec, with a warning: same metadata is
the same catalog key to the engine. On the real corpus the gfx942 parity
config compacts to one variant group with one knob set.
…s source A rocKE trace asks for sinks through the use_sinks request field and has no _graph_features, so the contract never saw it. The gfx950 dispatcher serves those shapes and the catalog got a use_sinks=true kernel under the same catalog key as its sinkless twin, although the engine's graph_match declines sinks. contract_refusal now treats use_sinks as the bound sink_token feature for every shape, so a profile whose graph_contract does not admit sink_token reports such a shape as out of contract. The dispatcher is unchanged: the kernel may support sinks, the engine does not. On the real corpus the gfx950 catalog-key collision is gone (16 sink trace shapes out of contract, 781 servable, no _h digests); gfx942's 13 sink shapes no longer read as served only by this integration.
…ists for
knob_sweep built every arm by mutating the dispatcher's resolution of each
shape, so a shape the dispatcher's own tile refuses (Sq % 256 == 128,
Skv % 64 == 32) never reached any arm, and every tile arm reported the
same served count.
Such a shape is now probed at canonical multiples of the profile's
runtime_param_fields that keep the real values' equality and order. When
the dispatcher serves the probe with a spec that reads every runtime field
as a kernel argument, each arm builds that spec at the real values with its
own knobs, and the spec class and predicate decide whether the arm serves
it. Arms report COVERAGE separately from NARROWED, since coverage shapes
have no parity time.
runtime_param_fields entries may now name the request field
({canonical: 512, request: seqlen_k}); the gfx950 profile does so for
seqlen_kv. NARROWED now counts shapes, not deduplicated kernels.
launch_surface.find_repo_root looked only for a .git ancestor and otherwise returned the start directory. In a git archive extract that start is IngestorGenerator/tools, so dispatch_parity resolved the profile's repo-relative provider_root under tools/ and every real-dispatcher test failed with 'tools/dnn-providers/... does not exist'. Without .git it now takes the nearest ancestor holding projects/hipdnn. The real-dispatcher tests skip, with the reason, only when the rocKE tree is genuinely absent.
…pinned knob
--knobs pinned the tile after the policy-owned metadata had been resolved
at the dispatcher's default tile. On gfx942 v_row_pad follows block_n, so
the block_n=32 catalog said v_row_pad 0 where the binary is built with 32,
and hkp_pack refused it (942:S6-16).
Each knob combination is now applied to the spec first; the builder's
spec class and the predicate validate it (a refused combination is dropped
and reported), and only then is the metadata resolved from that spec.
--knobs also accepts a list of mappings that names each combination to
emit, since an approved tile set such as {256/64, 128/32} is not a
cross-product (942:S6-9).
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Output-tensor validation and --per-shape catalog semantics remain incorrect in edge cases.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Improves Stage 2 hipDNN corpus tooling to handle real-world graphs, dispatch refusals, runtime canonicalization, and knob coverage.
Changes:
- Preserves graph features and records unsupported dtypes as exclusions.
- Adds graph-contract reconciliation, stable catalogs, and canonical runtime fields.
- Improves knob probing, metadata resolution, and repository-root discovery.
| File | Description |
|---|---|
tools/reconcile_applicability.py |
Handles empty, merged, and out-of-contract corpora. |
tools/mine_shapes.py |
Mines layouts/features and records dtype exclusions. |
tools/launch_surface.py |
Finds repository roots without Git metadata. |
tools/knob_sweep.py |
Probes coverage shapes for alternate knob arms. |
tools/dispatch_parity.py |
Adds contracts, canonicalization, stable naming, and knob validation. |
tests/test_reconcile_applicability.py |
Tests refusal and merged-corpus reconciliation. |
tests/test_mine_shapes.py |
Tests exclusions and graph-feature mining. |
tests/test_launch_surface.py |
Tests archive root discovery. |
tests/test_knob_sweep.py |
Tests coverage-shape probing. |
tests/test_dispatch_parity.py |
Tests contracts, catalogs, dispatch, and knobs. |
configs/gfx950_attention_dense.profile.yaml |
Uses full dispatch and declares runtime/graph contracts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #12941 +/- ##
========================================
Coverage 70.68% 70.68%
========================================
Files 2940 2940
Lines 484570 484570
Branches 69880 69880
========================================
+ Hits 342485 342486 +1
- Misses 118190 118191 +1
+ Partials 23895 23893 -2
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
BrianHarrisonAMD
left a comment
There was a problem hiding this comment.
Thanks for running this, and bringing the fixes back!
We are planning to remove dispatch_parity.py, reconcile_applicability.py since they break frequently, and integrate at a level we can't rely on (constantly drift as rocKE updates).
I think this content can be removed from this PR (since we will follow up with removing these pieces):
- All changes to tools/dispatch_parity.py, tools/reconcile_applicability.py, tests/test_dispatch_parity.py and tests/test_reconcile_applicability.py. These are about 500 lines of the diff.
- That covers the declined/refused/out-of-contract handling, --per-shape, the --knobs pin ordering, dispatch.spec_attribute, the new kernel-name format and --declines keys.
- The open Copilot comment on --per-shape deduplication goes away with them.
- The graph_contract block and the dispatch, runtime_param_fields and metadata_fields changes in configs/gfx950_attention_dense.profile.yaml. The follow-up removes those blocks.
- coverage_probes, runtime_fields and the probes and gained plumbing in knob_sweep._arm, along with the COVERAGE output and its tests. They call the dispatcher through resolve_shapes, which conflicts with the future direction where knob_sweep depends only on predicate and arch_spec.
- The mine_shapes_graph_features and graph_contract plumbing, unless something outside the deleted tools consumes it. As far as I can tell nothing does, so it would be dead code after the follow up to remove these pieces.
|
Superseded by #12958, which combines this PR with #12940 and #12944 as requested. Following the review: every change to Kept in #12958: the |
…ing (#12943) Closes #12936 ## Motivation On the gfx942 (MI300A) create run of the `hipdnn-ingestor-engine` runbook, stage 2 could not mine the kernel owner's dense prefill benchmark. Its shape list is `_configs()` inside `benchmark_dense_prefill_live.py`, the script imports torch at module scope, and `--dry-run` writes no JSON, so `mine_shapes --rocke-bench` had nothing to read. The run transcribed 48 rows by hand. The gfx950 benchmark has the same structure. ## Technical Details Two commits. `dnn-providers/hip-kernel-provider/rocke/library/benchmarks/gfx942|gfx950/attention/prefill/benchmark_dense_prefill_live.py`: - New `--emit-shapes PATH`. It writes every shape the benchmark would measure as JSONL in the trace schema `mine_shapes --rocke-bench` reads, then exits. It needs no GPU and no torch. It respects the existing `--dtype`, `--hq`, `--hkv` and `--d` filters. - gfx942 emits modes all, swa and persistent (`all` leaves out the other two). gfx950 emits `--mode all`, which covers every mode there. gfx950's packed varlen rows (more than one sequence, the benchmark's own rule) are written with `varlen: true` and `seqlens`. - torch is imported only in `_bench_stream_handle`, `bench_dense` and `main` after the emit branch. gfx942 also defers its builder harness import, because `builders/gfx942/attention/prefill/attention_dense_prefill.py` imports torch. That harness is unchanged. Every other hunk in the benchmark diffs is one of these edits. - The emitted file is not committed, so it cannot drift from `_configs()`. `projects/hipdnn/tools/IngestorGenerator/tools/mine_shapes.py` (`from_rocke_bench`): - An explicit boolean `causal` key is honoured. Before, the 2 full-mode rows were read as causal (`[-1, -1]` means causal for captured traces) and merged into their causal twins, giving 19 distinct shapes instead of 21. - A non-causal record with a window is refused. - A `varlen: true` record is skipped and counted. `RUNBOOK.md` stage 2 names the `--emit-shapes` step, its filters, and that `--rocke-bench` takes one tree per run. It does not touch the text in #12796. What could break: running the benchmark normally still imports torch before it is used. A trace record with `causal: false` and a window is now refused. Merge notes: - #12958 (which replaces #12941) also appends test classes at the end of `tests/test_mine_shapes.py`. The conflict is trivial: keep both. `mine_shapes.py` merges cleanly. - Varlen records are skipped. This change does not depend on any graph-feature code. ## Test Plan - `tests/test_mine_shapes.py` on Windows, before and after. New reader tests, plus two tests that mine a synthetic list in the emitted schema (review feedback: the earlier tests ran the real benchmarks and pinned their shape counts). Host only. - Each benchmark's `--emit-shapes` run by hand with `sys.modules['torch'] = None`, and the emitted file mined. - The gfx942 emitted set compared with an independent extraction of `_configs()` by `ast`, by set equality of the request keys. - A merged build of this branch with the other runbook field-test fixes on gfx950 (MI355X), quick-start configure with clang-tidy ON: the generator pytest suite. ## Test Result - Before: the 3 reader tests and the 2 synthetic-list tests fail on develop's mine_shapes. After: all 47 tests in the file pass. - gfx942: 28 rows, 21 distinct shapes, 2 unmasked, windows {0, 512, 1024, 2048}. The 21 request keys are identical to the `ast` extraction. - gfx950: 28 rows, 15 distinct shapes, `8 rocKE varlen record(s) skipped`. - Merged gfx950 build: generator pytest 921 passed, 12 skipped. - black 25.12.0 gives no diff on the changed Python files. Not run: the benchmarks' GPU timing path. Only the torch import placement changed there. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

Closes #12935
Motivation
Stage 2 of the
hipdnn-ingestor-enginerunbook (mine shapes, check dispatch parity, reconcile applicability) did not run on the real dnn-benchmarking corpus in either field test, gfx950 (MI355X, extend) or gfx942 (MI300A, create). Both runs needed hand-written pre-filters.mine_shapesstopped at the first fp32 graph.dispatch_parityandreconcile_applicabilityexited 2 when the rocKE spec factory refused a shape, which is how the dispatcher itself declines. Mined shapes dropped sinks, varlen and layout, so the gap report overstated what the engine could serve. Later stages hit more problems in the same tools: kernel names that change with corpus order,knob_sweeparms that never reach the shapes a tile knob exists for, and--knobsmetadata resolved at the wrong tile, which failed packing on gfx942.Technical Details
Nine commits, all in
projects/hipdnn/tools/IngestorGenerator.mine_shapes.py:OutOfFamilyDtype. All three readers (CSV, graphs, rocKE bench) record the input in<out stem>.excluded.jsonwith a reason and skip it, and the summary printsexcluded : N {reason: count}. New--excluded-out. An unrecognised spelling still fails. A source where every input is excluded writes[], warns and exits 0.attributes,inputsandoutputs. Real graphs bind them under the node's inputs and outputs._graph_features = {layouts, features[, strides]}.layoutslists every named layout (BSHD, BHSD) all of Q/K/V/O are dense in, using the native unit-extent rule (hasBshdStridesinGfx950AttentionDenseNative.cpp).featureslists every bound optional tensor besides Q/K/V/O and the active attributes. Features are part of shape identity, so a BHSD or varlen graph no longer merges into its dense twin.dispatch_parity.pyandreconcile_applicability.py:ValueErrorfrom the spec factory is a per-shaperefusedwith its message, the same boundary assupport()in rocKEdispatch/attention/gfx942.pyandgfx950.py. Any other factory exception stays operational. The split is by exception type, not message text.graph_contract: {layouts, features}. A shape whose features or layouts fall outside it is[out_of_contract], with the reason, before the dispatcher is asked. Default: no features admitted, layout unchecked. The gfx950 profile declareslayouts: [BSHD], features: [], which is what its native graph_match accepts.use_sinkson any shape, including rocKE trace records, counts as thesink_tokenfeature.declined,refusedandout of contract, and--report-gapslists all three with the reason. A corpus where nothing is servable exits 1.runtime_param_fieldsis a mapping of spec field to canonical value (gfx950:batch: 1,seqlen_q: 512,seqlen_kv: {canonical: 512, request: seqlen_k}). The{canonical, request}form is for a field the request spells differently. By default the tool sets the fields that both the profile maps and the spec lists in its ownruntime_param_fields, revalidates, and collapses identical specs into one catalog entry. Specs that list none (ragged, windowed, persistent, moving-diagonal) keep their real values and print as[per-shape]. Applicability is still answered at the real shape.--per-shapekeeps one kernel per shape. The old list form fails with a message unless--per-shapeis given.dispatch.spec_attribute. The gfx950 profile now asks the library's full dispatcher,dispatch: {module: dispatch.attention, function: dispatch_attention, spec_attribute: spec}. The old entry,gfx950.dense_spec_for_request, pins the default 256/64 tile without an admit check, so 12 Sq=128 shapes that only the bm128 variant serves were reported as refused.prefix{value}for eachmetadata_fieldsentry, in profile order. That is the catalog key. It replaces every scalar spec field plusv{corpus index}.build_configsorts kernels by name. Colliding catalog keys get_h<spec digest>and a warning.--knobs: pins are applied to the spec first. The builder's spec class and the predicate validate the pinned spec; a refused combination is dropped and counted in a WARNING. Metadata, including policy fields such as gfx942v_row_pad, is resolved after that.--knobsalso takes a list of combinations, emitted as listed. The mapping form still crosses.reconcile_applicability.py:--declineskeys may be graph names or provenance paths, and a reused name is an error only when a key uses it. An empty source warns and exits 0. Out-of-contract shapes get their own bucket, and the reference is not asked about them.knob_sweep.py: each refused or declined shape is asked again at probe values ofruntime_param_fieldsthat keep the real values' equality and order (Sq == Skv stays equal, Sq < Skv stays smaller). If the dispatcher serves the probe, every arm builds that spec at the real shape with its knobs, and the spec class and predicate decide. Arms printCOVERAGE: serves K of N coverage shapes, andNARROWEDcounts shapes, not deduplicated kernels.launch_surface.py: with no.gitancestor,find_repo_roottakes the nearest ancestor holdingprojects/hipdnnbefore falling back to the start directory. The real-dispatcher tests skip, with the reason, when the rocKE tree is absent.What could break:
runtime_param_fieldsneeds the mapping form or--per-shape.--report-gapslines for refused shapes are long (up to about 20k characters), because the registry's message joins every candidate's reason. Full-corpus parity on gfx950 takes 11 s instead of 3 s.Merge notes:
tests/test_mine_shapes.py. The conflict is trivial: keep both classes.mine_shapes.pymerges cleanly.Test Plan
test_mine_shapes.py,test_dispatch_parity.py,test_reconcile_applicability.py,test_knob_sweep.py,test_launch_surface.pyandtest_factorise_config.py, before and after each commit, on Windows (Python 3.12, venv fromrequirements.txtplus numpy). The real-dispatcher tests use the shipped gfx950 profile and the in-tree rocKE dispatcher, and ran (not skipped).find_repo_root: the tests above in agit archiveextract on Windows, and on Linux on a gfx950 (MI355X) node in an extracted tree without.gitand again aftergit init.dispatch_parity --report-gapsand reconcile with no pre-filter. gfx950 uses the shipped profile; gfx942 uses the create run's draft profile converted to the mapping form.--knobs: the real gfx942 builder, comparing every emitted metadata field with the builder's readout for its pinned spec.Test Result
New tests, failing before their fix and passing after:
TestOutOfFamilyDtypesAreExcludedNotFatal: 2 failed before (unknown dtype spelling 'float', exit 1).TestGraphFeaturesTheRequestCannotCarry(4 failed:use_sinksFalse, no_graph_features) andTestGraphFeaturesOutsideTheContractAreNotServed(2 failed).test_dispatch_parity.pyafter the decline boundary: 7 failed before, 26 passed after.test_reconcile_applicability.py: 6 failed before, 112 passed after.test_a_shape_only_the_bm128_variant_serves_is_servable(real dispatcher, Sq=128 Skv=256): servable 0 before; after, served withblock_m == 128.TestEmittedNamesDoNotDependOnShapeOrder(real dispatcher, forward and reversed corpus give a byte-identical config): 3 failed before.TestTheGraphContractAppliesToEverySource(the realfull_sink_prefill_s512record): failed before.TestCoverageShapesReachTheArmsThatServeThem(real dispatcher, Skv 512 and 288): failed before.TestKnobPinsComeBeforeWhatDependsOnThem: 3 failed before.git archiveextract on Windows: 13 failed before, 74 passed after (dispatch_parity, knob_sweep, launch_surface). Linux gfx950 node extract: 26 failed and 47 passed before, 74 passed after; aftergit init, 74 passed.The existing operational test now raises
RuntimeErrorand still exits 2. The removedValueErrorrow oftest_a_failing_spec_factory_is_operational_under_every_flagpinned the old contract. It is replaced bytest_a_refusal_the_reference_does_not_share_is_unreconciled, which shows a wrong refusal still fails reconciliation.Real corpus, no pre-filter:
--per-shapegives 703 kernels.knob_sweep: develop showed the parity count for every tile arm. After: gfx942 probes 245 coverage shapes,block_m_128+18,block_n_32+12; gfx950 probes 216,block_n_32+12.--knobs: on develop,{"block_m":[128],"block_n":[32]}gives 55 kernels with 12v_row_padmismatches (metadata 0, builder 32). After,[{"block_m":256,"block_n":64},{"block_m":128,"block_n":32}]gives 110 kernels and 0 mismatches.Merged gfx950 build: generator pytest 921 passed, 12 skipped.
Not run: the gfx942 packing build with the fixed
--knobsoutput. The check above compares the same fieldshkp_packcompares, through the builder's own readouts.Submission Checklist