Repository navigation
Conversation
Add `projects` workflow input to enable building only specific projects within a stage, with intelligent stage-specific routing. Key changes: 1. Stage-specific project filtering: When both --stage and --projects are specified, configure_stage.py filters the projects to only those produced by that stage. This means: - `--stage compiler-runtime --projects hip rccl` -> enables only HIP - `--stage comm-libs --projects hip rccl` -> enables only RCCL - `--stage math-libs --projects hip rccl` -> falls back to stage defaults 2. New filter_projects_for_stage() method in build_topology.py 3. Workflow changes: Added `projects` input to reusable workflows that gets passed to configure_stage.py This enables external repos (like rocm-systems) to trigger selective builds where each stage only builds the artifacts it's responsible for, avoiding unnecessary feature enablement across stages. Depends on: feat(configure_stage): Add --projects flag (#6451) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For selective builds (e.g., external repos like rocm-systems or rocm-libraries), compute which build stages need to run based on which projects have changes. - Add compute_affected_stages.py to determine stages from projects - Add affected_stages input/output to setup_multi_arch.yml - Add affected_stages condition to each stage in build workflow - Stages not in affected_stages will be skipped (unless 'all') This allows rocm-systems (compiler-runtime, runtime-tests) and rocm-libraries (math-libs, ml-libs) to skip irrelevant stages. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
External repos like rocm-systems output paths like "projects/hip" but the topology alias map expects just "hip". Strip path prefixes to normalize the project names. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When computing affected stages, expand changed projects to include their TEST_SUBPROJECTS dependencies. For example, rocPRIM declares TEST_SUBPROJECTS: hipCUB, rocThrust, rocSPARSE. When rocprim changes, we need to also build rocsparse etc. to run the full test suite. This fixes the issue where rocsparse tests would fail in rocm-libraries CI because SPARSE feature wasn't enabled when only rocprim changed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The affected_stages computation correctly expanded projects to include TEST_SUBPROJECTS (e.g., rocprim -> rocsparse), but this expanded list wasn't passed to configure_stage.py for CMake flag generation. Add expanded_projects output from compute_affected_stages.py and pass it through setup_multi_arch -> multi_arch_ci_linux/windows -> build workflows so configure_stage.py receives the full list of projects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
External repos like rocm-systems and rocm-libraries don't build all components, so native Linux packages and PyTorch/JAX wheel builds would fail or produce incomplete results. Add IS_EXTERNAL_REPO env var and use it to disable: - build_native_linux (DEB/RPM packages) - build_pytorch (PyTorch wheel builds) - build_jax (JAX wheel builds) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
External repos like rocm-systems and rocm-libraries don't build all components, so Python package builds would fail or produce incomplete results. Add condition to skip build_python_packages job when external_repo_config is set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `inputs.external_repo_config == ''` check to Python-related jobs that depend on build_python_packages: - test_python_packages_per_family - build_pytorch_wheel_fat - build_jax_wheels (Linux only) These jobs were getting cancelled instead of skipped when build_python_packages was skipped for external repos. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Match Linux workflow behavior by adding affected_stages input to Windows CI workflows, enabling selective stage builds for external repo builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Skip the compute_affected_stages step for regular TheRock builds since all stages should run. The affected_stages output defaults to 'all'. 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. |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
🎉 All checks passed! This PR is ready for review. |
Use the already-imported generate_cmake_args from the top of the file instead of re-importing from build_tools.configure_stage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests for combined --stage and --projects behavior need explicit platform_name="linux" since some features (like comm-libs/rccl) are disabled on Windows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tony-davis
left a comment
There was a problem hiding this comment.
Thanks for pushing on selective builds, Geo! This is going to save a ton of CI time for the sub-repos, and the fact that it's a clean no-op for TheRock's own builds (everything defaults to all / empty) makes it low-risk to land. I've got a couple of things I'd like to see before merge, plus a few smaller notes.
The blocking one is small: _get_affected_stages() in configure_multi_arch_ci.py isn't called anywhere. I think it got orphaned by the split with #6660 (that PR is where changed-project detection actually lives now), and the real stage computation is in the new compute_affected_stages.py. So it's a third copy of "projects to stages" that nobody runs. I left a suggestion to delete it.
The other thing I care about is the ${{ inputs.projects }} interpolation going straight into the run: blocks in the three artifact workflows. That's the classic Actions script-injection shape, and the value ultimately comes from an external repo's changed_projects. Interestingly your own #6660 does this the safe way already (passes SHAs as gh api argv, no shell splicing), so this is just bringing these three in line. I left suggestions to move it into env:.
Testing is my one real gap concern. The configure_stage_test.py additions are good and they'd actually fail on a regression (nice, not padding). But the new entry point that carries the selective-build logic, compute_affected_stages.py, has no direct test, including the path-normalization and TEST_SUBPROJECTS expansion. That's the piece most likely to drift silently in the sub-repos' CI. Could we add a small unit test for it? (Worth noting the same missing-test issue has #6660 hard-blocked by the bot right now.)
Non-blocking, but while we're here:
- The stage gating uses
contains(inputs.affected_stages, 'math-libs'), which is a substring match on the joined string. No collision today, but it's brittle if a future stage name is a substring of another. A safe idiom iscontains(format(',{0},', inputs.affected_stages), ',math-libs,'). Happy to push suggestions for all of them if you want to tighten it now. - There are now three different project-name normalizers across #6660's
find_matched_subtrees, this PR'scompute_affected_stages.py(split('/')[-1]), andconfigure_stage.normalize_project_name. They agree on the normalcategory/namecase, so this is fine for now, but a shared helper would be nice before a fourth one shows up. - Small heads-up: a few Windows jobs are red on the latest run (hipblaslt, miopen, rocthrust, libhipcxx, Validate Artifact Structure). I'm fairly sure they're pre-existing/flaky since this change can't affect internal builds, but it'd be good to confirm they're not new before merge.
| def _get_affected_stages(changed_projects: str) -> str: | ||
| """Get build stages affected by the changed projects. | ||
|
|
||
| Args: | ||
| changed_projects: Comma-separated list of changed project paths/names | ||
|
|
||
| Returns: | ||
| Comma-separated list of affected stage names, or "all" if no projects | ||
| specified (full build) | ||
| """ | ||
| if not changed_projects or not changed_projects.strip(): | ||
| return "all" | ||
|
|
||
| projects = [p.strip() for p in changed_projects.split(",") if p.strip()] | ||
| if not projects: | ||
| return "all" | ||
|
|
||
| topology = get_topology() | ||
| affected = topology.get_stages_for_projects(projects) | ||
|
|
||
| if not affected: | ||
| # No stages found - fall back to full build | ||
| return "all" | ||
|
|
||
| return ",".join(sorted(affected)) | ||
|
|
||
|
|
There was a problem hiding this comment.
This helper isn't called anywhere (I checked; only the definition exists), and it duplicates the logic that actually ships in compute_affected_stages.py. I think the #6660 split orphaned it. Suggest removing it so we don't leave a third "projects to stages" implementation lying around.
| def _get_affected_stages(changed_projects: str) -> str: | |
| """Get build stages affected by the changed projects. | |
| Args: | |
| changed_projects: Comma-separated list of changed project paths/names | |
| Returns: | |
| Comma-separated list of affected stage names, or "all" if no projects | |
| specified (full build) | |
| """ | |
| if not changed_projects or not changed_projects.strip(): | |
| return "all" | |
| projects = [p.strip() for p in changed_projects.split(",") if p.strip()] | |
| if not projects: | |
| return "all" | |
| topology = get_topology() | |
| affected = topology.get_stages_for_projects(projects) | |
| if not affected: | |
| # No stages found - fall back to full build | |
| return "all" | |
| return ",".join(sorted(affected)) |
| run: | | ||
| PROJECTS_ARG="" | ||
| if [[ -n "${{ inputs.projects }}" ]]; then | ||
| PROJECTS_ARG="--projects ${{ inputs.projects }}" | ||
| fi |
There was a problem hiding this comment.
${{ inputs.projects }} expanded straight into the shell is the Actions injection footgun. Passing it through env: keeps it as a plain variable. The unquoted ${PROJECTS} on the args line still word-splits into multiple --projects values, so behavior is unchanged.
| run: | | |
| PROJECTS_ARG="" | |
| if [[ -n "${{ inputs.projects }}" ]]; then | |
| PROJECTS_ARG="--projects ${{ inputs.projects }}" | |
| fi | |
| env: | |
| PROJECTS: ${{ inputs.projects }} | |
| run: | | |
| PROJECTS_ARG="" | |
| if [[ -n "${PROJECTS}" ]]; then | |
| PROJECTS_ARG="--projects ${PROJECTS}" | |
| fi |
| run: | | ||
| PROJECTS_ARG="" | ||
| if [[ -n "${{ inputs.projects }}" ]]; then | ||
| PROJECTS_ARG="--projects ${{ inputs.projects }}" | ||
| fi |
There was a problem hiding this comment.
Same injection fix here.
| run: | | |
| PROJECTS_ARG="" | |
| if [[ -n "${{ inputs.projects }}" ]]; then | |
| PROJECTS_ARG="--projects ${{ inputs.projects }}" | |
| fi | |
| env: | |
| PROJECTS: ${{ inputs.projects }} | |
| run: | | |
| PROJECTS_ARG="" | |
| if [[ -n "${PROJECTS}" ]]; then | |
| PROJECTS_ARG="--projects ${PROJECTS}" | |
| fi |
| run: | | ||
| PROJECTS_ARG="" | ||
| if [[ -n "${{ inputs.projects }}" ]]; then | ||
| PROJECTS_ARG="--projects ${{ inputs.projects }}" | ||
| fi |
There was a problem hiding this comment.
And the same one here.
| run: | | |
| PROJECTS_ARG="" | |
| if [[ -n "${{ inputs.projects }}" ]]; then | |
| PROJECTS_ARG="--projects ${{ inputs.projects }}" | |
| fi | |
| env: | |
| PROJECTS: ${{ inputs.projects }} | |
| run: | | |
| PROJECTS_ARG="" | |
| if [[ -n "${PROJECTS}" ]]; then | |
| PROJECTS_ARG="--projects ${PROJECTS}" | |
| fi |
| from github_actions_api import gha_set_output | ||
|
|
||
|
|
||
| def compute_affected(changed_projects: str) -> tuple[str, str]: |
There was a problem hiding this comment.
This is the heart of the selective-build logic (normalization + TEST_SUBPROJECTS expansion + stage mapping) and there's no direct unit test for it. A small compute_affected_stages_test.py covering the empty input, a projects/x normalization case, and one expansion case (e.g. rocprim pulling in its TEST_SUBPROJECTS) would lock it down and match the pattern the bot expects.
- Fix Actions injection vulnerability by passing inputs.projects through env: instead of direct shell expansion - Remove unused _get_affected_stages function (duplicates compute_affected_stages.py logic) - Add unit tests for compute_affected_stages.py covering: - Empty input handling - Project path normalization (projects/x -> x) - TEST_SUBPROJECTS expansion - Stage mapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tony-davis
left a comment
There was a problem hiding this comment.
Thanks, Geo; all the blocking items are handled. The dead _get_affected_stages is gone, the three artifact workflows moved projects into env: (no more shell interpolation), and compute_affected_stages.py has real test coverage now for normalization, TEST_SUBPROJECTS expansion, and the stage mapping. Green across the bot, unit tests, and pre-commit.
I left the two contains()/normalizer notes as optional; they're fine as-is, just worth a shared helper if a third caller shows up. Not blocking. Approving.
ScottTodd
left a comment
There was a problem hiding this comment.
The design here is too hacky. Not seeing the full vision. Have you written down the migration plan somewhere we can discuss? I see PRs attached to #3343 but #3343 only has a small edit to the original issue saying "skip packaging workflows".
If I can't trust people to review changes to these workflows and scripts adequately I'll add myself to CODEOWNERS and block all changes.
| -DTHEROCK_ENABLE_FFT=ON | ||
| ... | ||
| """ | ||
| """Generate CMake configuration for building a specific stage or projects.""" |
There was a problem hiding this comment.
These usage comments were useful. Why are you deleting them?
| def normalize_project_name(name: str) -> str: | ||
| """Normalize a project name, handling paths like 'projects/hip' -> 'hip'. | ||
|
|
||
| The changed_projects input from external repos may include paths like: | ||
| - 'projects/hip' -> 'hip' | ||
| - 'projects/rocblas' -> 'rocblas' | ||
| - 'hip' -> 'hip' (already normalized) | ||
| """ | ||
| # Strip 'projects/' prefix if present | ||
| """Normalize 'projects/hip' -> 'hip'.""" |
| # Strip any trailing path components (e.g., 'hip/src/foo.cpp' -> 'hip') | ||
| if "/" in name: | ||
| name = name.split("/")[0] |
| # Disable native packages and PyTorch for external repos (incomplete builds) | ||
| IS_EXTERNAL_REPO: ${{ inputs.external_repo != '' && 'true' || '' }} |
There was a problem hiding this comment.
Why? We need the ability to build native packages and pytorch for changes to rocm-libraries and rocm-systems. That's one of the main reasons to migrate those projects to use "multi-arch CI".
A blunt "is external repo -> disable this job and that job and those other jobs" switch is not appropriate here. We already have filtering support in the CI configuration for individual jobs.
There was a problem hiding this comment.
i'll highlight my thought process in the gh issue
| # Skip for external repos (build_python_packages is skipped) | ||
| if: ${{ !failure() && !cancelled() && inputs.external_repo_config == '' && toJSON(fromJSON(inputs.build_config).test_python_packages_matrix) != '[]' }} |
There was a problem hiding this comment.
No, we need to be able to build/test packages for changes to external repositories.
i need to expand on those! let me do that - it was all in my head and forgot to update the issue |
I dropped more context in #3343. let me know, seems we are not aligned :( so i want to make sure we are! |
erman-gurses
left a comment
There was a problem hiding this comment.
Please see my comments below:
| affected = topology.get_stages_for_projects(expanded_list) | ||
|
|
||
| if not affected: | ||
| print("No stages found for projects, building all stages") | ||
| return "all", "" | ||
|
|
||
| stages = ",".join(sorted(affected)) | ||
| # Space-separated for --projects arg compatibility | ||
| projects_str = " ".join(expanded_list) | ||
| print(f"Affected stages: {stages}") | ||
| print(f"Expanded projects output: {projects_str}") | ||
| return stages, projects_str |
There was a problem hiding this comment.
Currently one global selection is passed to both Linux and Windows. For example, rocprofiler-sdk or amdsmi selects compiler-runtime, but those artifacts are disabled on Windows. filter_projects_for_stage() still returns the project because it does not consider disable_platforms; the later Windows feature resolution then drops it, leaving THEROCK_ENABLE_ALL=OFF with no selected project feature while the Windows stage job still runs.
We might either compute separate Linux/Windows selections or add platform_name filtering to the topology APIs and skip a platform when no applicable projects remain? A regression test using a Windows-disabled compiler-runtime project such as rocprofiler-sdk would cover this path.
| jobs: | ||
| setup: | ||
| runs-on: ubuntu-24.04 | ||
| outputs: | ||
| enable_build_jobs: ${{ steps.configure.outputs.enable_build_jobs }} | ||
| linux_build_config: ${{ steps.configure.outputs.linux_build_config }} | ||
| windows_build_config: ${{ steps.configure.outputs.windows_build_config }} | ||
| test_type: ${{ steps.configure.outputs.test_type }} | ||
| linux_test_labels: ${{ steps.configure.outputs.linux_test_labels }} | ||
| windows_test_labels: ${{ steps.configure.outputs.windows_test_labels }} | ||
| rocm_package_version: ${{ steps.rocm_package_version.outputs.rocm_package_version }} | ||
| rocm_deb_package_version: ${{ steps.rocm_package_version.outputs.rocm_deb_package_version }} | ||
| rocm_rpm_package_version: ${{ steps.rocm_package_version.outputs.rocm_rpm_package_version }} | ||
| ref: ${{ steps.checkout.outputs.commit }} | ||
| external_repo_config: ${{ steps.external_repo_config.outputs.config_json }} | ||
| affected_stages: ${{ steps.affected_stages.outputs.affected_stages }} | ||
| expanded_projects: ${{ steps.affected_stages.outputs.expanded_projects }} |
There was a problem hiding this comment.
Could we preserve the advertised "all" default in the setup job output?
When external_repo == '', the affected_stages step is skipped, so steps.affected_stages.outputs.affected_stages is empty rather than "all". Any caller that explicitly forwards this output will bypass the called workflow’s input default and can cause every stage condition to evaluate false.
| return "all", "" | ||
|
|
||
| # Normalize "projects/hip" -> "hip" | ||
| projects = [p.split("/")[-1] if "/" in p else p for p in raw_projects] |
There was a problem hiding this comment.
We can reuse the existing normalize_project_name() here instead of taking the final path component.
Please see the link: https://github.com/ROCm/TheRock/blob/main/build_tools/configure_stage.py#L58-L72
For example, projects/hip correctly becomes hip, but a changed path such as projects/hip/src/foo.cpp would become foo.cpp. That project would not map to a stage, causing this selective-build path to fall back to "all".
Can we also add a regression test for this nested-path case?
|
closing to do some rework on this. |
Enables external repositories (rocm-systems, rocm-libraries) to trigger selective builds in TheRock, only building and testing the stages/projects that are affected by their changes.
Key Changes:
math-libs, compiler-runtime), skipping unneeded stages entirely
configure_stage.py, enabling only touched components
also build/test hipcub which depends on it)
- Python packages build
- Native packages (DEB/RPM)
- PyTorch/JAX wheel builds
- Python package tests
Works for rocm-systems and rocm-libraries
ISSUE ID: #3343
Next: