Use sccache-dist build cluster for conda and wheel builds#848
Open
trxcllnt wants to merge 15 commits intoNVIDIA:mainfrom
Open
Use sccache-dist build cluster for conda and wheel builds#848trxcllnt wants to merge 15 commits intoNVIDIA:mainfrom
sccache-dist build cluster for conda and wheel builds#848trxcllnt wants to merge 15 commits intoNVIDIA:mainfrom
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:
📝 WalkthroughWalkthroughAdds compile_commands.json to .gitignore; expands sccache-related secrets and environment defaults in conda recipes; updates CMake to add argparse and CUDA options and link CUDA runtime in a test; and fixes a printf format specifier for a size_t seed in cuts.cpp. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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/tests/CMakeLists.txt`:
- Around line 40-47: The CMake snippet sets INTERFACE_POSITION_INDEPENDENT_CODE
on the test target ${CMAKE_TEST_NAME}, which is a transitive property and
ineffective for executables; change the property to POSITION_INDEPENDENT_CODE so
the test target itself is built with PIC. Locate the set_target_properties call
for ${CMAKE_TEST_NAME} and replace INTERFACE_POSITION_INDEPENDENT_CODE with
POSITION_INDEPENDENT_CODE (keeping the other properties like CXX_STANDARD and
CXX_STANDARD_REQUIRED unchanged).
…ACE_POSITION_INDEPENDENT_CODE
trxcllnt
added
non-breaking
trxcllnt
requested review from
rgsl888prabhu
and removed request for
msarahan
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/CMakeLists.txt (1)
520-528:⚠️ Potential issue | 🟠 MajorUse
argparse::argparseinstead of bareargparsefor CMake target linking.When
rapids_cpm_findresolves argparse viafind_package(e.g., system-installed in a conda environment), the CMake config exports only the namespacedargparse::argparsetarget. The bareargparsetarget would be unavailable, causing link failures. Usingargparse::argparseis compatible with both paths: the CPM-fetched source (which creates both the target and alias) and pre-installed packages (which export the namespaced target).♻️ Proposed fix
target_link_libraries(cuopt_cli PUBLIC cuopt OpenMP::OpenMP_CXX ${CUDSS_LIBRARIES} TBB::tbb PRIVATE - argparse + argparse::argparse )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` around lines 520 - 528, Change the CMake link target for argparse to use the namespaced target so linking works with both CPM-fetched and system-installed packages: in the target_link_libraries call for cuopt_cli, replace the bare argparse entry with argparse::argparse (i.e., update the target_line that currently lists argparse to argparse::argparse) so CMake links the correct exported target when find_package provides only the namespaced target.
🧹 Nitpick comments (1)
cpp/CMakeLists.txt (1)
256-263: Consider guarding theargparsefetch within theNOT BUILD_LP_ONLYblock.
argparseis only consumed bycuopt_cli, which is compiled only whenNOT BUILD_LP_ONLY. The unconditionalrapids_cpm_findwill fetch/resolve argparse even in LP-only builds where it is never used.♻️ Suggested refactor
Move the
rapids_cpm_find(argparse ...)block inside the existing guard (around line 495):-rapids_cpm_find( - argparse 3.2.0 - CPM_ARGS - GIT_REPOSITORY https://github.com/p-ranav/argparse.git - GIT_TAG v3.2 - GIT_SHALLOW TRUE - OPTIONS "ARGPARSE_INSTALL TRUE" -) - find_package(CUDSS REQUIRED) ... if(NOT BUILD_LP_ONLY) + rapids_cpm_find( + argparse 3.2.0 + CPM_ARGS + GIT_REPOSITORY https://github.com/p-ranav/argparse.git + GIT_TAG v3.2 + GIT_SHALLOW TRUE + OPTIONS "ARGPARSE_INSTALL TRUE" + ) add_executable(cuopt_cli cuopt_cli.cpp) ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/CMakeLists.txt` around lines 256 - 263, The rapids_cpm_find call for the argparse dependency is unconditional but argparse is only used by cuopt_cli which is built when NOT BUILD_LP_ONLY; move the rapids_cpm_find(argparse 3.2.0 ...) block into the existing conditional that guards the cuopt_cli target (the NOT BUILD_LP_ONLY branch) so argparse is only fetched for builds that actually compile cuopt_cli (update the block that checks BUILD_LP_ONLY accordingly and keep the existing CPM_ARGS/GIT_REPOSITORY/GIT_TAG/OPTIONS fields intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/cuts/cuts.cpp`:
- Line 2749: Replace the non-portable printf format specifiers for the size_t
variable seed: in the printf call printing the solution hash (currently using
"%20lx") change it to use "%20zx" (portable size_t specifier), and also update
the matching specifier inside read_saved_solution_for_cut_verification (which
currently uses "%20x" for size_t seed) to "%20zx"; ensure both uses reference
the same variable name seed and compile to verify no warnings remain.
In `@cpp/tests/linear_programming/CMakeLists.txt`:
- Around line 33-36: The target_link_libraries for the static target
c_api_tester is currently linking the entire CUOPT_PRIVATE_CUDA_LIBS set; change
it so c_api_tester links only CUDA::cudart because c_api_test.c (which includes
cuopt_c.h and cuda_runtime.h) does not directly use cuBLAS-Lt, cuRAND, cuSOLVER,
TBB, or OpenMP—those are provided by the cuopt shared library. Update the
target_link_libraries invocation for c_api_tester to reference only CUDA::cudart
instead of ${CUOPT_PRIVATE_CUDA_LIBS}, leaving the cuopt shared library to keep
the full private link set.
---
Outside diff comments:
In `@cpp/CMakeLists.txt`:
- Around line 520-528: Change the CMake link target for argparse to use the
namespaced target so linking works with both CPM-fetched and system-installed
packages: in the target_link_libraries call for cuopt_cli, replace the bare
argparse entry with argparse::argparse (i.e., update the target_line that
currently lists argparse to argparse::argparse) so CMake links the correct
exported target when find_package provides only the namespaced target.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 256-263: The rapids_cpm_find call for the argparse dependency is
unconditional but argparse is only used by cuopt_cli which is built when NOT
BUILD_LP_ONLY; move the rapids_cpm_find(argparse 3.2.0 ...) block into the
existing conditional that guards the cuopt_cli target (the NOT BUILD_LP_ONLY
branch) so argparse is only fetched for builds that actually compile cuopt_cli
(update the block that checks BUILD_LP_ONLY accordingly and keep the existing
CPM_ARGS/GIT_REPOSITORY/GIT_TAG/OPTIONS fields intact).
jameslamb
approved these changes
Feb 20, 2026
Member
jameslamb
left a comment
jameslamb
left a comment
There was a problem hiding this comment.
Looks great, yes we totally should do this.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
RAPIDS has deployed an autoscaling cloud build cluster that can be used to accelerate building large RAPIDS projects.
This PR updates the conda and wheel builds to use the build cluster.
Summary by CodeRabbit
Chores
Bug Fixes