Add C api callbacks for getting and setting solutions #779
Conversation
📝 WalkthroughWalkthroughAdd optional user_data propagation to MIP incumbent get/set callbacks across C, C++, and Python: new C API registration, extended callback interfaces and storage, updated invocation sites including presolve/papilo plumbing, solver-stats accessor changes, tests, bindings, examples, and server/job-queue wiring. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-githubusercontent-com.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@cpp/include/cuopt/linear_programming/cuopt_c.h`:
- Around line 706-728: The Doxygen for cuOptSetMipGetSolutionCallback and
cuOptSetMipSetSolutionCallback is missing documentation for the new user_data
parameter; update both comment blocks to add a `@param`[in] user_data description
(e.g., "User-defined pointer passed through to the callback") and mention that
it will be forwarded to the respective
cuOptMipGetSolutionCallback/cuOptMipSetSolutionCallback when invoked so the
public API documents the parameter contract.
In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp`:
- Around line 37-41: Update the Doxygen block for set_mip_callback to document
the new user_data parameter: add a brief `@param` description for user_data
explaining it is an opaque pointer passed to the callback (e.g., "pointer to
user-defined data forwarded to the callback"), and ensure the existing `@param`
for callback remains accurate; modify the comment above the function declaration
in solver_settings.hpp (the set_mip_callback declaration) so the public API docs
enumerate both callback and user_data.
In `@cpp/include/cuopt/linear_programming/utilities/internals.hpp`:
- Around line 34-36: Add brief Doxygen comments above the public accessor
methods set_user_data and get_user_data describing their purpose: annotate
set_user_data with "Store user-defined context data for callback invocation."
and get_user_data with "Retrieve user-defined context data passed to callbacks."
Place these comments immediately above the corresponding method declarations in
internals.hpp so the public solver callback interface methods are documented per
the header-file documentation guideline.
In `@cpp/src/mip/diversity/diversity_manager.cu`:
- Around line 446-450: There is an unconditional exit(0) that aborts the solver
flow and prevents LP solution handling; remove the exit(0) invocation so
execution continues into the subsequent LP handling (the if
(ls.lp_optimal_exists) block) after calling clamp_within_var_bounds, ensuring
normal callback and MIP behavior; search for the symbols
clamp_within_var_bounds, exit(0), and ls.lp_optimal_exists to locate and delete
the exit call so control can proceed.
In `@cpp/src/mip/solver_settings.cu`:
- Around line 27-31: The set_mip_callback function currently pushes a nullptr
into mip_callbacks_ because it only guards the set_user_data call; update
mip_solver_settings_t<i_t, f_t>::set_mip_callback to first check if callback is
nullptr and if so do not push it (either return early or ignore the call),
otherwise call callback->set_user_data(user_data) and then push_back(callback)
so mip_callbacks_ never contains null entries; this ensures later dereferences
of entries in mip_callbacks_ (e.g., where callbacks are invoked) are safe.
In `@python/cuopt/cuopt/linear_programming/internals/internals.pyx`:
- Around line 70-76: The runtime AttributeError happens because _user_data is
assigned dynamically in the cdef classes; declare the attribute in both callback
extension types (e.g., in the class body of GetSolutionCallback and
SetSolutionCallback) as a Cython Python-object attribute like "cdef object
_user_data" (or "cdef public object _user_data" if you need external access),
then keep the existing __init__ assignment and the get_user_data_ptr cast that
uses self._user_data; ensure the declaration appears in both classes so lines
assigning/reading _user_data no longer raise.
🧹 Nitpick comments (1)
cpp/include/cuopt/linear_programming/solver_settings.hpp (1)
84-85: Documentuser_dataownership/lifetime in the public API.Adding a brief Doxygen note will clarify that the pointer must remain valid for the duration of callback usage and that it’s passed through verbatim.
✍️ Suggested doc addition
- void set_mip_callback(internals::base_solution_callback_t* callback = nullptr, - void* user_data = nullptr); + /** + * Register a MIP solution callback. + * `@param` callback Callback instance (must outlive solver usage). + * `@param` user_data Opaque pointer passed back to the callback; must remain valid + * while the callback can be invoked. + */ + void set_mip_callback(internals::base_solution_callback_t* callback = nullptr, + void* user_data = nullptr);As per coding guidelines, public headers should keep docs in sync with API changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/include/cuopt/linear_programming/utilities/internals.hppcpp/src/linear_programming/cuopt_c.cppcpp/src/math_optimization/solver_settings.cucpp/src/mip/diversity/diversity_manager.cucpp/src/mip/diversity/population.cucpp/src/mip/solver_settings.cucpp/tests/linear_programming/c_api_tests/c_api_test.ccpp/tests/mip/incumbent_callback_test.cupython/cuopt/cuopt/linear_programming/internals/internals.pyxpython/cuopt/cuopt/linear_programming/solver/solver.pxdpython/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxpython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/linear_programming/cuopt_c.cpp
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{cu,cuh,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh,cpp,hpp,h}: Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
For concurrent CUDA operations (barriers, async operations), explicitly create and manage dedicated streams instead of reusing the default stream; document stream lifecycle
Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution
Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Check that hard-coded GPU de...
Files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cucpp/include/cuopt/linear_programming/utilities/internals.hpp
**/*.{h,hpp,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hpppython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pycpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/include/cuopt/linear_programming/utilities/internals.hpp
**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cpp,hpp,h}: Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files
Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/include/cuopt/linear_programming/utilities/internals.hpp
**/*.{cu,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cucpp/include/cuopt/linear_programming/utilities/internals.hpp
cpp/include/cuopt/**/*
⚙️ CodeRabbit configuration file
cpp/include/cuopt/**/*: For public header files (C++ API):
- Check if new public functions/classes have documentation comments (Doxygen format)
- Flag API changes that may need corresponding docs/ updates
- Verify parameter descriptions match actual types/behavior
- Suggest documenting thread-safety, GPU requirements, and numerical behavior
- For breaking changes, recommend updating docs and migration guides
Files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/include/cuopt/linear_programming/utilities/internals.hpp
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/src/mip/diversity/population.cucpp/src/mip/diversity/diversity_manager.cu
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/src/mip/diversity/population.cucpp/src/mip/diversity/diversity_manager.cu
**/*test*.{cpp,cu,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*test*.{cpp,cu,py}: Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Ensure test isolation: prevent GPU state, cached memory, and global variables from leaking between test cases; verify each test independently initializes its environment
Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations
Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Files:
cpp/tests/mip/incumbent_callback_test.cu
🧠 Learnings (23)
📚 Learning: 2025-10-22T14:25:22.899Z
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 527
File: cpp/src/mip/diversity/lns/rins.cu:167-175
Timestamp: 2025-10-22T14:25:22.899Z
Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Check that hard-coded GPU device IDs and resource limits are made configurable; abstract multi-backend support for different CUDA versions
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cu
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cu
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cpp,hpp,h} : Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cu
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Verify error propagation from CUDA to user-facing APIs is complete; ensure CUDA errors are caught and mapped to meaningful user error codes
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.cu : Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cu
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/src/math_optimization/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hppcpp/src/mip/diversity/diversity_manager.cu
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : For concurrent CUDA operations (barriers, async operations), explicitly create and manage dedicated streams instead of reusing the default stream; document stream lifecycle
Applied to files:
cpp/include/cuopt/linear_programming/cuopt_c.hcpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/math_optimization/solver_settings.cucpp/src/mip/solver_settings.cucpp/tests/mip/incumbent_callback_test.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/include/cuopt/linear_programming/utilities/callbacks_implems.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Ensure test isolation: prevent GPU state, cached memory, and global variables from leaking between test cases; verify each test independently initializes its environment
Applied to files:
cpp/include/cuopt/linear_programming/solver_settings.hppcpp/src/mip/solver_settings.cucpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/mip/diversity/population.cucpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Applied to files:
cpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2026-01-15T20:41:51.616Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 697
File: cpp/src/dual_simplex/diving_heuristics.cpp:200-228
Timestamp: 2026-01-15T20:41:51.616Z
Learning: In cpp/src/dual_simplex/diving_heuristics.cpp, the calculate_variable_locks function incorrectly indexes lp_problem.lower[nz_row] and lp_problem.upper[nz_row] where nz_row is a row index (0 to num_rows-1), but lower and upper are variable bounds of size num_cols indexed by column. This causes semantic errors and potential out-of-bounds access when num_rows > num_cols. Since lp_problem_t constraints are always equalities (A*x = rhs) after presolve, lock counting should treat each nonzero as contributing equally to both up_lock and down_lock, rather than checking constraint bounds.
Applied to files:
cpp/src/mip/diversity/diversity_manager.cu
📚 Learning: 2025-12-04T20:09:09.264Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 602
File: cpp/src/linear_programming/solve.cu:732-742
Timestamp: 2025-12-04T20:09:09.264Z
Learning: In cpp/src/linear_programming/solve.cu, the barrier solver does not currently return INFEASIBLE or UNBOUNDED status. It only returns OPTIMAL, TIME_LIMIT, NUMERICAL_ISSUES, or CONCURRENT_LIMIT.
Applied to files:
cpp/src/mip/diversity/diversity_manager.cucpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-12-06T00:22:48.638Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/tests/linear_programming/c_api_tests/c_api_test.c:1033-1048
Timestamp: 2025-12-06T00:22:48.638Z
Learning: In cuOPT's quadratic programming API, when a user provides a quadratic objective matrix Q via set_quadratic_objective_matrix or the C API functions cuOptCreateQuadraticProblem/cuOptCreateQuadraticRangedProblem, the API internally computes Q_symmetric = Q + Q^T and the barrier solver uses 0.5 * x^T * Q_symmetric * x. From the user's perspective, the convention is x^T Q x. For a diagonal Q with values [q1, q2, ...], the resulting quadratic terms are q1*x1^2 + q2*x2^2 + ...
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
📚 Learning: 2025-12-04T04:11:12.640Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/src/dual_simplex/scaling.cpp:68-76
Timestamp: 2025-12-04T04:11:12.640Z
Learning: In the cuOPT dual simplex solver, CSR/CSC matrices (including the quadratic objective matrix Q) are required to have valid dimensions and indices by construction. Runtime bounds checking in performance-critical paths like matrix scaling is avoided to prevent slowdowns. Validation is performed via debug-only check_matrix() calls wrapped in `#ifdef` CHECK_MATRIX.
Applied to files:
cpp/tests/linear_programming/c_api_tests/c_api_test.c
🧬 Code graph analysis (7)
cpp/include/cuopt/linear_programming/cuopt_c.h (1)
cpp/src/linear_programming/cuopt_c.cpp (4)
cuOptSetMipGetSolutionCallback(753-764)cuOptSetMipGetSolutionCallback(753-755)cuOptSetMipSetSolutionCallback(766-777)cuOptSetMipSetSolutionCallback(766-768)
cpp/include/cuopt/linear_programming/solver_settings.hpp (2)
cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
callback(40-41)cpp/include/cuopt/linear_programming/utilities/internals.hpp (1)
user_data(35-35)
cpp/src/math_optimization/solver_settings.cu (5)
cpp/src/mip/solver_settings.cu (2)
set_mip_callback(27-32)set_mip_callback(27-28)python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py (1)
set_mip_callback(248-305)cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
callback(40-41)cpp/include/cuopt/linear_programming/solver_settings.hpp (1)
callback(84-85)cpp/include/cuopt/linear_programming/utilities/internals.hpp (1)
user_data(35-35)
cpp/src/mip/solver_settings.cu (3)
cpp/include/cuopt/linear_programming/mip/solver_settings.hpp (1)
callback(40-41)cpp/include/cuopt/linear_programming/solver_settings.hpp (1)
callback(84-85)cpp/include/cuopt/linear_programming/utilities/internals.hpp (1)
user_data(35-35)
cpp/tests/mip/incumbent_callback_test.cu (3)
cpp/include/cuopt/linear_programming/utilities/callbacks_implems.hpp (12)
data(20-29)data(20-20)data(31-39)data(31-31)data(41-56)data(41-41)data(63-72)data(63-63)data(74-82)data(74-74)data(84-99)data(84-84)cpp/include/cuopt/linear_programming/utilities/internals.hpp (3)
data(47-47)data(56-56)user_data(35-35)cpp/src/linear_programming/cuopt_c.cpp (2)
data(57-63)data(57-57)
cpp/include/cuopt/linear_programming/utilities/internals.hpp (2)
cpp/include/cuopt/linear_programming/utilities/callbacks_implems.hpp (12)
data(20-29)data(20-20)data(31-39)data(31-31)data(41-56)data(41-41)data(63-72)data(63-63)data(74-82)data(74-74)data(84-99)data(84-84)cpp/src/linear_programming/cuopt_c.cpp (4)
data(57-63)data(57-57)data(73-78)data(73-73)
cpp/tests/linear_programming/c_api_tests/c_api_test.c (1)
cpp/src/linear_programming/cuopt_c.cpp (16)
cuOptCreateProblem(140-195)cuOptCreateProblem(140-153)cuOptCreateSolverSettings(619-625)cuOptCreateSolverSettings(619-619)cuOptSetMipGetSolutionCallback(753-764)cuOptSetMipGetSolutionCallback(753-755)cuOptSetMipSetSolutionCallback(766-777)cuOptSetMipSetSolutionCallback(766-768)cuOptSolve(792-840)cuOptSolve(792-794)cuOptDestroyProblem(387-393)cuOptDestroyProblem(387-387)cuOptDestroySolverSettings(627-632)cuOptDestroySolverSettings(627-627)cuOptDestroySolution(842-861)cuOptDestroySolution(842-842)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.11, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.1.0, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.1.0, 3.10, amd64, rockylinux8
🔇 Additional comments (16)
python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx (1)
185-198: LGTM — user_data is forwarded to the native callback.Nice, this preserves backward compatibility while wiring the user context into
set_mip_callback.python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py (1)
248-305: LGTM — user_data is preserved on the callback.This keeps the user context alive and aligns with the new callback plumbing.
cpp/src/math_optimization/solver_settings.cu (2)
3-3: No review comment needed.
385-388: LGTM: user_data is forwarded to MIP settings.Clean passthrough that aligns with the new callback API.
cpp/include/cuopt/linear_programming/solver_settings.hpp (1)
3-3: No review comment needed.cpp/src/mip/solver_settings.cu (1)
3-3: No review comment needed.python/cuopt/cuopt/linear_programming/solver/solver.pxd (2)
1-1: No review comment needed.
80-82: LGTM: Cython declaration matches the updated C++ API.The user_data parameter is correctly surfaced here.
cpp/src/mip/diversity/population.cu (2)
3-3: No review comment needed.
302-303: LGTM: user_data is forwarded to both GET and SET callbacks.This keeps the callback context intact through the MIP solution pipeline.
Also applies to: 324-326
python/cuopt/cuopt/linear_programming/internals/internals.pyx (1)
24-32: Signature alignment with native callbacks looks correct.cpp/tests/mip/incumbent_callback_test.cu (2)
43-45: LGTM for the updated set_solution signature.
68-70: LGTM for the updated get_solution signature.cpp/tests/linear_programming/c_api_tests/c_api_test.c (2)
12-12: No issues with the CUDA runtime include.
135-296: Good callback test coverage and cleanup flow.cpp/include/cuopt/linear_programming/utilities/callbacks_implems.hpp (1)
41-55: Backward-compatible user_data handling looks good.Also applies to: 84-98
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@python/cuopt/cuopt/linear_programming/internals/internals.pyx`:
- Around line 86-97: The _user_data attribute and get_user_data_ptr() are
duplicated and lack a setter in GetSolutionCallback/SetSolutionCallback; move
_user_data, its initialization and the get_user_data_ptr() implementation into
the PyCallback base class (add initialization in PyCallback.__init__ and expose
a setter there), remove the duplicated _user_data and get_user_data_ptr() from
GetSolutionCallback and SetSolutionCallback, and ensure subclasses call
super().__init__() (or otherwise inherit) so the native_callback.pyCallbackClass
assignment in their __init__ still references the correct self.
- Around line 66-77: The Cython cdef attribute _user_data is currently C-level
only so Python assigns a shadow attribute (making user_data passed to
set_mip_callback ignored); expose it to Python by adding a Python-accessible
property or declaring it cdef public (e.g., provide a user_data property with
getter/setter that stores the C-level _user_data and returns/accepts a Python
object) in the cdef class (adjust __init__ to use the new property if needed),
ensure get_user_data_ptr() reads the same backing field, and update caller sites
(solver_settings.set_mip_callback and the analogous SetSolutionCallback usage)
to set callback.user_data = user_data instead of callback._user_data = user_data
so the Python-assigned value is visible to get_user_data_ptr().
🧹 Nitpick comments (1)
python/cuopt/cuopt/linear_programming/internals/internals.pyx (1)
72-77: Document the lifetime contract for the returned pointer.The returned
uintptr_tis a borrowed reference — noPy_INCREFis performed. The pointer remains valid only whileself(the callback object) is alive. If C code caches this pointer beyond the callback's lifetime, it risks use-after-free.Consider adding a docstring clarifying that the caller must keep the callback object alive while using the pointer:
cpdef uintptr_t get_user_data_ptr(self): """Return raw pointer to user_data. Valid only while this callback is alive."""
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
python/cuopt/cuopt/linear_programming/internals/internals.pyxpython/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
🚧 Files skipped from review as they are similar to previous changes (1)
- python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
🔇 Additional comments (2)
python/cuopt/cuopt/linear_programming/internals/internals.pyx (2)
11-11: LGTM!Using
cpython.ref cimport PyObjectis the idiomatic Cython approach, cleaner than a manual extern declaration.
21-29: LGTM!The extern declarations correctly extend the callback signatures to include the
user_dataparameter, enabling per-callback user context propagation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| /** | ||
| * @brief Callback for injecting MIP solutions with user context. | ||
| * | ||
| * @param[out] solution - Device pointer to solution values to set. |
There was a problem hiding this comment.
We probably should mention the dimension of the solution_values (i.e. that it needs to be equal to the number of variables in the original problem).
Also, do we crush the solution from the original variables into the presolve variables when MIP presolve is turned on?
There was a problem hiding this comment.
And this should probably be a host pointer rather than a device pointer
There was a problem hiding this comment.
@chris-maes Good catch! All callbacks are disabled if 3rd party presolve(Papilo) is enabled. Our own presolve changes are handled though. I believe this is a blocker for primal integral experiments. I will give it a try.
There was a problem hiding this comment.
I have enabled third party presolve and handled crushing/uncrushing of solutions.
| * @param[in] solution - Device pointer to incumbent solution values. | ||
| * @param[in] objective_value - Device pointer to incumbent objective value. |
There was a problem hiding this comment.
I wonder if these should be host pointers instead, in terms of API design. So far the API has been abstracting away the reality of the CUDA implementation (such as the corresponding context, the streams used...) away from the user, so this may represent a break in the philosophy. Plus we don't expose the GPU problem structure either, so it is likely the user will have to copy it back to the host either way to do any useful work
Maybe device-space callbacks could be implemented later on if need be? We might have to discuss this
There was a problem hiding this comment.
I agree. The rest of the API uses abstracts out the device. Probably we should provide a host pointer here.
There was a problem hiding this comment.
We are agnostic to device/host memory in all APIs AFAIK. I can modify it to "Host/Device" pointer? The only caveat of supporting device pointers officialy is that the copies will be done by internal stream.
There was a problem hiding this comment.
I made all callbacks to input/output host pointers.
…acks are failing due to device host divide, pending decision
@rgsl888prabhu the callback structure is mostly the same. What changed is we changed pointers of callbacks from device to host. Also an additional parameter user_data is added. These are reflected in python tests, examples and docs. (I didn't write a new one) |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp/src/mip/solve.cu (1)
190-224: Presolve rebuild should preserve user tolerances.
Reconstructing the reduced problem withoutsettings.get_tolerances()can reset integrality/feasibility tolerances to defaults, changing behavior for users who customize tolerances.🔧 Proposed fix
- problem = detail::problem_t<i_t, f_t>(presolve_result->reduced_problem); + problem = detail::problem_t<i_t, f_t>(presolve_result->reduced_problem, + settings.get_tolerances());python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py (1)
248-310: Makeuser_dataparameter optional to maintain backward compatibility.The previous API signature was
set_mip_callback(self, callback). This change addsuser_dataas a required parameter, breaking existing code. Per the coding guidelines, maintain backward compatibility with deprecation warnings by makinguser_data=Noneand warning when omitted.♻️ Suggested compatibility patch
+import warnings from enum import IntEnum, auto @@ - def set_mip_callback(self, callback, user_data): + def set_mip_callback(self, callback, user_data=None): @@ - user_data : object - User context passed to the callback. + user_data : object, optional + User context passed to the callback. Passing None is deprecated. @@ - if callback is not None: - callback.user_data = user_data + if user_data is None: + warnings.warn( + "set_mip_callback(callback) without user_data is deprecated; " + "pass user_data explicitly.", + DeprecationWarning, + stacklevel=2, + ) + if callback is not None: + callback.user_data = user_data self.mip_callbacks.append(callback)cpp/src/mip/presolve/third_party_presolve.cpp (1)
29-30: Static globals create thread-safety issues in concurrent solver usage.
post_solve_storage_andmaximize_are static globals at module scope. If multiplethird_party_presolve_tinstances callapply()concurrently from different threads (e.g., in a server environment solving independent problems), these statics will race. A subsequentundo()call in one thread could read incorrect postsolve data written by another thread'sapply().Convert these to instance members of
third_party_presolve_t.
🤖 Fix all issues with AI agents
In `@cpp/src/mip/presolve/third_party_presolve.cpp`:
- Around line 513-527: uncrush_primal_solution currently uses the shared static
post_solve_storage_ which is not thread-safe; change the implementation to use
an instance-local copy (or guard access with a mutex) of post_solve_storage_
when constructing papilo::Postsolve<f_t> so concurrent presolvers do not race on
shared state, and add explicit handling for papilo::PostsolveStatus::kFailed
after calling post_solver.undo (e.g., throw a descriptive exception or log and
return an error) instead of only relying on check_postsolve_status; references:
third_party_presolve_t::uncrush_primal_solution, post_solve_storage_,
papilo::Postsolve::undo, and papilo::PostsolveStatus::kFailed.
In `@cpp/src/mip/problem/presolve_data.cuh`:
- Around line 78-93: Validate sizes and ranges before storing or using Papilo
presolve data: in set_papilo_presolve_data check that reduced_to_original.size()
== original_to_reduced.size() or that each mapping lies within [0,
original_num_variables) and that original_num_variables is > 0, and reject or
assert/log + do not set papilo_presolve_ptr/papilo_original_num_variables if
these checks fail; in papilo_uncrush_assignment verify the incoming
assignment.size() equals the reduced problem size expected by
papilo_presolve_ptr (or matches
original_to_reduced.size()/reduced_to_original.size() as appropriate) and
error/assert before calling the third-party uncrush_primal_solution to avoid
silent corruption. Use the existing symbols papilo_presolve_ptr,
reduced_to_original, original_to_reduced, papilo_original_num_variables,
papilo_uncrush_assignment and uncrush_primal_solution in your checks and
fail-fast when inconsistencies are detected.
In `@cpp/tests/mip/incumbent_callback_test.cu`:
- Around line 131-132: The test references nonexistent MPS files causing runtime
failure; update the test_instances vector in incumbent_callback_test.cu
(variable test_instances) to use existing dataset filenames (e.g., replace
"mip/swath1.mps" with a valid file from datasets/mip/, and if enabling the other
entries, use "50v-10-free-bound.mps" and "neos5-free-bound.mps" instead of
"mip/50v-10.mps" and "mip/neos5.mps"), or alternatively add the missing MPS
files to the repository; modify the string literals in the test_instances
initializer accordingly so they match actual files.
In `@cpp/tests/mip/mip_utils.cuh`:
- Around line 142-143: The top-level std::vector<double> viol declared alongside
residual is unused and is later shadowed by a loop variable named viol; remove
the outer declaration of viol (the std::vector<double> viol that parallels
residual and uses constraint_lower_bounds.size()) to eliminate dead code and the
shadowing issue, and apply the same removal to the corresponding unused viol
declaration in the other overload (the one at the earlier occurrence referenced
in the review).
🧹 Nitpick comments (8)
cpp/tests/linear_programming/utilities/pdlp_test_utilities.cuh (1)
59-81: Consider extracting shared logic to reduce duplication.The new overload is correct. Both overloads share identical core logic (size check, transform with
std::multiplies, reduce,EXPECT_NEAR). You could extract this into a helper that acceptsconst std::vector<double>&, then have thedevice_uvectoroverload call it afterhost_copy.♻️ Optional refactor to consolidate logic
+// Helper for objective sanity check on host vectors +static void test_objective_sanity_impl( + const std::vector<double>& primal_vars, + const std::vector<double>& c_vector, + double objective_value, + double epsilon) +{ + if (primal_vars.size() != c_vector.size()) { + EXPECT_EQ(primal_vars.size(), c_vector.size()); + return; + } + std::vector<double> out(primal_vars.size()); + std::transform(primal_vars.cbegin(), + primal_vars.cend(), + c_vector.cbegin(), + out.begin(), + std::multiplies<double>()); + double sum = std::reduce(out.cbegin(), out.cend(), 0.0); + EXPECT_NEAR(sum, objective_value, epsilon); +} + // Compute on the CPU x * c to check that the returned objective value is correct static void test_objective_sanity( const cuopt::mps_parser::mps_data_model_t<int, double>& op_problem, const rmm::device_uvector<double>& primal_solution, double objective_value, double epsilon = tolerance) { const auto primal_vars = host_copy(primal_solution, primal_solution.stream()); - const auto& c_vector = op_problem.get_objective_coefficients(); - if (primal_vars.size() != c_vector.size()) { - EXPECT_EQ(primal_vars.size(), c_vector.size()); - return; - } - std::vector<double> out(primal_vars.size()); - std::transform(primal_vars.cbegin(), - primal_vars.cend(), - c_vector.cbegin(), - out.begin(), - std::multiplies<double>()); - - double sum = std::reduce(out.cbegin(), out.cend(), 0.0); - - EXPECT_NEAR(sum, objective_value, epsilon); + test_objective_sanity_impl(primal_vars, op_problem.get_objective_coefficients(), objective_value, epsilon); } // Compute on the CPU x * c to check that the returned objective value is correct static void test_objective_sanity( const cuopt::mps_parser::mps_data_model_t<int, double>& op_problem, const std::vector<double>& primal_solution, double objective_value, double epsilon = tolerance) { - const auto& c_vector = op_problem.get_objective_coefficients(); - if (primal_solution.size() != c_vector.size()) { - EXPECT_EQ(primal_solution.size(), c_vector.size()); - return; - } - std::vector<double> out(primal_solution.size()); - std::transform(primal_solution.cbegin(), - primal_solution.cend(), - c_vector.cbegin(), - out.begin(), - std::multiplies<double>()); - - double sum = std::reduce(out.cbegin(), out.cend(), 0.0); - - EXPECT_NEAR(sum, objective_value, epsilon); + test_objective_sanity_impl(primal_solution, op_problem.get_objective_coefficients(), objective_value, epsilon); }cpp/src/dual_simplex/branch_and_bound.cpp (1)
326-351: Avoid unconditional printf under mutex; use debug logger after unlock.
set_new_solutionis a hot path and can be called frequently; the unconditionalprintfinside the lock can flood logs and add contention. Consider switching tolog.debugand emitting after releasingmutex_upper_.♻️ Proposed refactor
- mutex_upper_.lock(); - if (obj < upper_bound_) { + bool better_than_upper = false; + mutex_upper_.lock(); + if (obj < upper_bound_) { + better_than_upper = true; f_t primal_err; f_t bound_err; i_t num_fractional; is_feasible = check_guess( original_lp_, settings_, var_types_, crushed_solution, primal_err, bound_err, num_fractional); if (is_feasible) { upper_bound_ = obj; incumbent_.set_incumbent_solution(obj, crushed_solution); } else { attempt_repair = true; constexpr bool verbose = false; if (verbose) { settings_.log.printf( "Injected solution infeasible. Constraint error %e bound error %e integer infeasible " "%d\n", primal_err, bound_err, num_fractional); } } - } else { - settings_.log.printf( - "[DEBUG] set_new_solution: Solution objective not better than current upper_bound_. Not " - "accepted.\n"); } mutex_upper_.unlock(); + if (!better_than_upper) { + settings_.log.debug( + "set_new_solution: solution objective not better than current upper_bound_; not accepted."); + }python/cuopt_server/cuopt_server/utils/linear_programming/solver.py (1)
81-88: Consider replacing assertion with explicit error handling for production robustness.Using
assertfor runtime validation in a server context can be problematic—assertions can be disabled with-Oflag, andAssertionErrormay not provide meaningful diagnostics to API consumers. Consider raising a more specific exception.♻️ Suggested improvement
def get_solution(self, solution, solution_cost, user_data): - if user_data is not None: - assert user_data == self.req_id + if user_data is not None and user_data != self.req_id: + raise ValueError( + f"Callback user_data mismatch: expected {self.req_id}, got {user_data}" + ) self.sender( self.req_id, solution.tolist(), float(solution_cost[0]), )cpp/tests/mip/mip_utils.cuh (1)
45-70: Consider extracting common validation logic to reduce duplication.This overload duplicates the validation logic from the device-vector version (lines 17-43). While acceptable for test utilities, a templated helper or a version that operates on raw
const double*could serve both callers and reduce maintenance burden.♻️ Example refactor approach
// Internal helper that works with raw pointers static void test_variable_bounds_impl( const double* lower_bound_ptr, const double* upper_bound_ptr, const double* assignment_ptr, std::size_t size, double tolerance) { std::vector<int> indices(size); std::iota(indices.begin(), indices.end(), 0); bool result = std::all_of(indices.begin(), indices.end(), [=](int idx) { bool res = true; if (lower_bound_ptr != nullptr) { res = res && (assignment_ptr[idx] >= lower_bound_ptr[idx] - tolerance); } if (upper_bound_ptr != nullptr) { res = res && (assignment_ptr[idx] <= upper_bound_ptr[idx] + tolerance); } return res; }); EXPECT_TRUE(result); } // Then both overloads call the implcpp/tests/linear_programming/c_api_tests/c_api_test.c (1)
180-279: Add a data-integrity assertion for callbacks.
Right now the test only checks call counts and allocation errors. Consider validating that the objective seen in the get-solution callback matches the solver’s objective to confirm correct data transfer. As per coding guidelines.✅ Suggested enhancement
if (context.get_calls < 1 || context.set_calls < 1) { printf("Expected callbacks to be called at least once\n"); status = CUOPT_INVALID_ARGUMENT; goto DONE; } + + cuopt_float_t objective_value = 0; + status = cuOptGetObjectiveValue(solution, &objective_value); + if (status != CUOPT_SUCCESS) { + printf("Error getting objective value\n"); + goto DONE; + } + if (objective_value != context.last_objective) { + printf("Callback objective mismatch\n"); + status = CUOPT_INVALID_ARGUMENT; + goto DONE; + }cpp/src/mip/diversity/population.cu (1)
321-352: Verify size consistency when papilo presolve resizes the problem.The
incumbent_assignmentis allocated withcallback_num_variables(either original problem size or papilo original size), but the assertion on line 356 checks againstoutside_sol.assignment.size(). Afterpapilo_crush_assignment(line 350), theincumbent_assignmentwill be resized to the reduced problem size, which should matchoutside_sol.assignment.size().However, consider adding a comment clarifying this flow, as the size of
incumbent_assignmentchanges after crushing.📝 Suggested documentation improvement
if (problem_ptr->has_papilo_presolve_data()) { problem_ptr->papilo_crush_assignment(incumbent_assignment); } + // Note: incumbent_assignment is now resized to the reduced (presolved) problem size if (context.settings.mip_scaling) { context.scaling.scale_solutions(incumbent_assignment); }cpp/src/mip/problem/presolve_data.cu (2)
119-132: Host-side loop for free variable handling could be optimized.Lines 123-128 iterate over the assignment on the host to undo the free variable decomposition. This requires a device-to-host copy, host computation, and host-to-device copy. For large problems, this could be a performance bottleneck.
Consider keeping this on the GPU with a thrust kernel similar to the forward transformation in
pre_process_assignment.♻️ GPU-based alternative
- auto h_assignment = cuopt::host_copy(current_assignment, problem.handle_ptr->get_stream()); - cuopt_assert(additional_var_id_per_var.size() == h_assignment.size(), "Size mismatch"); - cuopt_assert(additional_var_used.size() == h_assignment.size(), "Size mismatch"); - for (i_t i = 0; i < (i_t)h_assignment.size(); ++i) { - if (additional_var_used[i]) { - cuopt_assert(additional_var_id_per_var[i] != -1, "additional_var_id_per_var is not set"); - h_assignment[i] -= h_assignment[additional_var_id_per_var[i]]; - } - } - raft::copy(current_assignment.data(), - h_assignment.data(), - h_assignment.size(), - problem.handle_ptr->get_stream()); + auto d_additional_var_used = + cuopt::device_copy(additional_var_used, problem.handle_ptr->get_stream()); + auto d_additional_var_id_per_var = + cuopt::device_copy(additional_var_id_per_var, problem.handle_ptr->get_stream()); + thrust::for_each( + problem.handle_ptr->get_thrust_policy(), + thrust::make_counting_iterator<i_t>(0), + thrust::make_counting_iterator<i_t>(current_assignment.size()), + [additional_var_used = d_additional_var_used.data(), + additional_var_id_per_var = d_additional_var_id_per_var.data(), + assgn = current_assignment.data()] __device__(auto idx) { + if (additional_var_used[idx]) { + assgn[idx] -= assgn[additional_var_id_per_var[idx]]; + } + });
181-203: Bounds check silently sets value to zero for invalid indices.Lines 191-194 set
reduced_assignment[i] = f_t{0}whenoriginal_idxis out of range. This silent fallback could mask bugs in the mapping construction. Consider logging a warning or using an assertion in debug builds.📝 Suggested improvement
if (original_idx < 0 || static_cast<size_t>(original_idx) >= h_assignment.size()) { + CUOPT_LOG_DEBUG("papilo_crush_assignment: invalid original_idx %d at reduced index %zu", + original_idx, i); reduced_assignment[i] = f_t{0}; continue; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cpp/tests/mip/mip_utils.cuh`:
- Around line 130-159: Add a defensive size check before the CSR SpMV in
test_constraint_sanity_per_row: compute the maximum index in indices (e.g., via
std::max_element) and assert with cuopt_assert that solution.size() > max_index
to prevent out-of-bounds access when indexing solution[indices[j]]; update the
test_constraint_sanity_per_row function to perform this check (and handle the
case of empty indices/offsets appropriately) before entering the nested loops.
🧹 Nitpick comments (2)
cpp/tests/linear_programming/utilities/pdlp_test_utilities.cuh (1)
59-81: Implementation is correct; consider extracting common logic to reduce duplication.The new overload correctly validates the objective value computation. However, it duplicates nearly all logic from the
rmm::device_uvectoroverload (lines 35-57).♻️ Optional: Extract common logic into a helper
+// Helper to compute and verify objective from iterators +template <typename Iter> +static void verify_objective_value( + Iter primal_begin, + Iter primal_end, + const std::vector<double>& c_vector, + double objective_value, + double epsilon) +{ + std::vector<double> out(std::distance(primal_begin, primal_end)); + std::transform(primal_begin, primal_end, c_vector.cbegin(), out.begin(), std::multiplies<double>()); + double sum = std::reduce(out.cbegin(), out.cend(), 0.0); + EXPECT_NEAR(sum, objective_value, epsilon); +} + // Compute on the CPU x * c to check that the returned objective value is correct static void test_objective_sanity( const cuopt::mps_parser::mps_data_model_t<int, double>& op_problem, const rmm::device_uvector<double>& primal_solution, double objective_value, double epsilon = tolerance) { const auto primal_vars = host_copy(primal_solution, primal_solution.stream()); const auto& c_vector = op_problem.get_objective_coefficients(); if (primal_vars.size() != c_vector.size()) { EXPECT_EQ(primal_vars.size(), c_vector.size()); return; } - std::vector<double> out(primal_vars.size()); - std::transform(primal_vars.cbegin(), - primal_vars.cend(), - c_vector.cbegin(), - out.begin(), - std::multiplies<double>()); - - double sum = std::reduce(out.cbegin(), out.cend(), 0.0); - - EXPECT_NEAR(sum, objective_value, epsilon); + verify_objective_value(primal_vars.cbegin(), primal_vars.cend(), c_vector, objective_value, epsilon); }Then the
std::vectoroverload can similarly delegate toverify_objective_value.cpp/src/mip/problem/problem.cu (1)
1764-1806: Avoid const_cast by making papilo helpers const-correct.The wrapper uses
const_casteven though the presolve helpers appear read-only onproblem. Consider changing the presolve_data signatures to takeconst problem_t&and update call sites so the wrapper stays const-safe.♻️ Suggested refactor (const-correctness)
-void papilo_uncrush_assignment(problem_t<i_t, f_t>& problem, +void papilo_uncrush_assignment(const problem_t<i_t, f_t>& problem, rmm::device_uvector<f_t>& assignment) const; -void papilo_crush_assignment(problem_t<i_t, f_t>& problem, +void papilo_crush_assignment(const problem_t<i_t, f_t>& problem, rmm::device_uvector<f_t>& assignment) const;-void problem_t<i_t, f_t>::papilo_uncrush_assignment(rmm::device_uvector<f_t>& assignment) const -{ - presolve_data.papilo_uncrush_assignment(const_cast<problem_t&>(*this), assignment); -} +void problem_t<i_t, f_t>::papilo_uncrush_assignment(rmm::device_uvector<f_t>& assignment) const +{ + presolve_data.papilo_uncrush_assignment(*this, assignment); +} -void problem_t<i_t, f_t>::papilo_crush_assignment(rmm::device_uvector<f_t>& assignment) const -{ - presolve_data.papilo_crush_assignment(const_cast<problem_t&>(*this), assignment); -} +void problem_t<i_t, f_t>::papilo_crush_assignment(rmm::device_uvector<f_t>& assignment) const +{ + presolve_data.papilo_crush_assignment(*this, assignment); +}
I meant testing user_data since it is a new parameter. |
It is tested here: https://github.com/NVIDIA/cuopt/pull/779/files#diff-3db7ef937cb0c65153bfa4aa4378af5326fb873b9fdd29fb240e141b1f9c68deR43 It is enough that we prove user_data is passed back to user within callback. We don't need to do various values here. |
…n, add more tests for get_solution+presolve
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/cuopt/source/cuopt-python/lp-qp-milp/examples/incumbent_solutions_example.py (1)
68-91: Documentuser_datain the callback and update the server example.This docstring doesn’t mention
user_data, and the cuopt-server example still shows the old callback signature (docs/cuopt/source/cuopt-server/examples/milp/examples/incumbent_callback_example.py, Lines 64‑69). Please update both so the public API change is fully documented.✍️ Suggested docstring fix
solution_bound : array-like The current best bound in user objective space + user_data : object + User context passed through from `set_mip_callback`As per coding guidelines: When code changes affect docs: Missing docs: If PR changes public APIs without updating docs, flag as HIGH priority.
python/cuopt_server/cuopt_server/utils/job_queue.py (1)
1094-1142: Add missingreturn_incumbent_set_solutions()forwarder toSolverBinaryJob.
SolverBinaryJobwraps method calls toself.resolved_jobbut is missing a forwarder forreturn_incumbent_set_solutions(). Line 1038 callsself.return_incumbent_set_solutions(), which will raiseAttributeErrorat runtime since the method doesn't exist on the wrapper. Add a forwarder matching the pattern ofreturn_incumbents():Suggested fix
def return_incumbents(self): return self.resolved_job.return_incumbents() + + def return_incumbent_set_solutions(self): + return self.resolved_job.return_incumbent_set_solutions()docs/cuopt/source/cuopt-server/examples/milp/examples/incumbent_callback_example.py (1)
25-28: Update the Expected Output to include bound.
The example now printsbound, but the Expected Output block still shows only cost.✏️ Doc fix
- Solution : [0.0, 5000.0] cost : 8500.0 + Solution : [0.0, 5000.0] cost : 8500.0 bound : 8500.0Also applies to: 64-70
🤖 Fix all issues with AI agents
In `@cpp/include/cuopt/linear_programming/cuopt_c.h`:
- Around line 684-712: The callback docs for cuOptMIPGetSolutionCallback and
cuOptMIPSetSolutionCallback must state that all pointer arguments (solution,
objective_value, solution_bound, user_data) refer to host memory and are only
valid for the duration of the callback invocation; update both Doxygen blocks to
explicitly say callers must not pass device/GPU pointers and that they must copy
data if they need it after the callback returns (also apply the same
clarification to the other related callback docs in the file covering the
get/set variants referenced nearby).
In `@cpp/include/cuopt/linear_programming/mip/solver_stats.hpp`:
- Line 15: The default constructor for solver_stats_t initializes solution_bound
with std::numeric_limits<f_t>::min(), which is the smallest positive float and
not a direction-neutral sentinel; change the initialization in solver_stats_t()
to use a direction-neutral sentinel such as ±infinity (e.g.,
std::numeric_limits<f_t>::infinity() or -std::numeric_limits<f_t>::infinity())
or another clearly-documented sentinel and ensure comments state that
solver_context (which sets the bound based on maximization/minimization) will
overwrite this default; update the constructor for solver_stats_t and add a
short comment referencing solver_context logic to avoid accidental misuse.
In `@cpp/src/mip/problem/problem.cuh`:
- Around line 59-61: The move constructor and move-assignment for problem_t are
marked noexcept but the class contains std::function members which are not
guaranteed noexcept-movable; remove the noexcept specification from
problem_t(problem_t<i_t,f_t>&& problem) and problem_t& operator=(problem_t&&)
(or alternatively replace the std::function members with a noexcept-moveable
callable wrapper) so the moves do not promise noexcept incorrectly—update the
declarations of problem_t(problem_t&&) and operator=(problem_t&&) to drop
noexcept, or swap std::function fields for a noexcept-friendly wrapper and keep
noexcept only if those wrappers are truly noexcept-moveable.
In `@python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py`:
- Line 248: The change to set_mip_callback(callback, user_data) breaks callers;
revert to a backward-compatible signature by making user_data optional (e.g.,
user_data=None) in set_mip_callback and update internal handling to accept None,
preserving existing behavior when user_data is not provided; also emit a
DeprecationWarning only if callers pass some legacy marker if needed, and update
any references to set_mip_callback to handle the optional user_data parameter
(search for set_mip_callback and its internal callback registration logic to
apply the change).
🧹 Nitpick comments (7)
cpp/src/mip/solver_context.cuh (1)
14-16: Consider moving#pragma onceto the top of the file.The
#pragma oncedirective is placed after the includes. While this works, convention is to place it at the very top of header files (before any#includedirectives) to prevent redundant parsing of the entire file including the headers it pulls in.python/cuopt_server/cuopt_server/utils/linear_programming/solver.py (1)
447-453: Consider guardingincumbent_set_solutionswhen no get-callback is wired.
Ifintermediate_senderisNone, this becomes a silent no-op. A warning or validation would make the behavior explicit.♻️ Possible guard
- if callback is not None: - solver_settings.set_mip_callback(callback, reqId) - if incumbent_set_solutions: - set_callback = CustomSetSolutionCallback(callback, reqId) - solver_settings.set_mip_callback(set_callback, reqId) + if callback is not None: + solver_settings.set_mip_callback(callback, reqId) + if incumbent_set_solutions: + set_callback = CustomSetSolutionCallback(callback, reqId) + solver_settings.set_mip_callback(set_callback, reqId) + elif incumbent_set_solutions: + logging.warning( + "incumbent_set_solutions ignored because no " + "intermediate_sender/get-callback is configured" + )Also applies to: 580-585
python/cuopt_server/cuopt_server/tests/test_incumbents.py (1)
46-76: Consider adding numerical validation of the bound value.The test verifies the presence and type of
boundbut doesn't validate its numerical correctness relative tocost. For MIP problems, the bound should represent a valid bound on the optimal objective.💡 Optional: Add bound-cost relationship check
assert "bound" in i assert isinstance(i["bound"], float) + # For a maximization problem, bound should be >= cost + # (or <= for minimization). Consider adding: + # assert i["bound"] >= i["cost"] # for maximization breakpython/cuopt/cuopt/tests/linear_programming/test_incumbent_callbacks.py (1)
26-70: Consider extracting shared callback classes to reduce duplication.The
CustomGetSolutionCallbackandCustomSetSolutionCallbackimplementations are nearly identical to those intest_python_API.py. Consider extracting these to a shared test utility module.💡 Suggested refactor
Create a shared test utility, e.g.,
python/cuopt/cuopt/tests/linear_programming/callback_test_utils.py:from cuopt.linear_programming.internals import ( GetSolutionCallback, SetSolutionCallback, ) class CustomGetSolutionCallback(GetSolutionCallback): def __init__(self, user_data): super().__init__() self.n_callbacks = 0 self.solutions = [] self.user_data = user_data def get_solution(self, solution, solution_cost, solution_bound, user_data): assert user_data is self.user_data self.n_callbacks += 1 assert len(solution) > 0 assert len(solution_cost) == 1 assert len(solution_bound) == 1 self.solutions.append({ "solution": solution.tolist(), "cost": float(solution_cost[0]), "bound": float(solution_bound[0]), }) # ... similar for CustomSetSolutionCallbackcpp/src/mip/problem/problem.cu (1)
1796-1800: Avoid const_cast in const method; consider adjusting the interface.The
const_caston*thiswithin aconstmember function is a code smell. The underlyingpapilo_uncrush_assignmentinpresolve_datatakes a non-constproblem_t&but only uses it for stream access. Consider either:
- Making this method non-const (since it modifies
assignment)- Changing
presolve_data.papilo_uncrush_assignmentto takeconst problem_t&or just the stream directly♻️ Suggested refactor
template <typename i_t, typename f_t> -void problem_t<i_t, f_t>::papilo_uncrush_assignment(rmm::device_uvector<f_t>& assignment) const +void problem_t<i_t, f_t>::papilo_uncrush_assignment(rmm::device_uvector<f_t>& assignment) { - presolve_data.papilo_uncrush_assignment(const_cast<problem_t&>(*this), assignment); + presolve_data.papilo_uncrush_assignment(*this, assignment); }cpp/src/mip/diversity/population.cu (1)
352-358: Missing stream synchronization before scaling.After copying from host to device at lines 353-356, the code immediately calls
context.scaling.scale_solutions(incumbent_assignment)without ensuring the copy completes. Whileraft::copyis typically synchronous for host-to-device copies, it's safer to add explicit synchronization.♻️ Suggested fix
raft::copy(incumbent_assignment.data(), h_incumbent_assignment.data(), incumbent_assignment.size(), sol.handle_ptr->get_stream()); + sol.handle_ptr->sync_stream(); if (context.settings.mip_scaling) { context.scaling.scale_solutions(incumbent_assignment); }cpp/src/mip/problem/presolve_data.cuh (1)
119-122: Document ownership semantics for papilo_presolve_ptr.The raw pointer
papilo_presolve_ptris non-owning (as evidenced by the shallow copy in the copy constructor), but this should be documented to prevent misuse. Consider adding a comment.📝 Suggested documentation
+ // Non-owning pointer to external Papilo presolver; lifetime managed by caller const third_party_presolve_t<i_t, f_t>* papilo_presolve_ptr{nullptr};
python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py
Show resolved
Hide resolved
akifcorduk
added
breaking
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cpp/include/cuopt/linear_programming/cuopt_c.h`:
- Around line 737-753: Remove or update the outdated note in the
cuOptSetMIPSetSolutionCallback documentation that claims "Registering a
set-solution callback disables presolve"; instead state that presolve is
supported and the solver will automatically crush/uncrush solutions when
invoking cuOptMIPSetSolutionCallback, and add a brief sentence that callback
solution indices/variables refer to the post-uncrushed (original) model space or
explain how indices map if users need to map between presolved and original
models. Ensure the updated doc text references cuOptSetMIPSetSolutionCallback
and cuOptMIPSetSolutionCallback and mentions crushing/uncrushing behavior.
In `@cpp/include/cuopt/linear_programming/mip/solver_stats.hpp`:
- Around line 36-40: Add Doxygen comments to the public struct solver_stats_t
and its new accessor methods (e.g., set_solution_bound() and
get_solution_bound()) describing purpose and usage, include a short
thread-safety note stating that solution_bound is now atomic and safe for
concurrent access while other members are not, add a breaking-change/migration
notice in the header comment pointing to docs/ migration guide for users of the
previous non-atomic member, and explicitly state the expectation about lock-free
behavior (that std::atomic<f_t> should be lock-free on supported platforms) in
the documentation block so users are aware of the change and its platform
implications.
🧹 Nitpick comments (4)
cpp/src/mip/problem/problem.cuh (1)
95-98: Consider avoiding extra copies of mapping vectors.If
set_papilo_presolve_datadoesn’tstd::movethese by-value parameters into storage, this can add large copies on big models. Consider moving them in the implementation or switching toconst std::vector<i_t>&/std::vector<i_t>&&to make intent explicit.cpp/include/cuopt/linear_programming/mip/solver_stats.hpp (2)
15-18: Comment is slightly misleading; copy constructor has minor inefficiency.
The comment says "Direction-neutral" but
infinity()(positive) is not truly neutral—it's the correct initial bound for minimization, while maximization would use-infinity(). Sincesolver_contextoverwrites this, consider rewording to clarify it's a "default placeholder, overwritten by solver_context based on optimization direction."The copy constructor
{ *this = other; }first default-initializessolution_boundtoinfinity(), then immediately overwrites it via assignment. This is functionally correct but performs an unnecessary atomic store. A member initializer list would be more efficient:♻️ Suggested improvement
- solver_stats_t(const solver_stats_t& other) { *this = other; } + solver_stats_t(const solver_stats_t& other) + : total_solve_time(other.total_solve_time), + presolve_time(other.presolve_time), + solution_bound(other.solution_bound.load(std::memory_order_relaxed)), + num_nodes(other.num_nodes), + num_simplex_iterations(other.num_simplex_iterations) + { + }
32-34: Add documentation andnoexceptspecifier for public API accessors.As this is a public header (
cpp/include/cuopt/**/*), consider adding brief Doxygen comments documenting the thread-safety guarantees and addingnoexceptsince atomic operations don't throw:📝 Suggested documentation
+ /// `@brief` Returns the current solution bound (thread-safe, relaxed ordering). + /// `@return` The solution bound value. - f_t get_solution_bound() const { return solution_bound.load(std::memory_order_relaxed); } + f_t get_solution_bound() const noexcept { return solution_bound.load(std::memory_order_relaxed); } + /// `@brief` Sets the solution bound (thread-safe, relaxed ordering). + /// `@param` value The new solution bound value. - void set_solution_bound(f_t value) { solution_bound.store(value, std::memory_order_relaxed); } + void set_solution_bound(f_t value) noexcept { solution_bound.store(value, std::memory_order_relaxed); }As per coding guidelines: public header files should have documentation comments, and thread-safety behavior should be documented.
cpp/include/cuopt/linear_programming/cuopt_c.h (1)
720-735: Registration function is well-documented.The documentation clearly explains the
user_dataforwarding behavior and includes the host memory warning.Consider adding a brief thread-safety note indicating whether the callback may be invoked from solver worker threads and any synchronization requirements for user code accessing shared state. As per coding guidelines: For public header files, suggest documenting thread-safety.
| /** | ||
| * @brief Register a callback to inject MIP solutions. | ||
| * | ||
| * @param[in] settings - The solver settings object. | ||
| * @param[in] callback - Callback function to inject solutions. | ||
| * @param[in] user_data - User-defined pointer passed through to the callback. | ||
| * It will be forwarded to ``cuOptMIPSetSolutionCallback`` when invoked. | ||
| * @note Registering a set-solution callback disables presolve. | ||
| * @note The callback arguments refer to host memory and are only valid during the callback | ||
| * invocation. Do not pass device/GPU pointers. Copy any data you need to keep after the callback | ||
| * returns. | ||
| * | ||
| * @return A status code indicating success or failure. | ||
| */ | ||
| cuopt_int_t cuOptSetMIPSetSolutionCallback(cuOptSolverSettings settings, | ||
| cuOptMIPSetSolutionCallback callback, | ||
| void* user_data); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for presolve behavior with callback registration
rg -n 'cuOptSetMIPSetSolutionCallback|cuOptSetMIPGetSolutionCallback|presolve.*callback|callback.*presolve' --type cpp cpp/src/ | head -50Repository: NVIDIA/cuopt
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Search for crushing/uncrushing logic related to presolve and callbacks
rg -n 'crush|uncursh' --type cpp cpp/src/linear_programming/ cpp/src/mip/ | head -50Repository: NVIDIA/cuopt
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Look for set-solution callback implementation details
rg -n -B5 -A10 'MIPSetSolutionCallback|set.*solution.*callback' --type cpp cpp/src/linear_programming/ | head -100Repository: NVIDIA/cuopt
Length of output: 4690
🏁 Script executed:
#!/bin/bash
# Find set_mip_callback implementation
rg -n -B3 -A15 'set_mip_callback' --type cpp | head -100Repository: NVIDIA/cuopt
Length of output: 7413
🏁 Script executed:
#!/bin/bash
# Search for presolve disabling logic
rg -n 'disable.*presolve|presolve.*disable|presolve\s*=\s*false' --type cpp cpp/src/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for presolve configuration with callbacks
rg -n -B5 -A5 'presolve' cpp/src/linear_programming/cuopt_c.cpp | head -100Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Find the implementation of set_mip_callback
find cpp/src -name "*.cpp" -type f | xargs rg -l 'set_mip_callback'Repository: NVIDIA/cuopt
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for presolve in solver_settings implementation files
rg -n 'presolve' --type cpp cpp/src/linear_programming/ | grep -i 'settings\|callback' | head -30Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at mip_solver_settings implementation
find cpp/src -name "*solver_settings*" -type f | xargs ls -laRepository: NVIDIA/cuopt
Length of output: 511
🏁 Script executed:
#!/bin/bash
# Check for presolve mention in MIP files
rg -n 'presolve' --type cpp cpp/src/mip/ | head -50Repository: NVIDIA/cuopt
Length of output: 5600
🏁 Script executed:
#!/bin/bash
# Search for MIP solver implementation where callbacks and presolve interact
find cpp/src/mip -name "*.cu" -o -name "*.hpp" | xargs grep -l "solve\|presolve" | head -10Repository: NVIDIA/cuopt
Length of output: 531
🏁 Script executed:
#!/bin/bash
# Check MIP solver main logic
rg -n 'crush|uncrush|callback' cpp/src/mip/mip_solver.cu | head -60Repository: NVIDIA/cuopt
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search for where presolve is disabled in MIP context
rg -n 'enable_presolve|presolve.*false|callbacks.*presolve' --type cpp --type cuda cpp/src/mip/ | head -50Repository: NVIDIA/cuopt
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Look at MIP solver settings implementation
cat cpp/src/mip/solver_settings.cuRepository: NVIDIA/cuopt
Length of output: 1941
🏁 Script executed:
#!/bin/bash
# Find MIP solver files
find cpp/src/mip -name "*.cpp" -o -name "*.hpp" | head -20Repository: NVIDIA/cuopt
Length of output: 480
🏁 Script executed:
#!/bin/bash
# Search for where callbacks are invoked in solve logic
rg -n 'get_mip_callbacks\|mip_callbacks_\|set_solution\|get_solution' --type cpp cpp/src/mip/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for presolve interaction with solve in header files
rg -n 'presolve.*callback|callback.*presolve|crush.*callback|callback.*crush' --type cpp --type hpp cpp/src/ | head -50Repository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find where solve methods are defined for MIP
rg -n 'void solve|cuOptSolve' --type cpp cpp/include/cuopt/ | head -40Repository: NVIDIA/cuopt
Length of output: 2342
🏁 Script executed:
#!/bin/bash
# Look at optimization_problem solve
find cpp/include -name "*.h" -o -name "*.hpp" | xargs grep -l 'solve'Repository: NVIDIA/cuopt
Length of output: 1027
🏁 Script executed:
#!/bin/bash
# Search for enable_presolve or related flags
rg -n 'enable_presolve|_presolve' --type cpp cpp/src/linear_programming/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check for RELEASE_NOTES or changelog mentioning callbacks and presolve
find . -name "RELEASE*" -o -name "CHANGELOG*" -o -name "*.md" | xargs grep -l "callback\|presolve" 2>/dev/null | head -10Repository: NVIDIA/cuopt
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Check RELEASE-NOTES for callback and presolve information
rg -n -B3 -A3 'callback.*presolve|presolve.*callback|crush.*callback' RELEASE-NOTES.mdRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at solve.hpp to understand how callbacks and presolve are handled
head -100 cpp/include/cuopt/linear_programming/solve.hppRepository: NVIDIA/cuopt
Length of output: 4543
🏁 Script executed:
#!/bin/bash
# Search in solve.hpp for presolve and callback interaction
rg -n 'presolve\|callback\|crush' cpp/include/cuopt/linear_programming/solve.hppRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check the entire RELEASE-NOTES.md for callback mentions
rg -n -B5 -A5 'callback' RELEASE-NOTES.md | head -100Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for where MIP solve is actually implemented
find cpp/src -name "*.cpp" | xargs rg -l 'solve_mip' | head -10Repository: NVIDIA/cuopt
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Search for callback invocation during solve
rg -n 'set_solution\|get_solution' --type cpp cpp/src/ | grep -v test | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at the actual solve implementation
rg -n 'solve_mip' --type cpp cpp/src/linear_programming/solve.cpp | head -20Repository: NVIDIA/cuopt
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Search for where presolve and callback interact more directly
rg -n 'uncrush\|crush.*solution' --type cpp cpp/src/mip/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at solve implementation in dual_simplex
wc -l cpp/src/dual_simplex/solve.cppRepository: NVIDIA/cuopt
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Check if solve.cpp is very large, if not read it
if [ $(wc -l < cpp/src/dual_simplex/solve.cpp) -lt 500 ]; then
cat cpp/src/dual_simplex/solve.cpp
else
head -200 cpp/src/dual_simplex/solve.cpp
fiRepository: NVIDIA/cuopt
Length of output: 7704
🏁 Script executed:
#!/bin/bash
# Look for cuOptSolve implementation
rg -n -A30 'cuopt_int_t cuOptSolve' cpp/src/linear_programming/cuopt_c.cppRepository: NVIDIA/cuopt
Length of output: 1883
🏁 Script executed:
#!/bin/bash
# Search for where presolve is enabled or disabled
rg -n 'enable.*presolve|presolve.*enable|presolve\s*=' --type cpp cpp/src/linear_programming/ cpp/src/dual_simplex/Repository: NVIDIA/cuopt
Length of output: 235
🏁 Script executed:
#!/bin/bash
# Find solve_mip in solve.cpp
rg -n 'solve_mip\|get_mip_callbacks' cpp/src/dual_simplex/solve.cppRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for solve_mip template instantiation or implementation
rg -n -B5 -A30 'template.*solve_mip|mip_solution_t.*solve_mip' --type cpp cpp/src/dual_simplex/Repository: NVIDIA/cuopt
Length of output: 1082
🏁 Script executed:
#!/bin/bash
# Look for branch_and_bound which likely handles MIP solving
rg -n 'branch_and_bound' --type cpp cpp/src/dual_simplex/solve.cppRepository: NVIDIA/cuopt
Length of output: 422
🏁 Script executed:
#!/bin/bash
# Read the rest of the solve.cpp file to find MIP solve logic
tail -400 cpp/src/dual_simplex/solve.cppRepository: NVIDIA/cuopt
Length of output: 17938
🏁 Script executed:
#!/bin/bash
# Find branch_and_bound implementation
find cpp/src -name "*branch_and_bound*" -type fRepository: NVIDIA/cuopt
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Search for callback invocation in branch and bound or MIP solver
rg -n 'callback\|set_solution\|get_solution' cpp/src/dual_simplex/ --type cpp -i | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at branch_and_bound.hpp header
head -150 cpp/include/cuopt/linear_programming/mip/solver_settings.hppRepository: NVIDIA/cuopt
Length of output: 3449
🏁 Script executed:
#!/bin/bash
# Search specifically for crushing/uncrushing applied to callbacks in MIP
rg -n 'uncrush.*callback|callback.*uncrush|crush.*set_solution' --type cpp cpp/src/mip/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check if presolve flag is modified based on callbacks
rg -n 'presolve\s*=' --type cpp cpp/src/dual_simplex/branch_and_bound.cpp | head -30Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Look at branch_and_bound.hpp to understand MIP solving
head -200 cpp/src/dual_simplex/branch_and_bound.hppRepository: NVIDIA/cuopt
Length of output: 7505
🏁 Script executed:
#!/bin/bash
# Search for where third_party_presolve is called in MIP context
rg -n 'third_party_presolve\|enable.*presolve' --type cpp cpp/src/dual_simplex/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check git log for recent changes to callback or presolve interaction
git log --oneline --all -20 -- cpp/include/cuopt/linear_programming/cuopt_c.hRepository: NVIDIA/cuopt
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Look at branch_and_bound.cpp to find presolve usage
wc -l cpp/src/dual_simplex/branch_and_bound.cppRepository: NVIDIA/cuopt
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Search for presolve in branch_and_bound.cpp
rg -n 'presolve' cpp/src/dual_simplex/branch_and_bound.cpp | head -40Repository: NVIDIA/cuopt
Length of output: 1564
🏁 Script executed:
#!/bin/bash
# Look for how settings.get_mip_callbacks is used
rg -n 'get_mip_callbacks' --type cpp cpp/src/ | head -30Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for third_party_presolve usage in dual_simplex context
rg -n 'third_party_presolve' --type cpp cpp/src/dual_simplex/ cpp/include/cuopt/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for solve_mip implementation in linear_programming namespace
rg -n 'template.*solve_mip|solve_mip.*{' --type cpp cpp/include/cuopt/linear_programming/ | head -30Repository: NVIDIA/cuopt
Length of output: 183
🏁 Script executed:
#!/bin/bash
# Find where third_party_presolve is used
rg -n 'third_party_presolve_t' --type cpp cpp/src/ | head -50Repository: NVIDIA/cuopt
Length of output: 822
🏁 Script executed:
#!/bin/bash
# Look for the actual MIP solve implementation that uses callbacks
find cpp/src -name "*.cpp" | xargs rg -l 'solve_mip.*{' | head -10Repository: NVIDIA/cuopt
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Check solve.hpp implementation for solve_mip
tail -300 cpp/include/cuopt/linear_programming/solve.hpp | head -150Repository: NVIDIA/cuopt
Length of output: 4997
🏁 Script executed:
#!/bin/bash
# Find the implementation file for solve_mip template
find cpp/src/linear_programming -name "*.cu" | xargs rg -l 'solve_mip'Repository: NVIDIA/cuopt
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Look for where MIP presolve is actually applied or disabled
rg -n 'settings.presolve|settings_\.presolve' --type cpp cpp/src/ | head -40Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin
bash
# Search for where callbacks are invoked during solve
rg -n 'set_solution\|set_new_solution' --type cpp cpp/src/ | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check solve.cu or solve implementation for MIP
find cpp/src -name "*solve*.cu" -o -name "*solve*.cpp" | head -20Repository: NVIDIA/cuopt
Length of output: 815
🏁 Script executed:
#!/bin/bash
# Look at the main MIP solve implementation
head -150 cpp/src/mip/solve.cuRepository: NVIDIA/cuopt
Length of output: 6708
🏁 Script executed:
#!/bin/bash
# Search for presolve in MIP solve
rg -n 'presolve\|callback' cpp/src/mip/solve.cu | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check MIP solver.cu for presolve and callback handling
head -150 cpp/src/mip/solver.cuRepository: NVIDIA/cuopt
Length of output: 6008
🏁 Script executed:
#!/bin/bash
# Look for where callbacks are invoked or presolve is disabled
rg -n 'get_mip_callbacks\|set_solution\|presolve.*=\|disable' cpp/src/mip/solver.cu | head -50Repository: NVIDIA/cuopt
Length of output: 38
Update documentation: set-solution callbacks now work with presolve.
The note at line 744 stating "Registering a set-solution callback disables presolve" is outdated. The solver now supports callbacks when presolve is enabled by properly handling crushing/uncrushing of solutions. Remove or update this note to reflect that presolve works with callbacks, and clarify the crushing/uncrushing behavior if users need to know about solution index mapping.
🤖 Prompt for AI Agents
In `@cpp/include/cuopt/linear_programming/cuopt_c.h` around lines 737 - 753,
Remove or update the outdated note in the cuOptSetMIPSetSolutionCallback
documentation that claims "Registering a set-solution callback disables
presolve"; instead state that presolve is supported and the solver will
automatically crush/uncrush solutions when invoking cuOptMIPSetSolutionCallback,
and add a brief sentence that callback solution indices/variables refer to the
post-uncrushed (original) model space or explain how indices map if users need
to map between presolved and original models. Ensure the updated doc text
references cuOptSetMIPSetSolutionCallback and cuOptMIPSetSolutionCallback and
mentions crushing/uncrushing behavior.
There was a problem hiding this comment.
That's not correct. presolve is disabled when set-solution callback is enabled.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where presolve is disabled based on set-solution callbacks
rg -n -B5 -A10 'set_solution.*callback.*presolve|presolve.*set.*callback' --type cpp cpp/src/mip/
# Also search for any warning messages about disabling presolve
rg -n -B3 -A3 'presolve.*disabled|disable.*presolve' --type cpp cpp/src/mip/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the solve.cu file more directly for callback and presolve logic
rg -n 'set_sol.*callback|get_set_solution_callback|mip_callbacks' cpp/src/mip/solve.cu | head -30
# Search for presolve variable or flag settings
rg -n 'enable_presolve|third_party_presolve' cpp/src/mip/solve.cu | head -30
# Look for any conditional logic around callbacks
rg -n -B2 -A8 'if.*callback|callback.*size\(\)' cpp/src/mip/solve.cu | head -50
Length of output: 1302
@akifcorduk, you're absolutely right—thank you for the correction! I apologize for the error.
The code in cpp/src/mip/solve.cu (lines 204-207) confirms that presolve is disabled when a set-solution callback is registered:
if (run_presolve && has_set_solution_callback) {
CUOPT_LOG_WARN("Presolve is disabled because set_solution callbacks are provided.");
run_presolve = false;
}The documentation note stating "Registering a set-solution callback disables presolve" at line 744 is correct and should remain as-is. My previous analysis was mistaken.
🧬 Code Graph Analysis Results
[cpp/include/cuopt/linear_programming/cuopt_c.h, lines 420-433]
/**
@briefRegister a callback to inject MIP solutions.@param[in] settings - The solver settings object.@param[in] callback - Callback function to inject solutions.@param[in] user_data - User-defined pointer passed through to the callback.- It will be forwarded to
cuOptMIPSetSolutionCallbackwhen invoked. @noteThe callback arguments refer to host memory and are only valid during the callback- invocation. Do not pass device/GPU pointers. Copy any data you need to keep after the callback
- returns.
@returnA status code indicating success or failure.
/
cuopt_int_t cuOptSetMIPSetSolutionCallback(cuOptSolverSettings settings,
cuOptMIPSetSolutionCallback callback,
void user_data);
| f_t total_solve_time = 0.; | ||
| f_t presolve_time = 0.; | ||
| std::atomic<f_t> solution_bound; | ||
| i_t num_nodes = 0; | ||
| i_t num_simplex_iterations = 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check f_t typedef definition and how solver_stats_t is used
rg -n "using f_t|typedef.*f_t|#define f_t" --type=cpp --type=hpp | head -20Repository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
# Check the full context of solver_stats.hpp to see if there's documentation
cat -n cpp/include/cuopt/linear_programming/mip/solver_stats.hppRepository: NVIDIA/cuopt
Length of output: 1791
🏁 Script executed:
# Search for migration guides, release notes, or breaking change documentation
fd -e md -e rst | xargs rg -l "breaking\|migration\|ABI" 2>/dev/null | head -10Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if solution_bound has any Doxygen or documentation comments
rg -B5 "solution_bound" cpp/include/cuopt --type=cpp --type=hpp | head -30Repository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
# Search for f_t typedef/using declarations
rg -n "using f_t|f_t =" cpp/include/cuopt -A2 | head -40Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if there are Doxygen comments in the header file
rg -B3 "struct solver_stats_t|solution_bound" cpp/include/cuopt/linear_programming/mip/solver_stats.hppRepository: NVIDIA/cuopt
Length of output: 849
🏁 Script executed:
# Search for any migration guides, docs, or breaking change notices in the repo
fd -type f \( -name "*.md" -o -name "*.rst" -o -name "CHANGELOG*" \) | xargs rg -l "solver_stats\|breaking.*ABI\|migration" 2>/dev/nullRepository: NVIDIA/cuopt
Length of output: 228
🏁 Script executed:
# Check if this struct is used elsewhere to understand access patterns
rg "solver_stats_t" cpp/src cpp/test --type=cpp -A2 | head -50Repository: NVIDIA/cuopt
Length of output: 104
🏁 Script executed:
# Find where solver_stats_t is instantiated to see what f_t is used as
rg "solver_stats_t<" cpp/include cpp/src --type=cpp -A1 -B1Repository: NVIDIA/cuopt
Length of output: 1387
🏁 Script executed:
# Check for any documentation or comments in the header about the atomic change
rg -n "solution_bound|atomic" cpp/include/cuopt/linear_programming/mip/solver_stats.hpp -B2Repository: NVIDIA/cuopt
Length of output: 797
🏁 Script executed:
# Search for any documentation files mentioning this struct
find . -name "*.md" -o -name "*.rst" | xargs grep -l "solver_stats" 2>/dev/nullRepository: NVIDIA/cuopt
Length of output: 40
🏁 Script executed:
# Check if there are any Doxygen-style comments in related headers
cat cpp/include/cuopt/linear_programming/mip/solver_stats.hpp | head -20Repository: NVIDIA/cuopt
Length of output: 695
Add Doxygen documentation and breaking change notice for public API header.
The std::atomic<f_t> change correctly implements concurrent access with load()/store() using relaxed memory ordering, and the custom copy assignment properly handles the atomic member. However, as a public header in cpp/include/cuopt/**, this breaking change requires:
- Doxygen documentation comments on the struct and new accessor methods
- Documentation of thread-safety behavior (specifically that
solution_boundis now thread-safe while other members are not) - A migration notice or deprecation guide for users of the old non-atomic member
- Explicit guarantee that
std::atomic<f_t>is lock-free on supported platforms
Consider adding:
/// Solver statistics. Note: solution_bound is now atomic for thread-safe concurrent access.
/// Breaking change: struct layout changed. See migration guide in docs/ for upgrading.
template <typename i_t, typename f_t>
struct solver_stats_t {
// ...
/// Thread-safe solution bound (updated via set_solution_bound())
std::atomic<f_t> solution_bound;🤖 Prompt for AI Agents
In `@cpp/include/cuopt/linear_programming/mip/solver_stats.hpp` around lines 36 -
40, Add Doxygen comments to the public struct solver_stats_t and its new
accessor methods (e.g., set_solution_bound() and get_solution_bound())
describing purpose and usage, include a short thread-safety note stating that
solution_bound is now atomic and safe for concurrent access while other members
are not, add a breaking-change/migration notice in the header comment pointing
to docs/ migration guide for users of the previous non-atomic member, and
explicitly state the expectation about lock-free behavior (that std::atomic<f_t>
should be lock-free on supported platforms) in the documentation block so users
are aware of the change and its platform implications.
There was a problem hiding this comment.
We don't officially support c++ api, so it is an intermediate class which is converted to public apis which are C and python.
There was a problem hiding this comment.
@akifcorduk, thank you for the clarification! That's helpful context. Since this is an internal/intermediate class and the C++ API is not officially supported, the extensive Doxygen documentation and migration guide suggestions I made are not necessary. The breaking change handling at the C and Python API layers is what matters.
I've noted this for future reviews so I don't flag similar internal C++ classes with the same concerns.
✏️ Learnings added
Learnt from: akifcorduk
Repo: NVIDIA/cuopt PR: 779
File: cpp/include/cuopt/linear_programming/mip/solver_stats.hpp:36-40
Timestamp: 2026-01-28T15:32:45.040Z
Learning: C++ headers under cpp/include/cuopt are internal/intermediate infrastructure, not public-facing. The officially supported public APIs for cuOpt are C (via cuopt_c.h) and Python. Documentation and breaking change requirements apply to those public APIs, not to internal C++ classes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 602
File: cpp/src/linear_programming/solve.cu:732-742
Timestamp: 2025-12-04T20:09:09.264Z
Learning: In cpp/src/linear_programming/solve.cu, the barrier solver does not currently return INFEASIBLE or UNBOUNDED status. It only returns OPTIMAL, TIME_LIMIT, NUMERICAL_ISSUES, or CONCURRENT_LIMIT.
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cpp,hpp,h} : Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{h,hpp,py} : Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to docs/**/*.{rst,md} : Document numerical tolerances, parameter ranges, and algorithm phase descriptions in API documentation when solver behavior or tolerances change
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 798
File: cpp/src/utilities/models/fj_predictor/quantize.cpp:1149-1161
Timestamp: 2026-01-26T14:17:25.105Z
Learning: The files in cpp/src/utilities/models/*/quantize.cpp (fj_predictor, cpufj_predictor, pdlp_predictor, dualsimplex_predictor, bounds_strengthening_predictor) are autogenerated model artifacts and should not have manual code changes suggested; any improvements should target the model generation tooling or calling code validation layer.
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip/local_search/local_search.cu (1)
249-261: Add null-safety check or assertion fordiversity_manager_ptrbefore dereferencing.The
context.diversity_manager_ptr->check_b_b_preemption()call at line 249 (and similarly at feasibility_pump.cu:481) assumes the pointer is non-null. Whilediversity_manager_ptris set to a valid address in solver.cu:242 before the normal execution path reaches this code, the pointer is initialized tonullptrby default and has no null checks or assertions protecting its dereference. Consider addingcuopt_assert(context.diversity_manager_ptr != nullptr, ...)or an explicit null check, consistent with the pattern used forcontext.problem_ptrelsewhere in the codebase.
🧹 Nitpick comments (1)
cpp/src/mip/solver.cu (1)
242-243: Consider clearingdiversity_manager_ptrafter solve.
It points to a stackdm, so resetting tonullptrbefore returning avoids a dangling pointer if the context is inspected afterrun_solver().♻️ Suggested fix
@@ context.diversity_manager_ptr = &dm; auto sol = dm.run_solver(); @@ if (!is_feasible.value(sol.handle_ptr->get_stream())) { CUOPT_LOG_ERROR( "Solution is not feasible due to variable bounds, returning infeasible solution!"); context.problem_ptr->post_process_solution(sol); + context.diversity_manager_ptr = nullptr; return sol; } @@ dm.rins.stop_rins(); + context.diversity_manager_ptr = nullptr; return sol;
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@cpp/src/mip/problem/presolve_data.cu`:
- Around line 73-80: Before calling thrust::gather, validate that every index in
variable_mapping is within [0, temp_assignment.size()) to avoid device
out-of-bounds reads; add a device-safe range check/assert (or validate at
construction) that checks variable_mapping.size() == problem.n_variables and
that all values satisfy value < temp_assignment.size(), and if any fail
log/error/abort; perform this check prior to the thrust::gather call that uses
problem.handle_ptr->get_thrust_policy(), referencing the existing symbols
variable_mapping, temp_assignment, assignment, and thrust::gather so the mapping
consistency between presolve/original/postsolve contexts is enforced.
- Around line 118-123: The thrust::for_each kernel writes
fixed_assgn[var_map[idx]] in parallel which races if var_map (variable_mapping)
contains duplicate targets; before launching the thrust::for_each (the block
using problem.handle_ptr->get_thrust_policy(), var_map, fixed_assgn, assgn and
current_assignment.size()) validate that var_map is a permutation/contains
unique indices (e.g., host-side check that builds a boolean/visited array or
sorts+checks for duplicates) and assert or return an error if duplicates exist,
or alternatively rewrite the kernel to handle duplicates deterministically (for
example by accumulating with atomics or a deterministic reduction into
fixed_assgn) and document the chosen approach.
- Around line 47-81: The GPU stream is synchronized via
problem.handle_ptr->sync_stream() but no CUDA error check follows; add a
RAFT_CHECK_CUDA(problem.handle_ptr->get_stream()) immediately after the
sync_stream() call to verify kernel/stream errors (follow the pattern used in
feasibility_test.cuh / iterative_refinement.hpp); locate the sync call in
presolve_data.cu and insert the RAFT_CHECK_CUDA check right after it so
subsequent thrust::gather/validation runs only on a verified stream.
- Around line 239-246: After calling
papilo_presolve_ptr->uncrush_primal_solution(h_assignment, full_assignment)
validate that full_assignment.size() equals papilo_original_num_variables before
resizing assignment; if it does not match, log an error using the same pattern
as set_papilo_presolve_data (include context and sizes) and return early to
avoid propagating an incorrect solution dimension. Use the symbols h_assignment,
full_assignment, assignment and papilo_original_num_variables to locate the code
and implement the guard and early return consistent with existing error
handling.
In
`@python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py`:
- Around line 828-831: The new IncumbentSolution model makes bound required; to
restore backward compatibility change the field declaration on the
IncumbentSolution class so bound has a default of None (e.g., use
Optional[float] or Union[float, None] with a default = None) and ensure typing
imports (Optional) are present; keep solution and cost unchanged and optionally
emit a deprecation warning where instances lacking an explicit bound are created
if you intend to phase the field out later.
| rmm::device_uvector<f_t> temp_assignment(additional_var_used.size(), | ||
| problem.handle_ptr->get_stream()); | ||
| // copy the assignment to the first part(the original variable count) of the temp_assignment | ||
| raft::copy( | ||
| temp_assignment.data(), assignment.data(), assignment.size(), problem.handle_ptr->get_stream()); | ||
| auto d_additional_var_used = | ||
| cuopt::device_copy(additional_var_used, problem.handle_ptr->get_stream()); | ||
| auto d_additional_var_id_per_var = | ||
| cuopt::device_copy(additional_var_id_per_var, problem.handle_ptr->get_stream()); | ||
|
|
||
| thrust::for_each( | ||
| problem.handle_ptr->get_thrust_policy(), | ||
| thrust::make_counting_iterator<i_t>(0), | ||
| thrust::make_counting_iterator<i_t>(problem.original_problem_ptr->get_n_variables()), | ||
| [additional_var_used = d_additional_var_used.data(), | ||
| additional_var_id_per_var = d_additional_var_id_per_var.data(), | ||
| assgn = temp_assignment.data()] __device__(auto idx) { | ||
| if (additional_var_used[idx]) { | ||
| cuopt_assert(additional_var_id_per_var[idx] != -1, "additional_var_id_per_var is not set"); | ||
| // We have two non-negative variables y and z that simulate a free variable | ||
| // x. If the value of x is negative, we can set z to be something higher than | ||
| // y. If the value of x is positive we can set y greater than z | ||
| assgn[additional_var_id_per_var[idx]] = (assgn[idx] < 0 ? -assgn[idx] : 0.); | ||
| assgn[idx] += assgn[additional_var_id_per_var[idx]]; | ||
| } | ||
| }); | ||
| assignment.resize(problem.n_variables, problem.handle_ptr->get_stream()); | ||
| assignment.shrink_to_fit(problem.handle_ptr->get_stream()); | ||
| cuopt_assert(variable_mapping.size() == problem.n_variables, "size mismatch"); | ||
| thrust::gather(problem.handle_ptr->get_thrust_policy(), | ||
| variable_mapping.begin(), | ||
| variable_mapping.end(), | ||
| temp_assignment.begin(), | ||
| assignment.begin()); | ||
| problem.handle_ptr->sync_stream(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "sync_stream" -A 3 -g'*.{cu,cuh,cpp,hpp,h}' | head -60Repository: NVIDIA/cuopt
Length of output: 4563
🏁 Script executed:
rg -n "CUDA_CHECK|CUOPT_CUDA|RAFT_CUDA|raft::cuda_error" -g'*.{cu,cuh,cpp,hpp,h}' | head -80Repository: NVIDIA/cuopt
Length of output: 8298
🏁 Script executed:
# Check how device_copy is implemented
rg -B 2 -A 10 "device_copy.*=" -g'*.{cu,cuh}' --type-add 'cpp:*.{cu,cuh,cpp,hpp,h}' | head -100Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look at the full context of the file in question
head -100 cpp/src/mip/problem/presolve_data.cuRepository: NVIDIA/cuopt
Length of output: 4374
🏁 Script executed:
# Find sync_stream implementation to understand error handling
rg -B 3 -A 10 "void sync_stream\|def sync_stream" -g'*.{cu,cuh,cpp,hpp,h}'Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check the handle_ptr type to understand sync_stream
rg -n "handle_ptr" cpp/src/mip/problem/presolve_data.cu | head -20Repository: NVIDIA/cuopt
Length of output: 1481
🏁 Script executed:
# Look for handle or stream-related classes
rg -n "class.*handle|struct.*handle" -g'*.{cuh,hpp,h}' | head -30Repository: NVIDIA/cuopt
Length of output: 445
🏁 Script executed:
# Check more context around sync_stream and raft operations in the file
sed -n '1,120p' cpp/src/mip/problem/presolve_data.cu | tail -50Repository: NVIDIA/cuopt
Length of output: 2160
🏁 Script executed:
# Find the handle_ptr type definition
rg -B 5 "handle_ptr" cpp/src/mip/problem/presolve_data.cuh | head -50Repository: NVIDIA/cuopt
Length of output: 871
🏁 Script executed:
# Search for the handle class that provides sync_stream
find cpp/src -name "*.cuh" -o -name "*.hpp" | xargs grep -l "sync_stream" | head -10Repository: NVIDIA/cuopt
Length of output: 513
🏁 Script executed:
# Look for sync_stream implementation
rg -B 2 -A 8 "sync_stream\(\)" cpp/src -g'*.{cuh,hpp,h}'Repository: NVIDIA/cuopt
Length of output: 26537
🏁 Script executed:
# Check if handle_ptr is raft_handle_t and look for its sync_stream
rg -n "raft_handle_t\|raft::handle_t" cpp/src -g'*.{cuh,hpp,h}' | head -20Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check raft handle implementation for sync_stream
rg -B 5 -A 10 "sync_stream" cpp/src -g'*.{cuh,hpp,h}' | grep -A 15 "raft::handle" | head -40Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look for raft library usage and error handling in CUDA operations
rg "RAFT_CUDA_TRY.*sync_stream\|sync_stream.*RAFT_CUDA_TRY" -g'*.{cu,cuh,cpp,hpp,h}' | head -20Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check what error handling is used after device_copy and thrust operations
sed -n '47,85p' cpp/src/mip/problem/presolve_data.cu | cat -nRepository: NVIDIA/cuopt
Length of output: 2507
🏁 Script executed:
# Check if there's error handling pattern at caller level for presolve functions
rg -B 5 -A 5 "pre_process_assignment\|post_process_assignment" cpp/src -g'*.{cu,cuh,cpp,hpp,h}' | grep -A 10 "cuopt_func_call\|try\|catch\|RAFT" | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look for RAFT_CHECK_CUDA pattern usage after sync calls
rg "sync_stream.*;" -A 3 cpp/src/mip -g'*.{cu,cuh}' | head -60Repository: NVIDIA/cuopt
Length of output: 3364
🏁 Script executed:
# Check if RAFT_CHECK_CUDA exists and is used for stream validation
rg "RAFT_CHECK_CUDA" cpp/src -g'*.{cu,cuh,cpp,hpp,h}' | head -15Repository: NVIDIA/cuopt
Length of output: 1469
🏁 Script executed:
# Check if there's error checking in the caller context (pre_process_assignment return value usage)
rg -B 10 "pre_process_assignment\|post_process_assignment" cpp/src/mip -g'*.{cu,cuh}' | head -80Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look at the return type and callers of pre_process_assignment
rg -B 3 -A 15 "bool pre_process_assignment" cpp/src -g'*.{cu,cuh,hpp,h}'Repository: NVIDIA/cuopt
Length of output: 3634
🏁 Script executed:
# Check what happens after sync_stream in lines after 81 in presolve_data.cu
sed -n '81,100p' cpp/src/mip/problem/presolve_data.cuRepository: NVIDIA/cuopt
Length of output: 754
Add CUDA error checking after stream synchronization.
Line 81 calls sync_stream() without verifying stream errors. Subsequent validation operations depend on correct GPU execution. Add RAFT_CHECK_CUDA(problem.handle_ptr->get_stream()); immediately after line 81, following the pattern used elsewhere in the codebase (e.g., feasibility_test.cuh, iterative_refinement.hpp).
🤖 Prompt for AI Agents
In `@cpp/src/mip/problem/presolve_data.cu` around lines 47 - 81, The GPU stream is
synchronized via problem.handle_ptr->sync_stream() but no CUDA error check
follows; add a RAFT_CHECK_CUDA(problem.handle_ptr->get_stream()) immediately
after the sync_stream() call to verify kernel/stream errors (follow the pattern
used in feasibility_test.cuh / iterative_refinement.hpp); locate the sync call
in presolve_data.cu and insert the RAFT_CHECK_CUDA check right after it so
subsequent thrust::gather/validation runs only on a verified stream.
| assignment.resize(problem.n_variables, problem.handle_ptr->get_stream()); | ||
| assignment.shrink_to_fit(problem.handle_ptr->get_stream()); | ||
| cuopt_assert(variable_mapping.size() == problem.n_variables, "size mismatch"); | ||
| thrust::gather(problem.handle_ptr->get_thrust_policy(), | ||
| variable_mapping.begin(), | ||
| variable_mapping.end(), | ||
| temp_assignment.begin(), | ||
| assignment.begin()); |
There was a problem hiding this comment.
Validate variable_mapping indices before thrust::gather.
If any mapping value is out of [0, temp_assignment.size()), the gather will read out-of-bounds on device. Add an assert/range check (or ensure the mapping is validated at construction) to guard against invalid presolve mappings.
Based on learnings: Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations.
🤖 Prompt for AI Agents
In `@cpp/src/mip/problem/presolve_data.cu` around lines 73 - 80, Before calling
thrust::gather, validate that every index in variable_mapping is within [0,
temp_assignment.size()) to avoid device out-of-bounds reads; add a device-safe
range check/assert (or validate at construction) that checks
variable_mapping.size() == problem.n_variables and that all values satisfy value
< temp_assignment.size(), and if any fail log/error/abort; perform this check
prior to the thrust::gather call that uses
problem.handle_ptr->get_thrust_policy(), referencing the existing symbols
variable_mapping, temp_assignment, assignment, and thrust::gather so the mapping
consistency between presolve/original/postsolve contexts is enforced.
| thrust::for_each(problem.handle_ptr->get_thrust_policy(), | ||
| thrust::make_counting_iterator<i_t>(0), | ||
| thrust::make_counting_iterator<i_t>(current_assignment.size()), | ||
| [fixed_assgn, var_map, assgn] __device__(auto idx) { | ||
| fixed_assgn[var_map[idx]] = assgn[idx]; | ||
| }); |
There was a problem hiding this comment.
Guard against duplicate indices in variable_mapping.
This kernel writes fixed_assgn[var_map[idx]] in parallel. If variable_mapping isn’t a permutation, duplicate targets will race and produce nondeterministic results. Please assert uniqueness or handle duplicates explicitly.
As per coding guidelines: Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations.
🤖 Prompt for AI Agents
In `@cpp/src/mip/problem/presolve_data.cu` around lines 118 - 123, The
thrust::for_each kernel writes fixed_assgn[var_map[idx]] in parallel which races
if var_map (variable_mapping) contains duplicate targets; before launching the
thrust::for_each (the block using problem.handle_ptr->get_thrust_policy(),
var_map, fixed_assgn, assgn and current_assignment.size()) validate that var_map
is a permutation/contains unique indices (e.g., host-side check that builds a
boolean/visited array or sorts+checks for duplicates) and assert or return an
error if duplicates exist, or alternatively rewrite the kernel to handle
duplicates deterministically (for example by accumulating with atomics or a
deterministic reduction into fixed_assgn) and document the chosen approach.
| auto h_assignment = cuopt::host_copy(assignment, problem.handle_ptr->get_stream()); | ||
| std::vector<f_t> full_assignment; | ||
| papilo_presolve_ptr->uncrush_primal_solution(h_assignment, full_assignment); | ||
| assignment.resize(full_assignment.size(), problem.handle_ptr->get_stream()); | ||
| raft::copy(assignment.data(), | ||
| full_assignment.data(), | ||
| full_assignment.size(), | ||
| problem.handle_ptr->get_stream()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the presolve_data.cu file around the specified lines
cat -n cpp/src/mip/problem/presolve_data.cu | sed -n '220,260p'Repository: NVIDIA/cuopt
Length of output: 1799
🏁 Script executed:
# Search for papilo_presolve_ptr and understand the function signature
rg "uncrush_primal_solution" --type cpp --type cu -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 82
🏁 Script executed:
# Look for papilo_original_num_variables usage
rg "papilo_original_num_variables" --type cpp --type cu -A 2 -B 2Repository: NVIDIA/cuopt
Length of output: 82
🏁 Script executed:
# Check if there are existing size validations in presolve_data.cu
rg "size()" cpp/src/mip/problem/presolve_data.cu -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 5316
🏁 Script executed:
# Understand the problem structure - what get_n_variables returns
rg "get_n_variables\(\)" --type cpp --type cu -B 1 -A 1 | head -40Repository: NVIDIA/cuopt
Length of output: 82
🏁 Script executed:
# Find where problem.original_problem_ptr is used and what it represents
rg "original_problem_ptr" --type cpp -A 2 -B 1 | head -60Repository: NVIDIA/cuopt
Length of output: 436
🏁 Script executed:
# Check if papilo_original_num_variables is used elsewhere and how
rg "papilo_original_num_variables" --type cpp -B 1 -A 1Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look for the Problem class definition to understand get_n_variables
rg "get_n_variables\(\)" --type cpp --type cuh -B 3 -A 3 | head -50Repository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
# Check if there are other postsolve validations that compare sizes
rg "full_assignment" --type cpp -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Let's check the postsolve_data.cu to understand similar patterns
find . -name "*postsolve*" -type f | head -5Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Search for papilo usage and understand the contract
rg "papilo_presolve_ptr" --type cpp -B 3 -A 3Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check problem_t structure to understand original_problem_ptr relationship
fd "problem.h" --type f | head -3 | xargs cat -n | head -100Repository: NVIDIA/cuopt
Length of output: 3995
🏁 Script executed:
# Look at the initialization to see if papilo_original_num_variables and get_n_variables are validated together
rg "papilo_original_num_variables.*get_n_variables" --type cpp -B 5 -A 5Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check what problem structure fields exist
rg "class problem_t|struct problem_t" --type cpp -A 30 | head -80Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look for papilo documentation or comments about uncrush contract
rg "uncrush" --type cpp -B 5 -A 5Repository: NVIDIA/cuopt
Length of output: 21403
🏁 Script executed:
# Check if papilo_original_num_variables is set and later compared to problem structure
rg "papilo_original_num_variables\s*=" --type cpp -B 2 -A 5Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look at initialization to understand relationship between papilo_original_num_variables and problem.original_problem_ptr
sed -n '190,230p' cpp/src/mip/problem/presolve_data.cuRepository: NVIDIA/cuopt
Length of output: 1525
🏁 Script executed:
# Check how the problem is set up when papilo is called
rg "papilo_uncrush_assignment" --type cpp -B 5 -A 10Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Let's check the third_party_presolve.cpp to see uncrush_primal_solution implementation
rg "void third_party_presolve_t.*uncrush_primal_solution" -A 20 cpp/src/mip/presolve/third_party_presolve.cppRepository: NVIDIA/cuopt
Length of output: 809
🏁 Script executed:
# Check if papilo_original_num_variables is ever used after being set
rg "papilo_original_num_variables" cpp/src/mip/problem/presolve_data.cu -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 251
🏁 Script executed:
# Look for papilo Solution resizing behavior in third_party_presolve
rg "full_primal\|full_sol" cpp/src/mip/presolve/third_party_presolve.cpp -B 3 -A 3Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if problem.original_problem_ptr->get_n_variables() is ever equal to papilo_original_num_variables
rg "original_num_variables" cpp/src/mip/problem/presolve_data.cu -B 5 -A 5 | head -60Repository: NVIDIA/cuopt
Length of output: 1985
🏁 Script executed:
# Look for any validation patterns after calling uncrush in other files
rg "uncrush_primal_solution" cpp/src/dual_simplex/presolve.cpp -A 5 | head -50Repository: NVIDIA/cuopt
Length of output: 1097
🏁 Script executed:
# Check if problem.original_problem_ptr is guaranteed to exist at papilo_uncrush_assignment call
rg "papilo_uncrush_assignment" --type cpp -B 10 -A 2Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Verify if problem structure always has original_problem_ptr set during uncrush
rg "original_problem_ptr" cpp/src/mip/problem/presolve_data.cu -B 3 -A 3Repository: NVIDIA/cuopt
Length of output: 1028
🏁 Script executed:
# Check the problem_t structure definition for original_problem_ptr
fd "problem.hpp" --type f | xargs rg "original_problem_ptr" -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 40
🏁 Script executed:
# Let me check what problem.original_problem_ptr->get_n_variables() returns
# by looking at optimization_problem.h
find . -name "optimization_problem.h" -type f | xargs grep -A 5 "get_n_variables"Repository: NVIDIA/cuopt
Length of output: 40
🏁 Script executed:
# Check if there are tests for papilo_uncrush_assignment
rg "papilo_uncrush_assignment" --type cppRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look at how papilo_uncrush_assignment is called to understand the context
rg "papilo_uncrush_assignment" --type cpp -B 10 -A 5 | head -80Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Let me understand the full flow - check if there are any other size checks after uncrush
sed -n '232,250p' cpp/src/mip/problem/presolve_data.cuRepository: NVIDIA/cuopt
Length of output: 976
🏁 Script executed:
# Check if papilo_original_num_variables is ever accessed or used
rg "papilo_original_num_variables" cpp/src/mip --type cpp -A 1 -B 1Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Look for papilo postsolve verification or documentation
rg "post_solve_storage_\|Postsolve" cpp/src/mip/presolve/third_party_presolve.cpp -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if the solution is validated after uncrush in any caller
rg "papilo_uncrush_assignment\|post_process_assignment" cpp/src/mip --type cpp -A 10 | head -100Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Let me search for how original_problem_ptr->get_n_variables() is used in relation to papilo
rg "original_problem_ptr->get_n_variables\(\)" cpp/src/mip --type cpp -B 3 -A 3 | head -50Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if papilo_original_num_variables should match original_problem_ptr->get_n_variables()
# by looking at the solve flow
rg "set_papilo_presolve_data\|papilo_uncrush_assignment" cpp/src/mip --type cpp -B 5 -A 1 | head -80Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check the MIP solver file to understand problem lifecycle
find cpp/src/mip -name "*.cpp" -type f | head -1 | xargs grep -l "papilo_uncrush_assignment"Repository: NVIDIA/cuopt
Length of output: 40
🏁 Script executed:
# Search for where set_papilo_presolve_data is called to understand papilo_original_num_variables usage
rg "set_papilo_presolve_data" cpp/src/mip --type cpp -B 5 -A 15Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if papilo_original_num_variables field exists and is defined
rg "papilo_original_num_variables" cpp/src/mip/problem -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 2577
🏁 Script executed:
# Look for the presolve_data_t class definition to see all member variables
rg "class presolve_data_t|papilo_original_num_variables" cpp/src/mip/problem --type cpp --type cuh -B 2 -A 2 | head -50Repository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
# Final check: see if there are any test cases for papilo_uncrush_assignment
find cpp -name "*test*.cpp" -type f -exec grep -l "papilo_uncrush" {} \;Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check if there are any error paths or exceptional cases documented for uncrush
rg "papilo_uncrush_assignment" -B 20 | grep -i "error\|fail\|return" | head -20Repository: NVIDIA/cuopt
Length of output: 690
Validate uncrushed assignment size before resize.
uncrush_primal_solution is an external papilo library call with no documented contract guaranteeing output size matches the original variable count. While the initialization phase (lines 200-230) validates the reduced-to-original and original-to-reduced mappings exhaustively with error logging, the output of uncrush_primal_solution is resized blindly at line 242 without verification. A size mismatch could propagate incorrect solution dimensions to downstream callbacks and solvers. Add a runtime check against papilo_original_num_variables and return early on mismatch, following the error-handling pattern established in set_papilo_presolve_data.
🔧 Suggested guard
papilo_presolve_ptr->uncrush_primal_solution(h_assignment, full_assignment);
+ if (full_assignment.size() != static_cast<size_t>(papilo_original_num_variables)) {
+ CUOPT_LOG_ERROR("Papilo uncrush size mismatch: got=%zu expected=%d",
+ full_assignment.size(),
+ papilo_original_num_variables);
+ return;
+ }
assignment.resize(full_assignment.size(), problem.handle_ptr->get_stream());🤖 Prompt for AI Agents
In `@cpp/src/mip/problem/presolve_data.cu` around lines 239 - 246, After calling
papilo_presolve_ptr->uncrush_primal_solution(h_assignment, full_assignment)
validate that full_assignment.size() equals papilo_original_num_variables before
resizing assignment; if it does not match, log an error using the same pattern
as set_papilo_presolve_data (include context and sizes) and return early to
avoid propagating an incorrect solution dimension. Use the symbols h_assignment,
full_assignment, assignment and papilo_original_num_variables to locate the code
and implement the guard and early return consistent with existing error
handling.
| class IncumbentSolution(StrictModel): | ||
| solution: List[float] | ||
| cost: Union[float, None] | ||
| bound: Union[float, None] |
There was a problem hiding this comment.
New required field breaks backward compatibility.
The bound field lacks a default value, making it a required field. Existing clients creating IncumbentSolution instances or deserializing data without bound will fail with a ValidationError.
Add a default value to maintain backward compatibility:
🔧 Proposed fix
class IncumbentSolution(StrictModel):
solution: List[float]
cost: Union[float, None]
- bound: Union[float, None]
+ bound: Union[float, None] = NoneAs per coding guidelines: maintain backward compatibility in Python and server APIs with deprecation warnings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class IncumbentSolution(StrictModel): | |
| solution: List[float] | |
| cost: Union[float, None] | |
| bound: Union[float, None] | |
| class IncumbentSolution(StrictModel): | |
| solution: List[float] | |
| cost: Union[float, None] | |
| bound: Union[float, None] = None |
🤖 Prompt for AI Agents
In `@python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py`
around lines 828 - 831, The new IncumbentSolution model makes bound required; to
restore backward compatibility change the field declaration on the
IncumbentSolution class so bound has a default of None (e.g., use
Optional[float] or Union[float, None] with a default = None) and ensure typing
imports (Optional) are present; keep solution and cost unchanged and optionally
emit a deprecation warning where instances lacking an explicit bound are created
if you intend to phase the field out later.
…lve, so that incumbents can be returned
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/cuopt_server/cuopt_server/tests/test_incumbents.py (1)
49-60: Add objective/constraint correctness checks for incumbents.Right now the test only validates presence/type. A broken uncrush or wrong objective could still pass. Please assert solution length, objective correctness, bound relation, and feasibility against bounds/constraints.
As per coding guidelines: “Write tests validating numerical correctness of optimization results (not just 'runs without error')” and “Add tests for problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations.”✅ Suggested test assertions (objective + feasibility)
i = res.json()[0] assert "solution" in i assert isinstance(i["solution"], list) - assert len(i["solution"]) > 0 + assert len(i["solution"]) == len(data["variable_names"]) assert "cost" in i assert isinstance(i["cost"], float) assert "bound" in i assert isinstance(i["bound"], float) + tol = 1e-6 + coeffs = data["objective_data"]["coefficients"] + computed_cost = sum(c * x for c, x in zip(coeffs, i["solution"])) + assert abs(i["cost"] - computed_cost) <= tol + # maximize: bound should not be worse than incumbent + assert i["bound"] >= i["cost"] - tol + # variable bounds + for v, lo, hi in zip( + i["solution"], + data["variable_bounds"]["lower_bounds"], + data["variable_bounds"]["upper_bounds"], + ): + assert lo - tol <= v <= hi + tol + # constraint bounds (CSR) + offsets = data["csr_constraint_matrix"]["offsets"] + indices = data["csr_constraint_matrix"]["indices"] + values = data["csr_constraint_matrix"]["values"] + for row in range(len(offsets) - 1): + start, end = offsets[row], offsets[row + 1] + row_val = sum( + values[k] * i["solution"][indices[k]] for k in range(start, end) + ) + lb = data["constraint_bounds"]["lower_bounds"][row] + ub = data["constraint_bounds"]["upper_bounds"][row] + assert lb - tol <= row_val <= ub + tol
🤖 Fix all issues with AI agents
In `@python/cuopt_server/cuopt_server/tests/test_incumbents.py`:
- Around line 82-87: Add edge-case parameterized tests for both callback paths
by updating test_incumbents to call _run_incumbent_callback with a set of
problem fixtures covering infeasible, unbounded, singleton (one-variable) and
free-variable cases; ensure you pass include_set_callback both False and True
(reuse test_incumbent_callback_get_only and test_incumbent_callback_get_set) and
extend _run_incumbent_callback to accept a problem parameter and assert the
expected status/bounds for each case (e.g., infeasible -> status infeasible,
unbounded -> unbounded ray/obj, singleton -> unique variable value, free-var ->
correct unbounded variable bounds), and add checks that validate state
transitions across presolve→solve (presolve disabled/enabled) to exercise the
uncrush/disable-presolve behavior.
| def test_incumbent_callback_get_only(cuoptproc): # noqa | ||
| _run_incumbent_callback(cuoptproc, include_set_callback=False) | ||
|
|
||
|
|
||
| def test_incumbent_callback_get_set(cuoptproc): # noqa | ||
| _run_incumbent_callback(cuoptproc, include_set_callback=True) |
There was a problem hiding this comment.
Add edge‑case incumbent tests (infeasible/unbounded/singleton/free‑var) for both callback paths.
The new get‑only/get+set paths still lack coverage for degenerate and edge cases and phase transitions (presolve→solve), which is a key risk for the new uncrush/disable‑presolve behavior. Consider parameterizing _run_incumbent_callback with at least one infeasible/unbounded/singleton/free‑var case and validating correct bounds/state.
As per coding guidelines: “Test degenerate cases (infeasible, unbounded, empty, singleton problems)” and “Add tests for algorithm phase transitions.”
🤖 Prompt for AI Agents
In `@python/cuopt_server/cuopt_server/tests/test_incumbents.py` around lines 82 -
87, Add edge-case parameterized tests for both callback paths by updating
test_incumbents to call _run_incumbent_callback with a set of problem fixtures
covering infeasible, unbounded, singleton (one-variable) and free-variable
cases; ensure you pass include_set_callback both False and True (reuse
test_incumbent_callback_get_only and test_incumbent_callback_get_set) and extend
_run_incumbent_callback to accept a problem parameter and assert the expected
status/bounds for each case (e.g., infeasible -> status infeasible, unbounded ->
unbounded ray/obj, singleton -> unique variable value, free-var -> correct
unbounded variable bounds), and add checks that validate state transitions
across presolve→solve (presolve disabled/enabled) to exercise the
uncrush/disable-presolve behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cuopt/source/cuopt-python/lp-qp-milp/examples/incumbent_solutions_example.py (1)
55-68: Docstring is misplaced after executable code.The docstring (lines 57-68) appears after the
assertstatement (line 56). In Python, docstrings must be the first statement in a function body to be recognized as documentation. Currently,get_solution.__doc__will beNone.🐛 Proposed fix
def get_solution(self, solution, solution_cost, solution_bound, user_data): - assert user_data is self.user_data """ Called whenever the solver finds a new incumbent solution. Parameters ---------- solution : array-like The variable values of the incumbent solution solution_cost : array-like The objective value of the incumbent solution solution_bound : array-like The current best bound in user objective space + user_data : object + User context passed to the callback """ + assert user_data is self.user_data self.n_callbacks += 1
🤖 Fix all issues with AI agents
In `@python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py`:
- Around line 319-321: The code unconditionally appends callbacks
(self.mip_callbacks.append(callback)) even when callback is None; update the
method in solver_settings.py to guard the append so only non-None callbacks are
added: check if callback is not None, set callback.user_data = user_data and
then append to self.mip_callbacks (or early-return when callback is None) to
match the C++ behavior and avoid storing None entries.
🧹 Nitpick comments (3)
cpp/tests/linear_programming/utilities/pdlp_test_utilities.cuh (1)
59-81: New overload looks correct; consider extracting shared logic.The new
std::vector<double>overload correctly supports testing host-side solution data from callbacks (per the PR's change to host memory pointers). The objective computation logic is sound.The implementation between lines 47-56 and 71-80 is nearly identical. Consider extracting the common computation into a helper template to reduce duplication:
♻️ Optional: Extract shared computation logic
namespace detail { template <typename Container> static double compute_objective(const Container& primal_solution, const std::vector<double>& c_vector) { std::vector<double> out(primal_solution.size()); std::transform(primal_solution.cbegin(), primal_solution.cend(), c_vector.cbegin(), out.begin(), std::multiplies<double>()); return std::reduce(out.cbegin(), out.cend(), 0.0); } } // namespace detail // Then both overloads can use: // double sum = detail::compute_objective(primal_vars, c_vector); // EXPECT_NEAR(sum, objective_value, epsilon);python/cuopt/cuopt/tests/linear_programming/test_python_API.py (2)
358-368: Consider adding length assertions for consistency.Unlike
CustomGetSolutionCallback.get_solutionwhich validateslen(solution) > 0andlen(solution_cost) == 1, theset_solutionmethod only asserts onsolution_bound. For consistency and defensive testing, consider adding similar assertions.♻️ Suggested assertion additions
def set_solution( self, solution, solution_cost, solution_bound, user_data ): assert user_data is self.user_data self.n_callbacks += 1 + assert len(solution) > 0 + assert len(solution_cost) == 1 assert len(solution_bound) == 1 if self.get_callback.solutions:
392-401: Consider usingpytest.approxfor floating-point comparison and verifying set_callback invocation.
Line 400 uses exact equality (
==) to compare computed objective with stored cost. While both values originate from the solver, floating-point arithmetic differences could cause flaky failures.When
include_set_callback=True, the test doesn't verifyset_callback.n_callbacks > 0, leaving the set_callback path untested for actual invocation.♻️ Suggested improvements
assert get_callback.n_callbacks > 0 + if include_set_callback: + assert set_callback.n_callbacks > 0 for sol in get_callback.solutions: x_val = sol["solution"][0] y_val = sol["solution"][1] cost = sol["cost"] assert 2 * x_val + 4 * y_val >= 230 assert 3 * x_val + 2 * y_val <= 190 - assert 5 * x_val + 3 * y_val == cost + assert 5 * x_val + 3 * y_val == pytest.approx(cost)As per coding guidelines: "Write tests validating numerical correctness of optimization results (not just 'runs without error')".
| if callback is not None: | ||
| callback.user_data = user_data | ||
| self.mip_callbacks.append(callback) |
There was a problem hiding this comment.
None callbacks are appended to the list, which may cause downstream issues.
The implementation appends the callback unconditionally, even when callback is None. This differs from the C++ implementation which has an early return for null callbacks. Consider guarding the append:
🔧 Suggested fix
- if callback is not None:
- callback.user_data = user_data
- self.mip_callbacks.append(callback)
+ if callback is not None:
+ callback.user_data = user_data
+ self.mip_callbacks.append(callback)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if callback is not None: | |
| callback.user_data = user_data | |
| self.mip_callbacks.append(callback) | |
| if callback is not None: | |
| callback.user_data = user_data | |
| self.mip_callbacks.append(callback) |
🤖 Prompt for AI Agents
In `@python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py`
around lines 319 - 321, The code unconditionally appends callbacks
(self.mip_callbacks.append(callback)) even when callback is None; update the
method in solver_settings.py to guard the append so only non-None callbacks are
added: check if callback is not None, set callback.user_data = user_data and
then append to self.mip_callbacks (or early-return when callback is None) to
match the C++ behavior and avoid storing None entries.
4 participants
This PR adds callbacks to C API. Users can now use C API to implement get_solution and set_solution callbacks.
This PR is a breaking PR that changes callback input/output pointers from device memory to host memory. We are also adding a user_data pointer for user to be more flexible inside the callbacks as well as dual bound for the B&B tree.
Previously callbacks were not enabled when third party presolved was enabled. Now we are able to handle uncrushing of solutions when third party presolve is enabled on get_solution callbacks. Registering a set_solution callback disables presolve as there is no way of crushing a solution in original space to reduced space when using third party presolve.
The examples and tests are modified to reflect the change.
Summary by CodeRabbit
New Features
Behavior
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.