Fix CPU-only build by skipping CUDA, rmm, raft deps#146
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCMake build files now gate all CUDA initialization, runtime setup, third-party dependencies, and GPU-specific target properties behind the ChangesConditional CUDA build support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/CMakeLists.txt (1)
318-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: CUDA-specific linker script applied in CPU-only builds
Issue:
fatbin.ldcontains CUDA-specific ELF sections (.nvFatBinSegment, .nv_fatbin) but is unconditionally passed to the linker.
Why: May cause linker warnings or errors in CPU-only builds when these sections are not recognized.
Impact: Potential build failure or linker noise in CPU-only mode.Consider making this conditional:
if(NVFOREST_ENABLE_GPU) target_link_options(${NVFOREST_CPP_TARGET} PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/fatbin.ld") endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/CMakeLists.txt` at line 318, The CMake target_link_options call unconditionally adds the CUDA-specific linker script fatbin.ld to ${NVFOREST_CPP_TARGET}, which can cause linker warnings/failures in CPU-only builds; wrap the target_link_options("${CMAKE_CURRENT_BINARY_DIR}/fatbin.ld") invocation in a conditional that checks the NVFOREST_ENABLE_GPU option (e.g., only add the fatbin.ld link option when NVFOREST_ENABLE_GPU is true) so the CUDA-specific linker script is applied only for GPU-enabled builds.cpp/tests/CMakeLists.txt (1)
86-86:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: CUDA test built unconditionally in CPU-only mode
Issue:
DEVICE_BUFFER_TESTincludesbuffer.cusource file but is not gated onNVFOREST_ENABLE_GPU.
Why: In CPU-only builds, CUDA language is not enabled, so CMake cannot compile.cufiles.
Impact: Build failure with "No known features for CUDA compiler" or similar error whenNVFOREST_ENABLE_GPU=OFF.Suggested fix:
ConfigureTest(NAME HOST_BUFFER_TEST raft_proto/buffer.cpp) if(NVFOREST_ENABLE_GPU) ConfigureTest(NAME DEVICE_BUFFER_TEST raft_proto/buffer.cu) endif() ConfigureTest(NAME FOREST_TRAVERSAL_TEST forest/traversal_forest.cpp)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tests/CMakeLists.txt` at line 86, The ConfigureTest invocation for DEVICE_BUFFER_TEST is adding a .cu source unconditionally, which breaks CPU-only builds; update the CMake so that ConfigureTest(NAME DEVICE_BUFFER_TEST raft_proto/buffer.cu) is wrapped in an NVFOREST_ENABLE_GPU guard (use the NVFOREST_ENABLE_GPU variable) and add a CPU fallback (e.g., a HOST_BUFFER_TEST using raft_proto/buffer.cpp) so CUDA sources are only configured when NVFOREST_ENABLE_GPU is ON; adjust nearby ConfigureTest calls (e.g., FOREST_TRAVERSAL_TEST) accordingly to keep test ordering/coverage.
🧹 Nitpick comments (1)
cpp/CMakeLists.txt (1)
171-175: ⚡ Quick winMEDIUM: CMAKE_CUDA_COMPILER_LAUNCHER set without CUDA language guard
Issue: Line 174 sets
CMAKE_CUDA_COMPILER_LAUNCHEReven when CUDA language may not be enabled (CPU-only builds).
Why: Inconsistent with the PR's goal to fully gate CUDA configuration; may cause warnings depending on CMake version.Consider wrapping in
NVFOREST_ENABLE_GPUguard for consistency:if(USE_CCACHE) set(CMAKE_C_COMPILER_LAUNCHER ccache) set(CMAKE_CXX_COMPILER_LAUNCHER ccache) if(NVFOREST_ENABLE_GPU) set(CMAKE_CUDA_COMPILER_LAUNCHER ccache) endif() endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/CMakeLists.txt` around lines 171 - 175, The current USE_CCACHE block sets CMAKE_CUDA_COMPILER_LAUNCHER unconditionally which can trigger warnings for CPU-only builds; update the block so that CMAKE_CUDA_COMPILER_LAUNCHER is only set when the CUDA/GPU feature is enabled by guarding it with NVFOREST_ENABLE_GPU. Locate the USE_CCACHE conditional and keep setting CMAKE_C_COMPILER_LAUNCHER and CMAKE_CXX_COMPILER_LAUNCHER as-is, but wrap the CMAKE_CUDA_COMPILER_LAUNCHER assignment in an if(NVFOREST_ENABLE_GPU) ... endif() so the CUDA launcher is only configured when NVFOREST_ENABLE_GPU is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/CMakeLists.txt`:
- Line 318: The CMake target_link_options call unconditionally adds the
CUDA-specific linker script fatbin.ld to ${NVFOREST_CPP_TARGET}, which can cause
linker warnings/failures in CPU-only builds; wrap the
target_link_options("${CMAKE_CURRENT_BINARY_DIR}/fatbin.ld") invocation in a
conditional that checks the NVFOREST_ENABLE_GPU option (e.g., only add the
fatbin.ld link option when NVFOREST_ENABLE_GPU is true) so the CUDA-specific
linker script is applied only for GPU-enabled builds.
In `@cpp/tests/CMakeLists.txt`:
- Line 86: The ConfigureTest invocation for DEVICE_BUFFER_TEST is adding a .cu
source unconditionally, which breaks CPU-only builds; update the CMake so that
ConfigureTest(NAME DEVICE_BUFFER_TEST raft_proto/buffer.cu) is wrapped in an
NVFOREST_ENABLE_GPU guard (use the NVFOREST_ENABLE_GPU variable) and add a CPU
fallback (e.g., a HOST_BUFFER_TEST using raft_proto/buffer.cpp) so CUDA sources
are only configured when NVFOREST_ENABLE_GPU is ON; adjust nearby ConfigureTest
calls (e.g., FOREST_TRAVERSAL_TEST) accordingly to keep test ordering/coverage.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 171-175: The current USE_CCACHE block sets
CMAKE_CUDA_COMPILER_LAUNCHER unconditionally which can trigger warnings for
CPU-only builds; update the block so that CMAKE_CUDA_COMPILER_LAUNCHER is only
set when the CUDA/GPU feature is enabled by guarding it with
NVFOREST_ENABLE_GPU. Locate the USE_CCACHE conditional and keep setting
CMAKE_C_COMPILER_LAUNCHER and CMAKE_CXX_COMPILER_LAUNCHER as-is, but wrap the
CMAKE_CUDA_COMPILER_LAUNCHER assignment in an if(NVFOREST_ENABLE_GPU) ...
endif() so the CUDA launcher is only configured when NVFOREST_ENABLE_GPU is
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2722a09a-162b-48c2-878a-7c01e8a3639f
📒 Files selected for processing (2)
cpp/CMakeLists.txtcpp/tests/CMakeLists.txt
|
@coderabbitai review |
✅ Action performedReview finished.
|
Closes #145
nvForest uses RMM and RAFT as dependencies, but they are not needed when building nvForest in the CPU-only mode.