Skip to content

feat(ci): add dynamic stage determination for external repos - #7130

Merged
geomin12 merged 6 commits into
mainfrom
users/geomin12/dynamic-determination-stage
Aug 7, 2026
Merged

feat(ci): add dynamic stage determination for external repos#7130
geomin12 merged 6 commits into
mainfrom
users/geomin12/dynamic-determination-stage

Conversation

@geomin12

@geomin12 geomin12 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Enable external repos (rocm-libraries, rocm-systems) to dynamically determine which TheRock stages need rebuilding based on their changes.

Changes:

  • setup_multi_arch.yml: Add EXTERNAL_REPO env var and GITHUB_TOKEN for baseline run API calls. Use THEROCK_REPOSITORY for baseline selection.
  • configure_multi_arch_ci.py: Add GitContext.from_external_repo() to create context with repo name as changed file. Skip path filtering for external repos and apply stage reuse from TheRock baselines.
  • stage_reuse_decision.py: Use THEROCK_REPOSITORY for baseline run selection so external repos can query TheRock baselines.
  • configure_external_repo_ci.py: Update docstring and exclude workflow files from full test triggers.

How it works:

  1. External repo passes EXTERNAL_REPO JSON with repository name
  2. configure_multi_arch_ci.py extracts repo name (e.g., "rocm-libraries")
  3. Stage impact analysis maps repo name to affected stages (e.g., math-libs)
  4. Unaffected stages are candidates for reuse from TheRock baselines
  5. Commit-compatible baseline runs provide prebuilt artifacts

Working here: https://github.com/ROCm/rocm-libraries/actions/runs/31032656859/job/92397575440?pr=10339#step:4:198 (however, no baseline as this is on a feature branch)

Writing job summary:
### Stage reuse analysis

- mode: `reuse-stage`
- full rebuild required: `False`
- baseline run checked: _none_
- unaffected candidates: `comm-libs`, `compiler-runtime`, `dctools-core`, `debug-tools`, `media-libs`, `profiler-apps`, `runtime-tests`, `storage-libs`, `wsl-rocdxg`
- available in baseline: _none_
- applied: _none_
- available per platform:
  - linux: _none_
  - windows: _none_

ISSUE ID: #3343

Enable external repos (rocm-libraries, rocm-systems) to dynamically
determine which TheRock stages need rebuilding based on their changes.

Changes:
- setup_multi_arch.yml: Add EXTERNAL_REPO env var and GITHUB_TOKEN for
  baseline run API calls. Use THEROCK_REPOSITORY for baseline selection.
- configure_multi_arch_ci.py: Add GitContext.from_external_repo() to
  create context with repo name as changed file. Skip path filtering
  for external repos and apply stage reuse from TheRock baselines.
- stage_reuse_decision.py: Use THEROCK_REPOSITORY for baseline run
  selection so external repos can query TheRock baselines.
- configure_external_repo_ci.py: Update docstring and exclude workflow
  files from full test triggers.

How it works:
1. External repo passes EXTERNAL_REPO JSON with repository name
2. configure_multi_arch_ci.py extracts repo name (e.g., "rocm-libraries")
3. Stage impact analysis maps repo name to affected stages (e.g., math-libs)
4. Unaffected stages are candidates for reuse from TheRock baselines
5. Commit-compatible baseline runs provide prebuilt artifacts

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@geomin12
geomin12 force-pushed the users/geomin12/dynamic-determination-stage branch from 719d363 to edf376c Compare August 5, 2026 18:05
@geomin12 geomin12 changed the title feat: add dynamic stage determination for external repos feat(ci): add dynamic stage determination for external repos Aug 5, 2026
@geomin12
geomin12 marked this pull request as ready for review August 5, 2026 18:06
@geomin12
geomin12 requested a review from ScottTodd as a code owner August 5, 2026 18:06
@therock-pr-bot

therock-pr-bot Bot commented Aug 5, 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 Aug 5, 2026

Copy link
Copy Markdown

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

@geomin12
geomin12 requested a review from dysu-amd August 5, 2026 18:07
@ammallya

ammallya commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The PR in general looks good but please add test cases in unit tests

