fix(ci): restore rocm-systems DVC pull for external rocm-libraries builds (wkmi) - #8204
Conversation
…ilds (wkmi) REPO_CONFIGS["rocm-libraries"] in detect_external_repo_config.py only lists dvc_projects: ["external-rocm-libraries"] (added in #8132 to fix #8131's missing hipdnn golden-data pull). fetch_sources.py's --dvc-projects flag replaces its own default project list (["rocm-libraries", "rocm-systems"] on Windows) rather than extending it. Before #8132, no --dvc-projects flag was emitted for rocm-libraries at all, so the default silently covered rocm-systems too. Since #8132, rocm-systems dropped out of the pull list, so the DVC-tracked wkmi prebuilt libs under TheRock's own rocm-systems submodule (shared/amdgpu-windows-interop/wkmi/{win,lnx}/lib/) are never pulled for a rocm-libraries-triggered external-repo build. This broke CLR linking on both Windows (compiler-runtime, missing wkmi.lib) and Linux (wsl-rocdxg, missing libwkmi.a). Add "rocm-systems" back to the dvc_projects list, and add regression tests at both the config-generation level and the fetch_sources.pull_large_files() level (mirroring the existing test_wsl_rocdxg_scenario, but for the rocm-libraries-triggered path). Fixes #8203 Co-authored-by: Cursor <cursoragent@cursor.com>
✅ 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. |
| "dvc_projects": ["external-rocm-libraries"], | ||
| # "external-rocm-libraries" pulls DVC-tracked hipdnn golden data from the | ||
| # external rocm-libraries checkout itself (#8131). "rocm-systems" | ||
| # additionally pulls TheRock's own rocm-systems submodule DVC data | ||
| # (e.g. the amdgpu-windows-interop/wkmi prebuilt libs that CLR links | ||
| # against on both Windows and Linux/wsl-rocdxg). fetch_sources.py's | ||
| # --dvc-projects replaces its default project list rather than | ||
| # extending it, so both entries must be listed explicitly here or the | ||
| # rocm-systems pull silently disappears again. | ||
| "dvc_projects": ["external-rocm-libraries", "rocm-systems"], |
There was a problem hiding this comment.
Something is wrong here. Why is this code choosing which DVC files to fetch based on the repository that the workflow is running in, rather than the build stage that it is fetching sources for?
There was a problem hiding this comment.
Okay to patch as needed to get CI working but this is confusing...
We also shouldn't be fetching rocm-systems sources during builds of the math-libs stage, see
There was a problem hiding this comment.
Thanks Scott, appreciate the context. I'm not steeped in the CI/build system the way you are, so I'll defer to you here.
This PR was really just a proposal to go along with the issue (#8203), meant to unblock the two failing rocm-libraries CI runs. Happy to have you all take it from here, whether that's approving it as-is, adjusting it, or closing it out in favor of something better.
Sounds like the deeper problem (fetching driven by which repo triggers a workflow, rather than the resolved stage/source-sets) is already something your team's been chipping away at (#460, #7560). I'll leave that one with you all.
There was a problem hiding this comment.
The usual way to handle an unexpected break is to revert and send back to the original author (#8132 + @adickin-amd in this case).
I don't mind the code here too much, but it's indicative of some preexisting brittleness / inefficiencies in the code (tracing through the history it's a relatively new code path used for just external repos, so some bugs and unforseen usage scenarios can be expected)
There was a problem hiding this comment.
🔵 Needs a closer look
The regression test must exercise the production-generated configuration to detect the original failure.
Pull request overview
Restores rocm-systems DVC pulls for external rocm-libraries builds so wkmi libraries are available.
Changes:
- Adds
rocm-systemsto the configured DVC projects. - Updates configuration and regression tests.
- Adds coverage for the affected DVC pull scenario.
File summaries
| File | Summary |
|---|---|
build_tools/tests/fetch_sources_dvc_test.py |
Adds affected-scenario coverage, but the regression test should derive projects from the production configuration so it fails if rocm-systems is removed. |
build_tools/github_actions/tests/dvc_external_repo_test.py |
Verifies the updated configuration and generated arguments. |
build_tools/github_actions/detect_external_repo_config.py |
Adds the missing rocm-systems DVC project. |
Review details
Suppressed comments (1)
build_tools/tests/fetch_sources_dvc_test.py:365
- This test hard-codes the corrected
dvc_projectslist instead of deriving it fromREPO_CONFIGSor the generatedfetch_sources_args. As a result, reverting the production config to the pre-fix['external-rocm-libraries']still leaves this test passing; it only exercises the already-coveredpull_large_filestwo-project case. Wire the scenario through the external-repo config (or parse the generated arguments) so the regression test fails when the config dropsrocm-systems.
dvc_projects = ["external-rocm-libraries", "rocm-systems"]
projects = ["rocm-systems"] # rocm-systems submodule is enabled
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Trim the added comments/docstrings in detect_external_repo_config.py and the two test files down to match the existing terse, present-tense style. Historical context now lives only in the PR description. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Tested in https://github.com/ROCm/rocm-libraries/actions/runs/34909648491?pr=12082, where this fixes the problem. |
Summary
Fixes #8203.
REPO_CONFIGS["rocm-libraries"]indetect_external_repo_config.pyonly lists
dvc_projects: ["external-rocm-libraries"], butfetch_sources.py --dvc-projectsreplaces its own default project list rather than extending it. That silently dropped
rocm-systemsfrom the DVC pull for rocm-libraries-triggered external-repo builds, so theDVC-tracked
wkmiprebuilt libs under TheRock's ownrocm-systemssubmodule(
shared/amdgpu-windows-interop/wkmi/{win,lnx}/lib/) never get pulled — breaking CLR linkingon both Windows (
compiler-runtime, missingwkmi.lib) and Linux (wsl-rocdxg, missinglibwkmi.a).Risk Assessment
Low risk (2/5). Single-entry addition to an existing list, isolated to the
rocm-librariesexternal-repo path, mirrors the already-shipped
rocm-systemsentry exactly, and does notchange behavior for any other external repo (
rocm-systems,rocgdb).Related
(Windows), ROCm/rocm-libraries#12067
(Linux)
Device / Architecture Coverage
Build/CI-tooling change, not device/arch-scoped. Affects Windows and Linux external-repo
builds of rocm-libraries equally (both are currently broken the same way; this fix addresses
both).
Testing Summary
REPO_CONFIGS, and generatedfetch_sources_argsfor bothrocm-librariesandrocm-systems.test_rocm_libraries_also_pulls_rocm_systems_dvc) that fails againstthe pre-fix config and passes with the fix.
(
test_rocm_libraries_scenario_pulls_rocm_systems_wkmi), mirroring the existingtest_wsl_rocdxg_scenario(written for the rocm-systems-triggered path), but for therocm-libraries-triggered path — the one that was actually broken.
dvc_projects=["external-rocm-libraries"](rocm-systems/wkmi never pulled) and passes withthe fix.
Testing Checklist
python -m unittest build_tools/github_actions/tests/dvc_external_repo_test.py- Status: Passedpython -m unittest build_tools/tests/fetch_sources_dvc_test.py- Status: Passedpython -m unittest build_tools/github_actions/tests/detect_external_repo_config_test.py- Status: PassedFlags / Guardrails
None.
Adjacent Tests Considered
Checked the
rocm-systemsandrocgdbREPO_CONFIGSentries — unaffected by this change.Considered making
fetch_sources.py --dvc-projectsadditive to its default instead ofreplacing it, which would prevent this whole class of regression at the source; leaving that
as a follow-up idea rather than folding it into this fix, to keep this PR minimal and low-risk.
Technical Changes
"rocm-systems"toREPO_CONFIGS["rocm-libraries"]["dvc_projects"]indetect_external_repo_config.py, with a comment explaining why both entries are required.dvc_external_repo_test.py: relax therocm-librariesdvc_projectsequalityassertions to
assertIn(list now has two entries) and add a dedicated regression test.fetch_sources_dvc_test.pyfor therocm-libraries-triggered path.