[CI] Minor CMake refactoring#3227
Conversation
|
/te-ci |
1 similar comment
|
/te-ci |
NCCL is now always required for all native build paths (due to a change sometime between v2.16 and main). This updates CMake to only search for the headers in a single place. This does not impact the NCCL EP dependency resolution. Signed-off-by: Fred Heinecke <fheinecke@nvidia.com>
This will now also search Python's site-packages, making it much easier
to pull in headers from `nvidia-nccl-cu{12,13}`.
Signed-off-by: Fred Heinecke <fheinecke@nvidia.com>
Previously this was provided every time the C++ or CUDA compiler was invoked for everything built by CMake, including compiler detection and compilation, instead of just when linking the TE library. Signed-off-by: Fred Heinecke <fheinecke@nvidia.com>
30d92a7 to
eef16c6
Compare
Greptile SummaryThis PR performs three targeted CMake improvements in
Confidence Score: 5/5Safe to merge. All three changes are scoped, correct, and reduce unintended side-effects of the previous implementation. The linker-script fix removes a real CMake anti-pattern where a link-only flag was injected into every compiler invocation project-wide. The NCCL consolidation is functionally equivalent for the existing call sites and adds a useful new search path. No existing override mechanisms (e.g. -DNCCL_INCLUDE_DIR) are broken. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CMake configure start] --> B[find_path NCCL_INCLUDE_DIR\nHINTS: Python site-packages,\n/opt/nvidia/nccl, /usr/local/nccl\nREQUIRED]
B -->|found| C[get_nccl_version\nNCCL_VERSION]
B -->|not found| FAIL1[FATAL_ERROR]
C --> D[add_library transformer_engine SHARED]
D --> E[target_link_options PRIVATE\nLINKER:--version-script=\nlibtransformer_engine.version]
D --> F[target_include_directories PRIVATE\nNCCL_INCLUDE_DIR]
G{NVTE_WITH_CUBLASMP?} -->|ON| H[Check NCCL_VERSION >= 2.30.0\nusing already-resolved NCCL_INCLUDE_DIR]
G -->|OFF| I[skip]
J{NVTE_WITH_NCCL_EP?} -->|ON| K[find NCCL_EP headers & lib\ntarget_include_directories NCCL_EP_INCLUDE_DIR]
J -->|OFF| L[compile stub ep_api.cpp]
Reviews (1): Last reviewed commit: "Make TE linker script be provided when l..." | Re-trigger Greptile |
Description
This fixes a couple of minor CMake bugs, and slightly simplifies NCCL dependency resolution.
Type of change
Changes
Please list the changes introduced in this PR:
site-packages/nvidia/ncclis now searched for NCCL headers, making it easier to resolve them when installing NCCL vianvidia-nccl-cu{12,13}wheels.Checklist: