Skip to content

Conversation

@chris-maes
Copy link
Contributor

@chris-maes chris-maes commented Dec 18, 2025

Fixes an issue where the basis can become rank-deficient during dual push. We were not correctly calling basis repair in this case.

Summary by CodeRabbit

  • Chores

    • Streamlined dependency version constraints across all environments and projects
    • Updated pre-commit configuration
  • Bug Fixes

    • Improved solver stability with automatic basis repair on factorization failures
  • Refactor

    • Enhanced thread-safety mechanisms in concurrent solver operations
  • New Features

    • Added node exploration limits in diving phase optimization

✏️ Tip: You can customize this high-level summary in your review settings.

@chris-maes chris-maes requested review from a team as code owners December 18, 2025 20:38
@copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 18, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@chris-maes chris-maes changed the base branch from main to release/25.12 December 18, 2025 20:39
@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

📝 Walkthrough

Walkthrough

This PR removes prerelease version constraints (>=0.0.0a0) from dependency specifications across conda environments and Python project files, converts C++ concurrency primitives from volatile int* to std::atomic<int>*, adds node exploration limiting in branch-and-bound diving, improves basis repair handling in crossover, refactors logger initialization to use reference counting, and adds a new pre-commit hook for alpha spec verification.

Changes

Cohort / File(s) Summary
Pre-commit Configuration
\.pre-commit-config\.yaml
Added verify-alpha-spec hook (rapidsai/pre-commit-hooks v1.2.1) with release mode arguments; restructured YAML to move python: python3 to top-level.
Conda Environment Dependency Pinning
conda/environments/all_cuda-129_arch-aarch64\.yaml, all_cuda-129_arch-x86_64\.yaml, all_cuda-130_arch-aarch64\.yaml, all_cuda-130_arch-x86_64\.yaml
Removed prerelease lower-bound constraints (>=0.0.0a0) from cudf, libraft-headers, librmm, pylibraft, rapids-dask-dependency, rapids-logger, and rmm; simplified to exact minor version pins (25.12.* and 0.2.*).
Dependency Manifest Files
dependencies\.yaml, python/cuopt/pyproject\.toml, python/cuopt/cuopt/linear_programming/pyproject\.toml, python/libcuopt/pyproject\.toml, python/cuopt_server/pyproject\.toml, python/cuopt_self_hosted/pyproject\.toml
Systematically removed prerelease constraint qualifiers across multiple packages and dependency sections, aligning version specs to unified 25.12.* and 0.2.* patterns.
C++ Concurrency Type Updates
cpp/include/cuopt/linear_programming/pdlp/solver_settings\.hpp, cpp/src/dual_simplex/simplex_solver_settings\.hpp, cpp/src/dual_simplex/branch_and_bound\.hpp, cpp/src/linear_programming/solve\.cu, cpp/src/mip/diversity/diversity_manager\.cuh, cpp/src/mip/relaxed_lp/relaxed_lp\.cuh
Replaced volatile int* with std::atomic<int>* for concurrent halt synchronization primitives across solver settings and global state.
Dual Simplex Algorithm Improvements
cpp/src/dual_simplex/branch_and_bound\.cpp, cpp/src/dual_simplex/crossover\.cpp
Added node exploration counter in diving thread to limit work per phase; introduced factorization rank checking with basis repair fallback; modified phase-1 solution to preserve non-dual fields and reordered dual computation in primal push.
Logger Infrastructure Refactoring
cpp/src/utilities/logger\.hpp, cpp/src/utilities/logger\.cpp
Implemented reference-counted logger configuration via shared_ptr guard mechanism to enable sharing across multiple init_logger_t instances; added global guard weak_ptr and mutex for coordination; removed explicit destructor in favor of RAII guard cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Dependency pinning changes (15+ files): Highly repetitive pattern of removing >=0.0.0a0 constraints—quick verification of consistency
  • Concurrency type migrations (6 files): Straightforward volatile-to-atomic replacements with consistent semantics
  • Algorithm logic changes: Branch-and-bound node limiting and crossover basis repair require closer examination of correctness and interaction with existing control flow
  • Logger refactoring: Reference-counted guard design and interaction between weak_ptr, mutex, and buffer replay requires careful review of thread-safety guarantees