baseline_run_id = auto_stage_reuse.baseline_run_id
# For external repos, use their baseline_repository (ROCm/TheRock)
if ci_inputs.baseline_repository:
baseline_repository = ci_inputs.baseline_repository

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.

This is already done in line 917

- Remove redundant baseline_repository assignment (already set at line 917
  from ci_inputs.baseline_repository, which gets its value from THEROCK_REPOSITORY
  env var set by external repos like rocm-libraries)
- Add unit tests for external repo functionality:
  - CIInputs.external_repo field from EXTERNAL_REPO env var
  - GitContext.from_external_repo() method
  - should_skip_ci() path filter bypass for external repos

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@geomin12
geomin12 requested a review from ammallya August 5, 2026 20:22

@ammallya ammallya 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

@erman-gurses erman-gurses 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.

Please see my comments:

The external repo name maps to a submodule in TheRock.
"""
print(f"External repo detected: {external_repo_name}")
return GitContext(changed_files=[external_repo_name])

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.

Should this also populate submodule_paths?

The docstring says the external repository name maps to a TheRock submodule, but returning only changed_files means has_submodule_changes evaluates to None.

That changes downstream test behavior: _determine_test_type() does not apply the submodule-change test policy, and families with submodule_bump_tests_only have their test runners disabled because they require has_submodule_changes is True.

An external rocm-libraries or rocm-systems change appears semantically equivalent to changing that submodule in TheRock, so should this return:

GitContext(
    changed_files=[external_repo_name],
    submodule_paths=[external_repo_name],
)

Could we also add a test asserting that GitContext.from_external_repo(...).has_submodule_changes is True?

Comment on lines +477 to +481
# THEROCK_REPOSITORY is set by setup_multi_arch.yml to the repository input
# (ROCm/TheRock for external repos, or github.repository for normal runs).
github_repository = os.environ.get(
"THEROCK_REPOSITORY", os.environ.get("GITHUB_REPOSITORY", "ROCm/TheRock")
)

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.

This correctly redirects external-repo baseline queries to TheRock. However, with this new external-repo path, could we fail closed if the branch-history query below fails or returns no commits while STAGE_REUSE_CURRENT_SHA is set?
The current fallback clears effective_commit_sha, allowing baseline selection without verifying compatibility with the pinned TheRock commit. It would be safer to select no baseline and rebuild in that case.

SKIP_PATH_FILTERS: ${{ inputs.external_repo != '' && 'true' || '' }}
# For baseline run selection, use repository input (defaults to GITHUB_REPOSITORY).
# External repos set repository to "ROCm/TheRock" to fetch TheRock baseline runs.
THEROCK_REPOSITORY: ${{ inputs.repository || github.repository }}

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.

The PR description mentions adding GITHUB_TOKEN for the baseline API calls, but I don't see it passed into this step's environment.

Should this include:

GITHUB_TOKEN: ${{ github.token }}

The stage-reuse path now queries branch history and baseline workflow runs, so explicitly passing the workflow token would ensure those requests are authenticated.

# changes should still use stage reuse to determine which stages to rebuild.
# The workflow itself doesn't affect TheRock build stages.
FULL_TEST_TRIGGER_PATTERNS = [
".github/workflows/therock*",

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 understand the intent to let workflow-only changes reuse unaffected build-stage artifacts.

Could you clarify how removing this pattern affects test coverage downstream? FULL_TEST_TRIGGER_PATTERNS controls run_all_tests, so a change only under .github/workflows/therock* now appears to fall through with run_all_tests=False and no matched projects.

Stage reuse and test selection seem independent: we could reuse unchanged build artifacts while still running the full test suite to validate the workflow being modified. Is that still guaranteed elsewhere?

Comment on lines +369 to +377
def test_external_repo_skips_path_filter(self, mock_filter):
"""External repo builds skip path filtering and always run CI."""
inputs = self._inputs(
external_repo='{"repository":"ROCm/rocm-libraries","ref":"abc123"}'
)
git = cm.GitContext(changed_files=["rocm-libraries"])
self.assertFalse(cm.should_skip_ci(inputs, git))
# Path filter should not be called for external repos
mock_filter.assert_not_called()

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.

can you add a test covering the full external-repo stage-reuse path?

The tests verify that EXTERNAL_REPO is parsed and that path filtering is skipped, but they do not check that an external repository name is passed into stage-impact analysis and that unaffected stages are applied as PREBUILT from a TheRock baseline.

1. from_external_repo now populates submodule_paths so that
   has_submodule_changes returns True, enabling submodule_bump_tests_only
   families to run their tests for external repo builds.

2. Fail closed on branch-history query failure for external repos.
   If commit history cannot be fetched when building against a pinned
   TheRock commit, select no baseline and rebuild instead of risking
   incompatible baselines.

3. Add GITHUB_TOKEN to setup_multi_arch.yml for authenticated GitHub
   API calls (branch history, baseline workflow runs).

4. Restore .github/workflows/therock* to FULL_TEST_TRIGGER_PATTERNS.

5. Add tests for from_external_repo and external repo stage reuse path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@geomin12
geomin12 requested a review from erman-gurses August 6, 2026 17:01

@erman-gurses erman-gurses 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.

Some additional comments:

Comment on lines +532 to +536
if is_external_repo:
logger.warning(
"%s empty branch history for external repo; "
"failing closed - no baseline will be selected."
)

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.

Can you add LOG_PREFIX here, as in the warning above?

# External repos set repository to "ROCm/TheRock" to fetch TheRock baseline runs.
THEROCK_REPOSITORY: ${{ inputs.repository || github.repository }}
# Token for GitHub API calls (branch history, baseline workflow runs)
GITHUB_TOKEN: ${{ github.token }}

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.

Thanks for adding GITHUB_TOKEN. Do we also need actions: read in the workflow permissions?

Comment on lines +664 to +690
def test_external_repo_stage_reuse_uses_repo_as_changed_file(self):
"""External repo builds pass repo name to stage-impact analysis.

