Repository navigation
feat: Adding Artifact reuse and stage skip functionality to multi-arch CI - #6264
Conversation
✅ 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. |
There was a problem hiding this comment.
Pull request overview
Introduces an automatic “stage reuse” decision layer for the multi-arch CI setup so that unaffected build stages can be satisfied by copying artifacts from a compatible baseline workflow run. The feature is gated behind STAGE_REUSE_MODE, defaulting to report-only (dry-run) and enabling actual skipping in skip-stage.
Changes:
- Added
stage_reuse_decision.pyto compute candidate stages, verify baseline artifact availability, and generate console/step-summary reporting. - Integrated auto stage-reuse evaluation into
configure_multi_arch_ci.py, merging eligible stages intoprebuilt_stageswhen enabled. - Plumbed a new
stage_reuse_modeworkflow input through the setup and multi-arch CI workflows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| build_tools/github_actions/stage_reuse_decision.py | New logic for stage impact + baseline verification + reporting for stage reuse. |
| build_tools/github_actions/configure_multi_arch_ci.py | Invokes stage reuse logic and merges results into per-stage build decisions. |
| .github/workflows/setup_multi_arch.yml | Adds stage_reuse_mode input and exports related env vars for the setup step. |
| .github/workflows/multi_arch_ci.yml | Adds stage_reuse_mode input and passes it into the setup workflow. |
Comments suppressed due to low confidence (1)
build_tools/github_actions/configure_multi_arch_ci.py:675
- In
skip-stagemode this code marks stages asPREBUILT, but it does not setbaseline_run_idfrom the selected baseline run. That will cause downstream workflows to skip build stages without copying artifacts (copy step is gated onbaseline_run_id != '').
# Only skip-stage mode returns non-empty applied_reuse_stages.
for stage in auto.applied_reuse_stages:
stage_decisions.setdefault(stage, JobAction.PREBUILT)
build_rocm = BuildRocmDecision(
action=JobAction.RUN,
stage_decisions=stage_decisions,
baseline_run_id=ci_inputs.baseline_run_id,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto = compute_auto_stage_reuse( | ||
| changed_files=git_context.changed_files, | ||
| mode=StageReuseMode.from_environ(), | ||
| target_families=target_families, | ||
| ) |
| def compute_auto_stage_reuse( | ||
| *, | ||
| changed_files: Sequence[str] | None, | ||
| mode: StageReuseMode, | ||
| platform: str | None = None, | ||
| target_families: Sequence[str] = (), | ||
| topology=None, | ||
| baseline_selector: BaselineSelector | None = None, | ||
| ) -> AutoStageReuse: | ||
| """Compute auto stage-reuse decisions, verified against a baseline run.""" |
erman-gurses
left a comment
There was a problem hiding this comment.
Looks good to me overall - please see my comments.
| render_step_summary, | ||
| ) | ||
| target_families = _stage_reuse_target_families(ci_inputs) | ||
| auto = compute_auto_stage_reuse( |
There was a problem hiding this comment.
compute_auto_stage_reuse() now performs stage-impact analysis, baseline selection, artifact verification, and reporting. Have we considered separating the planning portion from the reporting/execution plumbing to keep the decision logic independently reusable?
| from stage_reuse_decision import ( | ||
| StageReuseMode, | ||
| compute_auto_stage_reuse, | ||
| render_step_summary, | ||
| ) |
There was a problem hiding this comment.
Is there a reason these imports are local rather than module-level? If it's to avoid import cycles, it might be worth adding a short comment.
There was a problem hiding this comment.
Nope - follow the style guide. These imports must be moved to the top. https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/python_style_guide.md#import-organization
No import cycles here. If there are cycles we should break them instead of use local imports.
| STAGE_REUSE_CURRENT_SHA: ${{ steps.checkout.outputs.commit }} | ||
| STAGE_REUSE_MAX_AGE_HOURS: "72" | ||
| STAGE_REUSE_COMMIT_HISTORY: "50" |
There was a problem hiding this comment.
These are becoming part of the stage-reuse interface - will make sense to document this environment contract in the module docstring.
| PREBUILT_STAGES: ${{ inputs.prebuilt_stages }} | ||
| BASELINE_RUN_ID: ${{ inputs.baseline_run_id }} | ||
| STAGE_REUSE_MODE: ${{ inputs.stage_reuse_mode }} | ||
| STAGE_REUSE_CURRENT_SHA: ${{ steps.checkout.outputs.commit }} |
There was a problem hiding this comment.
you can actaully get this from ${{ github.sha }} (current sha)
There was a problem hiding this comment.
It depends on what commit ref you want.
steps.checkout.outputs.commit gets the TheRock commit even when running in rockrel (and maybe rocm-libraries / rocm-systems?), see the workflow output:
For runs in rocm-libraries and rocm-systems, we may want to use baseline runs from those repositories instead of TheRock though.
Ultimately I think the best artifact reuse we'll be able to get will be some query against a database where we say "here are the artifact fingerprints that my current source code produces, fetch me artifacts for any with matching fingerprints from the database", which we'd want to abstract away from the github actions runs API entirely. We wouldn't need to scan commit history or workflow runs with that architecture.
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
8e94a55 to
397e824
Compare
5dc1564 to
e7086a2
Compare
|
Have we considered the case where we have ASAN build stages and we want to reuse those as well? |
|
@ammallya, I only glanced at this but I know @ScottTodd worked on a prototype (#3399 (comment)) and thus he has opinion for sure and it would be valuable to wait for his review before this is merged. |
ScottTodd
left a comment
There was a problem hiding this comment.
The general approach taken here and plumbing through the scripts is reasonable. I spotted quite a few issues with the specific changes though. Please watch for these coding errors more carefully (local review iterations via coding agents can help there)
Do you have a plan for which sorts of PR runs to first get working with prebuilt artifacts? If you are aiming to get rocm-systems and rocm-libraries using them, a faster path would be to directly set which workflow runs and prebuilt artifacts to use in the workflows whenever the TheRock commit is updated, as that can use the existing manual inputs and none of this extra code:
- rocm-libraries can use a prebuilt compiler-runtime stage
- rocm-systems with changes to hip/CLR unfortunately can't reuse artifacts this way since the compiler and runtimes are together in a stage, we'd need finer-grained artifact fprint-based reuse for that
- rocm-systems with changes to RCCL can reuse all but the comm-libs stage
- rocm-systems with changes to the "storage libs" / "profiler apps" / etc. stages can reuse other stages
| PREBUILT_STAGES: ${{ inputs.prebuilt_stages }} | ||
| BASELINE_RUN_ID: ${{ inputs.baseline_run_id }} | ||
| STAGE_REUSE_MODE: ${{ inputs.stage_reuse_mode }} | ||
| STAGE_REUSE_CURRENT_SHA: ${{ steps.checkout.outputs.commit }} |
There was a problem hiding this comment.
It depends on what commit ref you want.
steps.checkout.outputs.commit gets the TheRock commit even when running in rockrel (and maybe rocm-libraries / rocm-systems?), see the workflow output:
For runs in rocm-libraries and rocm-systems, we may want to use baseline runs from those repositories instead of TheRock though.
Ultimately I think the best artifact reuse we'll be able to get will be some query against a database where we say "here are the artifact fingerprints that my current source code produces, fetch me artifacts for any with matching fingerprints from the database", which we'd want to abstract away from the github actions runs API entirely. We wouldn't need to scan commit history or workflow runs with that architecture.
| from stage_reuse_decision import ( | ||
| StageReuseMode, | ||
| compute_auto_stage_reuse, | ||
| render_step_summary, | ||
| ) |
There was a problem hiding this comment.
Nope - follow the style guide. These imports must be moved to the top. https://github.com/ROCm/TheRock/blob/main/docs/development/style_guides/python_style_guide.md#import-organization
No import cycles here. If there are cycles we should break them instead of use local imports.
Shameless plug, but debug-tools is fairly simple to get working with prebuilt artifacts. It only depends on compiler-runtime. I could help test that setup (I've been using some local scripting to do that). |
ScottTodd
left a comment
There was a problem hiding this comment.
Thanks for the changes. No more major concerns from me. Having this default to dry-run will let us evaluate it and then choose when to enable. Notably we won't want to reuse artifacts for release builds (the release workflows already don't use that data).
| class BuildPlatformsTest(unittest.TestCase): | ||
| def test_no_families_is_empty(self): | ||
| self.assertEqual(srd._build_platforms((), ()), ()) | ||
|
|
||
| def test_linux_only(self): | ||
| self.assertEqual(srd._build_platforms(["gfx94x"], ()), ("linux",)) | ||
|
|
||
| def test_windows_only(self): | ||
| self.assertEqual(srd._build_platforms((), ["gfx110x"]), ("windows",)) | ||
|
|
||
| def test_both_platforms(self): | ||
| self.assertEqual( | ||
| srd._build_platforms(["gfx94x"], ["gfx110x"]), ("linux", "windows") | ||
| ) |
There was a problem hiding this comment.
What are these tests checking?
The test cases are pretty hard to read with the function call and checks inlined and positional arguments being used instead of named keyword arguments.
|
Something in here nearly doubled the time it takes to run project unit tests, see #6752 locally I see this with |
Motivation
Issue ID Issue 3399
TheROCK already has the building blocks for stage-aware prebuilt-artifact reuse:
stage_impact.pycomputes which build stages a changed-file set affectsbaseline_runs.pyselects a healthy, commit-compatible baseline workflow run and verifies it contains required artifactsprebuilt_stagesskip mechanism is plumbed through the multi-arch workflows — each stage already guards on!contains(inputs.prebuilt_stages, 'stage-name')This PR adds skip capability with a report-only default so we can watch the decisions across real runs before any stage is actually skipped, and flip a single switch (
STAGE_REUSE_MODE=skip-stage) to turn skipping on.Technical Details
New module
build_tools/github_actions/stage_reuse_decision.pythat reuses the existing tooling (no new impact or baseline logic) and applies two gates before a stage is reported reusable:stage_impact.analyze_stage_impact) — the change does not affect the stage → it is a candidate for reuse.baseline_runs.select_baseline_run) — a single healthy, commit-compatible baseline run actually contains the artifacts that stage would produce. This runs even in dry-run, so the report reflects what could really be skipped, not just what is logically unaffected.Two-way
STAGE_REUSE_MODEswitch:dry-run(default) — runs both gates and prints, per stage,[STAGE-REUSE] stage 'X' ... WOULD be skipped (dry-run)to the console and the step summary (including the baseline run id it would copy from), but returns no stages, soprebuilt_stagesis unchanged and every stage still builds.skip-stage— merges only the confirmed-available stages into the per-stage decisions so the orchestrator copies their artifacts and skips the build.Safety:
prebuilt_stages/baseline_run_idworkflow_dispatch inputs are never touched — they keep working unchanged in every mode.build_tools/, workflow, or topology changes), runs without a changed-file list (schedule / dispatch), no baseline found, or any baseline-lookup error.Test Plan
build_tools/github_actions/tests/stage_reuse_decision_test.pyusing an injected fake topology and an injected baseline selector:stage_impact_test.py,baseline_runs_test.py, andconfigure_multi_arch_ci_test.pycontinue to passTest Result
Writing job summary:
Submission Checklist