Repository navigation
Conversation
The nightly passed `changed_projects: ''`, TheRock's "test everything" signal, so every scheduled run spent GPU runner time on ROCm system components (hip-tests, rocrtst, rocgdb, rocprofiler-*, amdsmi, rccl, ...) that no change in this repository can affect. Derive the test scope from .github/repos-config.json so the nightly resolves tests through TheRock's consumer graph, the same path presubmit already takes. Reading the config rather than hardcoding a list keeps the scope correct as subtrees are added or removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
Bot
added
github actions
project: none
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
| .github/repos-config.json | ||
| sparse-checkout-cone-mode: false | ||
|
|
||
| - name: Collect rocm-libraries subtrees |
There was a problem hiding this comment.
can we move this please into a proper script? that is horrible to read and debug here
The inline heredoc-Python and shell step-summary block in the multi-arch nightly workflow were hard to read and impossible to run locally. Move the logic into .github/scripts/therock_nightly_test_scope.py, reusing the existing load_repo_config/get_valid_prefixes helpers instead of re-parsing repos-config.json ad hoc, so there is one definition of a `category/name` subtree. Output is identical to the inline version. An empty subtree list now fails the job, since TheRock reads an empty changed_projects as "test everything" -- the opposite of this job's purpose. The configure job checks out .github and sets up Python to match therock-ci-nightly.yml.
Record why the script exists and what has to land in TheRock before it, its test, and the configure job that consumes it can be deleted.
Trim the module docstring to the temporary-workaround rationale and cut the tests to the three cases that carry weight: the category/name mapping, the empty-config guard, and a smoke check against the real repos-config.json. Use the runner's preinstalled python3 instead of actions/setup-python, which drops a step but needs --break-system-packages under PEP 668, as noted in .github/scripts/DEVELOPMENT.md. Move the job to ubuntu-26.04.
Pinning the interpreter via actions/setup-python installs into a Python the action owns, so pip needs no --break-system-packages escape hatch and the job is unaffected by whatever the runner image ships.
| A scheduled run has no diff to narrow testing against, so TheRock would | ||
| otherwise test every component it builds, including ROCm system components | ||
| (hip-tests, rocrtst, rocgdb, rocprofiler-*, amdsmi, rccl, ...) that no change | ||
| in this repository can affect. This script emits every subtree registered in | ||
| .github/repos-config.json in `category/name` form, which the workflow passes to | ||
| TheRock as `changed_projects` so testing stays scoped to the code this | ||
| repository owns (plus the consumers TheRock's dependency graph pulls in). |
There was a problem hiding this comment.
See this issue please:
We can build something into the scheduled CI that limits which builds/tests to run to those affected by specific submodules only. Consider that there are 11 submodules in https://github.com/ROCm/TheRock/blob/main/.gitmodules and each may want to run some tests (filtered following https://github.com/ROCm/TheRock/blob/main/docs/rfcs/RFC0013-Consumer-Based-Test-Selection.md). As implemented here, each of up to those 11 repositories may want to fork this 200 lines of code. We should teach the common code how to handle this. Each repository shouldn't need to maintain its own list of repos-config.json files for the CI system to understand how to test changes in it.
There was a problem hiding this comment.
good point! that's a better approach than this (hacky for sure)
|
Hi all! I'm picking this up from Geo (thanks Geo!). I think @ScottTodd is right that this belongs in TheRock and not in each repo. Here's the plan. I'd love a sanity check before I go too far.
I checked parity locally. TheRock's own view of rocm-libraries selects the same 38 test jobs as this PR's script, and no rocm-systems jobs. The thing is, step 2 lands in the same spot as ROCm/TheRock#8827 and Geo's follow-up to ROCm/TheRock#9062. @geomin12, would you rather I fold it into your part 3, or go first and you rebase? Either is fine with me. One behavior note: this keeps direct consumers, so a rocm-systems component that consumes a library can still run. I think that's what we want, since it's a break rocm-libraries could actually cause. It's the main difference from the I'll leave this PR open until the TheRock side is in. cc: @ |
|
approach is rather hacky. starting a new worktree |
|
Regarding the design points, one idea I've had is for the to contain a general top down view of the full virtual monorepo, e.g. list of files changed like then we'd associate each file with some nodes in the build graph, e.g.
Note how the mapping is more difficult for the superproject code in TheRock but we have a pretty good idea from just the build system code itself what each path in rocm-libraries/rocm-systems affects. Where a full file list is difficult to generate (API limitations, large diffs), we could at least hint to the CI system that a particular submodule is what changed and ensure that only the relevant builds/tests get scheduled. I'm not opposed to incremental approaches to better scoping of builds/tests, but I want us to keep in mind:
|
This is a temporary fix to trigger rocm-libraries tests only for
scheduleruns.Currently, this logic ONLY applies to PRs + pushes, however, after ROCm/TheRock#9062 lands, we will make a permanent fix in TheRock in order for rocm-libraries to not run unnecessary rocm-systems tests!
test ran here: https://github.com/ROCm/rocm-libraries/actions/runs/40338314942
local output:
Closes ROCm/TheRock#9401