Let CMake handle NCCL detection instead of our handcrafted Python script. by xuhdev · Pull Request #22930 · pytorch/pytorch

xuhdev · 2019-07-16T21:32:06Z

How does the current code subsume all detections in the deleted nccl.py?

The dependency of USE_NCCL on the OS and USE_CUDA is handled as dependency options in CMakeLists.txt.
The main NCCL detection happens in FindNCCL.cmake, which is called by nccl.cmake. When USE_SYSTEM_NCCL is false, the previous Python code defer the detection to find_package(NCCL). The change in nccl.cmake retains this.
USE_STATIC_NCCL in the previous Python code simply changes the name of the detected library. This is done in IF (USE_STATIC_NCCL).
Now we only need to look at how the lines below line 20 in nccl.cmake are subsumed. These lines list paths to header and library directories that NCCL headers and libraries may reside in and try to search these directories for the key header and library files in turn. These are done by find_path for headers and find_library for the library files in FindNCCL.cmake.
- The call of find_path (Search for NO_DEFAULT_PATH in the link) by default searches for headers in <prefix>/include for each <prefix> in CMAKE_PREFIX_PATH and CMAKE_SYSTEM_PREFIX_PATH. Like the Python code, this commit sets CMAKE_PREFIX_PATH to search for <prefix> in NCCL_ROOT_DIR and home to CUDA. CMAKE_SYSTEM_PREFIX_PATH includes the standard directories such as /usr/local and /usr. NCCL_INCLUDE_DIR is also specifically handled.
- Similarly, the call of find_library (Search for NO_DEFAULT_PATH in the link) by default searches for libraries in directories including <prefix>/lib for each <prefix> in CMAKE_PREFIX_PATH and CMAKE_SYSTEM_PREFIX_PATH. But it also handles the edge cases intended to be solved in the Python code more properly:
  - It only searches for <prefix>/lib64 (and <prefix>/lib32) if it is appropriate on the system.
  - It only searches for <prefix>/lib/<arch> for the right <arch>, unlike the Python code searches for lib/<arch> in a generic way (e.g., the Python code searches for /usr/lib/x86_64-linux-gnu but in reality systems have /usr/lib/x86_64-some-customized-name-linux-gnu, see https://unix.stackexchange.com/a/226180/38242 ).

Regarding for relevant issues:

Add some more locations to search for nccl. #12063 and Fix detection of NCCL_INCLUDE_DIR #2877: These are properly handled, as explained in the updated comment.
Improve Windows Compatibility(for csrc/scripts) #2941 does not changes NCCL detection specifically for Windows (it changed CUDA detection).
b7e258f A versioned library detection is added, but the order is reversed: The unversioned library becomes preferred. This is because normally unversioned libraries are linked to versioned libraries and preferred by users, and local installation by users are often unversioned. Like the document of find_library suggests:

When using this to specify names with and without a version suffix, we recommend specifying the unversioned name first so that locally-built packages can be found before those provided by distributions.

xuhdev · 2019-07-16T21:33:25Z

Waiting for CI... This is identical to #22818

xuhdev · 2019-07-16T22:46:11Z

@ezyang This PR works quite well on PR CIs, but fails on those that are only run on master. Is there a way to see what's happening in those environments?

ezyang · 2019-07-19T20:05:30Z

@xuhdev There is only a janky way, which is to edit .circleci so that it adds the other environments, and then push a temporary commit for the testing. Go to .circleci/cimodel/data/pytorch_build_data.py, edit the configs you want to be XImportant, and then run .circleci/regenerate.sh

…ipt. --- How does the current code subsume all detections in the deleted `nccl.py`? - The dependency of `USE_NCCL` on the OS and `USE_CUDA` is handled as dependency options in `CMakeLists.txt`. - The main NCCL detection happens in [FindNCCL.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/Modules/FindNCCL.cmake), which is called by [nccl.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/External/nccl.cmake). When `USE_SYSTEM_NCCL` is false, the previous Python code defer the detection to `find_package(NCCL)`. The change in `nccl.cmake` retains this. - `USE_STATIC_NCCL` in the previous Python code simply changes the name of the detected library. This is done in `IF (USE_STATIC_NCCL)`. - Now we only need to look at how the lines below line 20 in `nccl.cmake` are subsumed. These lines list paths to header and library directories that NCCL headers and libraries may reside in and try to search these directories for the key header and library files in turn. These are done by `find_path` for headers and `find_library` for the library files in `FindNCCL.cmake`. * The call of [find_path](https://cmake.org/cmake/help/v3.8/command/find_path.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for headers in `<prefix>/include` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. Like the Python code, this commit sets `CMAKE_PREFIX_PATH` to search for `<prefix>` in `NCCL_ROOT_DIR` and home to CUDA. `CMAKE_SYSTEM_PREFIX_PATH` includes the standard directories such as `/usr/local` and `/usr`. `NCCL_INCLUDE_DIR` is also specifically handled. * Similarly, the call of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for libraries in directories including `<prefix>/lib` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. But it also handles the edge cases intended to be solved in the Python code more properly: - It only searches for `<prefix>/lib64` (and `<prefix>/lib32`) if it is appropriate on the system. - It only searches for `<prefix>/lib/<arch>` for the right `<arch>`, unlike the Python code searches for `lib/<arch>` in a generic way (e.g., the Python code searches for `/usr/lib/x86_64-linux-gnu` but in reality systems have `/usr/lib/x86_64-some-customized-name-linux-gnu`, see https://unix.stackexchange.com/a/226180/38242 ). --- Regarding for relevant issues: - pytorch#12063 and pytorch#2877: These are properly handled, as explained in the updated comment. - pytorch#2941 does not changes NCCL detection specifically for Windows (it changed CUDA detection). - b7e258f A versioned library detection is added, but the order is reversed: The unversioned library becomes preferred. This is because normally unversioned libraries are linked to versioned libraries and preferred by users, and local installation by users are often unversioned. Like the document of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) suggests: > When using this to specify names with and without a version suffix, we recommend specifying the unversioned name first so that locally-built packages can be found before those provided by distributions.

[ci all]

xuhdev · 2019-07-23T03:34:53Z

@ezyang The PR should be ready now. The error before was caused by the default value of USE_SYSTEM_NCCL -- it was 0 but I changed it to 1 (The Docker image contains no system NCCL). Feel free to review when you have time!

xuhdev · 2019-07-23T08:11:53Z

There are currently two commits in this PR. The first one is the real content and the second one is CI tweak.

ezyang · 2019-07-23T14:57:48Z

This LGTM. I'll go ahead and remove the CI twiddle and land.

This reverts commit ceb772a.

facebook-github-bot

@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2019-07-23T16:06:53Z

@ezyang merged this pull request in 60c46dd.

…ipt. (#22930) Summary: --- How does the current code subsume all detections in the deleted `nccl.py`? - The dependency of `USE_NCCL` on the OS and `USE_CUDA` is handled as dependency options in `CMakeLists.txt`. - The main NCCL detection happens in [FindNCCL.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/Modules/FindNCCL.cmake), which is called by [nccl.cmake](https://github.com/pytorch/pytorch/blob/8377d4b32c12206a0f9401e81a5e5796c8fc01a8/cmake/External/nccl.cmake). When `USE_SYSTEM_NCCL` is false, the previous Python code defer the detection to `find_package(NCCL)`. The change in `nccl.cmake` retains this. - `USE_STATIC_NCCL` in the previous Python code simply changes the name of the detected library. This is done in `IF (USE_STATIC_NCCL)`. - Now we only need to look at how the lines below line 20 in `nccl.cmake` are subsumed. These lines list paths to header and library directories that NCCL headers and libraries may reside in and try to search these directories for the key header and library files in turn. These are done by `find_path` for headers and `find_library` for the library files in `FindNCCL.cmake`. * The call of [find_path](https://cmake.org/cmake/help/v3.8/command/find_path.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for headers in `<prefix>/include` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. Like the Python code, this commit sets `CMAKE_PREFIX_PATH` to search for `<prefix>` in `NCCL_ROOT_DIR` and home to CUDA. `CMAKE_SYSTEM_PREFIX_PATH` includes the standard directories such as `/usr/local` and `/usr`. `NCCL_INCLUDE_DIR` is also specifically handled. * Similarly, the call of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) (Search for `NO_DEFAULT_PATH` in the link) by default searches for libraries in directories including `<prefix>/lib` for each `<prefix>` in `CMAKE_PREFIX_PATH` and `CMAKE_SYSTEM_PREFIX_PATH`. But it also handles the edge cases intended to be solved in the Python code more properly: - It only searches for `<prefix>/lib64` (and `<prefix>/lib32`) if it is appropriate on the system. - It only searches for `<prefix>/lib/<arch>` for the right `<arch>`, unlike the Python code searches for `lib/<arch>` in a generic way (e.g., the Python code searches for `/usr/lib/x86_64-linux-gnu` but in reality systems have `/usr/lib/x86_64-some-customized-name-linux-gnu`, see https://unix.stackexchange.com/a/226180/38242 ). --- Regarding for relevant issues: - pytorch/pytorch#12063 and pytorch/pytorch#2877: These are properly handled, as explained in the updated comment. - pytorch/pytorch#2941 does not changes NCCL detection specifically for Windows (it changed CUDA detection). - b7e258f81ef61d19b884194cdbcd6c7089636d46 A versioned library detection is added, but the order is reversed: The unversioned library becomes preferred. This is because normally unversioned libraries are linked to versioned libraries and preferred by users, and local installation by users are often unversioned. Like the document of [find_library](https://cmake.org/cmake/help/v3.8/command/find_library.html) suggests: > When using this to specify names with and without a version suffix, we recommend specifying the unversioned name first so that locally-built packages can be found before those provided by distributions. Pull Request resolved: pytorch/pytorch#22930 Differential Revision: D16440275 Pulled By: ezyang fbshipit-source-id: 11fe80743d4fe89b1ed6f96d5d996496e8ec01aa

jithunnair-amd · 2019-08-01T19:18:42Z

@xuhdev Do I interpret the below change correctly to mean that USE_SYSTEM_NCCL will always be set to OFF by cmake, unless the user explicitly sets it to ON during the build using the USE_SYSTEM_NCCL=1 env variable?

cmake_dependent_option(
    USE_SYSTEM_NCCL "Use system-wide NCCL" OFF
    "USE_NCCL" OFF)

xuhdev · 2019-08-01T20:01:20Z

@jithunnair-amd What you said is right, but both for before and after this change. The changed lines you have quoted also make sure that the option USE_SYSTEM_NCCL does not show up if USE_NCCL is OFF (e.g., in cmake-gui, ccmake, etc.)

jithunnair-amd · 2019-08-02T16:41:52Z

@xuhdev Need some cmake expertise: in the file CMakeLists.txt, I need to ensure that USE_NCCL can be set for either CUDA or ROCM builds (for which the option will enable the RCCL library instead). How do I change the below to enable that?

cmake_dependent_option(
    USE_NCCL "Use NCCL" ON
"USE_CUDA;UNIX;NOT APPLE" OFF)

xuhdev · 2019-08-02T16:56:52Z

@jithunnair-amd The cleaning way I think is to add a USE_GPU variable, because USE_CUDA OR USE_ROCM and its equivalent seem being used a lot:

if (USE_CUDA OR USE_ROCM)
  set(USE_GPU TRUE)
else()
  set(USE_GPU FALSE)
endif()

Then replace USE_CUDA in that statement with USE_GPU.

pytorchbot added module: build Build system issues module: nccl Problems related to nccl support labels Jul 16, 2019

xuhdev changed the title ~~Let CMake handle NCCL detection instead of our handcrafted Python script.~~ [WIP] Let CMake handle NCCL detection instead of our handcrafted Python script. Jul 16, 2019

ezyang added the open source label Jul 16, 2019

xuhdev changed the title ~~[WIP] Let CMake handle NCCL detection instead of our handcrafted Python script.~~ Let CMake handle NCCL detection instead of our handcrafted Python script. Jul 18, 2019

xuhdev mentioned this pull request Jul 18, 2019

Let CMake handle NCCL detection instead of our handcrafted Python script. #22818

Closed

xuhdev requested a review from ezyang July 19, 2019 17:51

cpuhrsch added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 19, 2019

xuhdev force-pushed the build/detect-nccl branch from 8c162a3 to 7ac63e7 Compare July 22, 2019 20:32

pytorchbot added the module: ci Related to continuous integration label Jul 22, 2019

xuhdev force-pushed the build/detect-nccl branch 7 times, most recently from d998fda to f9ae21f Compare July 22, 2019 23:22

xuhdev added 2 commits July 22, 2019 20:33

Enable more tests on CI.

ceb772a

[ci all]

xuhdev force-pushed the build/detect-nccl branch from f9ae21f to ceb772a Compare July 23, 2019 03:33

Revert "Enable more tests on CI."

09a7a0a

This reverts commit ceb772a.

facebook-github-bot reviewed Jul 23, 2019

View reviewed changes

facebook-github-bot closed this in 60c46dd Jul 23, 2019

facebook-github-bot added the merged label Jul 23, 2019

xuhdev deleted the build/detect-nccl branch July 23, 2019 17:09

jithunnair-amd mentioned this pull request Aug 1, 2019

Changes to build files needed to enable NCCL path for PyTorch ROCm builds #22460

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let CMake handle NCCL detection instead of our handcrafted Python script.#22930

Let CMake handle NCCL detection instead of our handcrafted Python script.#22930
xuhdev wants to merge 3 commits intopytorch:masterfrom
xuhdev:build/detect-nccl

xuhdev commented Jul 16, 2019

Uh oh!

xuhdev commented Jul 16, 2019 •

edited

Loading

Uh oh!

xuhdev commented Jul 16, 2019

Uh oh!

ezyang commented Jul 19, 2019

Uh oh!

xuhdev commented Jul 23, 2019

Uh oh!

xuhdev commented Jul 23, 2019

Uh oh!

ezyang commented Jul 23, 2019

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot commented Jul 23, 2019

Uh oh!

jithunnair-amd commented Aug 1, 2019

Uh oh!

xuhdev commented Aug 1, 2019

Uh oh!

jithunnair-amd commented Aug 2, 2019

Uh oh!

xuhdev commented Aug 2, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

xuhdev commented Jul 16, 2019

Uh oh!

xuhdev commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xuhdev commented Jul 16, 2019

Uh oh!

ezyang commented Jul 19, 2019

Uh oh!

xuhdev commented Jul 23, 2019

Uh oh!

xuhdev commented Jul 23, 2019

Uh oh!

ezyang commented Jul 23, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jul 23, 2019

Uh oh!

jithunnair-amd commented Aug 1, 2019

Uh oh!

xuhdev commented Aug 1, 2019

Uh oh!

jithunnair-amd commented Aug 2, 2019

Uh oh!

xuhdev commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

xuhdev commented Jul 16, 2019 •

edited

Loading

xuhdev commented Aug 2, 2019 •

edited

Loading