Skip to content

Add an THEROCK_ENABLE_MPI option#1283

Merged
marbre merged 4 commits intomainfrom
users/marbre/mpi
Aug 19, 2025
Merged

Add an THEROCK_ENABLE_MPI option#1283
marbre merged 4 commits intomainfrom
users/marbre/mpi

Conversation

@marbre
Copy link
Copy Markdown
Member

@marbre marbre commented Aug 19, 2025

This allows to build rccl-tests with message passing interface enabled, which was previously set to OFF via hard coding USE_MPI. As long as TheRock does not vendor MPI, this requires an MPI system installation, e.g. via vcpkg.

This allows to build rccl-tests with message passing interface enabled,
which was previously set to OFF via hard coding `USE_MPI`. As long as
TheRock does not vendor MPI, this requires an MPI system installation,
e.g. via vcpkg.
CMakeLists.txt Outdated
set(THEROCK_ARTIFACT_ARCHIVE_SUFFIX "" CACHE STRING "Suffix to add to artifact archive file stem names")

option(THEROCK_BUNDLE_SYSDEPS "Builds bundled system deps for portable builds into lib/rocm_sysdeps" ON)
option(THEROCK_ENABLE_MPI "Enables building components with message passing interface support" OFF)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.md Outdated
Comment on lines +167 to +171
Further flags allow to build components with specific features enabled.

| Other flags | Description |
| --------------------------- | ------------------------------------------------------------------------ |
| `-DTHEROCK_ENABLE_MPI=OFF` | Enables building components with Message Passing Interface (MPI) support |
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I was thinking about just adding to https://github.com/ROCm/TheRock/blob/main/docs/development/dependencies.md for now. That's lower visibility than the root README.md which seems appropriate for now.

I guess it's fine to add to here too.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That document rather has what TheRock ships as additional dependencies which it can bundle (or consume from the system) thus it didn't seemed appropriate for that particular option.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could see adding a new section for "currently unbundled deps" with MPI, then MPI could be moved from that section into the main section once we build/vendor it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's follow up on this. Maybe want to add Fortran to this as well?

Copy link
Copy Markdown
Member

@ScottTodd ScottTodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ~ the open documentation comment threads (preferred as part of this PR but can be deferred to later)

Co-authored-by: Scott Todd <scott.todd0@gmail.com>
@marbre marbre merged commit 5f35280 into main Aug 19, 2025
5 checks passed
@marbre marbre deleted the users/marbre/mpi branch August 19, 2025 23:25
@github-project-automation github-project-automation bot moved this from TODO to Done in TheRock Triage Aug 19, 2025
marbre pushed a commit that referenced this pull request Sep 15, 2025
#1283 probably meant to link to
#1284 instead of 128
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants