Bump CI to TheRock 7.13#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
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #4952 +/- ##
===========================================
+ Coverage 92.71% 92.72% +0.01%
===========================================
Files 589 590 +1
Lines 31160 31233 +73
===========================================
+ Hits 28888 28960 +72
- Misses 2272 2273 +1 🚀 New features to boost your workflow:
|
| ADD tools/requirements-py.txt /requirements-py.txt | ||
| RUN CMAKE_ARGS="-DONNX_USE_PROTOBUF_SHARED_LIBS=ON" pip3 install -r /requirements-py.txt && \ | ||
| rm /requirements-py.txt |
Motivation
Move CI to TheRock 7.13
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