Skip to content

feat(ci): consolidate GPU family matrix with trigger-based config - #8322

Merged
geomin12 merged 5 commits into
mainfrom
users/geomin12/re-work-amdgpu-family-matrix.py
Sep 29, 2026
Merged

geomin12 merged 5 commits into
mainfrom
users/geomin12/re-work-amdgpu-family-matrix.py

Conversation

@geomin12

@geomin12 geomin12 commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Replace three separate dictionaries (presubmit, postsubmit, nightly) with a single unified amdgpu_family_info_matrix. Each family now explicitly specifies builds_on_trigger and tests_on_trigger lists to control when builds and tests run.

Going forward, no more need for one-off flags and this creates the ability to be specific on where builds/tests are (and easy understanding, no more tribal knowledge!)

This addresses closes #8222 by making trigger logic explicit and composable. For example, gfx950 can now build on postsubmit but only test on submodule_bump - a scenario that was impossible to express with the previous flag-based system.

Key changes:

  • Add builds_on_trigger and tests_on_trigger fields to all family entries
  • Add _get_current_triggers() and _should_run_tests_for_family() helpers
  • Remove deprecated flags: nightly_check_only_for_family, submodule_bump_tests_only, trigger_test_label_only
  • workflow_dispatch (on_demand) implicitly allows all families - no need to list it explicitly in trigger fields
  • Empty trigger list in get_all_families_for_trigger_types() returns all families (used for workflow_dispatch/schedule)
  • Update tests for new trigger-based behavior

Next PR: adding label support for more granular build / test triggers for architectures (as of now, labels run both build + test)

ISSUE ID: #8222

Replace three separate dictionaries (presubmit, postsubmit, nightly) with
a single unified amdgpu_family_info_matrix. Each family now explicitly
specifies builds_on_trigger and tests_on_trigger lists to control when
builds and tests run.

This addresses issue #8222 by making trigger logic explicit and composable.
For example, gfx950 can now build on postsubmit but only test on
submodule_bump - a scenario that was impossible to express with the
previous flag-based system.

Key changes:
- Add builds_on_trigger and tests_on_trigger fields to all family entries
- Add _get_current_triggers() and _should_run_tests_for_family() helpers
- Remove deprecated flags: nightly_check_only_for_family,
  submodule_bump_tests_only, trigger_test_label_only
- workflow_dispatch (on_demand) implicitly allows all families - no need
  to list it explicitly in trigger fields
- Empty trigger list in get_all_families_for_trigger_types() returns all
  families (used for workflow_dispatch/schedule)
- Update tests for new trigger-based behavior

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@therock-pr-bot

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

Copy link
Copy Markdown

Pre-commit check failed

⛔ pre-commit failed

Please run locally:

  • python -m pip install pre-commit
  • pre-commit install
  • pre-commit run --all-files --show-diff-on-failure

This repo uses .pre-commit-config.yaml.

@ScottTodd

Copy link
Copy Markdown
Member

We might also want to delete https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/new_amdgpu_family_matrix.py at this point (such is the cost of maintaining a forked version of a file without completing the migration 😢)

Here's how that file tried to solve for this:

amdgpu_family_predefined_groups = {
# The 'presubmit' matrix runs on 'pull_request' triggers (on all PRs).
"amdgpu_presubmit": [
"gfx94X-dcgpu",
"gfx110X-all",
"gfx1151",
"gfx120X-all",
"gfx125X-dcgpu",
],
# The 'postsubmit' matrix runs on 'push' triggers (for every commit to the default branch).
"amdgpu_postsubmit": ["gfx950-dcgpu"],
# The 'nightly' matrix runs on 'schedule' triggers.
"amdgpu_nightly": [
"gfx90X-dcgpu",
"gfx90c",
"gfx101X-dgpu",
"gfx103X-all",
"gfx1150",
"gfx1152",
"gfx1153",
],
}

Merge TestFamilyTestFilters and TestTriggerBasedBuildTestMatrix into a
single TestTriggerBasedTestFiltering class using table-driven tests.
Also consolidate TestTriggerHelpers tests.

- Reduces test code by ~500 lines while maintaining same coverage
- Uses subtests for better test organization
- Combines redundant mock family definitions

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@geomin12
geomin12 force-pushed the users/geomin12/re-work-amdgpu-family-matrix.py branch from 2c01b73 to 24d3f21 Compare September 17, 2026 23:13
@therock-pr-bot

Copy link
Copy Markdown

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

This experimental file is no longer needed now that the main
amdgpu_family_matrix.py has been refactored with trigger-based
build/test configuration.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@HereThereBeDragons

Copy link
Copy Markdown
Contributor

