Symbol visibility controls and exports (remove-internal approach) - #1625
Open
ramakrishnap-nv wants to merge 33 commits into
Open
Symbol visibility controls and exports (remove-internal approach)#1625ramakrishnap-nv wants to merge 33 commits into
ramakrishnap-nv wants to merge 33 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. |
ramakrishnap-nv
added
non-breaking
Collaborator
Author
|
/ok to test b1644c1 |
CI Test Summary✅ All 31 test job(s) passed. |
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
Collaborator
Author
|
/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>
Collaborator
Author
|
/ok to test b0ce83e |
Collaborator
Author
|
/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>
Collaborator
Author
|
/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>
Collaborator
Author
|
/ok to test e9d92d7 |
Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
Collaborator
Author
|
/ok to test eed31e1 |
1 similar comment
Collaborator
Author
|
/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>
Collaborator
Author
|
/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 (2)
📝 WalkthroughWalkthroughAdds a unified 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 💡 1📝 Generate docstrings 💡
🧪 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.
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
…ST static The main merge added standalone PDLP_TEST and MPS_PARSER_TEST from pdlp_test.cu and parser_test.cpp — but those files are already compiled into the static NUMOPT_INTERNAL_TEST. Under hidden symbol visibility the standalone (shared- linked) copies fail to link against internal symbols, breaking conda-cpp-build. Remove the duplicates (they still run via NUMOPT_INTERNAL_TEST, no coverage lost, no extra fat binary) and give the genuinely-new multi-GPU PDLP_MG_TEST STATIC_LIB so it can reach the internal distributed-PDLP symbols. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
check_symbols.sh only rejected forbidden symbols, so a library whose visibility was over-tightened to hide the public API would still pass while being unusable. Add a small stability anchor asserting core C API lifecycle entrypoints (cuOptReadProblem/CreateProblem/Solve/DestroyProblem) remain exported. Addresses review feedback. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
8 tasks
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.
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.