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 92.90% 92.90%
========================================
Files 603 603
Lines 32526 32526
========================================
Hits 30217 30217
Misses 2309 2309 🚀 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
Mirrors the same fix applied to Jenkinsfile in 667c6fe. The rbuild prepare step installs rocm-cmake into /usr/local inside the SLES Docker image, but without this prefix path cmake falls back to the older system-installed package which lacks rocm_add_version_resource, causing the configure step to fail with "Unknown CMake command". Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
| # Arch-suffixed blas package: amdrocm-blas<ver>-<arch> | ||
| if(_name MATCHES "^amdrocm-blas([0-9][0-9.]*)-(gfx[0-9a-z]+)$") | ||
| if(NOT _version) | ||
| set(_version "${CMAKE_MATCH_1}") | ||
| endif() | ||
| if(NOT _first_arch) | ||
| set(_first_arch "${CMAKE_MATCH_2}") | ||
| endif() | ||
| list(APPEND _all_archs "${CMAKE_MATCH_2}") | ||
| # Non-arch blas package: amdrocm-blas<ver> (version fallback only) | ||
| elseif(_name MATCHES "^amdrocm-blas([0-9][0-9.]*)$") | ||
| if(NOT _version_no_arch) | ||
| set(_version_no_arch "${CMAKE_MATCH_1}") | ||
| endif() | ||
| endif() |
| // TODO: JIRA Ticket TBD - rocBLAS solution-index validation | ||
| // (rocblas_gemm_flags_check_solution_index) does not report invalid indices on | ||
| // gfx1201; it returns rocblas_status_success for any index, so an invalid index | ||
| // is never reset to the default. Skip on gfx1201 until rocBLAS is fixed. | ||
| if(migraphx::gpu::get_gfx_name(migraphx::gpu::get_device_name()) == "gfx1201") | ||
| { | ||
| test::skip("rocBLAS solution-index validation is broken on gfx1201 (JIRA Ticket TBD)"); | ||
| } |
| # TheRock installs into a versioned root (/opt/rocm/core-<ver>). Expose the | ||
| # conventional /opt/rocm/{bin,lib,llvm,...} layout expected by MIGraphX tooling. | ||
| RUN mkdir -p /opt/rocm && \ | ||
| for d in bin lib libexec include share llvm amdgcn; do \ | ||
| ln -snf core-${ROCM_VERSION}/$d /opt/rocm/$d; \ | ||
| done && \ | ||
| echo "/opt/rocm/lib" > /etc/ld.so.conf.d/rocm.conf && \ | ||
| echo "/opt/rocm/llvm/lib" > /etc/ld.so.conf.d/rocm-llvm.conf && \ | ||
| ldconfig | ||
|
|
||
| ENV ROCM_PATH=/opt/rocm | ||
| ENV PATH=/opt/rocm/bin:/opt/rocm/llvm/bin:$PATH |
| # TheRock installs into a versioned root (/opt/rocm/core-<ver>). Expose the | ||
| # conventional /opt/rocm/{bin,lib,llvm,...} layout expected by MIGraphX tooling. | ||
| RUN mkdir -p /opt/rocm && \ | ||
| for d in bin lib libexec include share llvm amdgcn; do \ | ||
| ln -snf core-${ROCM_VERSION}/$d /opt/rocm/$d; \ | ||
| done && \ | ||
| echo "/opt/rocm/lib" > /etc/ld.so.conf.d/rocm.conf && \ | ||
| echo "/opt/rocm/llvm/lib" > /etc/ld.so.conf.d/rocm-llvm.conf && \ | ||
| ldconfig | ||
| ENV ROCM_PATH=/opt/rocm | ||
| ENV PATH=/opt/rocm/bin:/opt/rocm/llvm/bin:$PATH |
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