Symbol visibility controls and exports (remove-internal approach) - #1625
Symbol visibility controls and exports (remove-internal approach)#1625ramakrishnap-nv wants to merge 31 commits into
Conversation
Prerequisite structural work for enabling -fvisibility=hidden on libcuopt.so (issue #1213). - Introduce cuopt_objs OBJECT library so cuopt (shared) and cuopt_static (static, BUILD_TESTS only) share one compile step - Add STATIC_LIB option to ConfigureTest; when set, links cuopt_static and omits cuopttestutils to avoid transitive cuopt-shared pull-in - Route all tests with compiled internal symbol dependencies through STATIC_LIB: DUAL_SIMPLEX_TEST, LP_INTERNAL_TEST, PDLP_TEST, MPS_PARSER_TEST, most MIP tests, SOCP_TEST, ROUTING_INTERNAL_TEST - Create ROUTING_INTERNAL_TEST consolidating GES, scross, local-search candidate, and top-k tests; register previously unbuilt test files (l0_scross_test.cu, local_search_cand_test.cu, load_balancing_test.cu, feasibility_jump_tests.cu) - Add gRPC source files directly to GRPC_CLIENT_TEST and GRPC_INTEGRATION_TEST so they do not depend on internal symbols being exported from the shared library - Promote is_cusparse_runtime_mixed_precision_supported() to the public C API as cuOptIsCusparseRuntimeMixedPrecisionSupported(); update c_api_tests.cpp to use the public function Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Collapse 19 separate STATIC_LIB test binaries (dual_simplex, LP internal, PDLP, MPS parser, 11 MIP, SOCP) into a single NUMOPT_INTERNAL_TEST binary linked against cuopt_static. Routing internal tests remain in ROUTING_INTERNAL_TEST. This reduces total static-linked binary count from 19 to 2, cutting link time and disk usage. Also fix cuopt_c.cpp to forward-declare is_cusparse_runtime_mixed_precision_supported() instead of including cusparse_view.hpp, which pulled CUDA intrinsics into a CXX translation unit. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Fix four issues discovered when building NUMOPT_INTERNAL_TEST and ROUTING_INTERNAL_TEST: - omp_helpers.hpp: closing namespace brace was outside #ifdef _OPENMP, causing "expected a declaration" when compiled without -fopenmp. - cuopt_static: propagate OpenMP::OpenMP_CXX PUBLIC and -fopenmp for CUDA TUs so test binaries that link cuopt_static compile OMP-heavy internal headers correctly. - cuopt shared: restore CUSPARSE_ENABLE_EXPERIMENTAL_API and CUOPT_LOG_ACTIVE_LEVEL as PUBLIC compile definitions; they were lost when compile definitions moved from cuopt to cuopt_objs (the $<TARGET_OBJECTS:...> pattern does not carry INTERFACE properties). - Combined binaries: add CUOPT_DISABLE_TEST_MAIN guard to CUOPT_TEST_PROGRAM_MAIN() in base_fixture.hpp; provide a single main.cu entry point per binary; make init_handler static in three MIP test files to resolve ODR violations; compile check_constraints.cu directly into ROUTING_INTERNAL_TEST for check_route; add missing src/ and src/io/ include paths to GRPC_CLIENT_TEST and GRPC_INTEGRATION_TEST. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
All tests from these directories moved to NUMOPT_INTERNAL_TEST. The subdirectories and add_subdirectory calls serve no purpose now. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
$<TARGET_OBJECTS:cuopt_objs> does not propagate PRIVATE link dependencies to consuming targets; PSLP must be linked explicitly on both cuopt_static and cuopt (shared). Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
…test files Files compiled into NUMOPT_INTERNAL_TEST and ROUTING_INTERNAL_TEST no longer need their own main(); remove the call directly rather than suppressing it via the CUOPT_DISABLE_TEST_MAIN guard. Simplifies base_fixture.hpp and the CMakeLists files. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Remove cuOptIsCusparseRuntimeMixedPrecisionSupported from the public C API. The c_api_test already includes internal headers; use the compile-time CUSPARSE_VERSION macro instead, which is accurate for cuOpt's deployment model (compile-time and runtime toolkit match). Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
When cuSPARSE < 12.5, cuopt_expects throws ValidationError before the solve begins, so the C API always returns an error — CUOPT_SUCCESS with non-optimal is impossible. Assert status != CUOPT_SUCCESS directly. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
After thrust::exclusive_scan, offsets.back() already equals the total number of non-zeros (standard CSR sentinel). The previous + 1 produced one entry beyond the last row, which is invalid CSR and caused cusparseXcsrsort to produce incorrect results when assertions are enabled (PR CI uses -a / DEFINE_ASSERT=ON which enables -UNDEBUG). The resulting out-of-bounds column index triggered the check_csr_representation assertion in compute_transpose_of_problem, crashing NUMOPT_INTERNAL_TEST with SIGABRT in PR CI. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The compile-time #if CUSPARSE_VERSION >= 12500 guard only checks the header version used during build. On systems where the runtime cuSPARSE library is older than 12.5 (e.g., oldest-deps conda builds), the solver correctly throws ValidationError via is_cusparse_runtime_mixed_precision_supported(), but the test unconditionally expected CUOPT_SUCCESS. Fix by calling cusparseGetProperty() at runtime in the test to mirror the same check the solver performs, routing the test to the success or failure assertion accordingly. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
top_k.cu was moved to ROUTING_INTERNAL_TEST because it uses internal symbols (routing/detail). It was also the only source in ROUTING_UNIT_TEST that provided CUOPT_TEST_PROGRAM_MAIN(), which sets up the RMM device memory resource before running tests. Without RMM setup, the routing solver allocations use the default cuda_memory_resource, causing SIGSEGV in vehicle_breaks.non_uniform_breaks (and potentially other tests that stress the allocator). Fix by adding internal/main.cu to ROUTING_UNIT_TEST sources. The custom main() it provides takes precedence over GTest::gmock_main's default main and properly initializes the RMM memory resource. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The SPDX header used bare `/* text` lines without closing `*/` on each line, leaving an unclosed C comment. Subsequent `/*` markers triggered -Werror=comment when internal/main.cu was compiled into new targets (e.g. ROUTING_UNIT_TEST). Use the standard block-comment style matching the rest of the test suite. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
cuopt_objs is the OBJECT library that compiles all CUDA TUs. Before the cuopt_objs refactor, cuopt SHARED had OpenMP::OpenMP_CUDA as a PRIVATE dep, which passed -fopenmp to nvcc and defined _OPENMP for all .cu files. After the refactor, cuopt_objs only inherited OpenMP::OpenMP_CXX (via CUOPT_PRIVATE_CUDA_LIBS), which only applies to CXX compilation. Without -fopenmp, omp_atomic_t<T> is undefined in CUDA TUs that include omp_helpers.hpp (e.g. via dual_simplex/solution.hpp), causing build failures. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
The fast MPS parser sources include simde/x86/avx2.h. Before the cuopt_objs refactor, simde::simde was a PRIVATE dep of cuopt SHARED and its include path was available during source compilation. After the refactor, sources are compiled in cuopt_objs, so simde::simde must be listed there. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
…test QcMatrixRowsMatchReferenceBitwise provided X1 X2 -2.5 but not the required matching X2 X1 -2.5. The classic MPS parser enforces that each off-diagonal (i,j) entry has an explicit (j,i) counterpart; verify_fixture_bitwise routes through both parsers and the reference parser was throwing ValidationError on the asymmetric input. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
…ty-remove-internal # Conflicts: # cpp/include/cuopt/mathematical_optimization/cuopt_c.h # cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
|
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. |
|
/ok to test b1644c1 |
CI Test Summary✅ All 13 test job(s) passed. (2 skipped) |
With hidden-by-default visibility, tests that reference internal symbols fail to link against the shared cuopt library (undefined references to the routing dataset generator, mps fast-parser internals, and grpc mapper helpers such as canonicalize_coo_matrix). This broke conda-cpp-build, which builds the gtest binaries; wheel builds were unaffected because they don't. Route these tests through the existing STATIC_LIB mechanism so they link cuopt_static: - routing: ROUTING_TEST, VEHICLE_ORDER_TEST, VEHICLE_TYPES_TEST, OBJECTIVE_FUNCTION_TEST, RETAIL_L1TEST, ROUTING_L1TEST, ROUTING_UNIT_TEST - linear_programming: MPS_FAST_PARSER_TEST - grpc: GRPC_CLIENT_TEST, GRPC_PIPE_SERIALIZATION_TEST, GRPC_INTEGRATION_TEST Add cuopttestutils_static (check_constraints.cu linked against cuopt_static) so STATIC_LIB tests resolve test-utility symbols like check_route without mixing shared and static cuopt in one binary, and wire it into the ConfigureTest STATIC_LIB path. ROUTING_INTERNAL_TEST no longer needs to compile check_constraints.cu directly. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
…ty-remove-internal Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com> # Conflicts: # cpp/CMakeLists.txt # cpp/tests/CMakeLists.txt # cpp/tests/routing/CMakeLists.txt
|
/ok to test 0db00c3 |
Addresses review feedback on #1581: the trailing ' *' before the closing '*/' in the CUOPT_TEST_PROGRAM_MAIN doc block serves no purpose. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 969511b |
The conda libcuopt recipe invokes ./ci/check_symbols.sh directly (recipe.yaml), which requires the executable bit. It was committed as 100644, so conda-cpp-build failed with 'Permission denied' once the earlier link errors were resolved. All sibling ci/*.sh scripts are 100755. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test b0ce83e |
|
/ok to test d51eaed |
Each STATIC_LIB test binary embeds the full multi-arch cuopt_static device code, so giving every standalone routing test its own static binary exploded the libcuopt-tests conda package to ~4.24 GiB. Fold the active level-0 routing tests (ROUTING_TEST, VEHICLE_ORDER_TEST, VEHICLE_TYPES_TEST, OBJECTIVE_FUNCTION_TEST) into the existing consolidated ROUTING_INTERNAL_TEST static binary instead of 4 separate ones, matching the pattern documented in cpp/tests/internal/CMakeLists.txt. Drop CUOPT_TEST_PROGRAM_MAIN() from the folded files since the combined binary provides main via internal/main.cu (same treatment l0_ges_test.cu already has). Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 2980061 |
RETAIL_L1TEST and ROUTING_L1TEST are disabled (never run) yet each shipped as its own static binary embedding the full multi-arch cuopt_static. Combine both into a single disabled ROUTING_L1_TEST binary (sharing main via internal/main.cu), dropping one more fat binary from the libcuopt-tests package. Remove the now- redundant CUOPT_TEST_PROGRAM_MAIN() from both files. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test e9d92d7 |
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test eed31e1 |
1 similar comment
|
/ok to test eed31e1 |
ci/run_ctests.sh runs every installed binary matching '*_TEST'. Naming the combined level-1 regression binary ROUTING_L1_TEST pulled it into that glob, so the disabled/failing L1 regression tests ran in conda-cpp-tests and aborted (squeeze.cu min-vehicles assertion). Rename to ROUTING_L1TEST (matching the original RETAIL_L1TEST/ROUTING_L1TEST convention) so the glob skips it, and add a comment documenting the naming requirement. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test b5cfd94 |
|
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 (77)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (66)
📝 WalkthroughWalkthroughAdds a unified export-visibility mechanism for cuOpt symbols, configures hidden defaults with explicit public exports, validates installed shared-library symbols, and updates CMake test targets to use static cuOpt linkage and consolidated test entry points. ChangesSymbol visibility and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 22-26: The symbol checker in ci/check_symbols.sh must validate a
maintained required-export set, including C API entrypoints such as
solve_mps_file, in addition to rejecting forbidden symbols. Update
conda/recipes/libcuopt/recipe.yaml to pass the installed $PREFIX/lib/libcuopt.so
rather than cpp/build/libcuopt.so; apply the positive export checks in
ci/check_symbols.sh, while the recipe site requires only this library-path
change.
In `@cpp/include/cuopt/export.hpp`:
- Line 8: Replace `#pragma` once with unique `#ifndef/`#define include guards in
cpp/include/cuopt/export.hpp and cpp/include/cuopt/routing/solve.hpp, and close
each guard at the end of its respective header.
🪄 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: 7e3f21a8-a3b0-4c73-a929-33667edd30f1
📒 Files selected for processing (77)
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/linear_programming/CMakeLists.txtcpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/routing/CMakeLists.txtcpp/tests/routing/level0/l0_objective_function_test.cucpp/tests/routing/level0/l0_routing_test.cucpp/tests/routing/level0/l0_vehicle_order_match.cucpp/tests/routing/level0/l0_vehicle_types_test.cucpp/tests/routing/level1/l1_retail_test.cucpp/tests/routing/level1/l1_routing_test.cucpp/tests/utilities/base_fixture.hpp
💤 Files with no reviewable changes (5)
- cpp/tests/routing/level0/l0_routing_test.cu
- cpp/tests/utilities/base_fixture.hpp
- cpp/tests/routing/level0/l0_vehicle_order_match.cu
- cpp/tests/routing/level0/l0_objective_function_test.cu
- cpp/tests/routing/level0/l0_vehicle_types_test.cu
| readelf --dyn-syms --wide "${LIBRARY}" \ | ||
| | awk '$7 != "UND" && $5 != "WEAK" && $5 != "UNIQUE"' \ | ||
| | c++filt --no-params \ | ||
| | awk '$0 !~ /_error/' \ | ||
| > "${symbol_file}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate required exports on the installed library. The checker only rejects forbidden symbols, so a library with all intended APIs hidden passes; it also currently examines the build-tree artifact rather than the packaged one. Assert a maintained must-export set (at least the C API entrypoints such as solve_mps_file) and run it against the installed $PREFIX/lib/libcuopt.so.
ci/check_symbols.sh#L22-L26: add positive checks for required public symbols.conda/recipes/libcuopt/recipe.yaml#L111-L111: check the installed shared object, notcpp/build/libcuopt.so.
📍 Affects 2 files
ci/check_symbols.sh#L22-L26(this comment)conda/recipes/libcuopt/recipe.yaml#L111-L111
🤖 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 22 - 26, The symbol checker in
ci/check_symbols.sh must validate a maintained required-export set, including C
API entrypoints such as solve_mps_file, in addition to rejecting forbidden
symbols. Update conda/recipes/libcuopt/recipe.yaml to pass the installed
$PREFIX/lib/libcuopt.so rather than cpp/build/libcuopt.so; apply the positive
export checks in ci/check_symbols.sh, while the recipe site requires only this
library-path change.
| */ | ||
| /* clang-format on */ | ||
|
|
||
| #pragma once |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use #define include guards in the changed .hpp files.
cpp/include/cuopt/export.hpp#L8-L8: replace#pragma oncewith a unique#ifndef/#defineguard.cpp/include/cuopt/routing/solve.hpp#L8-L8: replace#pragma oncewith a unique#ifndef/#defineguard.
📍 Affects 2 files
cpp/include/cuopt/export.hpp#L8-L8(this comment)cpp/include/cuopt/routing/solve.hpp#L8-L8
🤖 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/export.hpp` at line 8, Replace `#pragma` once with unique
`#ifndef/`#define include guards in cpp/include/cuopt/export.hpp and
cpp/include/cuopt/routing/solve.hpp, and close each guard at the end of its
respective header.
Sources: Coding guidelines, Path instructions
Summary
Enables symbol visibility controls (hidden-by-default) and explicit exports across the cuOpt C/C++/Cython API surface, and removes the internal-symbol test-linkage workaround by consolidating internal tests.
Based on @arhag23's
fix-symbol-visibility-remove-internal, withmainmerged in. This is the remove-internal approach — an alternative to #1526 (fix-symbol-visibility-2, which keepscuopt_staticand minimizes its linkage).Key changes
CUOPT_EXPORTannotations across public headers (mathematical_optimization,routing,io,grpc,error).cpp/CMakeLists.txtandcpp/include/cuopt/export.hpp.check symbolsscript for CI verification.cuopt_staticinternal-test linkage stubs.Status
main(0 commits behind at time of push).cuopt_c.h(main's new generic-attribute getters kept inside the visibility push/pop) andc_api_tests.cpp(kept main's gmock include + mixed-precision decl).Supersedes draft #1620, which pointed at the fork branch that could not be updated directly.