build(cmake): split libcuopt into cuopt_base / cuopt_routing / cuopt_lp component libs - #1622
Draft
ramakrishnap-nv wants to merge 10 commits into
Draft
build(cmake): split libcuopt into cuopt_base / cuopt_routing / cuopt_lp component libs#1622ramakrishnap-nv wants to merge 10 commits into
ramakrishnap-nv wants to merge 10 commits into
Conversation
…lp + umbrella Introduce three STATIC component libraries that logically partition the cuOpt sources by domain, then fold them into the existing libcuopt.so umbrella via --whole-archive (LINK_LIBRARY:WHOLE_ARCHIVE). Component libraries: - cuopt_base — utilities + linear algebra (logger, work scheduler) - cuopt_routing — VRP / routing engine; links cuopt_base - cuopt_lp — LP / MIP / numerical optimization; links cuopt_base Umbrella: - cuopt SHARED — re-exports all symbols from the three statics via --whole-archive; backward-compatible for GAMS (-lcuopt / libcuopt.so) CMake aliases exposed: cuopt::base, cuopt::routing, cuopt::lp, cuopt::cuopt No source files moved. External build output (libcuopt.so, headers, install layout) is unchanged. SKIP_ROUTING_BUILD=ON continues to work by omitting cuopt_routing from the build and umbrella link. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Four issues found while validating the library split locally: - cuopt_routing was missing OpenMP::OpenMP_CUDA, causing routing CUDA files to reject #pragma omp directives as unknown in CUDA compiler mode - cuopt_lp was missing simde::simde, required by the fast MPS parser (io/experimental_mps_fast/) which uses SIMD intrinsics via simde headers - The umbrella cuopt target was missing src/io in its private include dirs, causing gRPC mapper files (grpc_problem_mapper.cpp) that include mps_parser_internal.hpp to fail to compile - WHOLE_ARCHIVE linkage on the umbrella was PUBLIC, propagating the static sub-libs as link dependencies to all consumers (test binaries). This caused double-definition errors when tests linked both libcuopt.so and the statics. Changed to PRIVATE and re-exposed the statics' transitive PUBLIC deps (rmm, raft, CCCL, CUDA libs) directly on the umbrella so that consumers receive the correct source-fetched include dirs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Add libcuopt_base.a, libcuopt_routing.a, and libcuopt_lp.a to the package_contents file check so CI fails fast if any of the three component static libraries are missing from the installed package. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
…to cuopt_lp Component libs (cuopt_base, cuopt_routing, cuopt_lp) are now SHARED instead of STATIC. The umbrella libcuopt.so becomes a thin stub (~15 KB) carrying only DT_NEEDED entries for the three component libs; no code or WHOLE_ARCHIVE baking. The gRPC bridge (mapper + Cython client) moves from the umbrella into cuopt_lp where it semantically belongs — LP/MIP remote solve is an LP concern. The umbrella drops all gRPC sources, include dirs, and protobuf/gRPC link deps. Both RPATH settings use $ORIGIN so component libs find each other when co-installed. Conda package_contents check updated from .a to .so. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Moves all gRPC infrastructure (proto mappers, Cython client, solve_remote) from cuopt_lp into a new cuopt_grpc SHARED component. cuopt_grpc links cuopt_lp + cuopt_routing (when built), keeping both core solver libs free of any gRPC/protobuf dependency. The grpc_server binary now links cuopt_grpc directly. The umbrella links cuopt_grpc when gRPC is built so -lcuopt continues to expose remote-solve symbols to existing consumers. When PR #1597 (VRP gRPC) lands, routing gRPC sources go into cuopt_grpc alongside the LP ones — no cross-dependency between cuopt_lp and cuopt_routing is needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
ramakrishnap-nv
added
non-breaking
Resolve conflicts between: - Our component library split (cuopt_base/routing/lp/grpc as SHARED + thin umbrella) - main's cuopt_objs OBJECT library approach added in #1581 Both coexist: cuopt_objs + cuopt_static serve internal tests; the SHARED component libs + umbrella serve all other consumers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
/ok to test ec81ebc |
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangescuOpt componentized build
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
cpp/CMakeLists.txt (1)
902-918: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExported component names should match the documented API
cuopt::base/cuopt::lpare build-tree aliases only. The install/export set will expose the real targets (cuopt::cuopt_base,cuopt::cuopt_lp, etc.) unless those targets setEXPORT_NAME, so a consumer usingfind_package(cuopt)won’t be able to link againstcuopt::baseas documented.🤖 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 902 - 918, Update the component target export configuration used by the rapids_export INSTALL and BUILD calls so installed targets retain the documented names cuopt::base, cuopt::routing, cuopt::lp, and cuopt::grpc. Set the appropriate EXPORT_NAME values on the underlying cuopt_component targets, while preserving cuopt::cuopt as the umbrella target and keeping build-tree aliases consistent.
🧹 Nitpick comments (2)
cpp/CMakeLists.txt (2)
549-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive the git hash lookup a fallback for non-git source trees.
Without
RESULT_VARIABLE/ERROR_QUIET, tarball builds leak git's error to the configure log and bake an empty hash intobuild_info.hpp.♻️ Proposed fallback
execute_process( COMMAND git rev-parse --short HEAD WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} OUTPUT_VARIABLE GIT_COMMIT_HASH OUTPUT_STRIP_TRAILING_WHITESPACE + RESULT_VARIABLE _git_hash_result + ERROR_QUIET ) +if(NOT _git_hash_result EQUAL 0 OR GIT_COMMIT_HASH STREQUAL "") + set(GIT_COMMIT_HASH "unknown") +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 549 - 556, Update the git hash lookup in the top-level CMake configuration to capture the execute_process result and suppress stderr for source trees without Git metadata. When the lookup fails, assign a stable non-empty fallback hash before the existing GIT_COMMIT_HASH message and build_info.hpp generation; preserve the real short HEAD value for Git checkouts.
664-675: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
cuopt_objsduplicates the component include/definition setup.The object library re-declares papilo/pslp/dejavu includes, CUDSS defines, and architecture defines that
cuopt_configure_component(pluscuopt_lp) already establish. Sincecuopt_objsbackscuopt_staticfor the test builds, drift here means tests compile under a different configuration than shipped libraries. Consider factoring the shared include/definition block into a helper both paths call.Also applies to: 693-694, 720-724
🤖 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 664 - 675, Refactor the shared include directories and compile definitions currently duplicated by cuopt_objs and the cuopt_configure_component/cuopt_lp setup into a reusable CMake helper. Invoke that helper for both cuopt_objs and the shipped-library path, including the papilo/pslp/dejavu includes, CUDSS definitions, and architecture definitions, while preserving target-specific settings such as POSITION_INDEPENDENT_CODE and logging definitions.
🤖 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`:
- Around line 902-918: Update the component target export configuration used by
the rapids_export INSTALL and BUILD calls so installed targets retain the
documented names cuopt::base, cuopt::routing, cuopt::lp, and cuopt::grpc. Set
the appropriate EXPORT_NAME values on the underlying cuopt_component targets,
while preserving cuopt::cuopt as the umbrella target and keeping build-tree
aliases consistent.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 549-556: Update the git hash lookup in the top-level CMake
configuration to capture the execute_process result and suppress stderr for
source trees without Git metadata. When the lookup fails, assign a stable
non-empty fallback hash before the existing GIT_COMMIT_HASH message and
build_info.hpp generation; preserve the real short HEAD value for Git checkouts.
- Around line 664-675: Refactor the shared include directories and compile
definitions currently duplicated by cuopt_objs and the
cuopt_configure_component/cuopt_lp setup into a reusable CMake helper. Invoke
that helper for both cuopt_objs and the shipped-library path, including the
papilo/pslp/dejavu includes, CUDSS definitions, and architecture definitions,
while preserving target-specific settings such as POSITION_INDEPENDENT_CODE and
logging definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d788dd1-855a-4038-900e-9a3eab8f07be
📒 Files selected for processing (13)
conda/recipes/libcuopt/recipe.yamlcpp/CMakeLists.txtcpp/src/CMakeLists.txtcpp/src/barrier/CMakeLists.txtcpp/src/branch_and_bound/CMakeLists.txtcpp/src/cuts/CMakeLists.txtcpp/src/dual_simplex/CMakeLists.txtcpp/src/io/CMakeLists.txtcpp/src/linear_algebra/CMakeLists.txtcpp/src/math_optimization/CMakeLists.txtcpp/src/mip_heuristics/CMakeLists.txtcpp/src/pdlp/CMakeLists.txtcpp/src/routing/CMakeLists.txt
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
/ok to test a54e62a |
CI Test Summary18 failed · 13 passed · 0 skipped
|
libcuopt.so is now a thin umbrella with DT_NEEDED on libcuopt_base.so, libcuopt_routing.so, libcuopt_lp.so, and libcuopt_grpc.so. auditwheel traverses DT_NEEDED transitively and failed when it couldn't locate the component libs. Exclude them the same way libcuopt.so is excluded — they ship with the libcuopt wheel and are available at runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…builds main appends GRPC_INFRA_FILES to CUOPT_SRC_FILES before creating cuopt_objs so cuopt_static (used by NUMOPT_INTERNAL_TEST) gets solve_lp_remote / solve_mip_remote. We dropped that line when we moved those files into cuopt_grpc, causing undefined-reference link failures in tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
/ok to test 705167f |
…lution cuopt_lp.so calls solve_lp/mip_remote (under CUOPT_ENABLE_GRPC) which are defined in cuopt_grpc.so. With --as-needed the linker was dropping libcuopt_grpc.so from executables that never directly referenced a grpc symbol, leaving solve_lp_remote unresolved at runtime. Route remote solves in cuopt_cli directly through solve_lp/mip_remote so libcuopt_grpc.so is a genuine DT_NEEDED of the binary; --as-needed then keeps it in the link and the symbol is in scope when libcuopt_lp.so needs it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
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
1 participant
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.
Splits
libcuopt.sointo four component SHARED libraries (cuopt_base,cuopt_routing,cuopt_lp,cuopt_grpc) plus a thin umbrellalibcuopt.sothat links them all — existing-lcuoptconsumers are unaffected.🤖 Generated with Claude Code