fix(hipdnn): fail fast on plan-only override execute without override enabled#8881
Merged
SamuelReeder merged 5 commits intoJun 30, 2026
Merged
Conversation
…t override enabled The frontend override execute() overload fails fast when override args are supplied to a graph that did not enable override shapes, but only for full graphs. The plan-only (compiled-plan-deserialized) branch skipped the override-enabled check, so misuse on a plan-only object was only caught later at the backend dispatch guard. deserialize_compiled_plan hard-coded the flag to false, so it could not be checked as-is. The flag is preserved in the backend ExecutionPlanDescriptor (serialized into the plan flatbuffer, restored on deserialize) but was not reachable from the frontend: getAttribute did not surface it, and no operation graph or engine config descriptor is reconstructed on the plan-only path. The only no-new-attribute alternative is parsing the backend flatbuffer in the header-only frontend, which couples it to the wire format and version gate. Surface the flag through the existing C attribute API instead: - Add read-only HIPDNN_ATTR_EXECUTION_PLAN_IS_OVERRIDE_SHAPE_ENABLED_EXT and a getAttribute case mirroring ENGINE_GLOBAL_INDEX_EXT (no serialization change). - Recover it in deserialize_compiled_plan via a new frontend helper. - Check it in the plan-only branch, returning INVALID_VALUE to match the full-graph branch and short-circuit before any provider dispatch.
SamuelReeder
changed the title
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
Author
|
I think therock-pr-bot expects a different test name format than ours. I could add additional tests but it will not fix the warning for the time-being. |
BrianHarrisonAMD
approved these changes
Jun 29, 2026
BrianHarrisonAMD
left a comment
BrianHarrisonAMD
left a comment
Contributor
There was a problem hiding this comment.
Looks good.
Some minor clean-up, and testing improvements, but approving.
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (76.92%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #8881 +/- ##
===========================================
- Coverage 71.34% 71.32% -0.02%
===========================================
Files 2613 2613
Lines 408997 409193 +196
Branches 61114 61118 +4
===========================================
+ Hits 291775 291817 +42
- Misses 95825 95977 +152
- Partials 21397 21399 +2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
SamuelReeder
deleted the
users/sareeder/almiopen-2244-planonly-override-failfast
branch
JP-Fernando
pushed a commit
that referenced
this pull request
Jul 1, 2026
… enabled (#8881) ## Summary The frontend override `execute()` overload fails fast when override args are supplied to a graph that did not enable override shapes — but only for full graphs. The plan-only [deserialize_compiled_plan](https://github.com/ROCm/rocm-libraries/blob/f1230db5ef05ba6cf6413741a1b58b6a80473a8a/projects/hipdnn/frontend/include/hipdnn_frontend/Graph.hpp#L1571) branch skipped the override-enabled check, so misuse on a plan-only object was only caught later at the backend dispatch guard. This makes the plan-only branch fail fast in the frontend, matching the full-graph branch. Part of the overridable tensor shapes work ([RFC 0008](https://github.com/ROCm/rocm-libraries/blob/develop/projects/hipdnn/docs/rfcs/0008_OverridableTensorShapesDesign.md)). JIRA ID : ALMIOPEN-2244 ## Risk Assessment Medium risk. Adds a read-only backend execution-plan attribute (API addition) and changes the plan-only override-execute path to reject earlier. The new rejection only fires for misuse the backend dispatch guard already rejected authoritatively, so no legitimate path changes: plan-only overrides on an override-enabled plan still execute, and full-graph behavior is unchanged. No serialization format change. Blast radius is ASIC-independent (see below), so passing PR CI is sufficient. ## Testing Summary - Backend unit tests: new attribute round-trips through deserialize → `getAttribute` for both flag states; existing plan-descriptor, serialization, and enum-string suites unaffected. - Frontend GPU integration tests: new plan-only fail-fast regression (rejects with `INVALID_VALUE` and never dispatches to the provider); full override-execute suite for no regressions. - Commit hooks (clang-format, whitespace) on all changed files. ## Testing Checklist - [x] hipDNN backend unit tests - `hipdnn_backend_tests --gtest_filter='TestExecutionPlanDescriptor.*:*SerializationApi*:*BackendEnumStringUtils*:*IsOverrideShapeEnabledExt*'` - ASICs: gfx90a - Status: Passed (74) - [x] hipDNN frontend override integration tests - `hipdnn_public_frontend_tests --gtest_filter='*Override*'` - ASICs: gfx90a - Status: Passed (35) - [x] Commit hooks - `pre-commit run --files <changed>` - Status: Passed - [ ] PR CI - GitHub PR checks - Status: Pending ## Technical Changes - Add read-only `HIPDNN_ATTR_EXECUTION_PLAN_IS_OVERRIDE_SHAPE_ENABLED_EXT` with a `getAttribute` case on `ExecutionPlanDescriptor` (mirrors `ENGINE_GLOBAL_INDEX_EXT`; no serialization change) plus its enum-name mapping. The attribute is required because the override-enabled flag, while already preserved across the plan round-trip in the backend (serialized into the plan flatbuffer, restored on deserialize), was unreachable from the header-only frontend: `getAttribute` did not surface it, `deserialize_compiled_plan` hard-coded it to `false`, and no operation-graph or engine-config descriptor is reconstructed on the plan-only path. The only no-new-attribute alternative — parsing the backend plan flatbuffer in the frontend — would couple the frontend to the backend wire format and version gate. - Recover the flag in `deserialize_compiled_plan` via a new `getExecutionPlanOverrideShapeEnabled` frontend helper instead of forcing `false`. - Reject override args in the plan-only branch of the override `execute()` overload with `INVALID_VALUE`, short-circuiting before any provider dispatch. - Add backend tests for the attribute (both flag states) and enum-name mapping, and a frontend GPU regression test for the plan-only fail-fast.
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
3 participants
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
The frontend override
execute()overload fails fast when override args are supplied to a graph that did not enable override shapes — but only for full graphs. The plan-only deserialize_compiled_plan branch skipped the override-enabled check, so misuse on a plan-only object was only caught later at the backend dispatch guard. This makes the plan-only branch fail fast in the frontend, matching the full-graph branch. Part of the overridable tensor shapes work (RFC 0008).JIRA ID : ALMIOPEN-2244
Risk Assessment
Medium risk. Adds a read-only backend execution-plan attribute (API addition) and changes the plan-only override-execute path to reject earlier. The new rejection only fires for misuse the backend dispatch guard already rejected authoritatively, so no legitimate path changes: plan-only overrides on an override-enabled plan still execute, and full-graph behavior is unchanged. No serialization format change. Blast radius is ASIC-independent (see below), so passing PR CI is sufficient.
Testing Summary
getAttributefor both flag states; existing plan-descriptor, serialization, and enum-string suites unaffected.INVALID_VALUEand never dispatches to the provider); full override-execute suite for no regressions.Testing Checklist
hipdnn_backend_tests --gtest_filter='TestExecutionPlanDescriptor.*:*SerializationApi*:*BackendEnumStringUtils*:*IsOverrideShapeEnabledExt*'- ASICs: gfx90a - Status: Passed (74)hipdnn_public_frontend_tests --gtest_filter='*Override*'- ASICs: gfx90a - Status: Passed (35)pre-commit run --files <changed>- Status: PassedTechnical Changes
HIPDNN_ATTR_EXECUTION_PLAN_IS_OVERRIDE_SHAPE_ENABLED_EXTwith agetAttributecase onExecutionPlanDescriptor(mirrorsENGINE_GLOBAL_INDEX_EXT; no serialization change) plus its enum-name mapping. The attribute is required because the override-enabled flag, while already preserved across the plan round-trip in the backend (serialized into the plan flatbuffer, restored on deserialize), was unreachable from the header-only frontend:getAttributedid not surface it,deserialize_compiled_planhard-coded it tofalse, and no operation-graph or engine-config descriptor is reconstructed on the plan-only path. The only no-new-attribute alternative — parsing the backend plan flatbuffer in the frontend — would couple the frontend to the backend wire format and version gate.deserialize_compiled_planvia a newgetExecutionPlanOverrideShapeEnabledfrontend helper instead of forcingfalse.execute()overload withINVALID_VALUE, short-circuiting before any provider dispatch.