Repository navigation
fix(rocprofiler-sdk): add openmpi to BUILD_DEPS - #6776
Conversation
…cmake_subproject_declare(rocprofiler-sdk ...)
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
ScottTodd
left a comment
There was a problem hiding this comment.
You may also be interested in #6610 and #5627 (cc @minseobshin11). I'm not sure if we want to keep THEROCK_ENABLE_MPI given the packaging/distribution complexity there. If it's just a runtime dep used at test time then we can hopefully satisfy it via the test dockerfile, so long as the projects are at least able to build with dynamic linking MPI support somehow...
| BUILD_DEPS | ||
| ${_openmpi_optional_dep} | ||
| RUNTIME_DEPS | ||
| aqlprofile |
There was a problem hiding this comment.
Should this _openmpi_optional_dep be a build dep or a runtime dep?
The other uses have it in RUNTIME_DEPS:
TheRock/profiler/CMakeLists.txt
Lines 257 to 268 in 29faf3b
TheRock/profiler/CMakeLists.txt
Lines 348 to 361 in 29faf3b
There was a problem hiding this comment.
I believe, It should stay in BUILD_DEPS. The find_package(MPI) in rocprofiler-sdk fires at configure time. the dep provider intercepts it before a single source file is compiled. I feel, the other subprojects that put _openmpi_optional_dep in RUNTIME_DEPS are most likely only using MPI for test execution, not as a compile-time linking dependency.
There was a problem hiding this comment.
All RUNTIME_DEPS are also BUILD_DEPS, not all BUILD_DEPS are RUNTIME_DEPS, see
TheRock/cmake/therock_subproject.cmake
Lines 244 to 248 in 29faf3b
If MPI is only used at build time then BUILD_DEPS is appropriate. If it's also used at run time then it should go in RUNTIME_DEPS.
There was a problem hiding this comment.
Thanks @ScottTodd for linking #6610 and #5627, Keeping BUILD_DEPS, MPI is intentionally not distributed with ROCm artifacts (from #6610 which added the dedicated OpenMPI test image for exactly this reason, and #5627 which wires rocprofiler-sdk tests to use it). libmpi.so is satisfied at test runtime via the container
image, not shipped with release artifacts, so BUILD_DEPS would be accurate here?
There was a problem hiding this comment.
Maybe? I'd prefer local consistency with a small fix like this.
If we need MPI available at configure time but we don't want to distribute it (leaving it as an optionally dynamically linked runtime dependency), then I think BUILD_DEPS could work, but we'd want to be careful about how we represent that in the build and packaging files. Pretty much all of our other dependencies expect to be vendored and distributed together with the libraries and tests.
There was a problem hiding this comment.
Good point on local consistency, I'll move it to RUNTIME_DEPS to match rocprofiler-compute and rocprofiler-systems. The proper long-term fix (guarding find_package(MPI) in rocprofiler-sdk's cmake so it's not reached in standard builds at all) will come as a follow-up PR in rocprofiler-sdk, which will make the dep declaration here moot when THEROCK_ENABLE_MPI=OFF. Moving now to unblock RCCL CI.
marbre
left a comment
There was a problem hiding this comment.
Post-review: Adding Open MPI as a runtime dep is indeed correct here.
Description: Bumps the pinned TheRock ref from `22004a6bc8a` (2026-07-20) to `2e7c190b99c` (2026-07-22). ## What's in this bump Includes the fix from ROCm/TheRock#6776 which adds `openmpi` to rocprofiler-sdk's `RUNTIME_DEPS` in the TheRock profiler CMakeLists. This resolves the rocprofiler-sdk configure failure caused by `find_package(MPI)` being intercepted by TheRock's dep provider after openmpi was added as a super-project component. ## Why Unblocks RCCL CI which has been failing since the previous TheRock bump to `22004a6` introduced openmpi as a super-project component without a corresponding dep declaration in rocprofiler-sdk. ## Follow-up A separate PR in rocprofiler-sdk will guard the `find_package(MPI)` call at the source level as the longer-term fix, keeping MPI out of standard builds entirely. Co-authored-by: Aravind Ravikumar <arravikum@amd.com>
Problem
When openmpi was added as a super-project component in TheRock, the dep provider (
therock_subproject_dep_provider.cmake:68) began interceptingfind_package(MPI)calls fromsubprojects. rocprofiler-sdk calls
find_package(MPI)incmake/rocprofiler_config_interfaces.cmake:408— this was added as part of rocshmem tracing support (AIPROFSDK-886) becauserocshmem's installed CMake config references
MPI::MPI_CXXin its link interface and must be located beforefind_package(rocshmem)can succeed.Since openmpi was not declared in rocprofiler-sdk's
therock_cmake_subproject_declare, the dep provider hard-errors with:CMake Error at cmake/therock_subproject_dep_provider.cmake:68 (message):
Project contains find_package(MPI) for a package available in the
super-project but not declared: Add a BUILD_DEPS or RUNTIME_DEPS
appropriately
Commit in therock causing this: f1a8031
After this commit: The package registry is disabled inside all subprojects. Now when rocprofiler-sdk calls
find_package(MPI), the dep provider enforces that MPI must be declared inBUILD_DEPSorRUNTIME_DEPS. Since the RCCL CI build passes-DTHEROCK_ENABLE_MPI=ON(viaTHEROCK_ENABLE_COMM_LIBS=ON), MPI is inTHEROCK_STRICT_PROVIDED_PACKAGES— but rocprofiler-sdk's declaration inTheRock/profiler/CMakeLists.txtlists no MPI dependency →FATAL_ERROR.This caused rocprofiler-sdk configure to fail, which in turn caused the RCCL CI build to fail in rocm-systems after the TheRock hash bump to
22004a6(2026-07-20).Fix
Add
openmpitoBUILD_DEPSin thetherock_cmake_subproject_declare(rocprofiler-sdk ...)block inprofiler/CMakeLists.txt. This tells the dep provider thefind_package(MPI)call is intentional and allows configure to proceed normally.
Testing
Test rocm-systems run with pinned TheRock commit: https://github.com/ROCm/rocm-systems/actions/runs/29940406058/job/88993031854?pr=9029