Repository navigation
test(hipdnn): add runtime pass-by-value scalar integration test bundles - #9600
GarbyGilbertAMD merged 11 commits into
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. |
f3671dc to
0552af4
Compare
0552af4 to
a179f27
Compare
088bcdc to
fd8f63e
Compare
Providers that update to support runtime pass-by-value scalars may incorrectly
claim support and produce wrong output if they ignore the runtime value and fall
back to a baked-in compile-time default. This adds integration test bundles that
expose that failure mode.
Each bundle uses the runtime-with-default form: the scalar tensor carries
is_runtime_pass_by_value=true alongside a compile-time default chosen to differ
from the correct runtime value. A provider that falls back to the compile-time
default produces output that diverges from the CPU reference. Synthesis fills
the runtime scalar and the CPU reference executor verifies the output.
The CPU reference executor (SdpaFwdPlan) is extended to read the attention scale
from the variantPack when attn_scale_value is absent from node attributes, enabling
it to generate correct golden data for the runtime-scale SDPA bundles.
Coverage added (integration_test_bundles/quick/):
- PointwiseFwdRuntimeScalar/{Add,Mul}/Small: binary pointwise with runtime scalar,
compile-time defaults 0.0 (ADD) and 1.0 (MUL)
- BatchnormFwdInferenceRuntimeScalar/Epsilon/Small: negative test — frontend
correctly rejects runtime epsilon via validateScalarParameter
- BatchnormFwdTrainingRuntimeScalar/Small: forward training with runtime momentum,
compile-time default 0.5
- SdpaFwdRuntimeScale/bhsd/{bf16,fp16,fp8}/...: runtime attention scale across all
13 existing SdpaFwd compile-time bundle variants (hd128/hd192, nomask/causal,
batch/group/stats), compile-time bake-in 2.0 with attn_scale_value null
ALMIOPEN-2267
… values Template-sweep tensor patches can flip a tensor's is_runtime_pass_by_value flag to true while still leaving its original value_type/value from the template. When that happens, the CPU reference and the provider both silently short-circuit to the baked value, and the runtime injection path is never exercised even though the test appears to cover it. applyTensorPatches now checks this invariant after all patches are applied and throws if a tensor carries is_runtime_pass_by_value=true alongside a value_type or value. The existing bundle-loading error handling converts this into an INVALID_SWEEP_CASE, so a misconfigured case is excluded from test discovery and logged rather than passing without actually testing runtime injection. Runtime scalar coverage for Batchnorm and RMSNorm is consolidated into the existing template-sweep bundles as additional cases rather than standalone bundles, removing the now-redundant BatchnormFwdInferenceRuntimeScalar, BatchnormFwdTrainingRuntimeScalar, and PointwiseFwdRuntimeScalar bundles along with their golden data generator scripts. The SDPA runtime-scale golden data is updated to match, and its generator script is renamed to generate_sdpa_runtime_scale_test_bundle.py to reflect what it produces. The reference data generation scripts gain a requirements.txt pinning their torch dependency. ALMIOPEN-2267
c6f2167 to
8306871
Compare
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
A continuation line in applyTensorPatches's invariant-check error message was indented two spaces short of clang-format's expected alignment for the concatenated string literals, which CI's clang-format hook flagged. The local pre-commit should now be configured correctly and catch this type of issue in advance. ALMIOPEN-2267
|
AI flagged this:
I think this is acceptable given the test bundle design, but could consider adding golden refs for these. |
…e tests TestSdpaFwdPlan.cpp passed an extra std::nullopt at two SdpaFwdParams call sites, shifting every subsequent positional argument by one slot and causing a compile failure whenever SDPA is enabled. generate_sdpa_runtime_scale_test_bundle.py still baked a compile-time value_type/value onto the scale tensor alongside is_runtime_pass_by_value, even though the checked-in bundle data had already dropped those fields. A tensor can't carry both without silently short-circuiting to the baked value, so the runtime injection path this bundle is meant to exercise would never actually run. The script now matches the corrected bundle data and the pattern already enforced for the Batchnorm and RMSNorm runtime-scalar tests. ALMIOPEN-2267
Three SDPA integration tests set attn_scale as both a tensor and a scalar on the same node, which SdpaFwdNode and SdpaBwdNode validation rejects since a graph cannot specify attn_scale both ways at once. Removing the leftover scalar-form call and its corresponding attn_scale_value assertion leaves each test exercising only the tensor pass-by-value form it is meant to cover. ALMIOPEN-2267
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #9600 +/- ##
===========================================
- Coverage 68.85% 68.81% -0.04%
===========================================
Files 2754 2754
Lines 453326 453530 +204
Branches 66802 66807 +5
===========================================
- Hits 312121 312076 -45
- Misses 118432 118708 +276
+ Partials 22773 22746 -27
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
|
LGTM! |
BrianHarrisonAMD
left a comment
There was a problem hiding this comment.
Updates look good thanks!
The original issues are addressed.
I got an agent to do a sweep on latest, and it found these issues we should address before merging:
- The new PBV+baked-value invariant only covers the sweep path.
applyTensorPatches has a single call site (IntegrationTestBundle.hpp:790), inside loadIntegrationTestBundle(DiscoveredBundle). The direct single-graph path, loadIntegrationTestBundle(jsonPath) at line 701, never calls it. I took the real bhsd/bf16/hd128_nomask_batch/Small bundle, injected a baked value_type/value next to its existing is_runtime_pass_by_value=true, and it loaded clean, with both the runtime flag and the baked value surviving into the flatbuffer. The same contradiction via the sweep path is correctly rejected, so the gap is the load path, not the tensor shape. This is the configuration Samuel flagged. Probably wants to live in buildGraphBuffer so both paths get it.
- When the invariant does fire, it deletes the test instead of failing it.
The throw is caught and mapped to INVALID_SWEEP_CASE, and BundleRegistration.hpp:140-146 logs it and continues, so no test gets registered. I corrupted a single case in the real RMSNorm/Variant3 sweep (dropped its "remove" clause) and got: 43 cases -> 42 registered, 1 silently dropped, suite still green. Worth noting the check scans every tensor in the expanded graph, so one bad patch can drop a case that has no patch at all. For a guard whose whole job is preventing silent masking, failing silently is an awkward outcome.
- Both hd128_nomask_batch_stats bundles pin the wrong uid, so the detection value is never applied.
In bf16 and fp16 hd128_nomask_batch_stats/SmallStats, the scale tensor is uid 10, but the meta says inputs: { "4": { kind: fixed, value: 2.0 } }. uid 4 is the LSE output, which synthesis excludes, so that entry is discarded and uid 10 falls through to the SDPA default FillSpec::free(0.1, 1.0). I ran the actual synthesis pipeline: the stats bundle gets scale = 0.500349 (random), while the non-stats sibling correctly gets 2.0. So the mechanism works, it is just the uid that is wrong. The notes text in those two files still says "uid=4" as well, and generator_version is still 1.0.0 while the siblings are 1.1.0.
- generate_sdpa_runtime_scale_test_bundle.py can only produce 5 of the 16 committed bundles.
I ran the generator and diffed its output against the committed files. These 5 reproduce the graph exactly (modulo the name field): bf16/Small, bf16 and fp16 hd128_nomask_batch, bf16 and fp16 hd192_nomask_batch. The other 11 cannot be generated because the script hardcodes causal_mask/causal_mask_bottom_right/diagonal_alignment/bounds, seq_len_q/kv, generate_stats/stats_tensor_uid, and descale_q/k/v all to None or False. It also hardcodes UID_SCALE = 4 while 5 bundles use uid 10, has no fp8 in DTYPE_MAP, still self-reports the old name "generate_sdpa_runtime_scale_golden.py" in the meta it
writes, and emits generator_sha256 / reference_backend / rocm_version / config plus .tensor*.bin blobs that do not exist in the committed tree. Not a blocker on its own, but right now most of those bundles cannot be regenerated by anyone.
Two minor things:
- bhsd/bf16/Small is byte-identical to bhsd/bf16/hd128_nomask_batch/Small apart from the name field. I compared the built flatbuffers, not just the JSON. Two tests, one graph.
- tensor_patches is a new sweep-schema field with no mention in integration_test_bundles/README.md or anywhere under docs/. Given adickin is landing sweep documentation in #9771, worth adding a short section there.
… fix scale-tensor uid collision A bundle whose runtime-pass-by-value tensor still carried a baked value or value_type was logged and silently skipped like any other malformed bundle, shrinking the suite without a red test anywhere. classifyBundle() now registers a synthetic failing test for that specific contradiction instead, and the check itself moves from applyTensorPatches into buildGraphBuffer so it covers every load path, not just the template-sweep path. The migration pipeline's smoke-test step used to collapse any non-zero exit into a single "may need GPU" warning, which would have masked this new hard failure too. It now greps loader output for "Failed to load bundle" to tell a real regression apart from a missing GPU. The SdpaFwdRuntimeScale bundle family is regenerated to move the runtime scale tensor off uid=4, which collided with the stats/descale UID slots. The generator drops its PyTorch/golden-blob dependency (these bundles synthesize inputs and reference-verify output at test time), adds causal/group/stats/fp8 variants, and gains a regenerate_bundles.sh script. README documents the tensor_patches mechanism these cases use. ALMIOPEN-2267
…IOPEN-2267/hipdnn-runtime-pbv-tests # Conflicts: # dnn-providers/integration-tests/integration_test_bundles/quick/Batchnorm/Variant2/sweep.json # dnn-providers/integration-tests/reference-data-scripts/generate_sdpa_runtime_scale_test_bundle.py # dnn-providers/integration-tests/reference-data-scripts/requirements.txt # projects/hipdnn/backend/tests/descriptors/TestGraphDescriptor.cpp
…DPA tests The compatible-optional-attributes tests for SDPA forward and backward descriptor lifting/lowering already verified that dropout_probability and the diagonal band bounds are captured correctly as runtime scalars, but did not exercise attn_scale through the same path even though it takes the same tensor-or-scalar dual representation. Setting attn_scale via set_attn_scale() and asserting attn_scale_value on the lowered/lifted attributes closes that gap, alongside the existing assertion that the tensor form of attn_scale remains unset in these tests. ALMIOPEN-2267
…eWithMessage The test name validator rejects GTest case names containing keywords such as "Test", since they belong in the suite name rather than the case name. Renaming the case to FailedBundleLoadRecordsFailureWithMessage satisfies that rule without changing what the test covers. ALMIOPEN-2267
#9600 added a manual runtime_epsilon case to quick/RMSNorm/{Default, Variant2,Variant3,Variant4}. It promotes epsilon to a runtime pass-by-value scalar and is the only RMSNorm test of that path. #12107 regenerated these sweeps from C++ test captures. The case has no C++ test behind it, so it was dropped, while the sibling support.json files kept its gfx1151/windows claim. verify_support_claims.py then reported "case id 'runtime_epsilon' not found in sweep.json" for all four bundles. Append the case to each sweep.json exactly as it was at c9702d9^. The four graph templates still have the same tensor names and uids, and each case's tensor patch targets that variant's epsilon tensor.
Closes #12913 ## Motivation `verify_support_claims.py` fails on develop with `case id 'runtime_epsilon' not found in sweep.json` for `quick/RMSNorm/{Default,Variant2,Variant3,Variant4}`. #9600 added `runtime_epsilon` there as a manual case. It makes epsilon a runtime pass-by-value scalar and is the only RMSNorm test of that path. #12107 regenerated these sweeps from C++ test captures with `place_bundles.py`. The case has no C++ test behind it, so the regeneration dropped it, while the `support.json` files kept its gfx1151/windows claim. Details in #12913. ## Technical Details Two commits: - Append the `runtime_epsilon` case to the four `sweep.json` files, copied unchanged from `c9702d99f61^` (436 lines added, nothing else in the files changes). The claims stay as they are. The graph templates have the same tensor names and uids as before #12107, and each case's tensor patch targets that variant's epsilon tensor (uid 3, 2, 1 and 2 for Default, Variant2, Variant3 and Variant4). - `migration-scripts/place_bundles.py` keeps the manual cases (`"generator": "manual"`) of an existing `sweep.json` when it rewrites it. A manual case that no longer fits the regenerated template (different tensor uid set, a patch on a uid the template lacks, an unresolved placeholder, or an id a captured case now uses) makes the run exit 1 and leaves that sweep and its template unchanged. Documented in `migration-scripts/README.md`. What could break: a `place_bundles.py` run over a tree with a manual case that no longer fits now fails where it used to drop the case. On develop the check flags two: `runtime_epsilon` in `quick/BatchnormBackward/Default` and `runtime_epsilon_momentum` in `quick/BatchnormPointwise/Default`. Their tensor sets match the `Variant2` template of the same operation, not `Default`. Neither has a support claim. This PR does not move them. ## Test Plan Host only, no GPU. `verify_support_claims.verify_all` on the bundle root at develop and on this branch. `scripts/test_verify_support_claims.py`. `migration-scripts/test_migration.py` with the new and the old `place_bundles.py`. The new manual-case check run against every sweep on this branch that has manual cases. ## Test Result - `verify_all`: 4 errors on develop (the four `runtime_epsilon` claims), 0 on this branch. - `test_verify_support_claims.py`: 28/28 passed. - `test_migration.py`: 14/14 passed. With the old `place_bundles.py`, the new `test_place_keeps_manual_cases` fails (the manual case is gone after the rerun) and the other 13 pass. - Manual-case check: 12 sweeps have manual cases. 10 fit their templates, including the four quick RMSNorm sweeps. The two Batchnorm cases above do not. Not run: the restored case on a GPU. Its claim records a pass on gfx1151/windows before #12107. It has not run since #12107 changed the templates (added `alignment`, an explicit `is_runtime_pass_by_value: false` and `min_required_engine_api_version`). ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
…(ALMIOPEN-2332) Two runtime pass-by-value cases from #9600 were written against another template's uids: - BatchnormBackward/Default `runtime_epsilon` lists case tensors 6 and 7 (a BatchnormPointwise tensor set; its metadata says so), but the template holds uids 0-5, and BatchnormBackward has no epsilon at all. - BatchnormPointwise/Default `runtime_epsilon_momentum` lists the 13 tensors of Variant2's template (momentum at uid 7), but Default holds uids 0-7, where 7 is scale. buildCaseTensorMap() rejects a case tensor uid the template lacks, so the harness reports both as INVALID_SWEEP_CASE and skips them: neither has ever run. Removing them changes nothing the harness executes. The runtime epsilon + momentum coverage the second one meant to add for Batchnorm + pointwise still needs a case on Variant2. The overview drops them from the denominators: MIOPEN_ENGINE's BatchnormBackward boxes go from 192/193 to a full 192/192 on every arch it claims -- a gap that was only ever the dead case. Co-Authored-By: Claude <noreply@anthropic.com>
…he bundles they match (ALMIOPEN-2332) 2ac4fd0 dropped two cases from #9600 because the harness could not load them. Their uids were right; their bundle was wrong. Each matches another bundle's template exactly: - `runtime_epsilon` (filed under BatchnormBackward/Default, which has no epsilon) lists the 8 tensors of BatchnormPointwise/Default, epsilon at uid 6; its metadata already said "BatchnormPointwise". It gains the null relu-clip attributes that template reads. - `runtime_epsilon_momentum` (filed under BatchnormPointwise/Default) lists the 13 tensors of BatchnormPointwise/Variant2, momentum at uid 7. Both now load, so for the first time they check that a provider reads epsilon (and momentum) from the variant pack at execute time. They carry no claims yet: the overview's BatchnormPointwise denominators go from 600 to 602 and stay there until CI harvests them. An engine that takes either graph and ignores the runtime value fails the test regardless. Co-Authored-By: Claude <noreply@anthropic.com>
…es to the bundles they match (#13747) ## Summary Two runtime pass-by-value sweep cases from #9600 were filed under the wrong bundle. The harness rejects their tensor uids, reports them as `INVALID_SWEEP_CASE`, and skips them, so neither has ever run. This PR moves each one to the bundle whose template it was actually written for, so both load and run for the first time. JSON only, no code changes. Split out of #11659 (support-matrix renderer), which is based on this branch so it can stay Python-only. ## How to review **Review commit by commit.** Commit 1 only deletes lines and commit 2 only adds lines. The combined "Files changed" view for `BatchnormPointwise/Default/sweep.json` pairs unrelated neighbouring cases, so it looks as though four case ids were edited. No id changes. Comparing each moved case field by field against develop: - `runtime_epsilon_momentum` (Default → Variant2) is **byte-for-byte identical**. - `runtime_epsilon` (BatchnormBackward/Default → BatchnormPointwise/Default) is identical except for one added block, `attributes: {relu_lower_clip: null, relu_lower_clip_slope: null, relu_upper_clip: null}`. BatchnormPointwise/Default's template reads those three attributes. All 11 cases in that bundle carry an `attributes` block, and 8 of them use exactly these nulls. BatchnormBackward's template has no relu clip, which is why the case never needed them before. ## Details | Case | Was in | Problem | Now in | |---|---|---|---| | `runtime_epsilon` | `BatchnormBackward/Default` | Lists case tensors 6 and 7; the template only has uids 0-5, and BatchnormBackward has no epsilon. Its own metadata already says "BatchnormPointwise". | `BatchnormPointwise/Default` (8 tensors, epsilon at uid 6) | | `runtime_epsilon_momentum` | `BatchnormPointwise/Default` | Lists 13 tensors; Default only has uids 0-7 (uid 7 is scale). | `BatchnormPointwise/Variant2` (13 tensors, momentum at uid 7) | - `buildCaseTensorMap()` rejects a case tensor uid that isn't in the template. That is why both were silently skipped and develop's CI stayed green. - The case data and uids are unchanged. The only addition is null relu-clip attributes on `runtime_epsilon`, because the BatchnormPointwise/Default template reads `relu_lower_clip`, `relu_lower_clip_slope`, and `relu_upper_clip`. - Two commits: the first removes the cases from the wrong bundles, the second adds them to the right ones. Reviewing them in order makes the move obvious. - No support claims are added for either case. Claims can be harvested once CI shows which engines pass. **What to watch in CI:** these cases now check, for the first time, that a provider reads epsilon (and momentum) from the variant pack at execute time. If one fails, the failure origin says whether the engine or the CPU reference is at fault. - **Engine at fault:** that's a real provider bug. I'll file a ticket and reduce this PR to the deletion commit. - **Reference at fault:** that's a harness fix, which goes in its own PR. ## Test plan - [ ] CI: `quick_BatchnormPointwise_Default` and `quick_BatchnormPointwise_Variant2` run the two moved cases (no `INVALID_SWEEP_CASE`) - [x] The support-matrix renderer from #11659 (which mirrors `buildCaseTensorMap()`) loads both cases with no errors. All 2053 renderer tests pass with this branch merged in. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Adam Dickin <adam.dickin@amd.com>
Summary
JIRA ID : ALMIOPEN-2267
Add data-driven integration test bundles for runtime pass-by-value (PBV) scalar tensors. These bundles detect providers that claim support for graphs using runtime scalars but produce incorrect output by ignoring the runtime value and falling back to a compile-time default — a correctness gap the existing compile-time bundles cannot exercise.
Also adds an invariant check in the bundle-loading harness rejecting any tensor that declares
is_runtime_pass_by_value=truewhile still carrying a bakedvalue_type/value, since that combination is contradictory (the harness would not know which source of truth to trust).Risk Assessment
Low risk (2/5). Bundle JSON/meta files and the reference-data generation script are additive/data-only test infrastructure. The one production-code change (
IntegrationTestBundle.hpp'sapplyTensorPatches) is an additive validation check: it only rejects a previously-nonsensical tensor configuration and does not alter behavior for any existing bundle.ASIC Coverage
ASIC-independent. All changes are in test infrastructure (bundle JSON/meta files, the harness invariant check, and an offline reference-data generation script) — no kernel selection, provider dispatch, or device-facing behavior is touched. Passing PR CI is sufficient; no multi-arch sweep is required.
Testing Summary
tensor_patchescases inside existing template-sweep bundles (Batchnorm,BatchnormInferenceAttributesVarianceExt(Pointwise),BatchnormPointwise,RMSNorm), promoting select scalars (epsilon, momentum, etc.) tois_runtime_pass_by_value=trueper case. The harness supplies the correct runtime value viainputs; the CPU reference executor and provider must both read it from the variantPack.SdpaFwdRuntimeScalebundle family (13 cases across bf16/fp16/fp8, hd128/hd192, causal/nomask, batch/group/stats) uses a pure-runtime strategy: the attention scale carriesis_runtime_pass_by_value=truewith no baked value; a provider that falls back to a hardcoded scale diverges from the CPU reference.is_runtime_pass_by_value=truethat also carries a bakedvalue_type/valueis now rejected at bundle-load time.Testing Checklist
hipdnn-integration-tests-unit-check- Status: Passed (3/3, 1991 tests)Adjacent Tests Considered
Existing compile-time
Batchnorm,RMSNorm, andSdpaFwdbundles are untouched — the new runtime-PBV cases are added alongside them in the same sweep files, not replacing anything.Technical Changes
IntegrationTestBundle.hpp—applyTensorPatchesnow rejects any tensor withis_runtime_pass_by_value=truethat still carries a bakedvalue_type/value, since runtime-injected tensors must come from the variantPack only.tensor_patchescases promoting selected scalars (e.g.runtime_epsilon_momentuminBatchnorm/Default) to runtime PBV, testing that providers correctly read them from the variantPack rather than a compile-time value.SdpaFwdRuntimeScalebundles (new, 13 cases) — pure-runtime attention scale coverage across bf16/fp16/fp8 and hd128/hd192 causal/nomask/batch/group/stats variants.generate_sdpa_runtime_scale_test_bundle.pyand newreference_data_scripts/requirements.txtpinning the generator'storchdependency.TestSdpaFwdPlan.cpp— updated call sites to pass an additional optional parameter for the CPU referenceforward()helper.