Areas requiring extra attention:

  • cpp/src/dual_simplex/crossover.cpp: Verify basis repair path doesn't break existing recovery logic and that partial solution update (y, z only) doesn't inadvertently lose state
  • cpp/src/utilities/logger.cpp: Confirm shared guard mechanism doesn't introduce lifecycle issues or race conditions in multi-threaded logger initialization scenarios
  • cpp/src/dual_simplex/branch_and_bound.cpp: Validate that the 1000-node limit doesn't negatively impact search quality or termination conditions

Possibly related PRs

  • Update to 26.02 #596: Modifies the same dependency manifest files (conda environments, pyproject.toml, dependencies.yaml) with overlapping version pinning changes.

Suggested reviewers

  • rgsl888prabhu

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Dual push crossover rank deficient' directly addresses the main code change: basis repair in dual_push when rank deficiency is detected during factorization in the crossover process.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
cpp/src/dual_simplex/branch_and_bound.cpp (1)

1138-1138: Consider making the node limit configurable and handling abandoned nodes.

The 1000-node limit caps diving work per subtree, which is reasonable for preventing resource monopolization. However:

  1. The magic number 1000 could be made configurable via settings_ for tuning across different problem sizes.
  2. When breaking early (line 1156), nodes remaining on stack are abandoned without being pushed back to diving_queue_ or heap_. This could leave promising subtrees unexplored.
🔎 Consider pushing remaining nodes when breaking early:
         if (nodes_explored >= 1000) { break; }
+        // Note: Remaining nodes on stack will be abandoned.
+        // Consider pushing them to diving_queue_ if they should be explored later.

Also applies to: 1156-1157, 1171-1172

cpp/src/dual_simplex/simplex_solver_settings.hpp (1)

148-149: Good: Correct migration from volatile int* to std::atomic<int>*.

Using std::atomic<int>* provides proper memory ordering guarantees for the concurrent halt signal, whereas volatile does not guarantee atomicity or synchronization.

Note: This is an API change. If external users set concurrent_halt, they'll need to update their code to use std::atomic<int>*. Consider adding a Doxygen comment documenting the expected usage and thread-safety semantics.

cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp (1)

215-215: Good: Atomic type for concurrent halt signal.

The migration to std::atomic<int>* is correct for thread-safe concurrent termination signaling.

As per coding guidelines for public header files, consider adding a Doxygen-style comment documenting the semantics (e.g., nullptr = ignored, 0 = continue, 1 = halt), thread-safety guarantees, and ownership expectations.

cpp/src/utilities/logger.cpp (1)

48-49: Consider making messages private.

The messages vector is public, which allows bypassing the mutex protection. While current usage is safe (only accessed via class methods), making it private would prevent accidental unsynchronized access in future modifications.

🔎 Suggested fix
+ private:
   std::vector<buffered_entry> messages;
   mutable std::mutex mutex;
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a08824 and 9120ce2.

