Repository navigation
Conversation
⏳ Waiting on Required Checks⏳ Required checks have not all concluded yet. This bot run is exiting without failing so those checks can report independently.
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
⏳ Waiting for required checks to start or finish:
|
2 of 3 tasks
BrianHarrisonAMD
added a commit
that referenced
this pull request
Oct 28, 2026
…14566) ## Summary Fixes gaps in the `hipdnn-deep-review` skill that a full review round of #14457 exposed. In that round, findings and coverage rows were dropped without any error, follow-up finding IDs were numbered wrongly, and the drafted comments needed a hand rewrite. The scripts now fail or explain in each of those cases. JIRA ID : ALMIOPEN-2945 ## Risk Assessment Risk 1. Changes are limited to the skill's scripts, docs and tests under `projects/hipdnn/tools/ai/`. No product code, build or packaging change. ## Testing Summary - Skill tooling tests: 22 new or extended regression cases, each failing before its fix and passing here, plus 3 guard cases for inputs that must keep passing. - Skill validation: `validate-skills.py` validates all 16 skills. - End-to-end smoke run of `seed`, `ids`, `cluster`, `coverage`, `draft` and `orient-gate` on a scratch repository. ## Testing Checklist - [x] Skill tooling tests - `python3 -m pytest projects/hipdnn/tools/ai/tests` - Status: Passed (334 passed, 7 skipped) - [x] Skill validation - `python3 projects/hipdnn/tools/ai/validate-skills.py` - Status: Passed (16 skills) - [ ] PR CI - GitHub PR checks - Status: Pending ## Technical Changes - **Findings no longer vanish.** `ledger.py ids` fails when a report holds a finding block it cannot read: an `ID:` line in bulleted, numbered, bold, quoted, heading or table form, a partly fenced block, or a block with no `ID:` line. It also fails when a Findings section cites a provisional ID that no report of the round parsed, which catches findings written as table rows, including a merged agent's other lanes. Lane notes after a `##` heading or `---` no longer leak into the last field of a finding. - **Follow-up IDs.** Provisional IDs must be `<LANE>-p<n>`. Follow-up reports had written `RUN-f1-p1`, which became the final ID `RUN-f-1`. - **Coverage.** `ledger.py coverage` explains why an owner lane's row missed its file (short path, grouped or glob row) instead of leaving the file `pending` with no reason. - **Clustering.** New `ledger.py cluster <kept> <duplicate>...` replaces hand edits of `ID-MAP.md`. It keeps chains of superseded findings valid. - **Orient gate.** New `partition.py orient-gate` replaces the shell pipeline. It accepts a cited file the change adds. - **Draft comments.** Each suggestion or future-work item becomes its own comment, grouped by tier and kind. - **Brief changes.** A new `Scope:` input records notes such as deprecated areas. Verifiers no longer treat a mutant in code the test binary does not compile as proof of a test gap.
EwanC
marked this pull request as ready for review
October 29, 2026 10:06
Add --benchmark-reference to measure reference preparation, compute or golden output loading, and expected-output transfers without engine execution or numerical comparison. Export per-reference GoogleTest properties and verified totals, and preserve explicit fallback status. Validated with 1098 passing unit tests (2 skipped), matched CPU/GPU batchnorm smoke cases, generated golden CPU/GPU-validator smoke cases, JSON phase accounting, combined engine/reference benchmarking, and invalid-flag rejection.
Limit --benchmark-reference to bundle tests: remove its branch from the C++ graph harness, which is being retired in favour of bundles, and document which tests it does not measure. Report the reference status as an enum and share the refusal predicate with --benchmark. Under the flag, an unreadable golden blob now fails with the same "tensor data failed to load" outcome as without it. Fix the TestConfig getter and its comment, add the SPDX header to ReferenceBenchmarkSync.hpp, and explain the non-obvious control flow. Pin the behaviour the review found untested: the refusal with the flag off, config and policy wiring with the flag on (death test), the production timer waiting for device work, a sync failure at every phase without fallback, golden outputs read inside the execute phase, device copies completing inside materialize, every report property, and verified-only totals. Add CTest coverage: a plugin-free refusal check, and an opt-in BENCHMARK_REFERENCE_SMOKE_FILTER on add_external_integration_test_target() that the MIOpen lane uses to run five quick batchnorm bundles with the flag in the quick tier, staged for the install tree.
The MIOpen smoke's pass and fail patterns were file-scope variables of HipdnnIntegrationTestHelpers.cmake. A provider configured in the same tree calls the helper from its own directory, read them empty, and the check fell back to the exit code. Set them inside the macro. The fail pattern now also matches the messages main() prints before a non-zero exit after the summary (FATAL:, Fatal error:, Error: zero tests ran). The refusal check was only in the build-tree CTest file, where CI runners have no GPU and pass it through the "No HIP devices available" skip. Stage it into the installed CTest file that the GPU jobs run, labelled from test_categories_external.yaml. It and the smoke's installed entry now carry the test environment the GTest suites get. Install-tree ENVIRONMENT lists are written unescaped, because ctest reads an escaped list as one variable. --benchmark-reference measures bundle tests only. With bundle tests off it reported measured=0 and exited 0, so it is now refused. The check runs after TestConfig::initialize(), which settles HIPDNN_TEST_ALLOW_BUNDLES against --no-bundles. HarnessDependencies now defaults to the production reference timer, so a caller cannot leave out the device synchronize. The deviceless test mocks install the no-op timer. Tests: verified totals over several samples, the applicability and placement checks inside prepare_ms, the bundles-off refusal, and the default timer waiting for queued device work. Docs: golden blobs are read under the flag only when golden outputs verify the test, the bundles-off refusal, the reference-benchmark lines a run prints, and what each CTest check catches.
EwanC
force-pushed
the
users/ewan-crawford/benchmark-reference
branch
from
November 3, 2026 15:27
9e47106 to
66faa83
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
--benchmark-referenceto the integration-test binary. For each bundle test it times the reference that produces the expected outputs, in three phases, and reports them per test and as per-reference totals:prepare_ms: executor creation and applicability check, reference output allocation, variant-pack setup, and any input transfer the reference needs. Inputs are shared with the engine, so filling or loading them is not included. Zero for golden outputs.execute_ms: CPU/GPU reference compute, or loading the golden output blobs.materialize_ms: moving expected outputs to the validator's host or device.The engine still runs once and its output is still compared; a mismatch fails the test as usual. Only engine execution and comparison are excluded from the reference timing. Each attempted reference prints
[ REFERENCE BENCHMARK ]and recordsreference_benchmark_<cpu|gpu|golden>_*GoogleTest properties with statusverified,mismatch,declinedorerror. Declined and errored attempts carry no timings.[ REFERENCE BENCHMARK SUMMARY ]totals verified references only and printsmeasured=0when there are none. A failed device synchronize fails the test instead of falling back to the next reference.Closes: #14456
Scope
BUILD_CPP_GRAPH_TESTSsuites, which are being retired in favour of bundles, and the default-builtConvForwardSerializeRoundTripandResampleBackwardtests. They run their reference as normal and print no reference line.docs/running-tests.md§5 says so.--skip-graph-validation,--capture-bundlesor--write-support-claims, which never run a reference, and when bundle tests are off (--no-bundlesorHIPDNN_TEST_ALLOW_BUNDLES=0), where there is nothing to measure.gpuorcpurun never compares against fails the run without the flag and passes with it.CI coverage
hipdnn_integration_tests_benchmark_reference_refusal(integration-tests, quick tier, no plugin): runs the binary with--benchmark-reference --skip-graph-validationand passes on the refusal message. It is in the build-tree CTest file and in the installed one, so the GPU jobs that run the install tree (TheRock) execute the refusal. On a runner with no HIP device the binary skips itself and the check passes without reaching the refusal.miopen-provider-external-integration-check-benchmark-reference-smoke(MIOpen lane, quick tier, build and install tree): runsquick_BatchnormFwdInference_Inference.*(5 bundles) with--benchmark-reference. It passes only when the summary reports at least one verified measurement, and fails on[ FAILED ],FATAL:,Fatal error:orError: zero tests ran. CTest ignores the exit code of a check with a pass pattern, so a non-zero exit after the summary that prints none of these is not caught; a crash on a signal still fails. The opt-inBENCHMARK_REFERENCE_SMOKE_FILTERkeyword onadd_external_integration_test_target()registers it; only MIOpen opts in.Testing
Example invocation (the
quick_BatchnormFwdInference_*filter selected 11 bundle tests in the available MI210build_releasetree; the CI smoke filter above selects 5):Quick tier (
ctest -L quick, presethipdnn-providers, 52 entries, serial) and the flag on each device:--validator cpu, and with--benchmark: 6/6 passed,measured=6eachmeasured=6eachmeasured=6eachbuild_releasetree)hipdnn-providerspreset.--validator cpu, and gpu with--benchmark: 11/11 passed,measured=11eachThe MI210 reference-benchmark runs used
quick_BatchnormFwdInference_*, which selected 11 bundles in this build tree (rather than the 6 reported for the other flag runs). All five runs passed with no skipped or failed bundle tests. The measured GPU-reference totals were about 874 ms, CPU-reference totals 91 ms, auto-mode totals 65 ms, GPU-reference with the CPU validator totals 91 ms, and GPU-reference with--benchmarktotals 201 ms. These totals depend on first use and device initialization.The MI210 standalone quick-tier CTest failures were:
hipdnn_integration_tests_unit_tests_quick_suiteandhipdnn_integration_tests_unit_tests_ffm-quick_suite:TestReferenceOpCoverage.RaggedBundlesAreNotListedAsGapsfailed because the discovered ragged-bundle set differed from the test's expected set.hipdnn_golden_data_tests_quick_suiteandhipdnn_golden_data_tests_ffm-quick_suite: eight SDPA backward bundle cases failed output comparison against their CPU/GPU references.Additional testing:
--no-bundles, andHIPDNN_TEST_ALLOW_BUNDLES=0.HIPDNN_TEST_ALLOW_BUNDLES=1with--no-bundlesruns and measures.hipdnn_integration_tests_unit_tests1127 tests, 1125 passed, 2 skipped (no golden data on the node). The benchmark tests (89) also pass on gfx950, gfx1100 and gfx1101. They cover: the refusals with the flag on and off; the production timer waiting for queued device work, including as theHarnessDependenciesdefault; a sync failure at each of the 8 reference and 4 golden synchronize points without fallback; golden outputs read inside the execute phase; the applicability and placement checks insideprepare_ms; host-to-device and device-to-host expected-output copies insidematerialize_ms(device tests); an unreadable golden blob; a reference declining at execute; every per-phase property, mismatch samples, and verified-only totals over several samples. The MI210 quick-tier unit-test failure above was in a separatebuild_releaserun against a different bundle inventory.main()ignoring the refusal) are all killed. The mutants that need the real synchronize are killed only by device tests, which skip on a host without a GPU.--no-bundles, and with a filter that matches no tests.quick_*:Smoke/*:Quick/*, 706 tests) on gfx90a gave identical outcomes with and without the flag (251 passed, 455 skipped, 0 failed).ASIC coverage
measured=0and fails.Risk
3 of 5. The feature is off by default and only changes behaviour when the flag is passed. Two new quick-tier CTest entries and a change to the shared external-integration CMake helper (opt-in per lane; only MIOpen opts in) are the parts that run in every CI job.