Repository navigation
feat(ci): Add consolidated external repo CI configuration script - #6660
Conversation
Consolidates duplicated CI configuration logic from rocm-systems and
rocm-libraries into a single Python script. Handles event detection,
merge-base computation, changed project matching, and skip/run-all logic.
External repos can call this script instead of maintaining separate
copies of get_changed_projects.py and its dependencies.
Usage:
python configure_external_repo_ci.py \
--event-name pull_request \
--base-sha ${{ github.event.pull_request.base.sha }} \
--head-sha ${{ github.event.pull_request.head.sha }}
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add required --github-repo argument so the script knows which repository it's running in. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch from git diff/merge-base to gh api compare endpoint. This removes the need for full repository checkout - only the repos-config.json file needs to be checked out now. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
therock-pr-bot
Bot
added
the
Not ready to Review
|
🎉 All checks passed! This PR is ready for review. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tony-davis
left a comment
tony-davis
left a comment
There was a problem hiding this comment.
Nice, Geo; consolidating the diverged get_changed_projects.py copies into one script is a real maintenance win, and moving to the gh api compare endpoint so you don't need a full checkout is a nice touch. A few things before this is ready.
The one that's already blocking you: the therock-pr-bot Unit Test check is red and it's added the "Not ready to Review" label. This script is very testable (the helpers are pure, and configure() just needs get_modified_paths_api mocked), so a configure_external_repo_ci_test.py covering the event branches, the skippable/CI-trigger logic, and subtree matching would both unblock the bot and lock the behavior. This is the same gap I flagged on #6709's compute_affected_stages.py, so it might be worth a small shared test pattern for both.
Two correctness things I'd like to sort out: the push path builds {head_sha}^ as the compare base, and the REST compare API doesn't understand git ^ ancestry (it wants a SHA or ref), so that call will 404 and, since the retry only catches timeouts, crash the step. And the compare endpoint caps files at 300, so a big monorepo PR would silently truncate the changed list and under-test. I left notes on both.
Nothing here is a showstopper for the approach; it's a solid direction. Details below.
One bigger-picture thought: since this and #6709 are a producer/consumer pair for the same feature, I think it'd be worth landing them together (with tests on both) rather than merging one ahead of the other.
| f.write(f"{k}={v}\n") | ||
|
|
||
|
|
||
| def configure( |
There was a problem hiding this comment.
This is the core decision logic and there's no test for it (which is what has therock-pr-bot blocking the PR). A small configure_external_repo_ci_test.py would do it: schedule/workflow_dispatch -> run-all; only *.md changed -> skip; a .github/workflows/therock* change -> run-all; and a projects/rocblas/... change -> changed_projects=projects/rocblas, with get_modified_paths_api monkeypatched so no network is needed. The pure helpers (is_skippable, find_matched_subtrees, matches_patterns) are trivial to cover too.
| elif event_name == "push" and head_sha: | ||
| # For push, compare HEAD^ to HEAD using the API | ||
| logger.info(f"Getting push diff via API: {head_sha}^...{head_sha}") | ||
| modified_paths = get_modified_paths_api(github_repo, f"{head_sha}^", head_sha) |
There was a problem hiding this comment.
The compare API takes a SHA or ref for the base, not git ^ ancestry, so {head_sha}^...{head_sha} will fail (and because retry only catches TimeoutExpired, the resulting CalledProcessError will crash the step rather than degrade gracefully). For push, the caller has the previous SHA in github.event.before; passing that in as --base-sha and comparing directly is the clean fix. Suggestion assumes that contract (and falls through to run-all if base_sha is missing).
| elif event_name == "push" and head_sha: | |
| # For push, compare HEAD^ to HEAD using the API | |
| logger.info(f"Getting push diff via API: {head_sha}^...{head_sha}") | |
| modified_paths = get_modified_paths_api(github_repo, f"{head_sha}^", head_sha) | |
| elif event_name == "push" and base_sha and head_sha: | |
| # For push, the caller passes the "before" SHA as base_sha; the compare | |
| # API does not understand git "^" ancestry syntax. | |
| logger.info(f"Getting push diff via API: {base_sha}...{head_sha}") | |
| modified_paths = get_modified_paths_api(github_repo, base_sha, head_sha) |
| result = subprocess.run( | ||
| [ | ||
| "gh", | ||
| "api", | ||
| f"repos/{github_repo}/compare/{base_sha}...{head_sha}", | ||
| "--jq", | ||
| ".files[].filename", | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| timeout=60, | ||
| ) | ||
| return set(result.stdout.splitlines()) |
There was a problem hiding this comment.
The compare endpoint returns at most 300 files (it paginates commits, not files), so a large PR in a monorepo would silently give a truncated list and we'd under-build/under-test; exactly the case where you most want full coverage. Worth guarding: if the returned file count hits the cap (or the response's truncation flag is set), fall back to run_all_tests. Even a simple "if len(files) >= 300: treat as run-all" is safer than quietly missing projects.
| try: | ||
| with open(config_path, "r", encoding="utf-8") as f: | ||
| data = json.load(f) | ||
| return [RepoEntry(**entry) for entry in data.get("repositories", [])] |
There was a problem hiding this comment.
repos-config.json is a shared, evolving file. The moment someone adds a new key to a repo entry, RepoEntry(**entry) raises TypeError, which the broad except Exception below swallows into an empty config, and then configure() silently forces a full build for all external CI. I'd filter to known fields so unknown keys are ignored, e.g. build a {f.name for f in dataclasses.fields(RepoEntry)} set and pass only those (needs adding fields to the dataclasses import). Happy to turn this into a two-part suggestion if you want it.
| return decorator # type: ignore[return-value] | ||
|
|
||
|
|
||
| @retry(max_attempts=3, delay_seconds=2, exceptions=(subprocess.TimeoutExpired,)) |
There was a problem hiding this comment.
Heads-up that the retry only covers TimeoutExpired, so a transient gh api failure (5xx / rate limit) raises CalledProcessError and isn't retried. If you want those retried, add it to the exceptions tuple, though note that'd also retry non-transient 4xx, so you may want to narrow it to specific exit conditions instead. Minor; take it or leave it.
- Add unit tests for all major code paths (configure_external_repo_ci_test.py) - Fix push event to use base_sha directly (GitHub API doesn't support ^ syntax) - Handle truncated API response (>300 files) by falling back to run_all_tests - Filter unknown fields in RepoEntry to prevent TypeError on config changes - Add CalledProcessError to retry decorator for transient API failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
therock-pr-bot
Bot
removed
the
Not ready to Review
tony-davis
left a comment
tony-davis
left a comment
There was a problem hiding this comment.
Thanks, Geo; this addresses everything cleanly. The new configure_external_repo_ci_test.py covers the event branches and the fallbacks with real assertions, the push path now uses base_sha instead of the ^ trick, the 300-file truncation falls back to run-all, and RepoEntry ignores unknown config keys now. Bot's green and the label's cleared. Nice work.
One non-blocking thought for later: the compare API's 300 cap is a "probably truncated" heuristic, so if you ever see a huge PR that should've been selective quietly go full-build, that's the spot to look. Not a merge blocker.
Since this and #6709 are a producer/consumer pair, I'd still land them together. Approving.
#9665) After ROCm/TheRock#6660 landed, we are now using the external configure ci script to maintain in one central location. We are incorporating this script in multi-arch CI! Working here: https://github.com/ROCm/rocm-libraries/actions/runs/29869577087/job/88766169112 ISSUE ID: ROCm/TheRock#3343 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
#8987) After ROCm/TheRock#6660 landed, we are now using the external configure ci script to maintain in one central location. We are incorporating this script in multi-arch CI! Working here: https://github.com/ROCm/rocm-systems/actions/runs/29869549929/job/88766078180 ISSUE ID: ROCm/TheRock#3343 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Consolidates duplicated CI configuration logic from rocm-systems and rocm-libraries into a single Python script. Handles event detection, merge-base computation, changed project matching, and skip/run-all logic.
Instead of the diverged python scripts in superrepos, we will now rely on this config script to handle for multi arch external CI
External repos can call this script instead of maintaining separate copies of get_changed_projects.py and its dependencies.
Working here:
ISSUE ID: #3343