Repository navigation
fix(hipdnn): ingestor field-test tools, build fixes and runbook docs - #12958
Conversation
✅ 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
Null input bindings can hide legacy sink bindings, and mixed line endings can violate splice byte-preservation guarantees.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Improves hipDNN ingestor field workflows, build behavior, validation tooling, and runbook guidance.
Changes:
- Adds descriptor splicing/inventory tools and mining fixes.
- Reduces commit-triggered rebuilds and fixes installed-tool RPATH.
- Expands field-tested runbook documentation and tests.
| File | Description |
|---|---|
tools/IngestorGenerator/tools/verify_variant_sets.py |
Adds kpack fallback discovery. |
tools/IngestorGenerator/tools/splice_additions.py |
Adds addition-only descriptor splicing. |
tools/IngestorGenerator/tools/mine_shapes.py |
Handles excluded dtypes and input bindings. |
tools/IngestorGenerator/tools/launch_surface.py |
Resolves archive repository roots. |
tools/IngestorGenerator/tools/inventory.py |
Adds engine inventory reporting. |
tools/IngestorGenerator/tests/test_verify_variant_sets.py |
Tests kpack discovery failures. |
tools/IngestorGenerator/tests/test_splice_additions.py |
Tests descriptor splicing. |
tools/IngestorGenerator/tests/test_packs_formatting.py |
Tests generated census formatting. |
tools/IngestorGenerator/tests/test_native_stub.py |
Generates split version headers. |
tools/IngestorGenerator/tests/test_mine_shapes.py |
Tests dtype exclusion and sinks. |
tools/IngestorGenerator/tests/test_launch_surface.py |
Tests archive root resolution. |
tools/IngestorGenerator/tests/test_inventory.py |
Tests inventory behavior. |
tools/IngestorGenerator/tests/test_generator.py |
Updates provenance expectations. |
tools/IngestorGenerator/tests/test_generate.py |
Tests LF output. |
tools/IngestorGenerator/templates/test_packs.cpp.j2 |
Matches clang-format output. |
tools/IngestorGenerator/README.md |
Documents new workflows. |
tools/IngestorGenerator/configs/gfx950_attention_dense.yaml |
Updates regeneration instructions. |
tools/IngestorGenerator/codegen/generator.py |
Forces LF output. |
tools/CMakeLists.txt |
Adds installed-tool RPATH. |
ai/skills/hipdnn-superbuild/SKILL.md |
Documents clang-tidy behavior. |
ai/skills/hipdnn-superbuild-test/SKILL.md |
Documents engine-specific tests. |
ai/skills/hipdnn-ingestor-engine/workloads.md |
Documents corpora and tolerances. |
ai/skills/hipdnn-ingestor-engine/RUNBOOK.md |
Expands field-tested procedures. |
ai/skills/hipdnn-ingestor-engine/graph-contract.md |
Corrects reference-plan guidance. |
ai/skills/hipdnn-ingestor-engine/extend.md |
Documents inventory and splicing. |
tests/test_plugins/TestPluginCommon.hpp |
Uses stable version numbers. |
test_sdk/version.h.in |
Splits stable version macros. |
test_sdk/CMakeLists.txt |
Installs version-number header. |
plugin_sdk/version.h.in |
Splits stable version macros. |
plugin_sdk/tests/ingestor/TestDescriptorLoader.cpp |
Tests silent provenance handling. |
plugin_sdk/include/.../DescriptorLoader.hpp |
Suppresses provenance warnings. |
plugin_sdk/CMakeLists.txt |
Installs version-number header. |
frontend/version.h.in |
Splits stable version macros. |
frontend/tests/TestBackendFactory.cpp |
Uses stable version numbers. |
frontend/include/.../BackendWrapper.hpp |
Avoids hash-triggered rebuilds. |
frontend/CMakeLists.txt |
Installs version-number header. |
flatbuffers_sdk/version.h.in |
Splits stable version macros. |
flatbuffers_sdk/CMakeLists.txt |
Installs version-number header. |
data_sdk/version.h.in |
Splits stable version macros. |
data_sdk/CMakeLists.txt |
Installs version-number header. |
cmake/VersionUtils.cmake |
Generates stable version headers. |
cmake/version_number.h.in |
Defines stable version macros. |
backend/version.h.in |
Splits stable version macros. |
backend/src/plugin/HeuristicPluginManager.hpp |
Removes unused version dependency. |
💡 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 #12958 +/- ##
===========================================
- Coverage 76.04% 76.04% -0.00%
===========================================
Files 2869 2869
Lines 475118 475118
Branches 71163 71163
===========================================
- Hits 361282 361278 -4
- Misses 89290 89292 +2
- Partials 24546 24548 +2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
…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.
f4a106e to
457c3a3
Compare
|
Rebased onto develop (86af0be) to clear the conflict. The branch had two develop merges inside it, so it is now one commit with the same 47-file change; the original commit subjects are listed in its message. The only real conflict was the CMake flag table in |
b1d2555 to
3ca8ab7
Compare
|
Rebased onto develop at 28af48c so CI reruns with the develop-side fix present. Four files conflicted, all against #13024.
One develop file is now touched too. Checks on the rebased tree. IngestorGenerator pytest with PR CI is green on the rebased head e3caa6f: pre-commit, Math CI Summary, Multi-Arch CI Summary, Component CI Summary and TheRock CI Summary all pass. |
Rebased onto develop 86af0be as one commit. The branch had two develop merges inside it, so replaying the commits one by one re-raised conflicts those merges had already resolved. Original commits: - fix(hipdnn): emit the census test in the provider's clang-format form - feat(hipdnn): add an addition-only descriptor splice tool - feat(hipdnn): add a baseline inventory tool for installed descriptor trees - docs(hipdnn): point the extend path at the splice and inventory tools - fix(hipdnn): stop warning on the packager's provenance block - fix(hipdnn): report a missing rocm_kpack as an environment error - fix(hipdnn): write generator output with LF line endings - fix(hipdnn): keep the git hash out of widely included version headers - fix(hipdnn): emit version_number.h in the native-stub compile helper - fix(hipdnn): let the installed hipdnn_list_engines find libhipdnn_backend - style(hipdnn): keep clang-format off the version_number.h.in include guard - fix(hipdnn): inventory digest skips symlinks and reports validator launch errors - fix(hipdnn): splice_additions counts the kernels of a new KDP - fix(hipdnn): verify_variant_sets fallback ignores a broken installed rocm_kpack - fix(hipdnn): mine_shapes excludes out-of-family dtypes and reads node inputs - fix(hipdnn): find the repo root in a tree without git metadata - docs(hipdnn): fill runbook gaps found by the ingestor field tests - docs(hipdnn): reword the descriptor row of the splice table - docs(hipdnn): runbook says when a build dir may be reused and why device CTest starts silent - docs(hipdnn): start the GPU torch install early and reuse its downloads - Update workload extraction paths and directories - docs(hipdnn): point the runbook's clang-tidy note at the build docs - docs(hipdnn): set the torch index in the early GPU torch install recipe - fix(hipdnn): a null input slot does not hide an attribute-bound UID - fix(hipdnn): splice_additions refuses a live file with mixed line endings - fix(hipdnn): splice_additions --check runs the byte-preservation refusal - fix(hipdnn): generate census against provider engine-name constant
3ca8ab7 to
e3caa6f
Compare
…suites in superbuild CI (#13385) ## Summary The IngestorGenerator and DescriptorGenerator pytest suites never ran in the hipDNN superbuild lanes: the CI venv had no PyYAML or Jinja2, nothing registered them with ctest, and several of their tests skipped silently without their inputs. This runs both suites as ordinary ctest entries in the Linux and Windows superbuild jobs (not TheRock) under a small pytest plugin that fails any skip a test has not explicitly allowed, and replaces the packaging-probe junit check with a configure-time minimum. JIRA ID : ALMIOPEN-2843 ## Risk Assessment Risk 3 of 5: no product code changes, but a new gate lands in the superbuild lanes, so a regression in these tools can now turn them red, and a lane that discovers fewer packaging probes than it requires now fails at configure. The suites register behind `HIPDNN_ENABLE_TOOLING_TESTS`, which defaults to OFF and is enabled only in `hipdnn-superbuild-ci.yml`; the probe minimum defaults to 0 and only the two superbuild lanes set it. TheRock and developer builds are unchanged. ## ASIC Coverage Host-only: the tests drive Python generators and compile generated C++ with `-fsyntax-only`, and the probe check runs at configure time. No device code, dispatch default or support surface changes, so passing PR CI is sufficient. ## Testing Summary - Both suites through `ctest` on Linux (ROCm container, the lane's flags, a built gfx942 packing tree) and on Windows. On Linux the ingestor suite also passes with no system C++ compiler on `PATH`, using only the compiler that builds the tree. - The plugin's own unit tests: a throwaway suite run with and without the plugin covers plain, `skipif`, fixture and module-level skips, xfail, and the `allow_skip` marker with and without a reason. - The packaging-probe minimum through real configures on Linux and Windows: it passes with the probes option ON, fails with a minimum above the declared count, and fails when set with the probes option OFF. ## Testing Checklist - [x] DescriptorGenerator suite under the plugin (Linux, Windows) - `ctest -R hipdnn_descriptor_generator_pytest` - Status: Passed (695, none skipped) - [x] IngestorGenerator suite under the plugin (Linux, Windows) - `ctest -R hipdnn_ingestor_generator_pytest` - Status: Passed with `rocm_kpack` installed, as the lanes do - [x] Plugin tests - `python -m pytest projects/hipdnn/tools/IngestorGenerator/tests/test_strict_skips_plugin.py` - Status: Passed (3) - [x] Packaging-probe minimum (Linux, Windows) - configure with `-DHIPKERNELPROVIDER_ENABLE_PACKAGING_PROBES=ON` and `-DHIPKERNELPROVIDER_MIN_PACKAGING_PROBES` set to 1, 99, and 1 with the probes option OFF - Status: passes, fails, fails as intended - [x] hkp-pack tests touched here (Linux, Windows) - `python -m pytest tests -m quick` in `descriptor-packaging` - Status: Passed - [ ] PR CI - GitHub PR checks - Status: Pending ## Technical Changes - `HIPDNN_ENABLE_TOOLING_TESTS` registers the two suites as ctest tests (build tree only). Configure fails if pytest, PyYAML, Jinja2 or the kernel ingestor is missing, and the CI requirements file gains PyYAML and Jinja2. - `hipdnn_strict_skips` (`projects/hipdnn/tools/pytest_plugins/`) is a pytest plugin that turns any skipped or xfailed test into a failure. A skip that is correct by design opts out with `@pytest.mark.allow_skip(reason=...)` beside the code that skips: the POSIX-only sweep tests and the coverage-gate test that runs a shebang script. A skip raised while a module is imported fails the collection. Run by hand the plugin is not loaded, so tests skip as before. - The gates the lanes can satisfy now run: `ctest` supplies the validator binary, the flatbuffers/HIP include directories, and puts the directory of the compiler that builds the tree on the test `PATH`, so the native-stub tests need no separate C++ compiler. The DescriptorGenerator CLI tests use `sys.executable`, and its file lists use `as_posix()` (23 Windows failures before). - `TestAgainstTheRealBuild` is deselected in the ingestor ctest entry: it gates a build packed with the gfx950 attention engine, which neither lane packs. - `HIPKERNELPROVIDER_MIN_PACKAGING_PROBES` fails configure when probe discovery declares fewer probes than a lane requires, or when it is set without the probes option. Both superbuild lanes set it to 1 and run `ctest` with `--no-tests=error`, which replaces `check_probe_junit.py`, its tests and the manifest it read. - hkp-pack: `test_specialization_agreement.py` moves to the quick tier, and the worker `sys.path` tests no longer depend on `HIPKERNELPROVIDER_ROCM_KPACK_DIR` or pass vacuously where the default start method forks. ## Related PRs - #13348 edits the same "Run tests" step. It filters `ctest` by label on dispatch runs, which could leave the tooling ctest entries or the packaging probes unselected; whichever merges second should make sure they stay in the selected set. - #12958 also edits `test_native_stub.py` and the IngestorGenerator README. --------- Co-authored-by: Brian Harrison <brian.harrison@amd.com>
TheRock Submodule Bump ActivityNewest first
|
…ree census shard (#14573) ## Summary The gfx950 census shard is written into the build tree as a real `CTestTestfile.cmake`, but its Gfx950AttentionDense tiered suites pass `--test-config hipkernel_Gfx950AttentionDense.toml` by bare name, and only `cmake --install` put that TOML beside the shard. Running the shard from the build tree therefore failed with `Error: Config path does not exist` before any case ran. This PR copies the files a shard reads by bare name beside its build-tree copy, adds a configure-time guard so a later shard-routed engine cannot reintroduce the gap, and keeps CTest's `Testing/` scratch directory out of the install. JIRA ID : ALMIOPEN-2879 ## Risk Assessment Risk 3 of 5. Build and test infrastructure only, with a new check that can fail configuration; no library, kernel, dispatch or default-selection code changes. The guard fails only for a shard that names a `*.toml` nothing places beside it, which is the defect itself. The one shipped-content rule touched, the wholesale `install(DIRECTORY)` of the test descriptor tree, gains one exclusion. ## ASIC Coverage Architecture-independent build wiring; no multi-arch sweep is needed. The superbuild lanes (gfx942, gfx1151) do not generate the gfx950 shard, so in PR CI only the new host-only test exercises the copy and the guard. The gfx950 path was verified on gfx950 hardware. ## Testing Summary - gfx950 hardware: the quick and standard Gfx950AttentionDense suites run from the build-tree census shard. Before the fix the quick suite fails on the TOML; after it, every case runs and none is skipped. - Regression test: `test_hkp_census_shard_files.py` runs the real `hkp_reserve_census_shard()` and `hkp_finalize_census_install()` under `cmake -P`. It fails if the copy, the guard or the argument check is removed. - Install: no `census/<arch>/Testing` directory is installed after a build-tree shard run; otherwise the installed file set is unchanged. - Audit of the other install-style CTest files: the gfx950 shard is the only one affected. The `HIP_MLOPS_ENGINE.toml` and `ASM_SDPA_ENGINE.toml` suites also use bare names, but in staging files (`install_CTestTestfile.cmake`, `CTestTestfile.cmake.install`) that CTest does not read in the build tree. ## Testing Checklist - [x] gfx950 quick suite from the build-tree shard - `ctest --test-dir <build>/lib/hipdnn_plugins/engines/test_arch_content/hip-kernel-provider/census/gfx950 --no-tests=error -V -R 'gfx950-attention-dense-external-integration_quick_suite$'` - Devices: gfx950 - Status: Passed (8 ran, 0 skipped; fails on develop with `Config path does not exist`) - [x] gfx950 standard suite from the build-tree shard - same command with `_standard_suite$` - Devices: gfx950 - Status: Passed (45 ran, 0 skipped) - [x] Shard registration - `ctest --test-dir <build>/lib/.../census/gfx950 -N -L hipkernel_Gfx950AttentionDense` - Status: Passed (5 tiered suites and `gpu_ref`; TOML byte-identical to `config/hipkernel_Gfx950AttentionDense.toml`) - [x] Regression test - `pytest descriptor-packaging/tests -m quick -k test_hkp_census_shard_files` - Status: Passed (4/4) - [x] Configure guard - drop `FILES` from `hkp_reserve_census_shard()` and reconfigure - Status: Passed (configure fails with the guard message) - [x] TOML edit propagates - edit the source TOML, `cmake --build` - Status: Passed - [x] Installed file set - `cmake --install` before and after the fix - Status: Passed (identical, except that develop also installs `census/gfx950/Testing/` after an in-place shard run) - [x] Full build, Linux - preset `hip-kernel-provider`, `GPU_TARGETS=gfx950`, kernel ingestor, SDPA and rocKE ON - Status: Passed - [x] PR CI - GitHub PR checks - Status: Passed ## Technical Changes - `HkpPackaging.cmake`: `hkp_reserve_census_shard(<out> <arch> [FILES <file>...])` records files that entries routed to the shard read by bare name, and rejects unrecognised arguments. A new `_hkp_write_census_shard()` copies them beside the shard's build-tree `CTestTestfile.cmake` and fails configuration if the shard names a `*.toml` that is not there. - `src/CMakeLists.txt`: the gfx950 dense TOML path is one variable, passed both as `TEST_CONFIG` and to the shard reservation. The existing `install(FILES)` for the TOML stays, since the integration-tests helper that issues it is shared with other providers. - `CMakeLists.txt`: the wholesale install of the test descriptor tree excludes `census/<arch>/Testing`. - `test_hkp_census_shard_files.py`: new quick test of the copy, the guard and the argument check. - RUNBOOK and the `hipdnn-superbuild-test` skill: the build-tree registration of the engine-pinned suites, the build-tree path of the census shard, the engine-label rule, and which CTest files are install-only. Related: #12958.

Closes #12937
Closes #12938
Refs #12935
Why
The ingestor runbook could not extend the live gfx950 attention bundle safely: regenerating descriptors replaced retained UUIDs, the generated census did not match the provider's formatting, and missing
rocm_kpacklooked like a failed applicability gate. The gfx942 create run also exposed rebuild churn and an installed engine-listing tool with no path to its backend library. The mining tool stopped on a valid out-of-family dtype and missed sink tokens bound under SDPAinputs.Changes
splice_additions.pykeeps retained descriptor identities and fields, appends only new kernels, preserves line endings, and refuses to write when the existing bundle changed.inventory.pyreports the actual KDP kernels, source kinds, catalog digest and validator diagnostics. Both tools include review fixes for new-pack counts, symlinks and validator-launch errors.mine_shapes.pyrecords recognized fp32/fp64 inputs as excluded and reads sink bindings from the SDPA node'sinputs. Unknown dtypes still fail.verify_variant_sets.pydistinguishes missingrocm_kpack(exit 3) from a failed gate and clears failed imports before trying the next installed location.engine.census_name_symboloptionally binds its engine lookup to a C++ constant fromIngestorPacks.hpp; the loader rejects malformed identifiers. The gfx950 config now regenerates its live census file byte for byte and reports a descriptor/pack engine-name mismatch explicitly. Other engines keep the literalengine.namepath.provenanceas tracking metadata without a warning; prefixed extension keys still warn and unknown keys still fail. Current develop stores packed UKD provenance in sidecars. A stableversion_number.havoids rebuilding every dependent object when the commit hash changes. The installedhipdnn_list_enginesuses an origin-relative backend library path.Verification
Tested commit
f4a106emerged with develop3feaa3ffin the pinned ROCm 10.2 gfx950 hipDNN image on an AMD Instinct MI355X.census_name_symbolis rejected. Black 25.12.0 left the changed Python files unchanged.hip-kernel-providerpreset built and installed with the ingestor, rocKE and tests enabled. Native descriptor-loader GTests: 221/221 passed. The packed gfx950 attention census CTest passed. The installed engine-listing tool ran without a customLD_LIBRARY_PATHand listed the gfx950 attention engine, ASM SDPA and HIP MLOPS (Slurm job68053151). The descriptor loader and its test also pass clang-format 18.hipdnn_validate_descriptors --expect-engine hipkernel:Gfx950AttentionDense: one expected engine, 7 INFO diagnostics, no WARN/ERROR or provenance warning (job68053186). A separate MI355X end-to-end run with dnn-benchmarking PR Test do no merge #72 selected and executedattention_dense.bf16_d128_hq16_kv16_c1_bm256_bn64.gfx950from this provider and passed against a CPU PyTorch reference (job68053182). This is functional evidence, not a performance result.Limits and review gate
The current Python run skipped platform-specific and optional-tool cases; a fresh gfx942 build and full native test matrix were not run on this head. Develop advanced five commits after
3feaa3ff; none touches a file changed in this PR. Their combined runtime behavior was not retested.version.hnow includes the new installedversion_number.h, so out-of-tree copies must ship both. Please re-review the current-develop provenance behavior and generated census-name binding before merging.