Repository navigation
ci: build the stages ci:run-wsl needs on narrowed builds - #9628
amd-justchen wants to merge 3 commits into
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. |
|
Tested with the same
|
| # A narrowed build (e.g. an external repo PR) usually leaves out wsl-rocdxg. | ||
| if ( | ||
| build_stages | ||
| and WSL_RUN_LABEL in pr_labels | ||
| and WSL_REQUIRED_STAGE not in build_stages | ||
| ): | ||
| build_stages.append(WSL_REQUIRED_STAGE) |
There was a problem hiding this comment.
rule of thumb: there should be zero mentions of specific build stages or subprojects in this file. The CI system should be general purpose and read dynamic data from the build system. See prior review comments on #7600 (comment).
What we actually want here is
- A pr label requested building/testing a particular subproject
- Ensure that ROCm artifacts are built (and not skipped/prebuilt)
- Ensure that the requested ROCm subproject is built
- Ensure that the ROCm build stage including that subproject is built
I would implement like so:
- Keep
CIInputsas-is- No data changing logic in here. Just read from the environment.
- Recategorize "build_stages" as "requested build stages" not "build stages allow-list"
- In
decide_jobs(), resolve requested build/test stages, labels, etc. to produce the actual build/skip stage lists - In
expand_build_configs(), use all the resolved decisions (some code there may still be a passthrough from inputs)
Sample code:
# Illustrative: these helpers would be added.
test_request = resolve_test_request(ci_inputs, targets)
required_artifacts = get_required_artifacts(test_request)
required_stages = topology.get_stages_for_artifacts(required_artifacts)
effective_stages = (
set(ci_inputs.build_stages) | required_stages
if ci_inputs.build_stages
else set(topology.get_all_stage_names())
)
skipped_stages = _resolve_skipped_stages(sorted(effective_stages))There was a problem hiding this comment.
Example code flow we should have:
ci:run-wsl label in inputs
→ test configuration requires artifact "wsl-rocdxg"
→ topology resolves its producing stage and dependencies
→ effective stages include "wsl-rocdxg" and "compiler-runtime"
Those stages are unioned with ci_inputs.build_stages to produce the effective stages. The original requested stages stay unchanged.
For example:
Input: compiler-runtime,dctools-core
Additional: compiler-runtime,wsl-rocdxg
Effective: compiler-runtime,dctools-core,wsl-rocdxg
Solving this generally should also make it easier to move the "changed projects --> stages/artifacts to build" into TheRock. Today if you change for example projects/rdc and request test:rocrtst I believe that will get rejected instead of expanding dynamically.
We should be able to remove configure_external_repo_ci.py [almost?] entirely, with the CI system used across each repository going down one sequence:
- Detect which projects changed (in submodules)
- Select builds/tests to run for those changes
- Resolve which artifacts and stages supply those tests
- Plan execution (build/reuse/skip), choose platforms, choose runner labels, etc.
- Run CI
There was a problem hiding this comment.
Thank you for the review! Will rework this!
There was a problem hiding this comment.
Reworked in f4ada56 along these lines:
CIInputsreadsbuild_stagesas-is (the requested stages). Theci:run-wslappend and the wsl-rocdxg check invalidate()are gone, andconfigure_multi_arch_ci.pyno longer names any stage.amdgpu_family_matrix.CI_LABEL_REQUIRED_ARTIFACTSdeclares what a label needs beyond the components' own artifacts (ci:run-wsl->wsl-rocdxg).decide_jobs()unions the requested stages withget_stages_for_artifacts()of those artifacts, then skips the rest.
Run on the branch (build_stages=compiler-runtime,runtime-tests, ci:run-wsl,test:hip-tests,test:rocrtst): https://github.com/ROCm/TheRock/actions/runs/41680234108
Requested build stages: ['compiler-runtime', 'runtime-tests']
Added for the requested tests: ['wsl-rocdxg']
wsl-rocdxg built, and WSL hip-tests and rocrtst passed.
STAGE_TO_TEST_LABELS and the test-label auto-select in from_environ() predate this PR. I can move them to the same requested -> effective flow in a follow-up, along with the changed-projects resolution.
External repo PRs pass a build_stages allowlist that usually leaves out wsl-rocdxg, so ci:run-wsl failed setup with "ci:run-wsl needs the wsl-rocdxg stage". Add that stage and the stages it builds on instead, so the label works for any caller (e.g. rocm-systems) without repo-side logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the _add_wsl_stages helper with a plain check where build_stages is parsed: with ci:run-wsl and a narrowed build_stages, append wsl-rocdxg. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per review: configure_multi_arch_ci.py should not name specific stages, and CIInputs should only read the environment. - CIInputs.build_stages is now the requested stages, read as-is. The ci:run-wsl append in from_environ() and the wsl-rocdxg check in validate() are removed. - amdgpu_family_matrix.CI_LABEL_REQUIRED_ARTIFACTS declares the artifacts a CI label needs beyond the components' own (ci:run-wsl -> wsl-rocdxg). - decide_jobs() unions the requested stages with the stages the topology says produce those artifacts (and their dependencies), and skips the rest. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2f618cf to
f4ada56
Compare
ScottTodd
left a comment
There was a problem hiding this comment.
Nice, this is pretty clean. Thanks for the edits.
Motivation
ci:run-wsl(#8457) fails setup on external repo PRs that narrow the build. rocm-systems passesbuild_stagesper PR, which usually leaves outwsl-rocdxg, andconfigure_multi_arch_ci.pythen raisesci:run-wsl needs the wsl-rocdxg stage. Per review on ROCm/rocm-systems#14472, this belongs in TheRock rather than in each caller.GitHub issue: https://github.com/ROCm/TheRock-Infra/issues/317
Technical Details
Following the review design:
build_stagesis the requested stages, anddecide_jobs()resolves the effective stages from what the requested tests need.CIInputsreadsbuild_stagesas-is. This removes theci:run-wslappend and the wsl-rocdxg check invalidate()(from feat(ci): run hip-tests and rocrtst on WSL GPU runners alongside native Linux #8457), soconfigure_multi_arch_ci.pyno longer names any stage.amdgpu_family_matrix.CI_LABEL_REQUIRED_ARTIFACTSdeclares the artifacts a CI label needs beyond the components' own:ci:run-wsl->wsl-rocdxg(the WSL libhsakmt thatinstall_rocm_from_artifactsinstalls on WSL).decide_jobs()unions the requested stages withget_topology().get_stages_for_artifacts()of those artifacts (producing stage plus dependencies) and skips everything else.Example: requested
compiler-runtime,dctools-core, pluscompiler-runtime,wsl-rocdxgfor the test, gives effectivecompiler-runtime,dctools-core,wsl-rocdxg. Full builds and PRs without the label are unchanged.Test Plan
python -m pytest build_tools/github_actions/testsbuild_stages=compiler-runtime,runtime-tests,pr_labels=ci:run-wsl,test:hip-tests,test:rocrtst, gfx110X, prebuilt compiler-runtime/runtime-tests.Test Result
configure_multi_arch_ci_test.py+amdgpu_family_matrix_test.py: 192 passed, 1 skipped. Fullbuild_tools/github_actions/tests: 1112 passed. The only failure isrun_jax_tests_test.py::test_a_symlink_leading_out_of_the_test_tree_is_fatal, which needs symlink rights and fails the same on main on this Windows machine.test_run_wsl_adds_wsl_rocdxg_to_partial_buildfeeds a labelled PR event throughfrom_environ()anddecide_jobs(). It checks that the requested stages are unchanged, and thatwsl-rocdxgis built only with the label.Requested build stages: ['compiler-runtime', 'runtime-tests']andAdded for the requested tests: ['wsl-rocdxg'].Submission Checklist