[#11312][chore] Make TRT/NCCL configurable in CMake find modules#11528
[#11312][chore] Make TRT/NCCL configurable in CMake find modules#11528xuantengh wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Signed-off-by: Xuanteng Huang <xuanteng.huang@outlook.com>
📝 WalkthroughWalkthroughThe pull request broadens library discovery for NCCL and TensorRT CMake find modules by expanding search paths and adding support for versioned library names, enabling more flexible installation layouts. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cpp/cmake/modules/FindNCCL.cmake`:
- Around line 18-21: The find_path call that sets NCCL_INCLUDE_DIR must also
search the custom NCCL_ROOT like the find_library call does; update the
find_path invocation (the call named find_path that produces NCCL_INCLUDE_DIR)
to include PATHS ${NCCL_ROOT} so headers (e.g., nccl.h) are resolvable when
--nccl_root is provided, matching the behavior of the existing find_library for
NCCL_LIBRARY.
🧹 Nitpick comments (1)
cpp/cmake/modules/FindNCCL.cmake (1)
20-20: Minor: versioned library name depends onNCCL_FIND_VERSION_MAJORbeing set.If
find_package(NCCL)is called without a version argument,NCCL_FIND_VERSION_MAJORwill be empty and the name expands to"libnccl.so.", which won't match anything. This is harmless (the plainncclname will still match), but worth being aware of. Consider guarding it:Suggested guard
- NAMES nccl "libnccl.so.${NCCL_FIND_VERSION_MAJOR}" + NAMES nccl $<$<BOOL:${NCCL_FIND_VERSION_MAJOR}>:libnccl.so.${NCCL_FIND_VERSION_MAJOR}>Or more idiomatically in CMake:
set(_nccl_names nccl) if(NCCL_FIND_VERSION_MAJOR) list(APPEND _nccl_names "libnccl.so.${NCCL_FIND_VERSION_MAJOR}") endif() find_library( NCCL_LIBRARY NAMES ${_nccl_names} PATHS ${NCCL_ROOT})
| find_library( | ||
| NCCL_LIBRARY | ||
| NAMES nccl "libnccl.so.${NCCL_FIND_VERSION_MAJOR}" | ||
| PATHS ${NCCL_ROOT}) |
There was a problem hiding this comment.
find_path for NCCL_INCLUDE_DIR (line 33) is missing NCCL_ROOT—headers won't be found from custom paths.
The find_library call now correctly searches ${NCCL_ROOT}, but the find_path on line 33 was not updated to match. When --nccl_root is set to a custom location, CMake will find the library but fail to locate nccl.h, causing the overall find to fail (since NCCL_INCLUDE_DIR is a REQUIRED_VARS on line 62).
Proposed fix
-find_path(NCCL_INCLUDE_DIR NAMES nccl.h)
+find_path(NCCL_INCLUDE_DIR NAMES nccl.h PATHS ${NCCL_ROOT}/include)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| find_library( | |
| NCCL_LIBRARY | |
| NAMES nccl "libnccl.so.${NCCL_FIND_VERSION_MAJOR}" | |
| PATHS ${NCCL_ROOT}) | |
| find_library( | |
| NCCL_LIBRARY | |
| NAMES nccl "libnccl.so.${NCCL_FIND_VERSION_MAJOR}" | |
| PATHS ${NCCL_ROOT}) | |
| find_path(NCCL_INCLUDE_DIR NAMES nccl.h PATHS ${NCCL_ROOT}/include) |
🤖 Prompt for AI Agents
In `@cpp/cmake/modules/FindNCCL.cmake` around lines 18 - 21, The find_path call
that sets NCCL_INCLUDE_DIR must also search the custom NCCL_ROOT like the
find_library call does; update the find_path invocation (the call named
find_path that produces NCCL_INCLUDE_DIR) to include PATHS ${NCCL_ROOT} so
headers (e.g., nccl.h) are resolvable when --nccl_root is provided, matching the
behavior of the existing find_library for NCCL_LIBRARY.
There was a problem hiding this comment.
On a related note, I think it's unlikely that the un-versioned "libnccl.so" symlink is missing while the NCCL headers are present. If the header files and the library come from different versions of NCCL, the version skew may cause some unexpected linking issues. Perhaps adding "libnccl.so.${NCCL_FIND_VERSION_MAJOR}" to the find_library isn't necessary? If you feel it is necessary, can you please explain how you obtain this combination of header and library files?
This comment (but not coderabbit's comment) also applies to TensorRT.
There was a problem hiding this comment.
@tburt-nv, you raise a valid point. In standard NCCL and TensorRT installations, the unversioned symlink (e.g., libnccl.so → libnccl.so.2) is always present alongside the headers as part of the development package. Adding the versioned library name to the search seems orthogonal to the PR's main objective, which is to respect custom installation paths via --nccl_root and --trt_root.
The addition of PATHS ${NCCL_ROOT} is clearly necessary to make the custom paths work, but the versioned library names (libnccl.so.${NCCL_FIND_VERSION_MAJOR}) appear to address a different scenario that isn't explained in the PR description.
@xuantengh, could you clarify the use case where:
- The versioned library file exists (e.g.,
libnccl.so.2) - But the unversioned symlink (
libnccl.so) is missing - Yet headers are still present
Without understanding this scenario, it's difficult to assess whether the versioned library name search is necessary or if it might inadvertently introduce version skew issues as tburt-nv mentioned. The same question applies to your TensorRT changes in FindTensorRT.cmake.
🧠 Learnings used
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 7763
File: cpp/tensorrt_llm/CMakeLists.txt:297-301
Timestamp: 2025-09-16T09:30:09.716Z
Learning: In the TensorRT-LLM project, NCCL libraries are loaded earlier by PyTorch libraries or the bindings library, so the main shared library doesn't need NCCL paths in its RPATH - the libraries will already be available in the process address space when needed.
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/thop/allreduceOp.cpp:352-446
Timestamp: 2025-09-23T15:12:38.312Z
Learning: In TensorRT-LLM NCCL device implementation, NCCL version 2.28+ requirements are handled at runtime in the nccl_device/config layer rather than with compile-time guards. This allows the allreduceOp to remain version-agnostic and delegates version compatibility validation to the appropriate lower-level components that can gracefully handle unsupported configurations.
Learnt from: nv-lschneider
Repo: NVIDIA/TensorRT-LLM PR: 7910
File: cpp/tensorrt_llm/kernels/nccl_device/config.cu:15-17
Timestamp: 2025-09-23T15:01:00.070Z
Learning: In TensorRT-LLM NCCL device kernels, the <sstream> header is not needed as an explicit include in config.cu because it's provided transitively through other headers. Local compilation testing confirms this works without the explicit include.
Learnt from: tongyuantongyu
Repo: NVIDIA/TensorRT-LLM PR: 6984
File: cpp/tensorrt_llm/CMakeLists.txt:297-299
Timestamp: 2025-08-18T09:08:07.687Z
Learning: In the TensorRT-LLM project, artifacts are manually copied rather than installed via `cmake --install`, so INSTALL_RPATH properties are not needed - only BUILD_RPATH affects the final artifacts.
svc-trtllm-gh-bot
added
the
Community want to contribute
| PATHS ${TensorRT_WELL_KNOWN_ROOT}/include ${TensorRT_ROOT}/include | ||
| ${TensorRT_ROOT}/include/tensorrt) |
There was a problem hiding this comment.
I think CMake will still select ${TensorRT_WELL_KNOWN_ROOT}/include if it's present, since it comes first in these lists. Shall we swap the order so that ${TensorRT_ROOT}/include and ${TensorRT_ROOT}/include/tensorrt take priority when the --trt_root argument is given? I think the same applies to all PATHS modifications in this patch.
| NAMES "nvonnxparser_${TensorRT_VERSION_MAJOR}" | ||
| "libnvonnxparser.so.${TensorRT_VERSION_MAJOR}" nvonnxparser |
There was a problem hiding this comment.
In this case, we should prioritize the un-versioned library when available.
| NAMES "nvonnxparser_${TensorRT_VERSION_MAJOR}" | |
| "libnvonnxparser.so.${TensorRT_VERSION_MAJOR}" nvonnxparser | |
| NAMES "nvonnxparser_${TensorRT_VERSION_MAJOR}" | |
| nvonnxparser "libnvonnxparser.so.${TensorRT_VERSION_MAJOR}" |
3 participants
Summary by CodeRabbit
Description
Currently,
scripts/build_wheel.pyoffers options to configure the custom installation path for NCCL/TRT, but the correspondingcpp/cmake/modules/*.cmakemodules don't respect the setting. This PR modifies thefind_library/find_pathin those CMake modules to make it respect the CMake options frombuild_wheel.py.Test Coverage
By default, nothing changes if
--trt_rootnor--nccl_rootis not specified inscripts/build_wheel.py. If a user manually sets them when invoking the build script, CMake build system will use the specified path to find the necessary libraries/headers.PR Checklist
GitHub Bot Help
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
Details
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.