When an external repo (e.g., rocm-libraries) triggers a build, the repo
name should be treated as a changed file for stage-impact analysis.
Unaffected stages should be marked as PREBUILT from the baseline.
"""
# Create git context as if from external repo
git = cm.GitContext.from_external_repo("rocm-libraries")

# The changed_files should contain the repo name
self.assertEqual(git.changed_files, ["rocm-libraries"])
self.assertEqual(git.submodule_paths, ["rocm-libraries"])
self.assertTrue(git.has_submodule_changes)

# When decide_jobs processes this, it should pass the repo name
# to stage-impact analysis (via auto_stage_reuse)
result = cm.decide_jobs(
self._inputs(
external_repo='{"repository":"ROCm/rocm-libraries","ref":"abc123"}'
),
git_context=git,
targets=cm.TargetSelection(),
)

# auto_stage_reuse should have processed the external repo context
self.assertIsNotNone(result.auto_stage_reuse)

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.

This test does not seem to exercise stage-impact analysis or baseline reuse because TargetSelection() has no Linux or Windows families. compute_auto_stage_reuse() therefore exits early with no build platforms selected.

TargetSelection() only constructs an object with empty family lists; actual target selection happens through select_targets(ci_inputs).

@dataclass(frozen=True)
class TargetSelection:
"""Which GPU families to build/test, per platform."""
linux_families: list[str] = field(default_factory=list)
windows_families: list[str] = field(default_factory=list)

Actual target selection happens here:

targets = select_targets(ci_inputs)

As a result, assertIsNotNone(result.auto_stage_reuse) only verifies that a result object was created. It does not verify the behavior described by the test name and docstring.

Could we either:

  • make this a plumbing test by mocking compute_auto_stage_reuse() and asserting that changed_files=["rocm-libraries"] is passed, an unaffected stage becomes PREBUILT, the baseline run ID is propagated, and baseline_repository remains ROCm/TheRock; or
  • provide non-empty target families and a fake baseline so the actual stage-impact/reuse path is exercised?

Comment on lines 516 to +537
@@ -511,6 +529,12 @@ def _default_baseline_selector(*, platform: str) -> BaselineSelector:
)
ordered_commit_shas = None
if not ordered_commit_shas:
if is_external_repo:
logger.warning(
"%s empty branch history for external repo; "
"failing closed - no baseline will be selected."
)
return lambda required_artifacts: None

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.

Non blocker: we could add unit tests for both external-repo fail-closed cases.

  • gha_query_recent_branch_commits() raises GitHubAPIError.
  • It returns an empty list.

Comment on lines +324 to +325
2. has_submodule_changes returns True, enabling submodule_bump_tests_only
families to run their tests

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.

This doesn't need to be part of this PR, but let's consider removing or reworking submodule_bump_tests_only to simplify here. I want the CI configuration to be as explicit as possible so it is easier to understand and edit.

The behavior that submodule_bump_tests_only provides is:

  • push events include "postsubmit" amdgpu families in addition to "presubmit" amdgpu familes
  • pull_request events can opt-in to any amdgpu families they want via labels (docs here).
    • _If a PR that does not modify submodules opts in via the ci:run-all-archs or a gfx**** label, the build will be included but not the tests
    • Our automated submodule update PRs apply the ci:run-all-archs label

Here are a few proposals that I think would simplify:

  1. Remove the "postsubmit" category entirely and rely on just "presubmit" (with opt-ins, notably all automated submodule update PRs) and "nightly". If someone adds an opt-in label to a PR that doesn't update submodules, run the tests as requested.
  2. Turn submodule_bump_tests_only into pull_request_tests_only to skip running tests on push but still run them on pull_request (any opt-in, submodule update or otherwise)

We should also consider moving gfx950 from postsubmit/nightly to presubmit (at least in rocm-systems, later rocm-libraries and TheRock) given its increasing importance. If we don't have enough runner capacity for tests, we could also split out "build targets" from "test targets" to give a more directly configurable way to say "this PR shouldn't just build gfx950, it should also run gfx950 tests". I don't want to hide "run tests" behind requirements like "only if the PR touches these files".

Here's the code:

  • # If submodule_bump_tests_only is set, only run tests when submodule changes
    # are detected or on workflow_dispatch (manual triggers).
    if (
    platform_info.get("submodule_bump_tests_only", False)
    and not ci_inputs.is_workflow_dispatch
    and git_context.has_submodule_changes is not True
    ):
    test_runs_on = ""
    print(
    f" {family_name}: submodule_bump_tests_only flag set, "
    f"disabling tests (no submodule changes detected)"
    )
  • # The 'postsubmit' matrix runs on 'push' triggers (for every commit to the default branch).
    amdgpu_family_info_matrix_postsubmit = {
    "gfx90a": {
    "linux": {
    "test-runs-on": "linux-gfx90a-1gpu-ossci-rocm",
    "family": "gfx90a",
    "fetch-gfx-targets": ["gfx90a"],
    "build_variants": ["release"],
    # Only run tests on submodule bumps (builds always run)
    "submodule_bump_tests_only": True,
    },
    "windows": {
    "test-runs-on": "",
    "family": "gfx90a",
    "fetch-gfx-targets": [],
    "build_variants": ["release"],
    },
    },
    "gfx950": {
    "linux": {
    "test-runs-on": "linux-gfx950-1gpu-ccs-ossci-rocm",
    "test-runs-on-multi-gpu": "linux-gfx950-8gpu-ccs-ossci-rocm",
    "family": "gfx950-dcgpu",
    "fetch-gfx-targets": ["gfx950"],
    "build_variants": ["release", "asan", "tsan"],
    # Only run tests on submodule bumps (builds always run)
    "submodule_bump_tests_only": True,
    }
    },
    }

@geomin12 geomin12 Aug 6, 2026

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.

ah good point! seems we just need to advance our labelling system and make it known.

As I was making updates for gfx90a, it was pretty messy with all the "conditionals" we are doing for postsubmit due to limited capacity. 100% agree! I'll do this in a follow up PR as this will also impact gfx90a

plus as this becomes the norm, we can educate ROCm devs about this and utilize (if they know it impacts gfx950, enable it!)

Comment on lines +621 to +625
# External repo builds skip path filtering - they always run CI and use
# stage reuse to determine which stages to rebuild.
if ci_inputs.external_repo:
print(" External repo build: skipping path filter checks, using stage reuse")
return False

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.

This doesn't seem flexible enough, at least on the surface. If a PR to rocm-libraries or rocm-systems only modifies non-code files like *.md documents or code files that are known to not affect CI (e.g. github actions workflow files unrelated to TheRock CI, .gitignore files, CMake files for experimental projects not yet integrated into the build system, etc.), the CI should still be able to short-circuit and skip. We shouldn't need to run expensive logic that decides "no builds affected, copy prebuilt files" and then "no tests affected, skip tests" IMO.

Can we reuse the skip path filters that currently exist in those repositories like https://github.com/ROCm/rocm-libraries/blob/7df08c3d5d7ca4087a21810122f19d65fc102975/.github/scripts/therock_configure_ci.py#L28-L70 ?

We could start with this as you have it, but we get frequent requests for ways to skip CI where it isn't applicable, so I don't want to regress there. For example,

@geomin12 geomin12 Aug 6, 2026

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.

yes! do not worry, I am aware of this. I'm trying to make incremental PRs all in parallel to handle scoped items so it isn't massive! this is at least letting CI know that rules that apply to TheRock do not apply to external repo (such as CMakeLists.txt edit, don't run everything!)

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.

Could add a TODO here or link to an issue with the plan so we don't lose track and if someone later finds the code they know what the ideal/planned state is.

Comment on lines +1427 to +1437
if ci_inputs.external_repo:
# External repo: use repo name for stage reuse analysis.
# external_repo is JSON like {"repository":"ROCm/rocm-libraries","ref":"..."}
try:
external_repo = json.loads(ci_inputs.external_repo)
repo_full_name = external_repo.get("repository", "")
external_repo_name = (
repo_full_name.split("/")[-1] if "/" in repo_full_name else ""
)
except (json.JSONDecodeError, TypeError):
external_repo_name = ""

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.

JSONDecodeError seems serious here. Might want to raise an exception rather than fallback to empty string, or at least log an error.

  • If the CI system is misconfigured, we should get an obvious error that we can fix
  • If github sometimes produces some output we don't expect, it is likely to also produce other output we don't expect elsewhere, so just continuing with fallback behavior seems risky

1. Add LOG_PREFIX to warning message for consistency
   (stage_reuse_decision.py)

2. Add actions:read permission for GitHub API calls to fetch
   workflow runs for baseline selection (setup_multi_arch.yml)

3. Raise ValueError on invalid EXTERNAL_REPO JSON instead of
   silently falling back - makes CI misconfigurations obvious
   (configure_multi_arch_ci.py)

4. Improve test to properly exercise stage-impact analysis by
   mocking compute_auto_stage_reuse and verifying correct
   arguments are passed (configure_multi_arch_ci_test.py)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +621 to +625
# External repo builds skip path filtering - they always run CI and use
# stage reuse to determine which stages to rebuild.
if ci_inputs.external_repo:
print(" External repo build: skipping path filter checks, using stage reuse")
return False

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.

Could add a TODO here or link to an issue with the plan so we don't lose track and if someone later finds the code they know what the ideal/planned state is.

TODO(#3343): Reuse skip path filters from external repos to
short-circuit CI for docs-only changes, experimental projects, etc.

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

@dysu-amd dysu-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.

TBH I'm not familiar enough with the content to give a meaningful review, but LGTM

@geomin12
geomin12 merged commit 93d6c2a into main Aug 7, 2026
9 checks passed
@geomin12
geomin12 deleted the users/geomin12/dynamic-determination-stage branch August 7, 2026 15:07
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Aug 7, 2026
@geomin12
geomin12 restored the users/geomin12/dynamic-determination-stage branch August 7, 2026 15:17
geomin12 added a commit that referenced this pull request Aug 7, 2026
…#7192)

Introduce break in workflow with
#7130

Tried to revert but got error: 
```
Revert failed: Repository rule violations found
Cannot create ref due to creations being restricted.
```

Creating manual revert

This reverts commit 93d6c2a.

## Motivation

<!-- Explain the purpose of this PR and the goals it aims to achieve.
-->

<!-- Most pull requests should be associated with at least one GitHub
issue -->

<!-- GitHub issue: #1234 -->

## Technical Details

<!-- Explain the changes along with any relevant GitHub links. -->

## Test Plan

<!-- Explain any relevant testing done to verify this PR. -->

## Test Result

<!-- Briefly summarize test outcomes. -->

## Submission Checklist

- [ ] Look over the contributing guidelines at
https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
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.

5 participants