refactor(tests): link internal tests against cuopt_static#1581
refactor(tests): link internal tests against cuopt_static#1581ramakrishnap-nv wants to merge 7 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>
|
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. |
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>
📝 WalkthroughWalkthroughThe PR refactors CUOPT’s CMake targets around shared object files, adds a public cuSPARSE mixed-precision capability API, and consolidates internal tests with shared entry points and static-library linking. ChangesCUOPT build and test refactor
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 |
CI Test Summary8 failed · 23 passed · 0 skipped |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cpp/CMakeLists.txt`:
- Around line 639-677: Update target_link_libraries for the shared cuopt target
to link PSLP directly, matching the existing cuopt_static linkage by reusing
$<TARGET_FILE:PSLP>. Place it with the other required PUBLIC dependencies so
third_party_presolve.cpp symbols resolve when building cuopt.
🪄 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: ca9f970f-6e82-46b2-bac4-a965fe3f4b2a
📒 Files selected for processing (18)
cpp/CMakeLists.txtcpp/include/cuopt/mathematical_optimization/cuopt_c.hcpp/src/pdlp/cuopt_c.cppcpp/src/utilities/omp_helpers.hppcpp/tests/CMakeLists.txtcpp/tests/dual_simplex/CMakeLists.txtcpp/tests/internal/CMakeLists.txtcpp/tests/internal/main.cucpp/tests/linear_programming/CMakeLists.txtcpp/tests/linear_programming/c_api_tests/c_api_tests.cppcpp/tests/linear_programming/grpc/CMakeLists.txtcpp/tests/mip/CMakeLists.txtcpp/tests/mip/bounds_standardization_test.cucpp/tests/mip/elim_var_remap_test.cucpp/tests/mip/multi_probe_test.cucpp/tests/routing/CMakeLists.txtcpp/tests/socp/CMakeLists.txtcpp/tests/utilities/base_fixture.hpp
💤 Files with no reviewable changes (3)
- cpp/tests/dual_simplex/CMakeLists.txt
- cpp/tests/linear_programming/CMakeLists.txt
- cpp/tests/socp/CMakeLists.txt
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>
| * the command line to customize test behavior, like the allocation mode used | ||
| * for creating the default memory resource. | ||
| * | ||
| * When CUOPT_DISABLE_TEST_MAIN is defined (e.g. when compiling multiple test |
There was a problem hiding this comment.
Do we need this macro? What about deleting CUOPT_TEST_PROGRAM_MAIN in the test files that no longer need a main?
…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>
Summary
Prerequisite structural work for enabling
-fvisibility=hiddenonlibcuopt.so(issue #1213). An earlier attempt usingCUOPT_INTERNAL_EXPORTannotations (PR #1574) was closed after the scope of symbols to annotate proved too large (including static data members that cannot carry visibility attributes).This PR puts the test infrastructure in place so that hidden visibility can be enabled in a follow-up.
cuopt_objsOBJECT library — compile sources once; bothcuopt(shared) andcuopt_static(static,BUILD_TESTSonly) consume the same object filesSTATIC_LIBoption inConfigureTest— linkscuopt_staticand omitscuopttestutils(which transitively pulls in the shared lib) for tests that need compiled internal symbolsROUTING_INTERNAL_TEST— new target consolidating GES solver, cross-route insertion, local-search candidate, and top-k tests; also registers four previously unbuilt files (l0_scross_test.cu,local_search_cand_test.cu,load_balancing_test.cu,feasibility_jump_tests.cu)GRPC_CLIENT_TESTandGRPC_INTEGRATION_TESTnow compile the gRPC client/mapper sources directly into the test binary, following the pattern already used byGRPC_PIPE_SERIALIZATION_TEST; no internal symbols need exporting from the shared libcuOptIsCusparseRuntimeMixedPrecisionSupported()— promoted from an internal function to the public C API;c_api_tests.cppupdated to call the public version