Remove wrong exception of unsupported QCQP#1493
Conversation
rg20
added
bug
|
/ok to test d789f4e |
| user_problem.var_types.assign(n, variable_type_t::CONTINUOUS); | ||
|
|
||
| // Rotated SOC: x^2 - 2*t*u <= 0 | ||
| // Q COO: (0,0,1) for x^2, (1,2,-2) for -2*t*u (canonical form: single cross term) |
There was a problem hiding this comment.
Try specifying the problem using the lp reader? I can't visually verify the correctness of the hardcoded matrix inputs.
CI Test Summary✅ All 31 test job(s) passed. |
|
/ok to test ae2fea1 |
📝 WalkthroughWalkthroughRemoves a rotated-SOC conversion check for quadratic objectives, adds a regression test for solving a QCQP with a rotated SOC constraint, and updates cuSPARSE SpMV algorithm selection to depend only on the cuSPARSE version. ChangesRotated SOC with quadratic objective support
cuSPARSE SpMV algorithm selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| ASSERT_EQ(problem.get_quadratic_constraints().size(), 1u); | ||
|
|
||
| pdlp_solver_settings_t<i_t, f_t> settings; | ||
| auto solution = solve_lp(&handle, problem, settings); |
There was a problem hiding this comment.
Mind leaving a TODO to redo all the other tests in this form?
|
/ok to test |
@rg20, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test fb18712 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/barrier/cusparse_view.cu (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the now-unused
num_rowsparameter.Since the selection logic no longer depends on
num_rows, consider removing the parameter entirely rather than marking it[[maybe_unused]], and updating the call sites (lines 208, 220, 232, 244, 314, 366) accordingly.♻️ Proposed cleanup
-static cusparseSpMVAlg_t get_spmv_alg([[maybe_unused]] int num_rows) +static cusparseSpMVAlg_t get_spmv_alg() {And update call sites, e.g.
get_spmv_alg(A_offsets_.size() - 1)→get_spmv_alg().🤖 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` at line 117, The get_spmv_alg helper no longer uses num_rows, so remove that parameter instead of keeping it marked [[maybe_unused]]. Update the get_spmv_alg function signature in cusparse_view.cu and fix every call site that still passes A_offsets_.size() - 1 (and any similar argument) to call get_spmv_alg() with no arguments.
🤖 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/src/barrier/cusparse_view.cu`:
- Around line 117-126: The ALG selection in get_spmv_alg is using the build-time
CUSPARSE_VER_* macros, which can diverge from the actually loaded cuSPARSE
library. Update this helper to use the runtime/library version instead,
preferably by reusing or extending the existing cuSPARSE loader/cache path
already used elsewhere in cusparse_view.cu, so the ALG1/ALG2 choice reflects the
real libcusparse.so and avoids the beta=1 accumulate bug.
---
Nitpick comments:
In `@cpp/src/barrier/cusparse_view.cu`:
- Line 117: The get_spmv_alg helper no longer uses num_rows, so remove that
parameter instead of keeping it marked [[maybe_unused]]. Update the get_spmv_alg
function signature in cusparse_view.cu and fix every call site that still passes
A_offsets_.size() - 1 (and any similar argument) to call get_spmv_alg() with no
arguments.
🪄 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: afc97cbb-e18b-4730-90d6-cdd7399fa480
📒 Files selected for processing (3)
cpp/src/barrier/cusparse_view.cucpp/src/barrier/translate_soc.hppcpp/tests/socp/general_quadratic_test.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/tests/socp/general_quadratic_test.cu
- cpp/src/barrier/translate_soc.hpp
| static cusparseSpMVAlg_t get_spmv_alg([[maybe_unused]] int num_rows) | ||
| { | ||
| // The older version of ALG2 has a bug with single row matrices | ||
| if (num_rows == 1 && | ||
| (CUSPARSE_VER_MAJOR * 1000 + CUSPARSE_VER_MINOR * 100 + CUSPARSE_VER_PATCH < 12603)) { | ||
| return CUSPARSE_SPMV_CSR_ALG1; | ||
| } | ||
| // ALG2 has a bug in cuSPARSE < 13.0 where beta=1 accumulate mode ignores existing y values. | ||
| // ALG1 uses a deterministic row-split algorithm, while ALG2 uses a merge-based | ||
| // algorithm that may be faster but can use atomics. ALG1 is safe for reproducibility. | ||
| constexpr int cusparse_version = | ||
| CUSPARSE_VER_MAJOR * 1000 + CUSPARSE_VER_MINOR * 100 + CUSPARSE_VER_PATCH; | ||
| if (cusparse_version < 13000) { return CUSPARSE_SPMV_CSR_ALG1; } | ||
| return CUSPARSE_SPMV_CSR_ALG2; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Can cusparseGetProperty runtime cuSPARSE version differ from CUSPARSE_VER_MAJOR/MINOR compile-time macros when linked dynamically?
💡 Result:
Yes, the version reported by the runtime function cusparseGetProperty can differ from the versions defined by the CUSPARSE_VER_MAJOR and CUSPARSE_VER_MINOR compile-time macros [1][2]. Compile-time macros such as CUSPARSE_VER_MAJOR, CUSPARSE_VER_MINOR, and CUSPARSE_VER_PATCH are defined in the cuSPARSE header files (e.g., cusparse.h) available at the time the application is compiled [3][4][5]. These reflect the version of the cuSPARSE SDK used during the build process [1][2]. In contrast, cusparseGetProperty is a runtime API that queries the version of the specific cuSPARSE library (libcusparse.so or.dll) that is dynamically loaded into the application's process at execution time [6][7]. When linking dynamically, the library found at runtime by the system's dynamic linker may be different from the one against which the application was originally compiled (e.g., due to differences in the environment's LD_LIBRARY_PATH or PATH) [8]. This discrepancy is a known challenge in complex deployment environments where the runtime environment may contain a different version of the NVIDIA CUDA Toolkit or individual libraries than the development environment [1][2]. Developers are generally advised to rely on the runtime API (cusparseGetProperty) for features dependent on the loaded library version to ensure compatibility with the actual environment [1][2].
Citations:
- 1: Use
CUSPARSE_VERSIONrather thanCUDA_VERSIONto choose how to use cuSPARSE TPL kokkos/kokkos-kernels#2009 - 2: https://carlpearson.net/post/20231023-cuda-releases/
- 3: https://github.com/cupy/cupy/blob/4e6b2720/cupy_backends/cuda/cupy_cusparse.h
- 4: https://github.com/cupy/cupy/blob/master/cupy_backends/cuda/cupy_cusparse.h
- 5: Support cuSparse generic API cupy/cupy#3242
- 6: https://docs.nvidia.com/cuda/cusparse/basic-api/management-reference.html
- 7: https://docs.nvidia.com/cuda/archive/13.0.2/cusparse/basic-api/management-reference.html
- 8: https://docs.nvidia.com/cuda/archive/12.1.1/cusparse/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and call sites.
git grep -n "get_spmv_alg" -- cpp/src/barrier/cusparse_view.cu
sed -n '1,170p' cpp/src/barrier/cusparse_view.cu
sed -n '170,390p' cpp/src/barrier/cusparse_view.cu
# Check whether the runtime-version handling comment is present nearby.
git grep -n "There can be a missmatch between compiled CUDA version and the runtime CUDA version" -- cpp/src/barrier/cusparse_view.cuRepository: NVIDIA/cuopt
Length of output: 18107
Use the loaded cuSPARSE version for ALG selection CUSPARSE_VER_* is build-time only, but this file already accounts for runtime/library mismatches elsewhere. Query the runtime cuSPARSE version here too (or cache it in the existing loader helper) so ALG1/ALG2 selection matches the actual libcusparse.so and doesn’t reintroduce the beta=1 accumulate bug.
🤖 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 117 - 126, The ALG selection
in get_spmv_alg is using the build-time CUSPARSE_VER_* macros, which can diverge
from the actually loaded cuSPARSE library. Update this helper to use the
runtime/library version instead, preferably by reusing or extending the existing
cuSPARSE loader/cache path already used elsewhere in cusparse_view.cu, so the
ALG1/ALG2 choice reflects the real libcusparse.so and avoids the beta=1
accumulate bug.
|
/ok to test d68726e |
|
/merge |
3 participants
Description
We were wrongly throwing an exception when the rotated cones and quadratic objectives were present.
Issue
Checklist