We might also want to delete https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/new_amdgpu_family_matrix.py at this point (such is the cost of maintaining a forked version of a file without completing the migration 😢)

yeah.. sadly. stuff like this needs commitment. and it is good that at least somehow it is moving forward.

still i think it would be good to have a discussion if we should have it like the pr does it or having the list-based approach like in the now deleted new_amdgpu_family_matrix.py.

if this is something that changes a lot maybe the list-based approach is the better thing to go for. we could have a function that takes those lists and enriches amdgpu_family_info_matrix so that for the consumer still only works on amdgpu_family_info_matrix without caring how it got populated.

any thoughts @geomin12 @ScottTodd ?

@geomin12

Copy link
Copy Markdown
Contributor Author

We might also want to delete https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/new_amdgpu_family_matrix.py at this point (such is the cost of maintaining a forked version of a file without completing the migration 😢)

yeah.. sadly. stuff like this needs commitment. and it is good that at least somehow it is moving forward.

still i think it would be good to have a discussion if we should have it like the pr does it or having the list-based approach like in the now deleted new_amdgpu_family_matrix.py.

if this is something that changes a lot maybe the list-based approach is the better thing to go for. we could have a function that takes those lists and enriches amdgpu_family_info_matrix so that for the consumer still only works on amdgpu_family_info_matrix without caring how it got populated.

any thoughts @geomin12 @ScottTodd ?

yes happy to update! however, i don't know if this will update too often (at least new entries), but Scott will have more insight there

Resolve merge conflicts:
- amdgpu_family_matrix.py: Integrate explicit_only families (gfx1250-strict)
  into the new trigger-based system with builds_on_trigger/tests_on_trigger
- configure_multi_arch_ci.py: Keep trigger-based test gating while adding
  support for explicit_only families in workflow_dispatch
- configure_multi_arch_ci_test.py: Keep trigger-based tests, remove tests
  for deprecated flags (submodule_bump_tests_only, trigger_test_label_only)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ScottTodd

Copy link
Copy Markdown
Member

Not sure when I'll get to reviewing this. Need to set aside enough time to think through the details.

geomin12 added a commit that referenced this pull request Sep 23, 2026
…_labels.py` script to keep it uniform (#8435)

Progress on #7375 with much more updates incoming as we expand our
labelling system

We are beginning the label enhancement work incrementally, with some
work starting when #8322 lands.

In this PR, we are handling:
- adding prefix ci: to gfx targets
- CI labels in multi-arch are available to external repos and TheRock.
must be consistent

We are adding a script to create labels and ensure uniformity across all
repos that use multi-arch CI. we are also adding `ci:` on the gfx
targets and using the script to ensure uniformity. The script has **no**
destructive properties, only creating or updating

Local dry run:
```
$ python3 configure_ci_labels.py --repo ROCm/TheRock --create --force --dry-run

