Bump CI to TheRock 7.14 - #4952
Conversation
There was a problem hiding this comment.
Pull request overview
This PR moves MIGraphX’s CI and default Docker build environment to ROCm “TheRock” multi-arch (amdrocm-*) packages targeting ROCm 7.13, while preserving older ROCm (7.2.x and earlier) flows via “legacy” Dockerfiles.
Changes:
- Added a cross-distro
tools/install_build_prereqs.shto install build prerequisites + TheRock ROCm components (optionally via wheels). - Updated CI image Dockerfile (
hip-clang.docker) and the defaultDockerfileto use Ubuntu 24.04 + ROCm 7.13 TheRock packages. - Introduced legacy Dockerfiles and updated docs/changelog to reflect the new default and legacy paths.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tools/install_build_prereqs.sh |
New shared installer script for build prerequisites and ROCm TheRock components. |
tools/docker/therock_deb.docker |
Removed older TheRock deb-based Dockerfile (replaced by new defaults/legacy split). |
tools/docker/legacy.dockerfile |
Added legacy dev Dockerfile for ROCm 7.2.x and older. |
README.md |
Updated Docker build instructions and documented legacy Dockerfile usage. |
hip-clang.docker |
Updated CI image to Ubuntu 24.04 + ROCm 7.13 TheRock, and prebuilds deps via rbuild. |
hip-clang-legacy.docker |
Added legacy CI image Dockerfile (older ROCm packaging). |
Dockerfile |
Updated default dev Dockerfile to Ubuntu 24.04 + ROCm 7.13 TheRock using the shared prereqs script. |
CHANGELOG.md |
Added entry for ROCm 7.13/TheRock build support. |
.github/workflows/ci.yaml |
Updated CI image tag hashing inputs to reflect the new prereqs script. |
pfultz2
left a comment
pfultz2
left a comment
There was a problem hiding this comment.
The AI generated dockers are really awful and should be simplified and made more readable like the original dockers. Too many commands put into the same line.
Also, this is missing the update to the SLES docker.
| hipsparselt \ | ||
| half \ | ||
| libssl-dev \ | ||
| zlib1g-dev && \ |
There was a problem hiding this comment.
We are missing packages here, like clang-17.
There was a problem hiding this comment.
While checking the ASAN issue I'm going to go with a suggested workaround using the clang version provided by TheRock since clang-17 didn't have a fix. runtime ASAN_OPTIONS=detect_odr_violation=0, or compile-time -mllvm -asan-use-private-alias=1 + use_odr_indicator=1.
There was a problem hiding this comment.
runtime ASAN_OPTIONS=detect_odr_violation=0, or compile-time -mllvm -asan-use-private-alias=1 + use_odr_indicator=1.
That workaround never fixed the issue in the past.
There was a problem hiding this comment.
Also, this is just making more unnecessary changes in this PR. This PR should only focus on updating to rocm 7.13, it shouldn't be making these change. The docker file should have a minimal amount of changes with most of the changes in install_preqs.sh file.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4952 +/- ##
========================================
Coverage 93.26% 93.26%
========================================
Files 623 623
Lines 33097 33097
========================================
Hits 30866 30866
Misses 2231 2231 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Why are the clang_tidy flags being changed?
There was a problem hiding this comment.
I assume these are new warnings from newer cland tidy version. We should address these in a seperate PR.
| RUN /install_prereqs.sh /usr/local / && rm /install_prereqs.sh && rm /requirements-py.txt | ||
| RUN test -f /usr/local/hash || exit 1 | ||
| COPY tools/install_prereqs.sh /tmp/install_prereqs.sh | ||
| RUN chmod +x /tmp/install_prereqs.sh && \ |
There was a problem hiding this comment.
Add install_preqs.sh like we did in the other dockerfile.
| "torchaudio==2.11.0+rocm${ROCM_VERSION}.0" | ||
|
|
||
| ADD tools/requirements-py.txt /requirements-py.txt | ||
| RUN CMAKE_ARGS="-DONNX_USE_PROTOBUF_SHARED_LIBS=ON" pip3 install -r /requirements-py.txt && \ |
There was a problem hiding this comment.
This should be done in the install_preqs.sh file.
| mkdir build | ||
| cd build | ||
| cmake -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DBUILD_DEV=On -DCMAKE_EXECUTE_PROCESS_COMMAND_ECHO=STDOUT -DMIGRAPHX_DISABLE_VIRTUAL_ENV=ON ${flags} .. | ||
| cmake -DCMAKE_PREFIX_PATH=/usr/local -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DBUILD_DEV=On -DCMAKE_EXECUTE_PROCESS_COMMAND_ECHO=STDOUT -DMIGRAPHX_DISABLE_VIRTUAL_ENV=ON ${flags} .. |
There was a problem hiding this comment.
/usr/local is already part of the prefix path in cmake. This should not be needed.
There was a problem hiding this comment.
it wasn't working without it. See the build logs
Paul wants to make some changes. This one will likely not get merged. |
The TheRock package name and the .deb/.rpm filename disagree: the filename is already amdrocm-migraphx (set in UFB) while rocm_create_package still emits migraphx, so apt/dnf see a different name than the file advertises. Fixing that only needs _MGX_PKG_NAME plumbed into rocm_create_package. Dropping the per-arch dependency block is a separate design question that overlaps with #4952, and it was holding up a rename that is otherwise independent. That work moves to kqian1/therock-drop-per-arch-deps. Co-authored-by: Cursor <cursoragent@cursor.com>
Motivation
Move CI to TheRock 7.14
Technical Details
Changes were significant enough to just rename original files to legacy and add the 7.13 to the default files
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot Applicable