[DRAFT] Minimize cuopt_static test linkage to avoid 5 GB package size explosion#1566
[DRAFT] Minimize cuopt_static test linkage to avoid 5 GB package size explosion#1566ramakrishnap-nv wants to merge 8 commits into
Conversation
Signed-off-by: Arha Gatram <agatram@nvidia.com>
…static archive Signed-off-by: Arha Gatram <agatram@nvidia.com>
Signed-off-by: Arha Gatram <agatram@nvidia.com>
Signed-off-by: Arha Gatram <agatram@nvidia.com>
…late instantiations
PR #1526 switched all tests to link cuopt_static, which inflates libcuopt-tests from ~75 MB to ~5 GB (confirmed from CI artifacts) because each of ~36 test executables embeds the full cuopt object graph. This commit introduces ConfigureInternalTest for tests that genuinely need access to hidden symbols, and keeps ConfigureTest on the shared library for tests that only use the public API. Only routing tests are switched to ConfigureInternalTest: they all include routing_test.cuh which pulls in cuopt::routing::detail types (solution_t, problem_t, fleet_order_constraints_t, md_utils, etc.). MIP, LP, QP, SOCP, dual_simplex, distance engine, CLI, and utility tests use only public cuopt API or raft::detail inline headers (which are compiled into the TU, not linked from cuopt.so), so they stay on the shared library. GRPC tests are left unchanged as they have explicit target_link_libraries and grpc_client.cpp symbols lack CUOPT_EXPORT. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
db8af85 to
702c97d
Compare
CI Test Summary1 failed · 30 passed · 0 skipped |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesSymbol visibility export and enforcement
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ci/check_symbols.sh and conda/recipes/libcuopt/recipe.yaml were missing the canonical suffix required by verify-copyright pre-commit hook. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
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 (2)
cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp (1)
22-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAnnotate
optimization_problem_interface_twithCUOPT_EXPORT
cpu_optimization_problem_tis exported, but its baseoptimization_problem_interface_t<i_t, f_t>is declared without any visibility annotation incpp/include/cuopt/mathematical_optimization/optimization_problem_interface.hpp. That leaves the explicit instantiation incpp/src/pdlp/cpu_optimization_problem.cppexposed to an exported-derived/hidden-base visibility mismatch.🤖 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/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp` around lines 22 - 45, Annotate the optimization_problem_interface_t declaration in cpp/include/cuopt/mathematical_optimization/optimization_problem_interface.hpp with CUOPT_EXPORT so its visibility matches the exported cpu_optimization_problem_t base class. The explicit instantiation in cpp/src/pdlp/cpu_optimization_problem.cpp requires no direct change; it is corrected by the interface visibility update.cpp/src/pdlp/solve.cu (1)
2309-2364: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExport the PDLP instantiations used by tests in
cpp/src/pdlp/solve.cu:2338-2364;run_batch_pdlp,compute_optimal_batch_size, andset_pdlp_solver_modeare called fromcpp/tests/linear_programming/pdlp_test.cuandcpp/tests/linear_programming/utilities/pdlp_test_utilities.cuh, so their explicit instantiations needCUOPT_EXPORTtoo.solve_lp_with_methodcan stay hidden.🤖 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/src/pdlp/solve.cu` around lines 2309 - 2364, Add CUOPT_EXPORT to the explicit instantiations of run_batch_pdlp, compute_optimal_batch_size, and set_pdlp_solver_mode in the INSTANTIATE macro so test callers can link them. Leave solve_lp_with_method unexported.
🧹 Nitpick comments (2)
cpp/CMakeLists.txt (1)
626-629: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
target_compile_optionsoncuopt_static.
cuopt_statichas no sources of its own ($<TARGET_OBJECTS:cuopt_objs>only), soPRIVATEcompile options here never apply to any actual compilation and are effectively dead. Safe to drop (unlike thePUBLICcompile definitions above it at Line 631, which matter as usage requirements for consumers).🤖 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 626 - 629, Remove the target_compile_options call for cuopt_static, including both CXX and CUDA generator-expression flags. Leave the nearby PUBLIC compile definitions unchanged because they remain required usage requirements for consumers.cpp/tests/CMakeLists.txt (1)
42-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ConfigureTestandConfigureInternalTestare near-duplicate functions.The two functions differ only in one linked library (
cuoptvscuopt_static); everything else (include dirs, GTest links, install rules, labels) is copy-pasted. Consider factoring the common body into a private helper taking the target library as a parameter to avoid future drift between the two.🤖 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` around lines 42 - 129, Refactor the duplicated ConfigureTest and ConfigureInternalTest implementations into a private helper that accepts the target library as a parameter. Move the shared executable setup, include directories, test registration, labels, install rules, and link dependencies into that helper, then have ConfigureTest pass cuopt and ConfigureInternalTest pass cuopt_static while preserving their current behavior.
🤖 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.
Inline comments:
In `@ci/check_symbols.sh`:
- Around line 19-20: Narrow the symbol filtering in ci/check_symbols.sh so it
exempts only the intended exception-type symbols rather than any demangled name
containing “_error”. Update the awk condition used by the symbol comparison flow
to match the specific exception symbols, while allowing public API names such as
get_error_status, get_error_message, and last_error to remain in the diff.
In `@cpp/CMakeLists.txt`:
- Around line 582-599: The build configuration for cuopt_objs, cuopt_static, and
cuopt is duplicated and can drift. Extract the shared compile definitions and
link libraries—including CUDA architecture definitions, logging/CUSPARSE
definitions, CUDA, RMM, RAFT, CCCL, CUDSS, private CUDA libraries, and
conditional protobuf/gRPC dependencies—into reusable variables or a helper
function, then apply them consistently to all three targets while preserving
their existing public/private visibility.
---
Outside diff comments:
In `@cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp`:
- Around line 22-45: Annotate the optimization_problem_interface_t declaration
in
cpp/include/cuopt/mathematical_optimization/optimization_problem_interface.hpp
with CUOPT_EXPORT so its visibility matches the exported
cpu_optimization_problem_t base class. The explicit instantiation in
cpp/src/pdlp/cpu_optimization_problem.cpp requires no direct change; it is
corrected by the interface visibility update.
In `@cpp/src/pdlp/solve.cu`:
- Around line 2309-2364: Add CUOPT_EXPORT to the explicit instantiations of
run_batch_pdlp, compute_optimal_batch_size, and set_pdlp_solver_mode in the
INSTANTIATE macro so test callers can link them. Leave solve_lp_with_method
unexported.
---
Nitpick comments:
In `@cpp/CMakeLists.txt`:
- Around line 626-629: Remove the target_compile_options call for cuopt_static,
including both CXX and CUDA generator-expression flags. Leave the nearby PUBLIC
compile definitions unchanged because they remain required usage requirements
for consumers.
In `@cpp/tests/CMakeLists.txt`:
- Around line 42-129: Refactor the duplicated ConfigureTest and
ConfigureInternalTest implementations into a private helper that accepts the
target library as a parameter. Move the shared executable setup, include
directories, test registration, labels, install rules, and link dependencies
into that helper, then have ConfigureTest pass cuopt and ConfigureInternalTest
pass cuopt_static while preserving their current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c78c9a15-603d-4cd0-a9f0-f714de6b6474
📒 Files selected for processing (71)
ci/check_symbols.shconda/recipes/libcuopt/recipe.yamlcpp/CMakeLists.txtcpp/include/cuopt/error.hppcpp/include/cuopt/export.hppcpp/include/cuopt/grpc/cython_grpc_client.hppcpp/include/cuopt/grpc/grpc_client_env.hppcpp/include/cuopt/mathematical_optimization/backend_selection.hppcpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hppcpp/include/cuopt/mathematical_optimization/cpu_optimization_problem_solution.hppcpp/include/cuopt/mathematical_optimization/cpu_pdlp_warm_start_data.hppcpp/include/cuopt/mathematical_optimization/cuopt_c.hcpp/include/cuopt/mathematical_optimization/io/data_model_view.hppcpp/include/cuopt/mathematical_optimization/io/mps_data_model.hppcpp/include/cuopt/mathematical_optimization/io/mps_writer.hppcpp/include/cuopt/mathematical_optimization/io/parser.hppcpp/include/cuopt/mathematical_optimization/io/utilities/cython_parser.hppcpp/include/cuopt/mathematical_optimization/io/writer.hppcpp/include/cuopt/mathematical_optimization/mip/solver_settings.hppcpp/include/cuopt/mathematical_optimization/mip/solver_solution.hppcpp/include/cuopt/mathematical_optimization/optimization_problem.hppcpp/include/cuopt/mathematical_optimization/optimization_problem_solution.hppcpp/include/cuopt/mathematical_optimization/pdlp/pdlp_warm_start_data.hppcpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hppcpp/include/cuopt/mathematical_optimization/pdlp/solver_solution.hppcpp/include/cuopt/mathematical_optimization/solve.hppcpp/include/cuopt/mathematical_optimization/solve_remote.hppcpp/include/cuopt/mathematical_optimization/solver_settings.hppcpp/include/cuopt/mathematical_optimization/utilities/cython_solve.hppcpp/include/cuopt/mathematical_optimization/utilities/cython_types.hppcpp/include/cuopt/routing/assignment.hppcpp/include/cuopt/routing/cython/cython.hppcpp/include/cuopt/routing/data_model_view.hppcpp/include/cuopt/routing/routing_structures.hppcpp/include/cuopt/routing/solve.hppcpp/include/cuopt/routing/solver_settings.hppcpp/src/grpc/client/solve_remote.cppcpp/src/grpc/grpc_problem_mapper.cppcpp/src/grpc/grpc_settings_mapper.cppcpp/src/grpc/grpc_solution_mapper.cppcpp/src/io/data_model_view.cppcpp/src/io/lp_parser.cppcpp/src/io/mps_data_model.cppcpp/src/io/mps_writer.cppcpp/src/io/parser.cppcpp/src/io/writer.cppcpp/src/math_optimization/solution_reader.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver_settings.cucpp/src/mip_heuristics/solver_solution.cucpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/cpu_pdlp_warm_start_data.cucpp/src/pdlp/optimization_problem.cucpp/src/pdlp/pdlp_warm_start_data.cucpp/src/pdlp/solution_conversion.cucpp/src/pdlp/solve.cucpp/src/pdlp/solver_settings.cucpp/src/pdlp/solver_solution.cucpp/src/routing/assignment.cucpp/src/routing/data_model_view.cucpp/src/routing/distance_engine/waypoint_matrix.cppcpp/src/routing/solve.cucpp/src/routing/solver_settings.cucpp/src/routing/utilities/cython.cucpp/src/utilities/logger.hppcpp/tests/CMakeLists.txtcpp/tests/examples/routing/CMakeLists.txtcpp/tests/linear_programming/CMakeLists.txtcpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/routing/CMakeLists.txt
| # Ignore symbols containing "_error" since these are likely exception types | ||
| # and should be exported. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
readelf --dyn-syms symbol visibility naming convention exception types "_error" suffix
💡 Result:
There is no single, universal naming convention in the ELF specification that mandates an "_error" suffix for symbols [1][2]. When you encounter a symbol with an "_error" suffix in the output of readelf --dyn-syms, it is typically a result of project-specific internal naming conventions rather than an ELF-level standard [3][4][5]. Naming Conventions and Exception Types: In large software projects like GNU Binutils or GDB, the "_error" suffix is frequently used as a naming convention to identify functions or variables responsible for handling, reporting, or defining error conditions [3][6][4][5]. For instance, GDB uses an "_ERROR" suffix for various exception types within its common-exceptions.h header to maintain naming consistency, even when the specific symbol is not strictly a fatal error [4][5]. Similarly, internal BFD (Binary File Descriptor) library functions—such as _bfd_x86_elf_link_report_tls_invalid_section_error—use this suffix to clearly denote their purpose in error reporting during the linking process [6]. Visibility and readelf --dyn-syms: The presence of such symbols in the dynamic symbol table (viewable via readelf --dyn-syms) indicates that these symbols are exported or required for dynamic linking [1]. In many cases, developers intentionally mark these internal symbols as hidden (using attributes like ATTRIBUTE_HIDDEN) to prevent them from being visible or preemptable outside of the library itself, even if they appear in the object file [3][6][7][8]. This practice reduces the size of the dynamic symbol table and improves encapsulation, effectively making them private to the DSO (Dynamic Shared Object) while keeping them accessible for internal build-time operations [3][7][8]. In summary, the "_error" suffix is a developer-chosen label for error-handling code, and its visibility in your dynamic symbol table is a separate characteristic managed by ELF symbol visibility attributes (such as STV_HIDDEN, STV_PROTECTED, or STV_DEFAULT) [6][1].
Citations:
- 1: https://gist.github.com/x0nu11byt3/bcb35c3de461e5fb66173071a2379779
- 2: https://www.ti.com/lit/pdf/SPRAB89
- 3: https://sourceware.org/pipermail/binutils/2025-October/145119.html
- 4: https://gnu.googlesource.com/binutils-gdb/+/bbc133a3083cebcb9d2e010bef47753448d0046f/gdbsupport/common-exceptions.h
- 5: https://github.com/trungnt2910/gdb-haiku/blob/gdb-15-haiku/gdbsupport/common-exceptions.h
- 6: https://sourceware.org/pipermail/binutils/2025-October/145134.html
- 7: https://sourceware.org/pipermail/binutils/2025-October/145133.html
- 8: https://sourceware.org/pipermail/binutils/2025-October/145135.html
🏁 Script executed:
#!/bin/bash
set -eEuo pipefail
# Cleanup on exit.
cleanup() {
if [[ -d "${TEMP_DIR:-}" ]]; then
rm -rf "${TEMP_DIR}"
fi
}
trap cleanup EXIT
LIB_NAME="${1}"
# Ignore symbols containing "_error" since these are likely exception types
# and should be exported.
readelf --dyn-syms --wide "${LIB_NAME}" \
| c++filt \
| awk '$0 !~ /_error/' \
| sed -n 's/.*\[\(.*\)\]/\1/p' \
| sort \
| uniq > "${TEMP_DIR}/exported_symbols.txt"
diff -u "${TEMP_DIR}/exported_symbols.txt" "${TEMP_DIR}/expected_symbols.txt"Repository: NVIDIA/cuopt
Length of output: 185
🏁 Script executed:
rg -n --hidden --glob '!**/.git/**' "_error|check_symbols\.sh|expected_symbols" .Repository: NVIDIA/cuopt
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -eEuo pipefail
rg -n --hidden --glob '!**/.git/**' "_error|check_symbols\.sh|expected_symbols" .Repository: NVIDIA/cuopt
Length of output: 50368
ci/check_symbols.sh:25 ignores too much. awk '$0 !~ /_error/' drops any demangled symbol containing _error, so public API names like get_error_status, get_error_message, or last_error never reach the diff. Narrow this to the specific exception symbols you want to exempt.
🤖 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 `@ci/check_symbols.sh` around lines 19 - 20, Narrow the symbol filtering in
ci/check_symbols.sh so it exempts only the intended exception-type symbols
rather than any demangled name containing “_error”. Update the awk condition
used by the symbol comparison flow to match the specific exception symbols,
while allowing public API names such as get_error_status, get_error_message, and
last_error to remain in the diff.
| target_compile_definitions(cuopt_objs PUBLIC | ||
| CUOPT_CUDA_ARCHITECTURES="${JOINED_CUDA_ARCHITECTURES}" | ||
| CUOPT_CPU_ARCHITECTURE="${CMAKE_SYSTEM_PROCESSOR}") | ||
|
|
||
| target_link_libraries(cuopt_objs | ||
| PUBLIC | ||
| CUDA::cublas | ||
| CUDA::cusparse | ||
| rmm::rmm | ||
| rapids_logger::rapids_logger | ||
| CCCL::CCCL | ||
| raft::raft | ||
| ${CUDSS_LIB_FILE} | ||
| PRIVATE | ||
| ${CUOPT_PRIVATE_CUDA_LIBS} | ||
| $<$<BOOL:${CUOPT_ENABLE_GRPC}>:protobuf::libprotobuf> | ||
| $<$<BOOL:${CUOPT_ENABLE_GRPC}>:gRPC::grpc++> | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Triplicated build config across cuopt_objs/cuopt_static/cuopt risks drift.
Because OBJECT-library usage requirements don't propagate through $<TARGET_OBJECTS:cuopt_objs>, the same target_compile_definitions/target_link_libraries blocks (CUOPT_CUDA_ARCHITECTURES, CUOPT_CPU_ARCHITECTURE, CUOPT_LOG_ACTIVE_LEVEL, CUSPARSE_ENABLE_EXPERIMENTAL_API, CUDA::cublas/cusparse, rmm, raft, CCCL, CUDSS, CUOPT_PRIVATE_CUDA_LIBS, protobuf/grpc++) are copy-pasted across cuopt_objs (582-599), cuopt_static (604-639), and later cuopt itself. A future dependency/define change applied to only one target will silently desync the other two, potentially causing subtle ABI/link mismatches between the static-test build and the shipped shared library.
Consider hoisting these into reusable CMake list/variable(s) (e.g. CUOPT_PUBLIC_LINK_LIBS, CUOPT_COMMON_COMPILE_DEFS) and reusing them in all three target_link_libraries/target_compile_definitions calls, or wrap the common setup in a small function()/macro applied to each target.
♻️ Sketch of a shared-variable approach
+set(CUOPT_COMMON_COMPILE_DEFS
+ "CUOPT_LOG_ACTIVE_LEVEL=RAPIDS_LOGGER_LOG_LEVEL_${LIBCUOPT_LOGGING_LEVEL}"
+ CUSPARSE_ENABLE_EXPERIMENTAL_API
+ "CUOPT_CUDA_ARCHITECTURES=${JOINED_CUDA_ARCHITECTURES}"
+ "CUOPT_CPU_ARCHITECTURE=${CMAKE_SYSTEM_PROCESSOR}")
+set(CUOPT_PUBLIC_LINK_LIBS CUDA::cublas CUDA::cusparse rmm::rmm
+ rapids_logger::rapids_logger CCCL::CCCL raft::raft ${CUDSS_LIB_FILE})
target_compile_definitions(cuopt_objs
- PUBLIC "CUOPT_LOG_ACTIVE_LEVEL=RAPIDS_LOGGER_LOG_LEVEL_${LIBCUOPT_LOGGING_LEVEL}"
- PUBLIC CUSPARSE_ENABLE_EXPERIMENTAL_API
+ PUBLIC ${CUOPT_COMMON_COMPILE_DEFS}
)
...
target_link_libraries(cuopt_static
- PUBLIC
- CUDA::cublas
- CUDA::cusparse
- rmm::rmm
- rapids_logger::rapids_logger
- CCCL::CCCL
- raft::raft
- ${CUDSS_LIB_FILE}
+ PUBLIC ${CUOPT_PUBLIC_LINK_LIBS}
PRIVATE
${CUOPT_PRIVATE_CUDA_LIBS}
...
)Also applies to: 604-639
🤖 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 582 - 599, The build configuration for
cuopt_objs, cuopt_static, and cuopt is duplicated and can drift. Extract the
shared compile definitions and link libraries—including CUDA architecture
definitions, logging/CUSPARSE definitions, CUDA, RMM, RAFT, CCCL, CUDSS, private
CUDA libraries, and conditional protobuf/gRPC dependencies—into reusable
variables or a helper function, then apply them consistently to all three
targets while preserving their existing public/private visibility.
Description
Follow-on to #1526 (includes all its commits). That PR correctly introduces hidden symbol visibility and
cuopt_staticfor tests that need internal symbols — but switches all tests via theConfigureTestmacro, causing a ~67× size explosion inlibcuopt-tests.Measured impact of #1526 as-is (from its own CI artifacts):
libcuopt-testscuda13 linux-64This PR adds one commit on top of #1526 that limits
cuopt_staticlinkage to only the tests that actually need it.Analysis
Tests that genuinely need
cuopt_static— routing tests only. They all includerouting_test.cuhwhich pulls incuopt::routing::detailtypes (solution_t,problem_t,fleet_order_constraints_t,fill_data_model_matrices, etc.), andlocal_search_cand_test.cu/top_k.cudirectly construct internal solver state.Tests that do NOT need
cuopt_static:raft::linalg::detail::cublassetpointermode/cusparsesetpointermode— these are raft inline headers compiled into the TU, not cuopt hidden symbols, and require no special linkagecuopttestutilskeepscuopt_staticPRIVATE (unchanged from Fix symbol visibility and add checks #1526); non-routing tests never callcheck_constraints.cufunctions so the linker never pulls in that object fileGRPC tests are left unchanged — they have explicit
target_link_librariesandgrpc_client.cpplacksCUOPT_EXPORT, socuopt_staticremains correct.Changes (one commit on top of #1526)
cpp/tests/CMakeLists.txt: KeepConfigureTest→cuoptshared; addConfigureInternalTest→cuopt_staticcpp/tests/routing/CMakeLists.txt: Switch all 8 routing targets toConfigureInternalTestLocal build verification
Checklist