Creating CI labels in ROCm/TheRock...
(--force: existing labels will be updated to match)
(DRY RUN - no changes will be made)

  [DRY RUN] Would create label: ci:gfx103X-linux
  [DRY RUN] Would create label: ci:gfx103X
  [DRY RUN] Would create label: ci:gfx90X-dcgpu
  [DRY RUN] Would create label: ci:gfx94X-dcgpu
  [DRY RUN] Would create label: ci:gfx950-dcgpu
  [DRY RUN] Would create label: ci:gfx110X-dgpu
  [DRY RUN] Would create label: ci:gfx110X-all
  [DRY RUN] Would create label: ci:gfx1103
  [DRY RUN] Would create label: ci:gfx1150
  [DRY RUN] Would create label: ci:gfx1151
  [DRY RUN] Would create label: ci:gfx1152
  [DRY RUN] Would create label: ci:gfx1153
  [DRY RUN] Would create label: ci:gfx120X-all
  [DRY RUN] Would create label: ci:gfx125x
  [DRY RUN] Would create label: ci:gfx125X-dcgpu
  [DRY RUN] Would create label: ci:gfx900
  [DRY RUN] Would create label: ci:gfx906
  [DRY RUN] Would create label: ci:gfx908
  [DRY RUN] Would create label: ci:gfx90a
  [DRY RUN] Would create label: ci:gfx90c
  [DRY RUN] Would update label: test:ocltst (color: #03B8ED -> #3FA7D6)
  [DRY RUN] Would update label: test:hip-tests (color: #20566E -> #3FA7D6)
  [DRY RUN] Would update label: test:hipkernelprovider (color: #20566E -> #3FA7D6)
  [DRY RUN] Would update label: test:amdsmi (color: #277804 -> #3FA7D6, desc: "" -> "For pull requests, runs full t...")
  [DRY RUN] Would update label: test:rocgdb-cpu (color: #20566E -> #3FA7D6)
  [DRY RUN] Would update label: test:rocgdb-gpu (color: #20566E -> #3FA7D6)
  [DRY RUN] Would update label: test:rocgdb (color: #20566E -> #3FA7D6)
  [DRY RUN] Would update label: test:rocprofiler-sdk-spm (desc: "To run rocprofier-sdk-spm jobs" -> "To run rocprofiler-sdk-spm job...")
  [DRY RUN] Would update label: test:rpp (desc: "" -> "For pull requests, runs full t...")

Summary: 20 created, 9 updated, 40 already matched
```

Local actual run:
```
$ python3 configure_ci_labels.py --repo ROCm/TheRock --create --force

Creating CI labels in ROCm/TheRock...
(--force: existing labels will be updated to match)
  Created label: ci:gfx103X-linux
  Created label: ci:gfx103X
  Created label: ci:gfx90X-dcgpu
  Created label: ci:gfx94X-dcgpu
  Created label: ci:gfx950-dcgpu
  Created label: ci:gfx110X-dgpu
  Created label: ci:gfx110X-all
  Created label: ci:gfx1103
  Created label: ci:gfx1150
  Created label: ci:gfx1151
  Created label: ci:gfx1152
  Created label: ci:gfx1153
  Created label: ci:gfx120X-all
  Created label: ci:gfx125x
  Created label: ci:gfx125X-dcgpu
  Created label: ci:gfx900
  Created label: ci:gfx906
  Created label: ci:gfx908
  Created label: ci:gfx90a
  Created label: ci:gfx90c
  Updated label: test:ocltst (color: #03B8ED -> #3FA7D6)
  Updated label: test:hip-tests (color: #20566E -> #3FA7D6)
  Updated label: test:hipkernelprovider (color: #20566E -> #3FA7D6)
  Updated label: test:amdsmi (color: #277804 -> #3FA7D6, desc: "" -> "For pull requests, runs full t...")
  Updated label: test:rocgdb-cpu (color: #20566E -> #3FA7D6)
  Updated label: test:rocgdb-gpu (color: #20566E -> #3FA7D6)
  Updated label: test:rocgdb (color: #20566E -> #3FA7D6)
  Updated label: test:rocprofiler-sdk-spm (desc: "To run rocprofier-sdk-spm jobs" -> "To run rocprofiler-sdk-spm job...")
  Updated label: test:rpp (desc: "" -> "For pull requests, runs full t...")

Summary: 20 created, 9 updated, 40 already matched
```

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

@ScottTodd ScottTodd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like a step in the right direction. I want to further break apart "submodule update PR" in the future though, so

  1. Any workflow run (in TheRock, or other repositories) can run any configuration through easy to understand configuration settings
  2. Workflow runs by default choose configurations that accurately reflect what the changes in the associated commits can affect

#7782 is a big part of that

Comment on lines +1279 to +1281
# Context-based triggers (can stack on top of event triggers)
if git_context.has_submodule_changes is True:
triggers.add("submodule_bump")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would hope that just allowing the labels to pick any registered label / gpu family and having the automation add those labels should be enough. Do we still need this "submodule_bump context-based trigger" too?

  1. Plenty of changes to submodules do not need to run CI jobs, or can run only a subset.
    For example:
  2. We already set ci:run-all-archs on PRs created with https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/bump_automation.py along with other explicit gfx opt-ins. Anyone should be able to add e.g. the ci:gfx125X-dcgpu label to a pull request and have the CI jobs opt in to the same workflow behavior that "submodule bump" PRs (modifying the git submodules) get.

I guess this does let us differentiate between "build the family/target" and "test the family/target" which we can't currently do with just the current labels, and separating out to ci:build-gfx#### and ci:test-gfx#### labels would create a lot of extra labels.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point on these! this PR is just the first step towards breaking away from "submodule specific". i wanted to get the main architecture cleared through, then start making improvements from there (as this is already pretty massive)

Resolved conflicts:
- amdgpu_family_matrix.py: Keep trigger-based logic, add asan-debug variant
  and trigger_test_label_only for gfx90a from main
- configure_multi_arch_ci_test.py: Keep MOCK_FAMILIES from branch
- new_amdgpu_family_matrix.py: Removed (superseded by unified amdgpu_family_matrix.py)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@geomin12
geomin12 force-pushed the users/geomin12/re-work-amdgpu-family-matrix.py branch from fc474ff to 650274b Compare September 29, 2026 15:55
@geomin12
geomin12 merged commit 0383100 into main Sep 29, 2026
187 of 189 checks passed
@geomin12
geomin12 deleted the users/geomin12/re-work-amdgpu-family-matrix.py branch September 29, 2026 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[issue] enhancing our build and test selection strategy based on architecture

3 participants