📒 Files selected for processing (21)
  • .pre-commit-config.yaml (2 hunks)
  • conda/environments/all_cuda-129_arch-aarch64.yaml (3 hunks)
  • conda/environments/all_cuda-129_arch-x86_64.yaml (3 hunks)
  • conda/environments/all_cuda-130_arch-aarch64.yaml (3 hunks)
  • conda/environments/all_cuda-130_arch-x86_64.yaml (3 hunks)
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp (1 hunks)
  • cpp/src/dual_simplex/branch_and_bound.cpp (3 hunks)
  • cpp/src/dual_simplex/branch_and_bound.hpp (2 hunks)
  • cpp/src/dual_simplex/crossover.cpp (3 hunks)
  • cpp/src/dual_simplex/simplex_solver_settings.hpp (1 hunks)
  • cpp/src/linear_programming/solve.cu (1 hunks)
  • cpp/src/mip/diversity/diversity_manager.cuh (1 hunks)
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh (1 hunks)
  • cpp/src/utilities/logger.cpp (2 hunks)
  • cpp/src/utilities/logger.hpp (1 hunks)
  • dependencies.yaml (17 hunks)
  • python/cuopt/cuopt/linear_programming/pyproject.toml (3 hunks)
  • python/cuopt/pyproject.toml (3 hunks)
  • python/cuopt_self_hosted/pyproject.toml (1 hunks)
  • python/cuopt_server/pyproject.toml (1 hunks)
  • python/libcuopt/pyproject.toml (2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{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/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/mip/diversity/diversity_manager.cuh
  • cpp/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh
  • cpp/src/utilities/logger.hpp
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
  • cpp/src/linear_programming/solve.cu
  • cpp/src/utilities/logger.cpp
  • cpp/src/dual_simplex/crossover.cpp
**/*.{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/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/utilities/logger.hpp
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.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/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/utilities/logger.hpp
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
  • cpp/src/utilities/logger.cpp
  • cpp/src/dual_simplex/crossover.cpp
**/*.{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/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/utilities/logger.hpp
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
  • cpp/src/linear_programming/solve.cu
  • cpp/src/utilities/logger.cpp
  • cpp/src/dual_simplex/crossover.cpp
**/*.{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/mip/diversity/diversity_manager.cuh
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh
  • cpp/src/linear_programming/solve.cu
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/pdlp/solver_settings.hpp
**/*.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/linear_programming/solve.cu
🧠 Learnings (13)
📚 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 **/*.{cpp,hpp,h} : Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state

Applied to files:

  • cpp/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/mip/diversity/diversity_manager.cuh
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
  • cpp/src/linear_programming/solve.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/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh
📚 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: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms

Applied to files:

  • cpp/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/crossover.cpp
📚 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/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh
  • cpp/src/linear_programming/solve.cu
  • cpp/src/dual_simplex/crossover.cpp
📚 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/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh
  • cpp/src/linear_programming/solve.cu
  • cpp/src/dual_simplex/crossover.cpp
📚 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/src/dual_simplex/simplex_solver_settings.hpp
  • cpp/src/linear_programming/solve.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/src/mip/diversity/diversity_manager.cuh
  • cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp
  • cpp/src/linear_programming/solve.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 **/*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/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/crossover.cpp
📚 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/src/dual_simplex/branch_and_bound.cpp
  • cpp/src/dual_simplex/branch_and_bound.hpp
  • cpp/src/mip/relaxed_lp/relaxed_lp.cuh
  • cpp/src/linear_programming/solve.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 : Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations

Applied to files:

  • cpp/src/linear_programming/solve.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} : Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution

Applied to files:

  • cpp/src/linear_programming/solve.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/src/linear_programming/solve.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} : 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/src/linear_programming/solve.cu
🧬 Code graph analysis (1)
cpp/src/utilities/logger.cpp (1)
cpp/src/utilities/logger.hpp (2)
  • reset_default_logger (34-34)
  • init_logger_t (42-42)
🔇 Additional comments (26)
python/cuopt_self_hosted/pyproject.toml (1)

22-22: LGTM!

Removing the prerelease lower bound >=0.0.0a0 is appropriate cleanup to standardize on stable version patterns across the project.

python/cuopt/cuopt/linear_programming/pyproject.toml (1)

23-23: LGTM!

Consistent removal of prerelease lower bounds for rapids-logger across project dependencies, test dependencies, and build requirements.

Also applies to: 42-42, 86-86

cpp/src/mip/diversity/diversity_manager.cuh (1)

98-98: LGTM - Proper thread synchronization.

Replacing volatile int with std::atomic<int> is the correct fix. volatile does not guarantee atomicity or proper memory ordering between threads—only std::atomic provides the required synchronization semantics for inter-thread signaling. This aligns with the existing std::atomic<bool> simplex_solution_exists pattern in the class. Based on learnings about preventing race conditions and ensuring proper synchronization of shared state.

cpp/src/dual_simplex/branch_and_bound.hpp (1)

116-117: LGTM - Consistent atomic synchronization.

The conversion from volatile int* / volatile int to std::atomic<int>* / std::atomic<int> properly aligns with the existing atomic usage pattern in this class (e.g., root_crossover_solution_set_ on line 171).

The setter on line 117 uses implicit atomic assignment which defaults to memory_order_seq_cst—this is correct and safe. For consistency with line 98's explicit store(true, std::memory_order_release), you could optionally use root_concurrent_halt_.store(value, std::memory_order_release), but the current implementation is functionally correct.

Also applies to: 173-173

cpp/src/linear_programming/solve.cu (1)

309-309: Fix critical race condition and type inconsistency in concurrent halt mechanism.

The change from volatile int global_concurrent_halt; to std::atomic<int> global_concurrent_halt{0}; corrects two issues:

  1. Thread safety: volatile alone is insufficient for thread safety; it prevents certain compiler optimizations but does not guarantee atomicity or correct memory ordering across threads. Using std::atomic<int> provides proper atomic operations with sequential consistency semantics by default.

  2. Type consistency: The settings structs (simplex_solver_settings.hpp:148, pdlp/solver_settings.hpp:215) already declare concurrent_halt as std::atomic<int>*. The global variable must match this type to safely pass its address. All usage sites correctly dereference via *settings.concurrent_halt to invoke the atomic assignment and comparison operators.

The fix is correct and necessary for both correctness and type safety.

conda/environments/all_cuda-129_arch-x86_64.yaml (1)

1-2: LGTM - generated file with consistent version spec cleanup.

The removal of >=0.0.0a0 prerelease suffixes from RAPIDS dependencies is appropriate for a release-mode configuration. Since this file is auto-generated from dependencies.yaml, the source changes are correctly driving this output.

Also applies to: 22-22, 38-39, 56-56, 62-63, 65-65

.pre-commit-config.yaml (2)

59-63: Good addition of verify-alpha-spec hook for release mode.

This hook enforces and auto-fixes removal of prerelease version specifiers (>=0.0.0a0) from dependency files. The --mode release configuration aligns with the version spec cleanup observed across the conda environment YAML files in this PR.


91-92: LGTM - Standard pre-commit configuration.

The default_language_version setting is correctly formatted.

conda/environments/all_cuda-129_arch-aarch64.yaml (1)

1-2: LGTM - consistent with x86_64 variant.

Generated file with identical prerelease constraint removal pattern. Changes are appropriate and consistent across architecture variants.

Also applies to: 22-22, 38-39, 56-56, 62-63, 65-65

conda/environments/all_cuda-130_arch-x86_64.yaml (1)

1-2: LGTM - CUDA 13.0 variant with consistent version spec cleanup.

Generated file follows the same prerelease constraint removal pattern as the CUDA 12.9 variants. The CUDA-specific dependency versions (cuda-python, cuda-version) appropriately reflect the 13.0 target.

Also applies to: 22-22, 38-39, 56-56, 62-63, 65-65

cpp/src/mip/relaxed_lp/relaxed_lp.cuh (1)

19-28: Add #include <atomic> to relaxed_lp.cuh.

The code uses std::atomic<int>* for concurrent_halt but <atomic> is not included directly or transitively through the included headers. Using std::atomic<int> requires #include <atomic>. Without this header, the code will fail to compile with "undefined type" errors. Add #include <atomic> near the top of the include section in relaxed_lp.cuh.

⛔ Skipped due to learnings
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: 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 **/*.{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 **/*.{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
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
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)
conda/environments/all_cuda-130_arch-aarch64.yaml (1)

22-22: LGTM! Dependency pins updated to stable releases.

Removing the >=0.0.0a0 prerelease lower bounds across cudf, libraft-headers, librmm, pylibraft, rapids-dask-dependency, rapids-logger, and rmm appropriately constrains these dependencies to stable 25.12.* (or 0.2.* for rapids-logger) releases.

Also applies to: 38-39, 56-56, 62-63, 65-65

python/cuopt_server/pyproject.toml (1)

23-23: LGTM! Dependency constraint simplified.

Removing the redundant prerelease lower bound aligns with the broader dependency cleanup across the repository.

cpp/src/utilities/logger.hpp (1)

36-43: LGTM! Ref-counted logger initialization pattern.

Using std::shared_ptr<void> for reference counting is a valid type-erasure pattern. The shared pointer's destructor will handle cleanup when the last init_logger_t instance is destroyed, eliminating the need for an explicit destructor. This design allows multiple init_logger_t instances to share the same underlying logger configuration.

cpp/src/dual_simplex/crossover.cpp (3)

505-516: Good fix: Proper basis repair handling on rank-deficient factorization.

This correctly handles the case where the basis becomes rank-deficient during dual push:

  1. Captures the rank from factorize_basis
  2. If rank != m, calls basis_repair with the deficient columns
  3. Re-factorizes and returns -1 if still failing
  4. Logs appropriately at each step

This pattern matches the existing handling in primal_push (lines 797-808) and addresses the bug mentioned in the PR objectives.


1217-1218: LGTM! Correct ordering of dual solution computation before diagnostic output.

Computing the dual solution before calling print_crossover_info ensures accurate reporting of dual infeasibility after the primal push completes.


1367-1368: Good fix: Selective copy preserves primal solution.

Changing from solution = phase1_solution to selectively copying only y and z is correct. This preserves solution.x (the primal solution computed earlier) while updating only the dual variables from phase 1. The previous assignment would have incorrectly overwritten the primal solution.

cpp/src/utilities/logger.cpp (2)

140-147: LGTM on RAII guard design.

The logger_config_guard combined with a weak_ptr-based sharing pattern provides reference-counted logger configuration management. The static mutex ensures safe concurrent initialization.


149-189: Approve with consideration: lock held during log replay.

The implementation correctly serializes concurrent initialization. However, the lock is held during drain_all() and the log replay loop (lines 180-183), which could increase contention if initialization happens frequently with many buffered messages.

For typical usage patterns (single initialization), this is acceptable. If contention becomes an issue, consider draining messages before acquiring the guard lock or releasing the lock after guard assignment and before replay.

dependencies.yaml (2)

300-301: LGTM on version constraint cleanup.

Removing the prerelease lower bounds (>=0.0.0a0) simplifies dependency specifications while maintaining the 25.12.* version series constraint. This is a clean normalization of version pins across the build configuration.


559-563: rapids-logger 0.2. fully supports sink manipulation and pattern setting operations.*

Rapids-logger 0.2.* provides flush_on() and set_pattern() methods as documented in production code examples, and the resulting logger exposes spdlog-like functionality via the PImpl idiom. No compatibility issues with RAII guard semantics.

python/libcuopt/pyproject.toml (2)

33-44: LGTM on generated dependency updates.

The dependency version changes align with the source dependencies.yaml and maintain consistency. The comment at line 44 correctly indicates these are auto-generated.


83-89: LGTM on build requirements update.

Build requirements are consistent with runtime dependencies and the source dependencies.yaml.

python/cuopt/pyproject.toml (3)

21-35: LGTM on generated dependency updates.

All dependency version changes are consistent with the source dependencies.yaml. The removal of prerelease lower bounds is uniform across all affected packages.


46-52: LGTM on test dependencies.

Test dependency for rapids-logger is consistent with runtime dependencies.


113-123: LGTM on build requirements.

Build requirements are consistent with the source dependencies.yaml and runtime dependencies.

@anandhkb anandhkb added this to the 26.02 milestone Dec 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants