Skip to content

fix(ci): test shared BLAS changes in multi-arch CI - #11667

Merged
tony-davis merged 1 commit into
developfrom
users/tony-davis/fix-multi-arch-shared-blas-tests
Sep 3, 2026
Merged

tony-davis merged 1 commit into
developfrom
users/tony-davis/fix-multi-arch-shared-blas-tests

Conversation

@tony-davis

Copy link
Copy Markdown
Contributor

ISSUE ID : ROCm/TheRock#3343

Motivation

The default TheRock Multi-Arch CI passes changed monorepo paths directly to TheRock's test dependency selector. Paths under shared/, including shared/stinkytofu, are not recognized as TheRock subprojects. This allowed shared BLAS changes to rebuild math-libs while running only sanity tests, as seen on PR #11643.

The multi-arch rollout issue requires at least the test coverage selected by the existing therock_matrix.py, but that mapping was only used by the legacy single-arch workflow.

Technical Details

  • Keep the original changed subtree paths for stage-impact analysis and artifact reuse.
  • Derive a separate test-project list from the existing therock_matrix.py mapping before invoking the Linux and Windows multi-arch workflows.
  • Restore BLAS coverage for shared/stinkytofu, shared/tensile, shared/origami, and shared/mxdatagenerator.
  • Select rocRoller tests for shared/rocroller changes, Origami tests for shared/origami changes, and rocRoller tests for shared/mxdatagenerator changes.
  • Fail configuration when a future shared/* subtree has no test mapping instead of silently producing a sanity-only green run.

Test Plan

  • Run the focused mapping regression tests.
  • Run the complete .github/scripts test suite.
  • Run pre-commit on the staged files.
  • Let PR CI exercise the modified workflow and its full-test trigger.

Test Result

  • python -m pytest .github/scripts/tests/multi_arch_test_projects_test.py -q: 9 passed.
  • python -m pytest .github/scripts/tests -q: 149 passed, 8 xfailed, 37 subtests passed.
  • pre-commit run: passed.
  • PR CI: pending.

Device / Architecture Coverage

This changes test selection rather than product behavior. PR CI should run the full available multi-arch matrix because the workflow itself changed. TheRock currently has no gfx1250 hardware runner, so this restores the available BLAS GPU lanes but does not add native gfx1250 execution for stinkytofu.

Adjacent Tests Considered

The existing single-arch mapping is retained as the minimum coverage source of truth. Origami and mxDataGenerator also select their directly affected component tests because those are not included in the legacy BLAS umbrella.

Related

Submission Checklist

Risk level

Medium. This changes test selection for every shared BLAS subtree and intentionally increases CI coverage. Unit tests lock each mapping, unknown shared paths fail closed, and the PR workflow change triggers the full available multi-arch CI matrix.

Co-authored-by: Cursor <cursoragent@cursor.com>
@therock-pr-bot

therock-pr-bot Bot commented Sep 3, 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 Sep 3, 2026

Copy link
Copy Markdown

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

@tony-davis
tony-davis marked this pull request as ready for review September 3, 2026 17:09
@tony-davis
tony-davis requested a review from a team as a code owner September 3, 2026 17:09
Copilot AI lite review requested due to automatic review settings September 3, 2026 17:09

Copilot AI 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.

🟢 Approval recommended

The workflow wiring and the new mapping script are consistent with the existing therock_matrix.py mapping source-of-truth and are covered by targeted unit tests that enforce the intended selection and fail-closed behavior.

Pull request overview

This PR adjusts the rocm-libraries default TheRock multi-arch CI workflow to derive a dedicated “test projects” list (based on the existing therock_matrix.py mapping) before invoking downstream Linux/Windows multi-arch workflows, preventing shared/* subtree changes from silently falling back to sanity-only coverage.

Changes:

  • Add a CI-side mapping step (multi_arch_test_projects.py) to translate shared/* changed subtrees into TheRock-recognized test project names, failing closed for unmapped shared paths.
  • Update the multi-arch workflow to pass the derived test_projects to downstream TheRock multi-arch workflows while preserving the original changed_projects for stage impact / artifact reuse logic.
  • Add focused unit tests covering shared-subtree mappings, deduplication behavior, and the fail-closed contract.
File summaries
File Description
.github/workflows/therock-multi-arch-ci.yml Adds a “Select multi-arch test projects” step and routes downstream changed_projects to the derived test-project list.
.github/scripts/multi_arch_test_projects.py Implements the shared-subtree → test-project selection logic using the existing therock_matrix.py mapping and emits test_projects for workflows.
.github/scripts/tests/multi_arch_test_projects_test.py Adds regression tests validating mappings for shared BLAS-related subtrees and fail-closed behavior.
.github/scripts/DEVELOPMENT.md Documents the new script and its tests in the .github/scripts developer guide.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@tony-davis

Copy link
Copy Markdown
Contributor Author

Validated this fix by replaying the original false-green case against it: #11669 is a throwaway draft PR that takes this fix as its base, reverts it back out, then re-applies the exact shared/stinkytofu changes from #11643 (the PR that only got sanity tests instead of the full BLAS suite).

That isolates the diff to just the stinkytofu files, so the only thing that changed between "before" and "after" is this mapping fix. The Configure Changed Projects job on #11669 confirms it's working:

Matched projects: ['shared/stinkytofu']
test_projects: 'hipblas,hipblaslt,rocblas,tensilelite'

Previously test_projects would have stayed as the literal shared/stinkytofu string, which TheRock's test-dependency selector doesn't recognize, causing it to silently fall back to sanity-only tests. Will update here once the full build/test matrix on #11669 finishes and we can confirm the actual hipBLASLt/rocBLAS/hipBLAS/TensileLite test jobs run.

@jayhawk-commits jayhawk-commits left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

Will make changes on TheRock side to address these gaps, but this works for the short-term.

tony-davis added a commit that referenced this pull request Sep 3, 2026
…ostsubmit + nightly) (#11677)

## Summary

Restores gfx125X (CDNA5/gfx1250) build-only coverage that was silently
dropped when #11542 made "TheRock Multi-Arch CI" the default, replacing
the legacy single-arch "TheRock CI" (now opt-in via `ci:single-arch`
label / `workflow_dispatch`).

Legacy `therock-ci.yml` built `gfx94X, gfx950, gfx125X` on every PR and
push, gfx125X build-only by deliberate design (9fa5244 -- "add
gfx125X build gating with deferred test enablement"). The new default
`therock-multi-arch-ci.yml` only builds `gfx94X, gfx950` -- gfx125X
coverage was lost entirely, not moved anywhere.

No GPU test hardware exists for gfx1250 anywhere in CI yet (see #11675),
so this only restores build validation, not test execution -- but that
is exactly what the legacy default provided too.

Two commits:
1. Add `gfx125X` build-only to `therock-multi-arch-ci-nightly.yml`'s
Linux family list.
2. Add `gfx125X` build-only to `therock-multi-arch-ci.yml`'s default
Linux family list, restoring per-PR/push parity with the legacy default
(so gfx1250 build breaks are caught same-PR, not up to ~24h later at
nightly).

The Windows family default also changed in the same migration (`gfx1151`
-> `gfx110X`), but that is not a regression -- `gfx110X` gets real test
execution on presubmit, while `gfx1151` was build-only there too
(`nightly_check_only_for_family` gates its tests to schedule/dispatch in
TheRock's own family matrix). `gfx1151` nightly coverage for
rocm-libraries is already intact in `therock-multi-arch-ci-nightly.yml`.

## Testing

- Validated YAML syntax and ran `pre-commit run --files <each changed
workflow>` (all hooks pass) on both commits.
- No test-suite changes needed. This is workflow-input-only.
- Will confirm on the next PR/push run (and the next scheduled nightly,
or a manual `workflow_dispatch`) that a `gfx125X` build shard appears
and completes in both workflows.

## Related

- Tracking issue: #11675
- Prior BLAS test-selection fix in Multi-Arch CI: #11667
- Migration that dropped this coverage: #11542
- Original gfx125X build-gating design: 9fa5244

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
@tony-davis
tony-davis merged commit 541ca9a into develop Sep 3, 2026
487 of 501 checks passed
@tony-davis
tony-davis deleted the users/tony-davis/fix-multi-arch-shared-blas-tests branch September 3, 2026 20:36
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

talumbau added a commit that referenced this pull request Sep 9, 2026
The workflows and scripts that decide which test suites and GPU families
run on every pull request are currently owned by the general /.github/
rule. A change to them silently changes test coverage for the math
libraries teams without requiring their review.

That happened in #11542, which made Multi-Arch CI the default gate. It
was approved by four reviewers, none of whom belonged to any math
libraries team, and it dropped BLAS test selection for shared/ subtrees
and gfx125X build coverage. Both required follow-up fixes (#11667,
#11677), and gfx1250 pre-submit coverage is still not restored.

Assign these paths solely to @ROCm/rocm-math-lib-build-infra so a math
libraries owner must approve changes to them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
davidd-amd pushed a commit that referenced this pull request Sep 10, 2026
## Problem

PR #11667 added a translation layer (`multi_arch_test_projects.py` +
`therock_matrix.py`'s `shared/*` entries) so Multi-Arch CI could map
`shared/*` monorepo subtrees (e.g. `shared/tensile`, `shared/origami`)
to the right TheRock test projects, back when TheRock's own selector had
no notion of these paths. TheRock's
`test_tools/determine_rocm_test_dependencies.py` now natively resolves
all five `shared/*` paths (confirmed already in the pinned ref,
`df3d451a`), so the rocm-libraries-side pre-resolution step is
redundant: raw `shared/*` paths can now pass straight through to
TheRock's selector, exactly like every `projects/*` path already does.

Tracked in #11784 (the remaining follow-up item from that issue),
following #11785.

## Changes

- **Deleted** `.github/scripts/multi_arch_test_projects.py` and its test
file. This script's only caller was `therock-multi-arch-ci.yml`'s
"Select multi-arch test projects" step.
- **`therock-multi-arch-ci.yml`**: removed that step; `test_projects`
now comes straight from `steps.configure.outputs.changed_projects`.
Trimmed the now-unused `ci_utils.py`/`therock_matrix.py` sparse-checkout
entries from the same job.
- **`therock_matrix.py`'s `shared/*` entries in `subtree_to_project_map`
are intentionally left untouched** -- they're still required by
`therock_configure_ci.py` (the opt-in `ci:single-arch` legacy path),
which never calls TheRock's native selector.

## Behavior delta (verified against the exact pinned TheRock commit,
`df3d451a`)

Ran `determine_rocm_test_dependencies.py` locally for all five
`shared/*` paths, before (today's `multi_arch_test_projects.py` output)
vs. after (native selector, default per-component levels, no CLI
override):

| Changed path | Before (legacy) | After (native) | Delta |
|---|---|---|---|
| `shared/origami` | hipblas, hipblaslt, origami, rocblas, tensilelite |
hipblas, hipblaslt, hipblasltprovider, hipdnn-samples, hipsolver,
hipsparse, hipsparselt, miopen, miopenprovider, origami, rocalution,
rocblas, rocsolver, rocsparse, rocwmma, tensilelite | superset -- safe |
| `shared/stinkytofu` | hipblas, hipblaslt, rocblas, tensilelite |
hipblas, hipblaslt, hipblasltprovider, hipdnn-samples, hipsolver,
hipsparse, hipsparselt, miopen, miopenprovider, rocalution, rocblas,
rocsolver, rocsparse, rocwmma, tensilelite | superset -- safe |
| `shared/mxdatagenerator` | hipblas, hipblaslt, rocblas, rocroller,
tensilelite | hipblas, hipblaslt, hipblasltprovider, hipdnn,
hipdnn-samples, hipsolver, hipsparse, hipsparselt, miopen,
miopenprovider, rocalution, rocblas, rocroller, rocsolver, rocsparse,
rocwmma, tensilelite | superset -- safe |
| `shared/rocroller` | rocroller | hipblaslt, hipdnn, miopen, rocroller
| superset -- safe |
| `shared/tensile` | hipblas, hipblaslt, rocblas, tensilelite | hipblas,
hipsolver, miopen, rocalution, rocblas, rocsolver, rocsparse, rocwmma |
**narrows**: drops hipblaslt, tensilelite; adds hipsolver, miopen,
rocalution, rocsolver, rocsparse, rocwmma |

**On the `shared/tensile` narrowing:** checked
`test_tools/therock_consumer_graph.json` -- `rocblas`'s consumers do not
include `hipblaslt` (the edge runs the other way: `hipblaslt` consumes
`rocblas`, not vice versa), so `hipblaslt`/`tensilelite` don't
architecturally depend on `shared/tensile` changes. Accepting this as a
correctness fix rather than papering over it with a manual addition, per
architecture.

## Testing

- `pytest .github/scripts/tests` -- 144 passed, 8 xfailed (pre-existing,
unrelated)
- `pre-commit run` on both changed files -- clean
- Local simulation above against the exact pinned TheRock commit
(`df3d451a`), not just current TheRock `main` -- default per-component
walk levels, no `--level` override

## Notes

Editing `therock-multi-arch-ci.yml` matches TheRock's own "run
everything" fallback trigger patterns (and rocm-libraries' own), so this
PR's live CI can't validate the underlying per-path selection logic on
itself -- same caveat as #11785. The table above is the primary
evidence.

## Submission Checklist

- [x] Look over the contributing guidelines at
https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

Co-authored-by: Cursor <cursoragent@cursor.com>
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.

5 participants