Skip to content

Changed default behaviour for RCCL_MSCCLPP_ENABLE based on ENABLE_MSCCLPP definition - #1294

Merged
corey-derochie-amd merged 1 commit into
ROCm:finalize-mscclppfrom
corey-derochie-amd:mscclpp-default-enable
Sep 6, 2024
Merged

corey-derochie-amd merged 1 commit into
ROCm:finalize-mscclppfrom
corey-derochie-amd:mscclpp-default-enable

Conversation

@corey-derochie-amd

@corey-derochie-amd corey-derochie-amd commented Aug 13, 2024 •

Copy link
Copy Markdown
Contributor

Details

Do not mention proprietary info or link to internal work items in this PR.

What were the changes?

  • Renamed RCCL_ENABLE_MSCCLPP to RCCL_MSCCLPP_ENABLE.
  • Set RCCL_MSCCLPP_ENABLE to 1 by default if ENABLE_MSCCLPP is defined, or 0 otherwise.
  • Added a log warning if RCCL_MSCCLPP_ENABLE is set to 1 but ENABLE_MSCCLPP is not defined.

Why were the changes made?

  • Consistent naming with MSCCL
  • Less work required by application developers
  • Removed likelihood of configuration error by user

Approval Checklist

Do not approve until these items are satisfied.

  • Verify the CHANGELOG has been updated, if
    • there are any NCCL API version changes,
    • any changes impact library users, and/or
    • any changes impact any other ROCm library.

@corey-derochie-amd corey-derochie-amd added noCI Disable Jenkins for this PR. noExtendedCI labels Aug 13, 2024
@corey-derochie-amd corey-derochie-amd self-assigned this Aug 13, 2024
@corey-derochie-amd
corey-derochie-amd force-pushed the mscclpp-default-enable branch 2 times, most recently from 195819d to 3239400 Compare September 5, 2024 22:57
@corey-derochie-amd
corey-derochie-amd marked this pull request as ready for review September 5, 2024 22:58
@corey-derochie-amd
corey-derochie-amd force-pushed the mscclpp-default-enable branch 2 times, most recently from d05bee3 to 2201f18 Compare September 5, 2024 23:12
@corey-derochie-amd corey-derochie-amd changed the title Changed default behaviour for RCCL_ENABLE_MSCCLPP based on ENABLE_MSCCLPP definition Changed default behaviour for RCCL_MSCCLPP_ENABLE based on ENABLE_MSCCLPP definition Sep 5, 2024
…MSCCL. Set `RCCL_MSCCLPP_ENABLE` to 1 by default if `ENABLE_MSCCLPP` is defined, or 0 otherwise. Added a log warning if `RCCL_MSCCLPP_ENABLE` is set to 1 but `ENABLE_MSCCLPP` is not defined.
@corey-derochie-amd
corey-derochie-amd requested a review from a team as a code owner September 6, 2024 21:59
@corey-derochie-amd
corey-derochie-amd merged commit 41876e0 into ROCm:finalize-mscclpp Sep 6, 2024
corey-derochie-amd added a commit that referenced this pull request Sep 11, 2024
* Added restrictions around calling MSCCL++ collectives (#1281)

* Added restriction to non-zero 32-byte multiple message sizes to MSCCL++ AllGather.

* Renamed and refactored some mscclpp types.

* Only transmit the MSCCL++ unique id for non-split comm init. For splitting comm, it has already been transmitted. Instead, save the MSCCL++ communicator in child communicators when calling `ncclCommSplit`. Only destroy MSCCL++ communicators when no RCCL communicators remain that use it. Also improved trace logging.

* Disable MSCCL++ when using managed memory buffers as it isn't supported.

* Added datatype and op constraints for MSCCL++ AllReduce.

* Added documentation on MSCCL++ restrictions to the README.

* [BUILD] Support custom CMake flags in MSCCLPP (#1275)

* [BUILD] Support custom CMAKE_PREFIX_PATH in MSCCLPP

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>

* [BUILD] CMake flags to support build-id in MSCCLPP

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>

* [BUILD] Fix CMake warnings in MSCCLPP build

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>

* Wrapped all cmake arguments passed to mscclpp to remove empty arguments and properly format them.

---------

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>
Co-authored-by: Corey Derochie <corey.derochie@amd.com>

* Link to libmscclpp_nccl statically (#1282)

* Switched mscclpp_nccl to static linking. Added a build step to rename the NCCL API functions.

* Undid separation of building libmscclpp_nccl from building librccl with MSCCL++ integration. With a static build, it's either fully enabled or fully disabled.

* `nm` isn't always available in docker containers due to being stripped down. Removed use of `nm` in `cmake` and hard-coded the output into mscclpp_nccl_syms.txt.

* Removed IBVerbs dependency for integrating with MSCCL++ (#1313)

* Renamed `RCCL_ENABLE_MSCCLPP` to `RCCL_MSCCLPP_ENABLE` to conform to MSCCL. Set `RCCL_MSCCLPP_ENABLE` to 1 by default if `ENABLE_MSCCLPP` is defined, or 0 otherwise. Added a log warning if `RCCL_MSCCLPP_ENABLE` is set to 1 but `ENABLE_MSCCLPP` is not defined. (#1294)

* Include mscclpp as a git submodule (#1314)

* Added the desired mscclpp commit as a git submodule.

* Added step to automatically checkout the mscclpp submodule if it isn't already present, in case the user forgot to clone recursively.

* Added instruction to README to clone using --recurse-submodules to get the mscclpp submodule.

* Enabled MSCCL++ feature build.

---------

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>
Co-authored-by: Nilesh M Negi <Nilesh.Negi@amd.com>
@corey-derochie-amd
corey-derochie-amd deleted the mscclpp-default-enable branch September 11, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

noCI Disable Jenkins for this PR. noExtendedCI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant