Skip to content

AIRUNTIME-2743: Add unsupported skip field to Catch2 config parser - #11308

Merged
sruscica merged 1 commit into
developfrom
users/sruscica/hip-tests-split-unsupported-disabled
Sep 28, 2026
Merged

sruscica merged 1 commit into
developfrom
users/sruscica/hip-tests-split-unsupported-disabled

Conversation

@sruscica

@sruscica sruscica commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Phase 1 of AIRUNTIME-2743: introduce an unsupported skip field in the HIP Catch2 test-config parser to distinguish permanent skips (an OS/arch/config genuinely does not support a test) from the existing disabled field, which covers temporary skips (slow, flaky, or currently failing tests). This is a behavior-preserving refactor that adds the machinery only — generated output for existing entries is byte-identical.

Technical Details

Both disabled and unsupported feed the identical Catch2 tag match and [exclude_<entry>] promotion logic, so generated headers are unchanged for every existing disabled-only entry.

Each skip field may take one of two shapes:

  • a legacy flat list of targets — disabled: [amd_windows] (accepted unchanged for the unmigrated corpus), or
  • a mapping carrying its own reason — so a test that is both disabled and unsupported can explain each independently:
disabled:
  targets: [amd_wsl]
  reason: AIRUNTIME-2335 - intermittent CI failure
unsupported:
  targets: [amd_windows]
  reason: PAL does not charge device memory at instantiate

Only targets drive tag generation; reason is metadata that the parser ignores.

Changes:

  • parse_config.py: Adds the unsupported skip field alongside disabled. A new _skip_targets() helper extracts the target list from either the flat-list or the {targets, reason} mapping form; the two fields are merged (skip_targets = disabled + unsupported, disabled first) and run through the existing tag-generation path. The --asan help text now mentions both fields.
  • check_config.py: Validates that disabled/unsupported, when present, are either a list or a mapping whose targets is a list; any other shape (e.g. a bare scalar) is rejected. The reason is not enforced yet — the ~1,100 existing configs have not been migrated to carry reasons, so requiring one now would fail them. Reason enforcement is deferred to AIRUNTIME-2744 (noted in an inline comment). The existing level-presence check is untouched.
  • configs/unit/graph.yaml: Migrates one real test (Unit_hipGraphStreamPool_InstantiateFootprint) from disabled: [amd_windows] to the unsupported mapping form with a reason, as a worked example of the larger migration to come.

Two CI-critical invariants are preserved:

  1. The bare [disabled] Catch2 tag that drives CTest DISABLED registration.
  2. The [exclude_<entry>] promotion tags consumed by the compute-utils hip test runner scripts.

Reclassifying the ~1,100 existing entries and enforcing the reason field in CI are explicitly deferred to Phase 2.

Issue Tracking

JIRA ID: AIRUNTIME-2743

Phase 2 follow-up: AIRUNTIME-2744

Test Plan

  • Regenerate headers across the amd, nvidia, and asan configuration triples and diff against the pre-change baseline.
  • Run the full test corpus through generation and validation (parse_config.py + check_config.py).
  • Exercise a test that is both disabled and unsupported with separate reasons.
  • Confirm malformed skip fields (bare scalar, mapping missing a targets list) are rejected.
  • Verify the migrated entry disables on Windows and runs on Linux in real CI.

Test Result

  • Headers regenerated across the amd/nvidia/asan triples are byte-identical to the pre-change baseline for every existing disabled-only entry.
  • Full corpus (4853 test defines) generates cleanly and check_config exits 0.
  • The both-fields case emits [disabled] on amd_wsl and amd_windows, runs on amd_linux, and carries both [exclude_amd_wsl] and [exclude_amd_windows] promotions.
  • A bare-scalar skip field and a mapping missing targets are both rejected with a clear group/case/field error.
  • Real CI confirmed the migrated Unit_hipGraphStreamPool_InstantiateFootprint: on Windows (gfx1151) it reported ***Not Run (Disabled) (unsupported → [disabled] → CTest skip); on Linux (gfx950) it ran — validating both invariants above.

Submission Checklist

🤖 Claude Code 🤖

@sruscica
sruscica requested a review from a team as a code owner September 8, 2026 13:51
@therock-pr-bot

therock-pr-bot Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/hip-tests/catch/config/check_config.py, projects/hip-tests/catch/config/parse_config.py; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 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 Sep 8, 2026

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@sruscica

sruscica commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

CI verification: unsupported disables identically to disabled

The migrated example Unit_hipGraphStreamPool_InstantiateFootprint (moved from disabled: [amd_windows] to the structured unsupported: [amd_windows]) was exercised on real CI in this PR's run:

Windows (gfx1151) — CTest skipped it, driven solely by the [disabled] tag that the unsupported field produced:

Test #358: Unit_hipGraphStreamPool_InstantiateFootprint ...***Not Run (Disabled)   0.00 sec
        358 - Unit_hipGraphStreamPool_InstantiateFootprint (Disabled)

Linux (gfx950) — the same test executed normally (it is not unsupported on Linux), confirming the field does not over-disable.

Platform Expected CI result
Windows (gfx1151) disabled ***Not Run (Disabled)
Linux (gfx950) runs executed

This confirms the CI-critical [disabled] skip tag (and the [exclude_<entry>] promotions the compute-utils runner scripts consume) are byte-for-byte preserved when an entry lives in unsupported instead of disabled.

All build jobs passed (NVIDIA, Linux gfx950/gfx125X/gfx94X, Windows gfx1151). The MI455 test-stage red is an unrelated runner/infra failure (255/1278 tests failing across unrelated groups, container exit 137), and therock-pr-bot shows a poll-window timeout — neither is related to this change.

🤖 Claude Code 🤖

@fabiomestre fabiomestre 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.

I think we need to update rocm-systems/projects/hip-tests/TESTING.md (and potentially other documentation) to reflect these changes.

Otherwise, looks good to me.

@sruscica
sruscica force-pushed the users/sruscica/hip-tests-split-unsupported-disabled branch 2 times, most recently from 35ac7be to 98af54f Compare September 21, 2026 21:43

@mangupta mangupta 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.

🤖 AI-assisted pre-review (for @mangupta)

@sruscica
sruscica requested a review from a team as a code owner September 24, 2026 14:34
@sruscica
sruscica force-pushed the users/sruscica/hip-tests-split-unsupported-disabled branch from ae89ca1 to 5372a65 Compare September 25, 2026 16:31
Introduce an `unsupported:` skip field alongside the existing `disabled:`
field in the Catch2 config YAML. Both share the same syntax (a flat target
list, or a `{targets, reason}` mapping) and emit the same skip tags; they
differ only in intent. `unsupported:` marks a genuine capability gap (a
platform, architecture, or runtime mode that cannot run the case), while
`disabled:` marks a temporary or tracked defect.

Reason-field enforcement is intentionally deferred (AIRUNTIME-2744); the
validator only guards that each skip field is a list or a mapping with a
`targets` list, since a scalar breaks tag generation.

Changes:
- parse_config.py: emit skip tags for the `unsupported:` field
- check_config.py: validate `disabled`/`unsupported` field shape
- README.md, TESTING.md: document the unsupported field
@sruscica
sruscica force-pushed the users/sruscica/hip-tests-split-unsupported-disabled branch from 5372a65 to 058abf4 Compare September 25, 2026 16:36
@sruscica
sruscica merged commit 0bf70ef into develop Sep 28, 2026
166 of 169 checks passed
@sruscica
sruscica deleted the users/sruscica/hip-tests-split-unsupported-disabled branch September 28, 2026 15:36
@systems-assistant

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.

3 participants