Skip to content

fix(integration-tests): throw on malformed bundle metadata instead of silent nullopt - #12870

Merged
bghimireamd merged 3 commits into
developfrom
users/bghimire/ALMIOPEN-2393/hardne_bundle_meta_data
Oct 7, 2026
Merged

bghimireamd merged 3 commits into
developfrom
users/bghimire/ALMIOPEN-2393/hardne_bundle_meta_data

Conversation

@bghimireamd

@bghimireamd bghimireamd commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Harden bundle metadata parsing: a malformed .meta.json or sweep-case metadata block now throws BundleMetadataError instead of silently returning std::nullopt and running the bundle with default metadata. The bundle then fails as a registered test, and the failure message carries the parser's detail (file or case, and what is wrong). Follow-up to #10244 (review comment r3715954787).

JIRA ID : ALMIOPEN-2393

Risk Assessment

Risk 2. Host-side integration-test harness only; no library, provider, or kernel code changes. All 59 checked-in .meta.json files and all 10,732 sweep metadata blocks pass the stricter parser (including 9,145 inputs keys, none non-numeric), so no existing bundle changes behavior. The .meta.json files in #12738 also pass.

ASIC Coverage

Arch-independent (JSON parsing in the test harness). Standard PR CI is sufficient.

Testing Summary

  • Unit tests for every malformed-metadata case, the error text naming its source, the format_version truncation case, and non-numeric inputs keys.
  • Load and classification tests for graph-only, golden, and sweep-case bundles: malformed metadata throws and classifies as a failed test whose message includes the detail; absent metadata keeps its existing behavior.

Testing Checklist

  • Build - cmake --build build --target hipdnn_integration_tests hipdnn_integration_tests_unit_tests - Status: Passed
  • Integration-test unit tests - build/bin/hipdnn_integration_tests_unit_tests - Status: Passed (897 passed, 2 pre-existing skips)
  • pre-commit - pre-commit run --files <changed files> - Status: Passed
  • PR CI - GitHub PR checks - Status: Pending

Technical Changes

  • BundleMetadata.hpp: add BundleMetadataError. parseBundleMetadataJson returns BundleMetadata and throws on malformed input, including an inputs key that is not entirely an integer ("x", "12abc"), which used to be dropped with a warning. loadBundleMetadata returns nullopt only when the file is absent. format_version is compared as a JSON value, so 1.5 and 2^32 + 1 are rejected.
  • IntegrationTestBundle.hpp: BundleMetadataError propagates out of both load paths, the same way RuntimePassByValueInvariantError already does. MISSING_METADATA and UNVALIDATABLE_GOLDEN_DATA now mean only "metadata absent".
  • BundleRegistration.hpp: classifyBundle turns BundleMetadataError into a FailedLoad whose message includes e.what(). Malformed metadata is red on every bundle type, including graph-only bundles, so a typo cannot silently skip a test.
  • Tests: TestBundleMetadata.cpp uses EXPECT_THROW and adds the TestParseBundleMetadataJson suite. TestBundleDiscovery.cpp covers malformed metadata on graph-only, golden, and sweep-case bundles, and the FailedLoad message detail.
  • GoldenReferenceCpu.hpp, listed in the ticket, was removed in cleanup(integration-tests): remove legacy per-op golden test classes #10343. Its remaining callers are the two load paths in IntegrationTestBundle.hpp, both covered here.
  • Compatible with feat(hipdnn, integration-tests): Update tests to accomodate SDPA ragged tensor bundles #12738: any new metadata field parsing there should throw BundleMetadataError.

🤖 Generated with Claude Code

@therock-pr-bot

therock-pr-bot Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Oct 1, 2026

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

@adickin-amd

Copy link
Copy Markdown
Contributor

[Warning] dnn-providers/integration-tests/src/harness/bundle/BundleRegistration.hpp:338

This block comment is stale after the PR. It still says golden blobs whose metadata is "missing or unparseable" map to LoadError::UNVALIDATABLE_GOLDEN_DATA. It also lists only RuntimePassByValue and UNVALIDATABLE as the red cases, with no mention of BundleMetadataError. After this change, unparseable metadata never reaches UNVALIDATABLE_GOLDEN_DATA: it throws, and it's red even for graph-only bundles. This is the comment someone reads to learn which failures go red, so please update it to match classifyBundle().

Generated by AI

@bghimireamd
bghimireamd marked this pull request as ready for review October 2, 2026 20:45
@bghimireamd
bghimireamd requested a review from a team as a code owner October 2, 2026 20:45
@bghimireamd
bghimireamd force-pushed the users/bghimire/ALMIOPEN-2393/hardne_bundle_meta_data branch 2 times, most recently from ed59482 to fb13190 Compare October 6, 2026 16:10
bghimireamd and others added 3 commits October 6, 2026 19:38
… silent nullopt

Bundle metadata (.meta.json and inline sweep-case metadata) that is
malformed (not an object, missing or non-integer format_version,
unsupported version, invalid enforcement_level, unreadable or
unparseable file) now throws BundleMetadataError instead of returning
std::nullopt and silently falling back to defaults. A missing
.meta.json still returns nullopt.

format_version is compared as a JSON value, so 1.5 is no longer
truncated to 1 and accepted.

IntegrationTestBundle translates the error to UNVALIDATABLE_GOLDEN_DATA
when golden outputs are present (registered as a failure), otherwise to
MISSING_METADATA / INVALID_SWEEP_CASE (logged and skipped).

Follow-up to #10244 (ALMIOPEN-2331).

Co-Authored-By: Claude <noreply@anthropic.com>
… and reject non-numeric inputs keys

BundleMetadataError now propagates out of the bundle loader instead of
being mapped to MISSING_METADATA / UNVALIDATABLE_GOLDEN_DATA /
INVALID_SWEEP_CASE. classifyBundle turns it into a FailedLoad whose
message carries the parser's detail, so malformed metadata is red on
every bundle type (graph-only included) and the author sees what to fix.
Those LoadError values now mean "metadata absent" only.

parseBundleMetadataJson throws on an inputs key that is not entirely an
integer instead of dropping it with a warning, and sets formatVersion
to the validated constant.

Co-Authored-By: Claude <noreply@anthropic.com>
Update the loadDiscoveredBundles() comment to list all three hard-fail
cases, including BundleMetadataError, and reflow the SkippedLoad comment
so the malformed-metadata note no longer splits a sentence.

Co-Authored-By: Claude <noreply@anthropic.com>
@bghimireamd
bghimireamd force-pushed the users/bghimire/ALMIOPEN-2393/hardne_bundle_meta_data branch from fb13190 to 384b8a6 Compare October 6, 2026 19:51

@adickin-amd adickin-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just running a quick test locally to test out a bunch of malformed bundles on my machine. If you dont see any comments after that feel free to merge.

@bghimireamd
bghimireamd merged commit 5f875a6 into develop Oct 7, 2026
99 checks passed
@bghimireamd
bghimireamd deleted the users/bghimire/ALMIOPEN-2393/hardne_bundle_meta_data branch October 7, 2026 22:44
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants