[DRAFT] Solver persistence pipeline#1518
Conversation
📝 WalkthroughWalkthroughIntroduces a reusable GPU barrier solve session ( ChangesLP Solve Session Reuse Feature
Standalone Benchmark and Verification Scripts
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp (1)
333-336: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDocument the pointer's lifetime/ownership contract.
lp_solve_sessionis a non-owning pointer set externally bycall_solve; ownership of the pointee later moves intolinear_programming_ret_t::lp_solve_session(aunique_ptr). The comment doesn't clarify how long this raw pointer stays valid or whether callers must reset it between logically-unrelated solves (e.g. when sparsity/structure changes). Given the PR's explicit "session reuse as a state-reset risk" concern, spelling this out here would help prevent future dangling-pointer misuse.As per path instructions, "Suggest documenting thread-safety, GPU requirements, numerical behavior" for public C++ headers under
cpp/include/cuopt/**/*.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp` around lines 333 - 336, Document the ownership and lifetime contract for lp_solve_session in solver_settings.hpp: make it explicit that this raw pointer is non-owning, is only valid for the duration of the matching call_solve/solve flow, and that ownership is transferred later into linear_programming_ret_t::lp_solve_session (the unique_ptr). Also note that callers should clear or replace it before unrelated solves or any structural/sparsity change, and add a short note on any thread-safety expectations if session_enabled and lp_solve_session are reused across calls.Source: Path instructions
python/cuopt/cuopt/linear_programming/problem.py (1)
1656-1660: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant conditional — both branches are identical.
n_rowscomputeslen(self.rhs)in both theifandelsearms.♻️ Simplify
- n_rows = ( - len(self.rhs) - if isinstance(self.rhs, np.ndarray) - else len(self.rhs) - ) + n_rows = len(self.rhs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1656 - 1660, The n_rows assignment in problem.py is using a redundant isinstance(self.rhs, np.ndarray) conditional because both branches call len(self.rhs); simplify the expression in the same location by removing the unnecessary if/else and keeping a single len(self.rhs) computation, preserving the existing behavior in the problem-related method that builds the row count.script_perf_eval.py (2)
208-218: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the dense diagonal materialization.
np.diag(info["D_diag"])builds ann × ndense matrix (n=5000 by default → ~200 MB) on every objective snapshot just to compute a diagonal quadratic form. Use the elementwise form instead.♻️ Proposed change
y = info["F"].T @ x_np z = np.abs(x_np - info["x0"]) - d_matrix = np.diag(info["D_diag"]) return ( -info["mu"] @ x_np + info["gamma"] - * (x_np @ d_matrix @ x_np + y @ info["Omega"] @ y) + * (np.sum(info["D_diag"] * x_np * x_np) + y @ info["Omega"] @ y) + info["tc_rate"] * np.sum(z) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script_perf_eval.py` around lines 208 - 218, The portfolio objective in portfolio_objective is materializing a dense diagonal matrix via np.diag(info["D_diag"]) just to compute the diagonal quadratic term. Replace the x_np @ d_matrix @ x_np path with the equivalent elementwise formulation using info["D_diag"] directly, and remove the d_matrix allocation so the objective stays memory-efficient.
489-548: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused cross-mode helpers
verify_objectives_across_modes,_print_objective_cross_check, and_write_resultsare no longer called;--mode alluses_compare_mode_objectivesand_write_bench_artifactinstead. Removing the stale helpers would keepscript_perf_eval.pyeasier to follow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@script_perf_eval.py` around lines 489 - 548, Remove the stale cross-mode objective helpers from script_perf_eval.py: verify_objectives_across_modes, _print_objective_cross_check, and _write_results are no longer used because --mode all now routes through _compare_mode_objectives and _write_bench_artifact. Delete these unused functions and any now-dead references so the module only keeps the active benchmark flow.cpp/src/barrier/barrier_factorization_sparsity_hash.hpp (1)
54-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftDuplicated augmented-KKT layout logic risks silent drift from
form_augmented.This function manually reconstructs the same row/diagonal/column ordering as
iteration_data_t::form_augmented(true)(per the comment on line 57), and it's the only cross-check available for the production hash path inhash_device_csr_sparsity_pattern. Thecuopt_assertcomparing host vs. device hashes inbarrier.cuonly runs under#ifndef NDEBUG, so any future divergence between this function and the real matrix-construction code would go undetected in release builds, potentially causing symbolic-cache reuse decisions to be made against a subtly wrong sparsity hash.Consider extracting the shared column-ordering logic (diagonal insertion, Q/A/AT traversal order) into a single helper used by both
form_augmentedand this hash function, so they cannot diverge independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/barrier/barrier_factorization_sparsity_hash.hpp` around lines 54 - 117, The augmented-KKT sparsity layout is duplicated here and in iteration_data_t::form_augmented(true), so the hash logic can silently drift from the real matrix construction. Refactor the shared ordering rules (Q diagonal handling, A row entries, and AT lower block traversal) into a common helper and have both hash_augmented_kkt_sparsity and form_augmented use it, so the host hash stays identical to the production layout and hash_device_csr_sparsity_pattern checks remain trustworthy.cpp/src/pdlp/utilities/cython_solve.cu (1)
144-170: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
ephemeral_handleis constructed even when a session handle is used, and the else-branchC01timer measures nothing.
ephemeral_stream/ephemeral_handleare created unconditionally at Lines 144-145, but in thewant_sessionpathsolve_handleis reassigned to the session handle (Line 161), leaving the freshly-constructedraft::handle_tunused — an avoidable allocation/CUDA-context setup. Also, the else-branch at Lines 164-169 records elapsed time between two immediately-adjacentsteady_clock::now()calls, so theC01sample is effectively zero and does not capture the handle-creation cost (which already happened at Line 145). Consider constructing the ephemeral handle only in the non-session branch and moving the timer around the actual construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/pdlp/utilities/cython_solve.cu` around lines 144 - 170, The setup in cython_solve.cu is doing unnecessary work and misreporting timing: `ephemeral_stream`/`ephemeral_handle` are created before the `memory_backend_t::GPU` and `want_session` checks even though `solve_handle` is switched to `active_session->handle_ptr()` in the session path, and the `else` branch around `C01` records time without measuring any construction. Update the control flow so `raft::handle_t` is only created in the non-session path, and place the `cache_profile::C01` timing around the actual ephemeral handle creation (using the existing `ephemeral_stream`, `ephemeral_handle`, and `lp_solve_session_t::create`/`handle_ptr` logic) so the sample reflects real setup cost.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/include/cuopt/linear_programming/utilities/lp_solve_session.hpp`:
- Around line 28-63: The public API on lp_solve_session_t is missing Doxygen and
an explicit threading contract. Add comments for create, handle_ptr,
stream_view, clear_symbolic_cache, and store_symbolic_cache describing
ownership, return values, and cache behavior. Also state clearly that
lp_solve_session_t is not thread-safe for concurrent use, and that the
unsynchronized cache mutators and reuse helpers must only be called from a
single thread or externally serialized context.
In `@cpp/include/cuopt/linear_programming/utilities/solver_cache_profiler.hpp`:
- Around line 54-116: Guard the singleton profiler state in profiler_t: reset(),
add(), and log_summary() all read/write the shared times_ array without
synchronization, so concurrent OMP task execution can race and corrupt cache
profiling. Add a mutex or other thread-safe protection around the mutable state
in profiler_t::instance(), profiler_t::reset(), profiler_t::add(), and any
readout in profiler_t::log_summary(), or explicitly restrict CUOPT_CACHE_PROFILE
to single-threaded use if locking is not desired.
In `@cpp/src/barrier/barrier.cu`:
- Around line 943-945: The transpose call in the warm-reuse path is using the
operands in the wrong order: in the barrier setup around the AD/AT rebuild,
`AT.transpose(AD)` overwrites the newly prepared `AD` instead of recomputing
`AT` from it. Update the logic so the updated `AD` is the source and `AT` is the
destination, using the `transpose` method on the `AT`/`AD` pair in the barrier
code path, so the subsequent `ad_mat().copy(...)` sees the refreshed values.
In `@cpp/src/barrier/cusparse_view.cu`:
- Around line 257-265: update_matrix_values in cusparse_view_t blindly copies
A.x and A_csr.x into A_T_data_ and A_data_ without checking nnz sizes match the
destination buffers. Add a size/assertion check at the start of
cusparse_view_t<i_t, f_t>::update_matrix_values to verify A.x.size() and
A_csr.x.size() are the same size as A_T_data_ and A_data_ before calling
raft::copy, so the warm-cache reuse path in barrier.cu cannot silently write out
of bounds if the sparsity invariant is broken.
In `@cpp/src/barrier/sparse_cholesky.cuh`:
- Around line 533-534: The cuDSS reuse state is inconsistent because
numeric_factor_valid_ is only being cleared and never actually drives any logic.
Update the sparse_cholesky.cuh cuDSS flow around symbolic_done_ and the
factorization/rebind path to either set and check numeric_factor_valid_ as part
of the reuse contract, or remove the member and its invalidation writes entirely
if it is unused. Make sure the relevant factorization/rebind methods in
SparseCholesky keep the state transitions consistent with the other validity
flags.
In `@cpp/src/pdlp/solve.cu`:
- Around line 1887-1890: The fingerprint computed in the C03 scope is currently
unused, so update the code around compute_problem_fingerprint and the C03 cache
profile block to either remove the call entirely or pass the result into the
cache path if it is needed there. If the intent is only to measure fingerprint
overhead, keep the call in the same scope but add a brief comment clarifying
that it exists for profiling only, and avoid leaving a dropped result with
[[maybe_unused]] unless it is truly intentional.
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1671-1701: The cached DataModel refresh in
_refresh_data_model_values only updates objective coefficients and constraint
bounds, so bound, variable type, and MIP start edits from setLowerBound,
setUpperBound, setVariableType, and setMIPStart remain stale on a reused
solve(). Update those setters to invalidate or sync the cached model state, or
extend _refresh_data_model_values to also push the remaining mutable fields
before calling solve(). Keep the fix centered on the Problem methods and model
refresh path so cached values always reflect the latest Python-side changes.
In `@python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py`:
- Around line 80-110: The capture_solver_output context manager currently
redirects stdout and stderr to pipes and only reads them after prob.solve()
finishes, which can deadlock on verbose output. Update capture_solver_output to
use file-backed temporary capture instead of os.pipe/os.dup2 pipes, and keep the
existing restore-and-replay behavior when gathering text from the captured
streams. Preserve the same public behavior of yielding the capture object while
fixing the implementation inside capture_solver_output.
In `@script_perf_eval.py`:
- Around line 110-129: The `_capture_solver_output` context manager can block
`prob.solve()` because it only reads from the pipe after the solver finishes,
allowing the `stderr` pipe buffer to fill up. Update `_capture_solver_output` to
avoid backpressure by either redirecting fd 2 to a temporary file or
continuously draining `read_fd` in a background reader while the solve runs,
then preserve the existing behavior of restoring stderr and forwarding captured
text afterward.
---
Nitpick comments:
In `@cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp`:
- Around line 333-336: Document the ownership and lifetime contract for
lp_solve_session in solver_settings.hpp: make it explicit that this raw pointer
is non-owning, is only valid for the duration of the matching call_solve/solve
flow, and that ownership is transferred later into
linear_programming_ret_t::lp_solve_session (the unique_ptr). Also note that
callers should clear or replace it before unrelated solves or any
structural/sparsity change, and add a short note on any thread-safety
expectations if session_enabled and lp_solve_session are reused across calls.
In `@cpp/src/barrier/barrier_factorization_sparsity_hash.hpp`:
- Around line 54-117: The augmented-KKT sparsity layout is duplicated here and
in iteration_data_t::form_augmented(true), so the hash logic can silently drift
from the real matrix construction. Refactor the shared ordering rules (Q
diagonal handling, A row entries, and AT lower block traversal) into a common
helper and have both hash_augmented_kkt_sparsity and form_augmented use it, so
the host hash stays identical to the production layout and
hash_device_csr_sparsity_pattern checks remain trustworthy.
In `@cpp/src/pdlp/utilities/cython_solve.cu`:
- Around line 144-170: The setup in cython_solve.cu is doing unnecessary work
and misreporting timing: `ephemeral_stream`/`ephemeral_handle` are created
before the `memory_backend_t::GPU` and `want_session` checks even though
`solve_handle` is switched to `active_session->handle_ptr()` in the session
path, and the `else` branch around `C01` records time without measuring any
construction. Update the control flow so `raft::handle_t` is only created in the
non-session path, and place the `cache_profile::C01` timing around the actual
ephemeral handle creation (using the existing `ephemeral_stream`,
`ephemeral_handle`, and `lp_solve_session_t::create`/`handle_ptr` logic) so the
sample reflects real setup cost.
In `@python/cuopt/cuopt/linear_programming/problem.py`:
- Around line 1656-1660: The n_rows assignment in problem.py is using a
redundant isinstance(self.rhs, np.ndarray) conditional because both branches
call len(self.rhs); simplify the expression in the same location by removing the
unnecessary if/else and keeping a single len(self.rhs) computation, preserving
the existing behavior in the problem-related method that builds the row count.
In `@script_perf_eval.py`:
- Around line 208-218: The portfolio objective in portfolio_objective is
materializing a dense diagonal matrix via np.diag(info["D_diag"]) just to
compute the diagonal quadratic term. Replace the x_np @ d_matrix @ x_np path
with the equivalent elementwise formulation using info["D_diag"] directly, and
remove the d_matrix allocation so the objective stays memory-efficient.
- Around line 489-548: Remove the stale cross-mode objective helpers from
script_perf_eval.py: verify_objectives_across_modes,
_print_objective_cross_check, and _write_results are no longer used because
--mode all now routes through _compare_mode_objectives and
_write_bench_artifact. Delete these unused functions and any now-dead references
so the module only keeps the active benchmark flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3bdaaade-7af5-444d-ad1e-def826ca93d5
📒 Files selected for processing (32)
cpp/include/cuopt/linear_programming/pdlp/solver_settings.hppcpp/include/cuopt/linear_programming/utilities/cython_solve.hppcpp/include/cuopt/linear_programming/utilities/cython_types.hppcpp/include/cuopt/linear_programming/utilities/lp_solve_session.hppcpp/include/cuopt/linear_programming/utilities/solver_cache_profiler.hppcpp/src/barrier/CMakeLists.txtcpp/src/barrier/barrier.cucpp/src/barrier/barrier.hppcpp/src/barrier/barrier_factorization_sparsity_hash.cucpp/src/barrier/barrier_factorization_sparsity_hash.hppcpp/src/barrier/barrier_symbolic_cache.hppcpp/src/barrier/cusparse_view.cucpp/src/barrier/cusparse_view.hppcpp/src/barrier/device_sparse_matrix.cuhcpp/src/barrier/sparse_cholesky.cuhcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/pdlp/CMakeLists.txtcpp/src/pdlp/solve.cucpp/src/pdlp/utilities/cython_solve.cucpp/src/pdlp/utilities/lp_solve_session.cupython/cuopt/cuopt/linear_programming/problem.pypython/cuopt/cuopt/linear_programming/solution/solution.pypython/cuopt/cuopt/linear_programming/solver/solver.pxdpython/cuopt/cuopt/linear_programming/solver/solver.pypython/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxpython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pxdpython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyxpython/cuopt/cuopt/tests/linear_programming/session_cache_helpers.pypython/cuopt/cuopt/tests/linear_programming/test_lp_solve_session.pyscript_perf_eval.pyscript_session_cache_tests.py
| /** | ||
| * @brief Lean GPU solve session: owns RAFT handle + stream and optional barrier symbolic cache. | ||
| * | ||
| * Created on first solve when session_enabled; reused on subsequent solves with the same capsule. | ||
| * Per-solve state (optimization_problem_t, presolve, barrier_lp) remains stack-local. | ||
| */ | ||
| class lp_solve_session_t { | ||
| public: | ||
| static std::unique_ptr<lp_solve_session_t> create(unsigned stream_flags); | ||
|
|
||
| lp_solve_session_t(lp_solve_session_t&&) noexcept; | ||
| lp_solve_session_t& operator=(lp_solve_session_t&&) noexcept; | ||
| ~lp_solve_session_t(); | ||
|
|
||
| [[nodiscard]] raft::handle_t* handle_ptr(); | ||
| [[nodiscard]] raft::handle_t const* handle_ptr() const; | ||
| [[nodiscard]] rmm::cuda_stream_view stream_view() const; | ||
|
|
||
| /** | ||
| * @brief Returns cached symbolic state when valid and @p handle matches the stored handle. | ||
| */ | ||
| [[nodiscard]] linear_programming::dual_simplex::barrier_symbolic_cache_t<int, double>* | ||
| symbolic_cache_for_reuse(raft::handle_t const* handle); | ||
|
|
||
| void clear_symbolic_cache(); | ||
|
|
||
| void store_symbolic_cache( | ||
| linear_programming::dual_simplex::iteration_data_t<int, double>& data); | ||
|
|
||
| private: | ||
| lp_solve_session_t(std::unique_ptr<rmm::cuda_stream> stream, | ||
| std::unique_ptr<raft::handle_t> handle); | ||
|
|
||
| struct impl; | ||
| std::unique_ptr<impl> impl_; | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files cpp/include/cuopt/linear_programming/utilities/lp_solve_session.hpp
printf '\n--- file outline ---\n'
ast-grep outline cpp/include/cuopt/linear_programming/utilities/lp_solve_session.hpp --view expanded || true
printf '\n--- file excerpt ---\n'
cat -n cpp/include/cuopt/linear_programming/utilities/lp_solve_session.hpp | sed -n '1,220p'
printf '\n--- search for lp_solve_session_t references ---\n'
rg -n "lp_solve_session_t|thread-safety|thread safety|concurrent|serialize access|session_enabled|symbolic_cache_for_reuse|store_symbolic_cache|clear_symbolic_cache" cpp/include cpp/src -g '!**/build/**' || trueRepository: NVIDIA/cuopt
Length of output: 29589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- lp_solve_session implementation ---\n'
cat -n cpp/src/pdlp/utilities/lp_solve_session.cu | sed -n '1,220p'
printf '\n--- cython solve usage around session lifecycle ---\n'
cat -n cpp/src/pdlp/utilities/cython_solve.cu | sed -n '120,260p'
printf '\n--- public header with session field docs ---\n'
cat -n cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp | sed -n '320,350p'Repository: NVIDIA/cuopt
Length of output: 13671
Add method docs and spell out the threading contract. The public methods here (create, handle_ptr, stream_view, clear_symbolic_cache, store_symbolic_cache) still need Doxygen comments, and the session should explicitly say whether callers can reuse one instance from multiple threads. The cache mutators are unsynchronized, so the intended access pattern should be stated plainly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/include/cuopt/linear_programming/utilities/lp_solve_session.hpp` around
lines 28 - 63, The public API on lp_solve_session_t is missing Doxygen and an
explicit threading contract. Add comments for create, handle_ptr, stream_view,
clear_symbolic_cache, and store_symbolic_cache describing ownership, return
values, and cache behavior. Also state clearly that lp_solve_session_t is not
thread-safe for concurrent use, and that the unsynchronized cache mutators and
reuse helpers must only be called from a single thread or externally serialized
context.
Source: Path instructions
| class profiler_t { | ||
| public: | ||
| static profiler_t& instance() | ||
| { | ||
| static profiler_t prof; | ||
| return prof; | ||
| } | ||
|
|
||
| bool enabled() const { return enabled_; } | ||
|
|
||
| void reset() | ||
| { | ||
| times_.fill(0.0); | ||
| } | ||
|
|
||
| void add(cache_id id, double seconds) | ||
| { | ||
| if (!enabled_) { return; } | ||
| times_[static_cast<int>(id)] += seconds; | ||
| } | ||
|
|
||
| double get(cache_id id) const { return times_[static_cast<int>(id)]; } | ||
|
|
||
| double total_measured() const | ||
| { | ||
| double sum = 0.0; | ||
| for (double t : times_) { | ||
| sum += t; | ||
| } | ||
| return sum; | ||
| } | ||
|
|
||
| void log_summary() const | ||
| { | ||
| if (!enabled_) { return; } | ||
| auto emit = [](const char* fmt, ...) { | ||
| va_list args; | ||
| va_start(args, fmt); | ||
| char buf[512]; | ||
| vsnprintf(buf, sizeof(buf), fmt, args); | ||
| va_end(args); | ||
| CUOPT_LOG_INFO("%s", buf); | ||
| fprintf(stderr, "%s\n", buf); | ||
| }; | ||
| emit("=== Solver cache profile (ms) ==="); | ||
| for (int i = 0; i < num_cache_ids; ++i) { | ||
| const double ms = times_[i] * 1000.0; | ||
| emit("Cache profile: %s %.3f", cache_id_label(static_cast<cache_id>(i)), ms); | ||
| } | ||
| emit("Cache profile: TOTAL measured %.3f", total_measured() * 1000.0); | ||
| emit("=== End solver cache profile ==="); | ||
| } | ||
|
|
||
| private: | ||
| profiler_t() | ||
| { | ||
| const char* env = std::getenv("CUOPT_CACHE_PROFILE"); | ||
| enabled_ = env != nullptr && env[0] != '\0' && std::strcmp(env, "0") != 0; | ||
| } | ||
|
|
||
| bool enabled_{false}; | ||
| std::array<double, num_cache_ids> times_{}; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the profiler and its call sites
git ls-files | rg 'solver_cache_profiler|cython_solve|solver_settings|solve\.cu|Concurrent'
# Show the profiler header with line numbers
sed -n '1,220p' cpp/include/cuopt/linear_programming/utilities/solver_cache_profiler.hpp
# Find all references to cache_profile::reset() and profiler_t::instance()
rg -n "cache_profile::reset\(|profiler_t::instance\(|CUOPT_CACHE_PROFILE|method_t::Concurrent|Concurrent" cpp include python -S
# Inspect the likely solve entry point and concurrent execution references
sed -n '1,260p' cpp/src/linear_programming/cython_solve.cu
sed -n '1,260p' cpp/include/cuopt/linear_programming/solver_settings.hpp
sed -n '1,260p' cpp/src/linear_programming/pdlp/solve.cuRepository: NVIDIA/cuopt
Length of output: 16099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the solve entry point and profiler reset site
sed -n '90,150p' cpp/src/pdlp/utilities/cython_solve.cu
# Inspect the method_t documentation and default concurrent behavior
sed -n '50,90p' cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
sed -n '300,330p' cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
# Inspect the main PDLP solve path around Concurrent handling
sed -n '1810,1865p' cpp/src/pdlp/solve.cu
# Check for any synchronization around the profiler or cache_profile namespace
rg -n "mutex|lock_guard|atomic|cache_profile::|CUOPT_CACHE_PROFILE_SCOPE" cpp/src/pdlp cpp/src/barrier cpp/src/dual_simplex cpp/include/cuopt/linear_programming -SRepository: NVIDIA/cuopt
Length of output: 12850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the concurrent solve implementation
sed -n '1,220p' cpp/src/pdlp/solve.cu | sed -n '1,220p'
rg -n "run_concurrent|std::thread|std::async|omp|launch|concurrent_halt|Barrier|DualSimplex" cpp/src/pdlp/solve.cu cpp/src/barrier cpp/src/dual_simplex -S
# Inspect the cache-profile usage sites in the concurrent backends
sed -n '1,120p' cpp/src/barrier/barrier.cu
sed -n '1820,1975p' cpp/src/pdlp/solve.cuRepository: NVIDIA/cuopt
Length of output: 50369
Guard the process-wide profiler state. profiler_t::reset() and add() mutate a singleton times_ array with no synchronization. Since run_concurrent() schedules work on OMP tasks, overlapping timers and concurrent solves can race with reset(), corrupting the reported cache profile. A mutex here would be small; otherwise document that CUOPT_CACHE_PROFILE is single-threaded only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/include/cuopt/linear_programming/utilities/solver_cache_profiler.hpp`
around lines 54 - 116, Guard the singleton profiler state in profiler_t:
reset(), add(), and log_summary() all read/write the shared times_ array without
synchronization, so concurrent OMP task execution can race and corrupt cache
profiling. Add a mutex or other thread-safe protection around the mutable state
in profiler_t::instance(), profiler_t::reset(), profiler_t::add(), and any
readout in profiler_t::log_summary(), or explicitly restrict CUOPT_CACHE_PROFILE
to single-threaded use if locking is not desired.
Source: Path instructions
| AD = A; | ||
| if (!use_augmented && n_dense_columns > 0) { AD.remove_columns(cols_to_remove); } | ||
| AT.transpose(AD); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm transpose semantics: X.transpose(Y) => Y = X^T
rg -nP --type=cpp -C3 '\btranspose\s*\(' cpp/src/dual_simplex/sparse_matrix.hpp
rg -nP -C1 '\.transpose\(' cpp/src/barrier/barrier.cuRepository: NVIDIA/cuopt
Length of output: 1453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sparse_matrix transpose declaration/definition =="
rg -n -C 6 '\btranspose\s*\(' cpp/src/dual_simplex/sparse_matrix.hpp cpp/src/dual_simplex/sparse_matrix.cuh cpp/src/dual_simplex/sparse_matrix.cu
echo
echo "== barrier.cu constructor around transpose =="
sed -n '575,605p' cpp/src/barrier/barrier.cu | cat -n
echo
echo "== barrier.cu refresh_lp_numerics around lines 930-990 =="
sed -n '930,990p' cpp/src/barrier/barrier.cu | cat -nRepository: NVIDIA/cuopt
Length of output: 1408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== transpose implementation in sparse_matrix.hpp =="
sed -n '1,170p' cpp/src/dual_simplex/sparse_matrix.hpp | cat -n
echo
echo "== barrier.cu constructor and refresh block =="
sed -n '585,605p' cpp/src/barrier/barrier.cu | cat -n
echo
sed -n '935,990p' cpp/src/barrier/barrier.cu | cat -nRepository: NVIDIA/cuopt
Length of output: 9787
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== class members / initialization paths mentioning AD and AT =="
rg -n -C 4 '\b(AD|AT)\b' cpp/src/barrier/barrier.cu
echo
echo "== refresh_lp_numerics full function =="
ast-grep outline cpp/src/barrier/barrier.cu --match refresh_lp_numerics --view expandedRepository: NVIDIA/cuopt
Length of output: 14726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== class members / initialization paths mentioning AD and AT =="
rg -n -C 4 '\b(AD|AT)\b' cpp/src/barrier/barrier.cu
echo
echo "== refresh_lp_numerics and nearby helpers =="
rg -n -C 40 'bool refresh_lp_numerics|refresh_lp_numerics\(' cpp/src/barrier/barrier.cuRepository: NVIDIA/cuopt
Length of output: 21557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the transpose implementation to confirm argument semantics.
rg -n -C 20 'transpose\(csc_matrix_t<i_t, f_t>& AT\) const' cpp/src/dual_simplex/sparse_matrix.hpp
# Show where AT is consumed after refresh_lp_numerics.
rg -n -C 3 '\bAT\b' cpp/src/barrier/barrier.cu | sed -n '1,120p'Repository: NVIDIA/cuopt
Length of output: 5294
Swap these transpose operands cpp/src/barrier/barrier.cu:943-945
transpose(out) writes out = this^T, so AT.transpose(AD) overwrites the fresh AD with AT^T instead of rebuilding AT from the updated AD. That leaves ad_mat().copy(AD, ...) uploading stale values into ADAT on the warm-reuse path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/barrier/barrier.cu` around lines 943 - 945, The transpose call in the
warm-reuse path is using the operands in the wrong order: in the barrier setup
around the AD/AT rebuild, `AT.transpose(AD)` overwrites the newly prepared `AD`
instead of recomputing `AT` from it. Update the logic so the updated `AD` is the
source and `AT` is the destination, using the `transpose` method on the
`AT`/`AD` pair in the barrier code path, so the subsequent `ad_mat().copy(...)`
sees the refreshed values.
| template <typename i_t, typename f_t> | ||
| void cusparse_view_t<i_t, f_t>::update_matrix_values(const csc_matrix_t<i_t, f_t>& A) | ||
| { | ||
| const auto stream = handle_ptr_->get_stream(); | ||
| raft::copy(A_T_data_.data(), A.x.data(), A.x.size(), stream); | ||
| csr_matrix_t<i_t, f_t> A_csr(A.m, A.n, 1); | ||
| A.to_compressed_row(A_csr); | ||
| raft::copy(A_data_.data(), A_csr.x.data(), A_csr.x.size(), stream); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add size validation before overwriting A_T_data_/A_data_.
update_matrix_values copies A.x.size() / A_csr.x.size() elements into A_T_data_ / A_data_ without checking those destination buffers were sized for the same nnz. This method is designed to be called only during warm symbolic-cache reuse where sparsity (and hence nnz) is guaranteed unchanged by the caller in barrier.cu, but that invariant isn't enforced here — a future caller or a bug in the hash-matching gate upstream would cause a silent out-of-bounds device write.
🔒 Proposed fix: assert size match before copy
template <typename i_t, typename f_t>
void cusparse_view_t<i_t, f_t>::update_matrix_values(const csc_matrix_t<i_t, f_t>& A)
{
const auto stream = handle_ptr_->get_stream();
+ cuopt_assert(A.x.size() == A_T_data_.size(),
+ "update_matrix_values: nnz mismatch for A_T_data_ (sparsity changed?)");
raft::copy(A_T_data_.data(), A.x.data(), A.x.size(), stream);
csr_matrix_t<i_t, f_t> A_csr(A.m, A.n, 1);
A.to_compressed_row(A_csr);
+ cuopt_assert(A_csr.x.size() == A_data_.size(),
+ "update_matrix_values: nnz mismatch for A_data_ (sparsity changed?)");
raft::copy(A_data_.data(), A_csr.x.data(), A_csr.x.size(), stream);
}📝 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.
| template <typename i_t, typename f_t> | |
| void cusparse_view_t<i_t, f_t>::update_matrix_values(const csc_matrix_t<i_t, f_t>& A) | |
| { | |
| const auto stream = handle_ptr_->get_stream(); | |
| raft::copy(A_T_data_.data(), A.x.data(), A.x.size(), stream); | |
| csr_matrix_t<i_t, f_t> A_csr(A.m, A.n, 1); | |
| A.to_compressed_row(A_csr); | |
| raft::copy(A_data_.data(), A_csr.x.data(), A_csr.x.size(), stream); | |
| } | |
| template <typename i_t, typename f_t> | |
| void cusparse_view_t<i_t, f_t>::update_matrix_values(const csc_matrix_t<i_t, f_t>& A) | |
| { | |
| const auto stream = handle_ptr_->get_stream(); | |
| cuopt_assert(A.x.size() == A_T_data_.size(), | |
| "update_matrix_values: nnz mismatch for A_T_data_ (sparsity changed?)"); | |
| raft::copy(A_T_data_.data(), A.x.data(), A.x.size(), stream); | |
| csr_matrix_t<i_t, f_t> A_csr(A.m, A.n, 1); | |
| A.to_compressed_row(A_csr); | |
| cuopt_assert(A_csr.x.size() == A_data_.size(), | |
| "update_matrix_values: nnz mismatch for A_data_ (sparsity changed?)"); | |
| raft::copy(A_data_.data(), A_csr.x.data(), A_csr.x.size(), stream); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/barrier/cusparse_view.cu` around lines 257 - 265,
update_matrix_values in cusparse_view_t blindly copies A.x and A_csr.x into
A_T_data_ and A_data_ without checking nnz sizes match the destination buffers.
Add a size/assertion check at the start of cusparse_view_t<i_t,
f_t>::update_matrix_values to verify A.x.size() and A_csr.x.size() are the same
size as A_T_data_ and A_data_ before calling raft::copy, so the warm-cache reuse
path in barrier.cu cannot silently write out of bounds if the sparsity invariant
is broken.
| symbolic_done_ = true; | ||
| numeric_factor_valid_ = false; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and symbol usages.
git ls-files cpp/src/barrier/sparse_cholesky.cuh
printf '\n--- symbol occurrences ---\n'
rg -n "numeric_factor_valid_|symbolic_done_|factorize|solve|invalidate_numeric_factor|rebind_csr_matrix|rebind_settings" cpp/src/barrier/sparse_cholesky.cuh
printf '\n--- targeted ranges around occurrences ---\n'
sed -n '120,170p' cpp/src/barrier/sparse_cholesky.cuh
sed -n '500,560p' cpp/src/barrier/sparse_cholesky.cuh
sed -n '860,970p' cpp/src/barrier/sparse_cholesky.cuhRepository: NVIDIA/cuopt
Length of output: 12870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file size and the whole symbol context in smaller slices if needed.
wc -l cpp/src/barrier/sparse_cholesky.cuhRepository: NVIDIA/cuopt
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the repository for all references to numeric_factor_valid_.
rg -n "numeric_factor_valid_" .Repository: NVIDIA/cuopt
Length of output: 575
numeric_factor_valid_ is dead state in the cuDSS path
numeric_factor_valid_ is only ever cleared here and in the invalidation paths, but never read or set true, so it does not participate in the reuse contract. Either wire it into the post-factorization/rebind flow or remove it to avoid misleading state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/barrier/sparse_cholesky.cuh` around lines 533 - 534, The cuDSS reuse
state is inconsistent because numeric_factor_valid_ is only being cleared and
never actually drives any logic. Update the sparse_cholesky.cuh cuDSS flow
around symbolic_done_ and the factorization/rebind path to either set and check
numeric_factor_valid_ as part of the reuse contract, or remove the member and
its invalidation writes entirely if it is unused. Make sure the relevant
factorization/rebind methods in SparseCholesky keep the state transitions
consistent with the other validity flags.
Source: Path instructions
| { | ||
| CUOPT_CACHE_PROFILE_SCOPE(cuopt::linear_programming::cache_profile::cache_id::C03); | ||
| [[maybe_unused]] const uint64_t fingerprint = compute_problem_fingerprint(op_problem); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant symbols first.
ast-grep outline cpp/src/pdlp/solve.cu --view expanded
# Read the surrounding section with line numbers.
sed -n '60,110p;1860,1915p' cpp/src/pdlp/solve.cu
# Search for all uses of the fingerprint helper and its return value.
rg -n "compute_problem_fingerprint|fnv1a64_mix|fingerprint" cpp/src/pdlp/solve.cu cpp/src/pdlp -SRepository: NVIDIA/cuopt
Length of output: 6866
Fingerprint result is unused inside the C03 scope — the value is computed from host copies and then dropped. Remove the call or wire it into the cache path; if it is only there to profile fingerprint cost, add a brief comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/pdlp/solve.cu` around lines 1887 - 1890, The fingerprint computed in
the C03 scope is currently unused, so update the code around
compute_problem_fingerprint and the C03 cache profile block to either remove the
call entirely or pass the result into the cache path if it is needed there. If
the intent is only to measure fingerprint overhead, keep the call in the same
scope but add a brief comment clarifying that it exists for profiling only, and
avoid leaving a dropped result with [[maybe_unused]] unless it is truly
intentional.
| def _refresh_data_model_values(self): | ||
| """Patch existing DataModel when sparsity structure is unchanged.""" | ||
| n = len(self.vars) | ||
| if ( | ||
| self.model is None | ||
| or self.constraint_csr_matrix is None | ||
| or self.objective is None | ||
| or len(self.objective) != n | ||
| or self.rhs is None | ||
| ): | ||
| self._to_data_model() | ||
| return | ||
|
|
||
| for j in range(n): | ||
| self.objective[j] = self.vars[j].getObjectiveCoefficient() | ||
|
|
||
| rhs_arr = ( | ||
| self.rhs | ||
| if isinstance(self.rhs, np.ndarray) | ||
| else np.asarray(self.rhs, dtype=np.float64) | ||
| ) | ||
| linear_row = 0 | ||
| for constr in self.constrs: | ||
| if constr.is_quadratic: | ||
| continue | ||
| rhs_arr[linear_row] = constr.RHS | ||
| linear_row += 1 | ||
| self.rhs = rhs_arr | ||
|
|
||
| self.model.set_objective_coefficients(self.objective) | ||
| self.model.set_constraint_bounds(rhs_arr) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="python/cuopt/cuopt/linear_programming/problem.py"
echo "== File size =="
wc -l "$FILE"
echo "== Outline =="
ast-grep outline "$FILE" --view expanded | sed -n '1,260p'
echo "== Relevant symbols search =="
rg -n "def (_refresh_data_model_values|update|perturb|set_.*objective|set_.*bound|ObjConstant|objective_qmatrix|constraint_csr_matrix|rhs|solve|addVariable|addConstraint)" "$FILE"Repository: NVIDIA/cuopt
Length of output: 5803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="python/cuopt/cuopt/linear_programming/problem.py"
sed -n '73,220p' "$FILE"
echo "-----"
sed -n '1284,1408p' "$FILE"
echo "-----"
sed -n '1452,1745p' "$FILE"
echo "-----"
sed -n '1792,1965p' "$FILE"
echo "-----"
sed -n '2315,2355p' "$FILE"Repository: NVIDIA/cuopt
Length of output: 27373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="python/cuopt/cuopt/linear_programming/problem.py"
sed -n '1945,1998p' "$FILE"
echo "-----"
sed -n '145,180p' "$FILE"
echo "-----"
sed -n '155,175p' "$FILE"
echo "-----"
sed -n '1831,1878p' "$FILE"Repository: NVIDIA/cuopt
Length of output: 5286
_refresh_data_model_values leaves bound/type edits stale.
solve() reuses the cached DataModel, but setLowerBound(), setUpperBound(), setVariableType(), and setMIPStart() only change Python-side state. Re-solving after one of those edits keeps the old model unless callers remember to call update() first. Either invalidate the cache in those setters or refresh the remaining mutable fields here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cuopt/cuopt/linear_programming/problem.py` around lines 1671 - 1701,
The cached DataModel refresh in _refresh_data_model_values only updates
objective coefficients and constraint bounds, so bound, variable type, and MIP
start edits from setLowerBound, setUpperBound, setVariableType, and setMIPStart
remain stale on a reused solve(). Update those setters to invalidate or sync the
cached model state, or extend _refresh_data_model_values to also push the
remaining mutable fields before calling solve(). Keep the fix centered on the
Problem methods and model refresh path so cached values always reflect the
latest Python-side changes.
| @contextmanager | ||
| def capture_solver_output(): | ||
| """Capture solver stdout and stderr (C++ logs use both).""" | ||
| read_out, write_out = os.pipe() | ||
| read_err, write_err = os.pipe() | ||
| saved_out = os.dup(1) | ||
| saved_err = os.dup(2) | ||
| capture = io.StringIO() | ||
| try: | ||
| os.dup2(write_out, 1) | ||
| os.dup2(write_err, 2) | ||
| os.close(write_out) | ||
| os.close(write_err) | ||
| yield capture | ||
| finally: | ||
| os.dup2(saved_out, 1) | ||
| os.dup2(saved_err, 2) | ||
| os.close(saved_out) | ||
| os.close(saved_err) | ||
| with os.fdopen(read_out, "r", encoding="utf-8", errors="replace") as reader_out: | ||
| out_text = reader_out.read() | ||
| with os.fdopen(read_err, "r", encoding="utf-8", errors="replace") as reader_err: | ||
| err_text = reader_err.read() | ||
| text = out_text + err_text | ||
| capture.write(text) | ||
| capture.seek(0) | ||
| if text: | ||
| sys.stdout.write(out_text) | ||
| sys.stdout.flush() | ||
| sys.stderr.write(err_text) | ||
| sys.stderr.flush() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file map ---\n'
git ls-files python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py
printf '\n--- relevant lines ---\n'
nl -ba python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py | sed -n '1,220p'
printf '\n--- usages ---\n'
rg -n "capture_solver_output|session_cache_helpers" python/cuopt/cuopt/tests -SRepository: NVIDIA/cuopt
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file map ---'
git ls-files python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py
echo
echo '--- relevant lines ---'
nl -ba python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py | sed -n '1,220p'
echo
echo '--- usages ---'
rg -n "capture_solver_output|session_cache_helpers" python/cuopt/cuopt/tests -SRepository: NVIDIA/cuopt
Length of output: 300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- file ---'
cat -n python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py | sed -n '1,220p'
echo
echo '--- usages ---'
rg -n "capture_solver_output|session_cache_helpers" python/cuopt/cuopt/tests -S
echo
echo '--- solver log mentions in linear_programming tests ---'
rg -n "stdout|stderr|cache-profile|iteration|barrier|log" python/cuopt/cuopt/tests/linear_programming -SRepository: NVIDIA/cuopt
Length of output: 27828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- solve_with_log and assertions ---'
sed -n '260,360p' python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py
echo
echo '--- session tests ---'
sed -n '1,260p' python/cuopt/cuopt/tests/linear_programming/test_lp_solve_session.pyRepository: NVIDIA/cuopt
Length of output: 12469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- log string definitions in C++ ---'
rg -n "Barrier: reusing cuDSS symbolic analysis|Barrier: rebuilt cuDSS symbolic analysis|Barrier: stored augmented symbolic cache hash|Barrier: stored ADAT symbolic cache hash|Barrier: hash match but numeric refresh failed|=== Solver cache profile ===|Cache profile: C" cpp python -S
echo
echo '--- verbosity / logging settings near barrier solver ---'
rg -n "iteration|log|printf|stderr|stdout|cache profile|CUOPT_CACHE_PROFILE" cpp/src python/cuopt/cuopt/tests -S | sed -n '1,220p'Repository: NVIDIA/cuopt
Length of output: 25031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- barrier logging around reuse path ---'
sed -n '4460,4565p' cpp/src/barrier/barrier.cu
echo
echo '--- barrier iteration logging sites ---'
rg -n "settings\.log\.printf|printf\(" cpp/src/barrier/barrier.cu -n -S | sed -n '1,200p'Repository: NVIDIA/cuopt
Length of output: 15328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- barrier iteration loop near log lines ---'
sed -n '4628,4868p' cpp/src/barrier/barrier.cu
echo
echo '--- logger default / iteration log frequency ---'
rg -n "iteration_log_frequency|first_iteration_log|debug|log\.printf\(" cpp/src/barrier cpp/src/dual_simplex -S | sed -n '1,220p'Repository: NVIDIA/cuopt
Length of output: 35675
Capture solver logs into files instead of pipes
capture_solver_output() drains stdout/stderr only after prob.solve() returns, so a verbose solve can fill the pipe buffer and block the writer indefinitely. A TemporaryFile-based capture avoids that hang.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cuopt/cuopt/tests/linear_programming/session_cache_helpers.py` around
lines 80 - 110, The capture_solver_output context manager currently redirects
stdout and stderr to pipes and only reads them after prob.solve() finishes,
which can deadlock on verbose output. Update capture_solver_output to use
file-backed temporary capture instead of os.pipe/os.dup2 pipes, and keep the
existing restore-and-replay behavior when gathering text from the captured
streams. Preserve the same public behavior of yielding the capture object while
fixing the implementation inside capture_solver_output.
| @contextmanager | ||
| def _capture_solver_output(): | ||
| """Capture C++ solver logs written directly to stderr (fd 2).""" | ||
| read_fd, write_fd = os.pipe() | ||
| saved_stderr = os.dup(2) | ||
| capture = io.StringIO() | ||
| try: | ||
| os.dup2(write_fd, 2) | ||
| os.close(write_fd) | ||
| yield capture | ||
| finally: | ||
| os.dup2(saved_stderr, 2) | ||
| os.close(saved_stderr) | ||
| with os.fdopen(read_fd, "r", encoding="utf-8", errors="replace") as reader: | ||
| text = reader.read() | ||
| capture.write(text) | ||
| capture.seek(0) | ||
| if text: | ||
| sys.stderr.write(text) | ||
| sys.stderr.flush() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant region with line numbers.
git ls-files | rg '^script_perf_eval\.py$|(^|/)script_perf_eval\.py$'
wc -l script_perf_eval.py
sed -n '1,260p' script_perf_eval.py | cat -n
# Find where _capture_solver_output is used and whether the captured output is read incrementally.
rg -n "_capture_solver_output|prob\.solve|stderr|CUOPT_CACHE_PROFILE|barrier" script_perf_eval.pyRepository: NVIDIA/cuopt
Length of output: 11625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the other stderr-capture call site and nearby logic.
sed -n '450,520p' script_perf_eval.py | cat -n
# Inspect the section that mentions stderr buffering / cache profile output.
sed -n '730,770p' script_perf_eval.py | cat -n
# Look for any background draining or threaded stderr handling in this script.
rg -n "thread|Thread|drain|read_fd|TemporaryFile|Popen|stderr" script_perf_eval.pyRepository: NVIDIA/cuopt
Length of output: 5325
Pipe-based stderr capture can stall prob.solve(). _capture_solver_output() only drains the pipe after the solve returns, so solver writes during the call can fill the bounded pipe buffer and block the process. Redirect to a temp file or drain it in a background reader instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script_perf_eval.py` around lines 110 - 129, The `_capture_solver_output`
context manager can block `prob.solve()` because it only reads from the pipe
after the solver finishes, allowing the `stderr` pipe buffer to fill up. Update
`_capture_solver_output` to avoid backpressure by either redirecting fd 2 to a
temporary file or continuously draining `read_fd` in a background reader while
the solve runs, then preserve the existing behavior of restoring stderr and
forwarding captured text afterward.
| self._to_data_model() | ||
| return | ||
|
|
||
| for j in range(n): |
There was a problem hiding this comment.
Vectorized update will be faster than looping over n.
| return None | ||
| if self._constraint_csr_scipy is not None: | ||
| return self._constraint_csr_scipy | ||
| n_rows = ( |
There was a problem hiding this comment.
Redundant branching here. Can set n_rows = len(self.rhs)
| for j in range(n): | ||
| self.objective[j] = self.vars[j].getObjectiveCoefficient() | ||
|
|
||
| rhs_arr = ( |
There was a problem hiding this comment.
Redundant branching here.
| ) | ||
| return self._constraint_csr_scipy | ||
|
|
||
| def _refresh_data_model_values(self): |
There was a problem hiding this comment.
Only linear cost and rhs of linear constraints are updated. Missing update of quadratic cost, lower and upper bounds of variables, constraint matrix, row sense and objective constraint.
| if A is None: | ||
| return False | ||
|
|
||
| rhs_arr = ( |
There was a problem hiding this comment.
Redundant branching here.
| self.model.set_objective_coefficients(self.objective) | ||
| self.model.set_constraint_bounds(rhs_arr) | ||
|
|
||
| def _populate_slacks_vectorized(self, primal_sol): |
There was a problem hiding this comment.
Why do we need to recompute slack values rather than extract them from existing values in the solver?
| constr.DualValue = dual_sol[linear_row] | ||
| constr.Slack = constr.compute_slack() | ||
| linear_row += 1 | ||
| if not ( |
There was a problem hiding this comment.
What are these new lines 2291-2312 for?`
| @@ -2212,10 +2337,13 @@ def solve(self, settings=solver_settings.SolverSettings()): | |||
| >>> problem.setObjective(x + y, sense=MAXIMIZE) | |||
| >>> problem.solve() | |||
| """ | |||
| if self.model is None: | |||
| if self.model is None or self.constraint_csr_matrix is None: | |||
There was a problem hiding this comment.
Why we need additional self.constraint_csr_matrix is None here?
| self._refresh_data_model_values() | ||
| active_session = session if session is not None else self._session | ||
| solution = solver.Solve(self.model, settings, session=active_session) | ||
| if getattr(settings, "session_enabled", False) and solution.lp_solve_session is not None: |
There was a problem hiding this comment.
If session_enabled=False. Why do we still set self._session = solution.lp_solve_session?
| */ | ||
| /* clang-format on */ | ||
|
|
||
| #include <cuopt/linear_programming/utilities/lp_solve_session.hpp> |
There was a problem hiding this comment.
It's better to move lp_solve_session files under barrier folder. barrier_resolve_session may be a better name.
| #include <optional> | ||
| #include <utility> | ||
|
|
||
| namespace cuopt::cython { |
There was a problem hiding this comment.
solver_session is not limited to the use in python. Move it out of namespace cuopt::cython.
| auto& pdlp_settings = solver_settings->get_pdlp_settings(); | ||
| const bool session_enabled = pdlp_settings.session_enabled; | ||
| const bool barrier_path = uses_barrier_session_path(*solver_settings, *data_model); | ||
| const bool want_session = (session_in != nullptr || session_enabled) && barrier_path && |
There was a problem hiding this comment.
Do we also have to exclude concurrent mode?
| const raft::handle_t handle_{stream}; | ||
| if (memory_backend == cuopt::linear_programming::memory_backend_t::GPU) { | ||
| if (want_session) { | ||
| if (active_session == nullptr) { |
There was a problem hiding this comment.
Moving timing one layer up instead of repetition in different branch.
| lp_solve_session_t* active_session = session_in; | ||
| pdlp_settings.lp_solve_session = nullptr; | ||
|
|
||
| rmm::cuda_stream ephemeral_stream(static_cast<rmm::cuda_stream::flags>(flags)); |
There was a problem hiding this comment.
Why do we create a new stream rather than using the existing one in lp_solve_session_t?
| bool session_enabled{false}; | ||
| /** Non-owning session pointer set by ``call_solve`` for barrier symbolic reuse. */ | ||
| cuopt::cython::lp_solve_session_t* lp_solve_session{nullptr}; |
There was a problem hiding this comment.
Can we separate the session pointer from solver_settings_t struct? It is set to nullptr in every call of call_solve in cython_solve.cu. Parameters in solver_settings_t shouldn't be changed within the algorithm call.
| namespace cuopt::mathematical_optimization { | ||
| namespace cuopt::linear_programming { | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
What are these lines of code for?
| @@ -1821,10 +1884,14 @@ optimization_problem_solution_t<i_t, f_t> solve_qcqp( | |||
| CUOPT_LOG_INFO("Writing user problem to file: %s", settings.user_problem_file.c_str()); | |||
| op_problem.write_to_mps(settings.user_problem_file); | |||
| } | |||
| { | |||
| { | ||
| } | ||
|
|
||
| void clear() |
There was a problem hiding this comment.
We also need to clear vector and matrix memory at the same time.
| raft::handle_t const* handle) const | ||
| { | ||
| return valid && handle != nullptr && handle_ptr == handle && use_augmented == augmented && | ||
| sparsity_hash == hash; |
There was a problem hiding this comment.
The hash comparison is a simplified but not theoretically equivalent one. Can we have an exact comparison of row, col vectors in csr format, e.g. the same size of vectors and the same index and shift values in them? This should also be efficient on GPU.
| handle_ptr_->get_stream().synchronize(); | ||
|
|
||
| symbolic_done_ = true; | ||
| numeric_factor_valid_ = false; |
There was a problem hiding this comment.
This seems to be never used.
| settings_ = &settings; | ||
| } | ||
|
|
||
| void invalidate_numeric_factor() override { numeric_factor_valid_ = false; } |
There was a problem hiding this comment.
The function can be removed since numeric_factor_valid_ is not used.
| this->positive_definite = positive_definite; | ||
| } | ||
|
|
||
| void rebind_settings(const simplex_solver_settings_t<i_t, f_t>& settings) override |
There was a problem hiding this comment.
Do we want additional parameter settings update beyond value update in objective and constraints?
| csc_matrix_t<i_t, f_t> Q(lp.num_cols, 0, 0); | ||
| std::unique_ptr<iteration_data_t<i_t, f_t>> owned_data; | ||
|
|
||
| auto finish_session = [&](lp_status_t status) -> lp_status_t { |
There was a problem hiding this comment.
Define finish_session explicitly outside of barrier_solver_t<i_t, f_t>::solve
| // Build the sparsity pattern of the augmented system | ||
| form_augmented(true); | ||
|
|
||
| auto adopt_augmented_symbolic = [&]() -> bool { |
There was a problem hiding this comment.
Define a function explicitly outside.
| rmm::device_uvector<i_t> d_augmented_diagonal_indices_; | ||
| rmm::device_uvector<i_t> d_cone_csr_indices_; | ||
| rmm::device_uvector<f_t> d_cone_Q_values_; | ||
| device_csr_matrix_t<i_t, f_t>* pinned_device_augmented_{nullptr}; |
There was a problem hiding this comment.
It's a bit strange we have pinned memory defined here. Why we need this additional pointer if we already have device_ vectors in the existing iteration_data_t.
If it is related to the issue that reusable space outlive iteration_data_t. We'd better group all reusable memory into a struct and shared it to iteration_data_t.
|
|
||
| template <typename i_t, typename f_t> | ||
| lp_status_t barrier_solver_t<i_t, f_t>::solve(f_t start_time, lp_solution_t<i_t, f_t>& solution) | ||
| lp_status_t barrier_solver_t<i_t, f_t>::solve(f_t start_time, |
There was a problem hiding this comment.
Some thoughts on the new solve function:
Current solve function of barrier_solver_t can be divided into two parts: the first phase is the initialization of all data structures and vectors we needed, and the second phase is the true solve phase (running a barrier method). We can split it into a setup for the first phase and a solve function for the second phase. Then, all changes relevant to reoptimization reside in setup function only.
Additionally, we can verify if reusing previous factorization is possible or not once the barrier_presolve is finished, by verifying objective and constraints (vectors and matrices) are the same as the previous problem. That means we can possibly reuse iteration_data_t by updating only the reusing workspace.
| static_cast<unsigned long long>(cache.sparsity_hash)); | ||
| } | ||
|
|
||
| bool refresh_augmented_values() |
There was a problem hiding this comment.
It's better to define refresh functions for each blocks (e.g. Q, A, ) respectively. Only Q and A entries in the augmented KKT needs to be refreshed in the setup of reoptimization, since other entries will be recomputed for every barrier iteration.
In addition, updating blocks seperately also help in speed, since users usually update a subset of problem coefficients and we may not need to update all numerical values.
| * that was analyzed, and path-specific GPU workspace (augmented KKT or ADAT + cuSPARSE). | ||
| */ | ||
| template <typename i_t, typename f_t> | ||
| struct barrier_symbolic_cache_t { |
There was a problem hiding this comment.
We need to save previous problem coefficients, e.g. Q, A and bounds, etc, for checking if reoptimization is possible.
| return sol, log_text, profile | ||
|
|
||
|
|
||
| def test_adat_session_warm_reuse(): |
There was a problem hiding this comment.
These tests seems to be generated by agent. It's best we clearly define a deterministic small examples rather than relying on random generation in unit tests.
Also, it would be clear to define a problem in lp format for better visualization.
| expect_augmented: bool = False, | ||
| ) -> None: | ||
| """Cold run stores cache; warm run reuses symbolic factorization.""" | ||
| if expect_adat: |
There was a problem hiding this comment.
Capturing lines in log is fragile since we may not be able to include so much info in logging and the logging will vary over time. The best way is to clearly define a data_update function where we update new problem coefficients and return the flag that if previous factorization could be reused or not.
| @@ -0,0 +1,150 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
It's a test file that have to be under test folder.
| f"(cold={bench.get('cold_reuse_log_count')}, warm={warm_reuse})" | ||
| ) | ||
| if c07_w < 50.0: | ||
| raise AssertionError( |
There was a problem hiding this comment.
The check based on time heuristic is very fragile, the result may not be consistent across different hardwares and also vary depending on the dimension of a problem. We need clear flag information.
| @@ -0,0 +1,927 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
It's a test file that have to be under test folder.
Also try to rename the file as portfolio_optimization_test.py.
|
|
||
| namespace cuopt::linear_programming::cache_profile { | ||
|
|
||
| enum class cache_id : int { |
There was a problem hiding this comment.
Is it only for temporary code verification?
| "C09": "Device buffer allocation sizes", | ||
| } | ||
|
|
||
| _CACHE_PROFILE_LINE = re.compile( |
There was a problem hiding this comment.
The same concern as in session_cache_helpers.py: We need clear flag information returned by a check function rather than capturing log or time heuristics.
2 participants
Description
Issue
Checklist