Java bindings for LP/MIP/QP#1524
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new standalone Java module (java/cuopt) implementing JNI-based bindings for cuOpt LP, MILP, and QP solving. It adds Java modeling classes (Problem, Variable, Constraint, expressions), native C++/JNI glue code, CMake/Maven build tooling, CI workflow wiring, tests, and Sphinx documentation. ChangesJava JNI Bindings Implementation
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
mlubin
left a comment
mlubin
left a comment
There was a problem hiding this comment.
I saw the PR is closed, sending my comments as I had them already written up.
| import java.util.List; | ||
|
|
||
| /** Compatibility entry point for Python's deprecated LP BatchSolve API. */ | ||
| public final class BatchSolve { |
There was a problem hiding this comment.
No need to add a java wrapper for the API that we're going to delete.
|
|
||
| extern "C" { | ||
|
|
||
| cuopt_int_t cuOptLoadParametersFromFile(cuOptSolverSettings settings, const char* path); |
There was a problem hiding this comment.
We should discuss merging these extensions into the C API.
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public final class PDLPWarmStartData { |
There was a problem hiding this comment.
Why are we exposing PDLPWarmStartData? I think this is internal.
| @@ -0,0 +1,1367 @@ | |||
| /* | |||
| * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
I'd recommend avoiding a test dependency on the python interface. The java interface should stand on its own.
| @@ -0,0 +1,25 @@ | |||
| # cuOpt Java bindings (beta) | |||
|
|
|||
| This directory is an isolated, customer-specific beta module for the cuOpt | |||
There was a problem hiding this comment.
Is this how we want to ship it?
There was a problem hiding this comment.
We would want to follow cuvs and try to publish to maven https://mvnrepository.com/artifact/com.nvidia.cuvs/cuvs-java
| =================================== | ||
|
|
||
| The Java LP/QP bindings are in the package | ||
| ``com.nvidia.cuopt.linearprogramming``. The public API is documented below by |
There was a problem hiding this comment.
Use com.nvidia.cuopt.mathematicalprogramming (or mathematical_programming if java conventions allow). The linear_programming name is deprecated. We changed it on the C++ side and someday will change it on the python side.
|
Sorry that was an accident. Reopening. |
| @@ -0,0 +1,28 @@ | |||
| /home/cbrissette/cuopt/java/cuopt/src/main/java/com/nvidia/cuopt/linearprogramming/BatchSolve.java | |||
There was a problem hiding this comment.
Do we need these files ? may be we can delete all the run time files so developers can concentrate on main parts.
ramakrishnap-nv
left a comment
ramakrishnap-nv
left a comment
There was a problem hiding this comment.
Focused review on APIs and shipping (vs how cuvs ships Java).
APIs: the surface is broad and, pleasingly, closely in sync with the Python API — the algebraic Problem layer matches Python's (camelCase) modeling methods almost 1:1, and DataModel maps cleanly (snake_case→camelCase). A few parity gaps and Java-idiom nits are noted inline.
Shipping: the main blockers — don't commit target/, and wire the build into CI/release the way cuvs does (ci/build_java.sh/ci/test_java.sh, dependencies.yaml java key, workflow jobs, version marker, docs toctree).
Non-blocking review comments below.
| @@ -0,0 +1,34 @@ | |||
| com/nvidia/cuopt/linearprogramming/SolverSettings$NativeHandle.class | |||
There was a problem hiding this comment.
Build output committed. The whole java/cuopt/target/ tree (~51 files: .class, surefire-reports/, maven-status/, generated-sources/) is Maven output and shouldn't be in git. Please remove it and add a java/.gitignore (target/, *.iml, hs_err*.log), as cuvs does.
| @@ -0,0 +1,65 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Not wired into CI/release — this is the "must be built on its own" gap. Consider ci/build_java.sh + ci/test_java.sh that pull the C++ artifact via rapids-download-from-github "$(rapids-artifact-name conda_cpp libcuopt cuopt --cuda "$RAPIDS_CUDA_VERSION")", plus java-build/conda-java-tests jobs in pr.yaml/test.yaml and a java file-key in dependencies.yaml — mirroring how cuvs ships Java.
|
|
||
| <groupId>com.nvidia</groupId> | ||
| <artifactId>cuopt</artifactId> | ||
| <version>26.8.0-SNAPSHOT</version> |
There was a problem hiding this comment.
Use the RAPIDS version format 26.08.00 (not 26.8.0-SNAPSHOT) and add a version-update marker (cuvs: <!--CUVS_JAVA#VERSION_UPDATE_MARKER_START-->...<!--...END-->) so ci/release/update-version.sh bumps it. Also consider groupId com.nvidia.cuopt.
| @@ -0,0 +1,42 @@ | |||
| ==================================== | |||
There was a problem hiding this comment.
These Java docs aren't referenced by any Sphinx toctree, so they won't render. Please add cuopt-java/index to the top-level docs toctree.
| } | ||
|
|
||
| /** Return true for maximize and false for minimize, matching Python get_sense(). */ | ||
| public boolean getSense() { |
There was a problem hiding this comment.
Parity note (not a rename request): getSense() correctly matches the Python DataModel.get_sense() (bool, True=maximize) — good. Two parity gaps vs Python though: (1) Python puts set_initial_primal_solution/set_initial_dual_solution on DataModel, whereas here they're on SolverSettings; (2) Python DataModel also exposes getters this class seems to lack: get_quadratic_objective_{values,indices,offsets}, get_variable_names/get_row_names, get_objective_name/get_problem_name, get_ascii_row_types.
| resetSolvedValues(); | ||
| } | ||
|
|
||
| public Object getObjective() { |
There was a problem hiding this comment.
getObjective() returns Object — callers must downcast. Prefer a typed return (or overloads). Same for SolverSettings.getTypedParameter() / getMipCallbacks(). (The modeling API otherwise tracks the Python Problem layer 1:1 — nice.)
| private final int[] columnIndices; | ||
| private final double[] values; | ||
|
|
||
| public CsrMatrix(int[] rowOffsets, int[] columnIndices, double[] values) { |
There was a problem hiding this comment.
Arg order is (rowOffsets, columnIndices, values), but DataModel.setCsrConstraintMatrix is (values, indices, offsets) (matching Python's set_csr_constraint_matrix(A_values, A_indices, A_offsets)). This class is the odd one out — reversing it here is easy to transpose silently; consider aligning.
There was a problem hiding this comment.
Python's order seems wrong though. The order should be rowOffset, columnIndices, values.
There was a problem hiding this comment.
Maybe we should fix Python?
| High-level model | ||
| ---------------- | ||
|
|
||
| ``Problem`` is the recommended entry point for models built in Java. |
There was a problem hiding this comment.
It's confusing to mix the terms model and Problem. I would use the term problem throughout and remove references to model
| - Set a quadratic objective with optional linear and constant terms. | ||
| * - ``solve()`` / ``solve(SolverSettings)`` | ||
| - Convert the model to a native ``DataModel`` and return a ``Solution``. | ||
| * - ``toDataModel()`` |
There was a problem hiding this comment.
Is it necessary to have a DataModel? I think we should deprecate this.
|
|
||
| The model also exposes ``getVariables``, ``getVariable``, ``getConstraints``, | ||
| ``getConstraint``, ``getNumVariables``, ``getNumConstraints``, | ||
| ``getNumNonZeros``, ``isMip``, ``isSolved``, ``getStatus``, |
There was a problem hiding this comment.
We should have consistent capitalization. Here we use isMip, but elsewhere we have MIPStats. I personally think we should always use MIP, LP and QP and cuOpt, even if it breaks Java convention. But failing that we should at least be consistent. Same for MPS and CSR
|
|
||
| ``SolverSettings`` owns native solver configuration and implements | ||
| ``AutoCloseable``. Parameters can be set with the overloaded | ||
| ``setParameter`` methods for ``String``, ``int``, ``double``, and ``boolean`` |
There was a problem hiding this comment.
Here we are mixing the terms Settings and Parameters. I suggest we use Setting throughout for consistency.
| * primal and dual initial solutions; | ||
| * ``dumpParametersToFile`` and ``loadParametersFromFile``; | ||
| * ``toDict``; | ||
| * ``setPdlpWarmStartData``; and |
There was a problem hiding this comment.
Capitialization of PDLP is inconsistent: we have setPdlpWarmStartData and PDLPSolverMode.
| * ``getSolveTime`` and ``getProblemCategory``; and | ||
| * ``getVars`` when variable names are available. | ||
|
|
||
| LP solutions additionally expose ``getLpStats`` and PDLP warm-start data. |
There was a problem hiding this comment.
Inconsistent capitalization: getLpStats vs LPStats.
| MPS, batching, and errors | ||
| ------------------------- | ||
|
|
||
| ``DataModel.read``, ``DataModel.parseMps``, ``Problem.read``, and |
There was a problem hiding this comment.
Inconsistent capitalization: parseMps and readMPS
| 0.0, | ||
| new double[] {1.0, 1.0}, | ||
| matrix, | ||
| new byte[] {(byte) 'G'}, |
There was a problem hiding this comment.
Is it possible to do this with just 'G' instead of (byte) 'G'?
| } | ||
|
|
||
| Only ``LE`` and ``GE`` quadratic constraints are supported. Calling | ||
| ``QuadraticExpression.eq`` or adding an equality quadratic constraint raises |
There was a problem hiding this comment.
We should not even have a QuadraticExpression.eq
| quadratic matrices, MPS I/O, and solver results. | ||
|
|
||
| Quadratic constraints are supported for ``LE`` and ``GE`` constraints. Equality | ||
| quadratic constraints are rejected by the Java API. Dedicated SOCP modeling |
There was a problem hiding this comment.
Remove the sentence beginning with "Dedicated SOCP modeling ... " I'm not sure what it means and I don't think it's necessary to say this.
| ==================================== | ||
|
|
||
| NVIDIA cuOpt provides experimental Java bindings for linear programming (LP), | ||
| mixed-integer linear programming (MILP), quadratic programming (QP), and |
There was a problem hiding this comment.
What about QCQP and SOCP?
| package com.nvidia.cuopt.linearprogramming; | ||
|
|
||
| public enum PDLPSolverMode { | ||
| STABLE1(0), |
There was a problem hiding this comment.
I'm worried that these are going to get out of date. Is there any way that this can get the values from the c++ code? Rather than hard-coding it into the interface?
| public enum ProblemCategory { | ||
| LP(0), | ||
| MIP(1), | ||
| IP(2); |
There was a problem hiding this comment.
We should deprecate IP across the whole code base.
| package com.nvidia.cuopt.linearprogramming; | ||
|
|
||
| public enum SolverMethod { | ||
| CONCURRENT(0), |
There was a problem hiding this comment.
I'm worried this is going to get out of date. Is there any way to get these values from the c++ code?
| package com.nvidia.cuopt.linearprogramming; | ||
|
|
||
| public enum TerminationStatus { | ||
| NO_TERMINATION(0), |
There was a problem hiding this comment.
I'm worried this is going to get out of date. Is there anyway to get these values from the C++ code?
| @@ -0,0 +1,151 @@ | |||
| package com.nvidia.cuopt.linearprogramming; | |||
|
|
|||
| public final class CuOptConstants { | |||
There was a problem hiding this comment.
Is this file autogenerated? If so, we should put a comment at the top saying it is autogenerated and add instructions on how to regenerate the file.
There was a problem hiding this comment.
It is autogenerated. Instructions are now in the README.
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
Thanks for adding the JAVA API. Let's make sure that capitalization is consistent before merging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/DataModelIntegrationTest.java (1)
354-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
expectSolutionValues/expectedObjectivefields for the QP case.
qp_diagonal_objectivepassesexpectSolutionValues=trueandexpectedObjective=Double.NaN, butverify()returns beforeassertSolution()for any casehasQuadraticObjective()(lines 37-41), so these values are never checked. Minor - not a functional issue given the explanatory comment, just slightly misleading at the call site.🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/DataModelIntegrationTest.java` around lines 354 - 389, The qp_diagonal_objective CaseSpec is setting expectSolutionValues and expectedObjective even though DataModelIntegrationTest.verify() exits early for cases with hasQuadraticObjective() before assertSolution() runs, so these values are misleading at the call site. Update this test setup to either remove those unused expectations from the CaseSpec builder chain or move the checks into the quadratic-path assertions in verify()/assertSolution() using the CaseSpec and its hasQuadraticObjective()/withQuadraticObjective() flow.java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java (2)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate test-scaffolding helpers across test classes.
assumeNativeLibrary()andassumeCudaDriverAvailable()are duplicated verbatim (with a small divergence - this file omitsredirectErrorStream(true),DataModelIntegrationTestincludes it) betweenNativeIntegrationTestandDataModelIntegrationTest. Consider extracting a shared test-support class to avoid drift between the two copies.♻️ Suggested extraction
// e.g. NativeTestSupport.java final class NativeTestSupport { static void assumeNativeLibrary() { String nativeDir = System.getProperty("cuopt.native.dir"); Assumptions.assumeTrue(nativeDir != null && !nativeDir.isBlank(), "cuopt.native.dir is unset"); Assumptions.assumeTrue( Files.exists(Path.of(nativeDir, System.mapLibraryName("cuopt_jni"))), "libcuopt_jni is not built"); } static void assumeCudaDriverAvailable() { try { Process process = new ProcessBuilder("nvidia-smi").redirectErrorStream(true).start(); Assumptions.assumeTrue(process.waitFor() == 0, "CUDA driver is unavailable"); } catch (InterruptedException e) { Thread.currentThread().interrupt(); Assumptions.assumeTrue(false, "CUDA driver check was interrupted"); } catch (Exception e) { Assumptions.assumeTrue(false, "CUDA driver check failed: " + e.getMessage()); } } }Also applies to: 172-178, 235-246
🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java` around lines 20 - 26, The native test setup helpers are duplicated between NativeIntegrationTest and DataModelIntegrationTest, so extract the shared logic into a single test-support helper (for example, a NativeTestSupport class) and have both tests call it. Move assumeNativeLibrary() and assumeCudaDriverAvailable() into that shared class, keep the existing behavior consistent across both callers, and align the ProcessBuilder setup so the two copies do not drift (including the redirectErrorStream(true) detail where needed).
169-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuadratic objective built from an unrelated
Problem's variables.
x0/x1come from a freshly createdshellProblem, not fromtinyLP()'sDataModel. This only works becauseshell's variables happen to get indices 0/1, matchingtinyLP()'s variable count/order. A comment explaining the intentional index-only usage (or building the expression againsttinyLP's actual variable count) would make the contract explicit and less brittle to future edits oftinyLP().🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java` around lines 169 - 184, The small QP test is building the quadratic objective from variables owned by a separate Problem, which relies on incidental matching indices and is brittle. Update solvesSmallQP to make the intent explicit by either constructing the expression from the DataModel/tinyLP variable context or adding a clear comment near the Problem/addVariable and QuadraticExpression.of usage that this is intentional index-only wiring, so future changes to tinyLP() don’t silently break the test.java/cuopt/scripts/build_native.sh (1)
12-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated JAVA_HOME auto-detection logic with
test.sh.This 7-line block (locate
javac/javafromPATH, deriveJAVA_HOME, validate) is repeated almost verbatim injava/cuopt/scripts/test.sh(lines 15-25). Consider extracting into a small sourced helper (e.g.scripts/_java_home.sh) to avoid the two copies drifting.🤖 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 `@java/cuopt/scripts/build_native.sh` around lines 12 - 23, The JAVA_HOME auto-detection logic in build_native.sh is duplicated in test.sh and should be shared to avoid drift. Extract the locate-derive-validate flow into a small sourced helper script (for example, a reusable Java-home setup helper) and have both build_native.sh and test.sh source it instead of maintaining separate copies; keep the existing JAVAC_PATH/JAVA_HOME detection and validation behavior centralized in that helper.java/cuopt/CMakeLists.txt (1)
11-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
find_package(CUDAToolkit)instead of hand-rolled path resolution.Manually locating
libcudart.soand CUDA include dirs viaCONDA_PREFIX/targets/<arch>-linuxduplicates whatfind_package(CUDAToolkit)already does portably (and would fix the aarch64 path issue above for free).🤖 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 `@java/cuopt/CMakeLists.txt` around lines 11 - 52, The CUDA runtime and include path resolution in CMakeLists.txt is hand-rolled and should be replaced with CUDAToolkit discovery. Update the cuopt_jni setup to use find_package(CUDAToolkit) and then reference its imported targets/variables instead of hardcoding CONDA_PREFIX, targets/x86_64-linux, and /usr/local/cuda paths. Keep the existing cuOpt-specific checks, but switch target_link_libraries and include directories to symbols from CUDAToolkit so the JNI build stays portable across architectures..github/workflows/build.yaml (1)
72-95: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGPU node requested for build-only job.
java-buildrunsci/build_java.shwithout--run-java-tests(build+package only, no GPU work), yet requestsnode_type: "gpu-l4-latest-1". If a CPU-only builder is available for the C++/CUDA-toolkit-linking step, using it here would avoid consuming GPU capacity for a job that doesn't execute GPU code.🤖 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 @.github/workflows/build.yaml around lines 72 - 95, The java-build workflow job is unnecessarily reserving a GPU node even though it only runs ci/build_java.sh for build/package and does not execute Java tests or GPU work. Update the java-build job’s node_type setting in the custom-job invocation to use a CPU-only builder if available, while keeping the rest of the matrix and artifact/upload configuration unchanged.java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/LinearExpression.java (1)
96-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo guard against division by zero in
dividedBy.
dividedBy(0.0)silently producesInfinity/NaNcoefficients that propagate into the constraint matrix passed to the native solver, rather than failing fast with a clear error at model-construction time.🛡️ Proposed guard
public LinearExpression dividedBy(double scalar) { + if (scalar == 0.0) { + throw new IllegalArgumentException("Cannot divide expression by zero"); + } return times(1.0 / scalar); }🤖 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 `@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/LinearExpression.java` around lines 96 - 98, Add a fail-fast guard in LinearExpression.dividedBy so scalar values of 0.0 are rejected before calling times(1.0 / scalar). Update the LinearExpression.dividedBy method to validate the input and throw a clear exception for zero (and any other invalid non-finite case if appropriate), so invalid coefficients cannot propagate into the native solver.java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/QuadraticExpression.java (1)
108-110: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame division-by-zero gap as
LinearExpression.dividedBy.Consider applying the same guard here for consistency.
🤖 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 `@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/QuadraticExpression.java` around lines 108 - 110, The QuadraticExpression.dividedBy method has the same missing division-by-zero guard as LinearExpression.dividedBy. Update QuadraticExpression.dividedBy(double scalar) to validate the scalar before calling times(1.0 / scalar), and make its behavior consistent with the corresponding linear expression method by rejecting zero (and any other unsupported values if applicable) with the same error handling pattern.java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java (1)
165-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic API marked
@Deprecatedis load-bearing for the entire solve/writeMPS path.
toDataModel()is annotated@Deprecated(since = "26.08")with"Use Problem directly", yetsolve(SolverSettings)(line 226) andwriteMPS(String)(line 238) both call it internally on every invocation. This is confusing: the class's own primary API depends on a method it tells external users to stop using. Consider extracting a private/package-private builder (e.g.buildDataModel()) for internal use and keeping the publictoDataModel()as a thin deprecated wrapper, so the deprecation signal is accurate and internal callers don't trigger deprecation warnings.🤖 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 `@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java` around lines 165 - 216, The deprecated public toDataModel() method is still being used as the internal implementation for solve(SolverSettings) and writeMPS(String), which makes the deprecation message misleading. Extract the shared conversion logic into a private/package-private builder method such as buildDataModel(), have solve and writeMPS call that internal helper, and keep toDataModel() as a thin deprecated wrapper so external callers still see the deprecation while internal paths avoid depending on the deprecated API.
🤖 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 @.github/workflows/build.yaml:
- Around line 63-71: The java-build-matrix job is still hardcoded to use
pull-request, which causes the wrong Java matrix for non-PR runs. Update the
workflow input in build.yaml so the uses: compute-matrix.yaml call for
java-build-matrix passes the same build_type used by the workflow instead of a
fixed value, keeping the matrix selection aligned with the trigger type.
In `@java/cuopt/CMakeLists.txt`:
- Around line 23-24: The CUOPT CUDA runtime and include paths are hardcoded to
x86_64 and will break supported aarch64 builds. Update the
CUOPT_CUDA_RUNTIME_DIR and related include-path logic in CMakeLists.txt to
derive the target architecture dynamically, using CMAKE_SYSTEM_PROCESSOR or CUDA
toolkit discovery instead of embedding x86_64-linux, so CUOPT_CUDART_LIBRARY
resolves correctly on both x86_64 and aarch64.
In
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/CSRMatrix.java`:
- Around line 15-22: Add explicit validation in CSRMatrix’s constructor before
calling Arrays.copyOf: reject null values, columnIndices, and rowOffsets with a
clear IllegalArgumentException, then validate CSR consistency by checking that
the rowOffsets array is non-empty, starts at 0, is monotonic, and that its last
entry matches values.length (and therefore the nnz count). Keep the existing
values/columnIndices length check, and ensure the constructor fails fast with a
descriptive message so malformed matrices cannot reach NativeCuOpt.createProblem
or setConstraintMatrix.
In
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java`:
- Around line 253-263: The quadratic objective cache in DataModel becomes stale
when mixing setQuadraticObjectiveMatrix(...) and
setQuadraticObjective(QuadraticExpression), because the latter updates native
state but leaves quadraticObjectiveValues/Indices/Offsets and
quadraticObjectiveMatrixSet unchanged. Update setQuadraticObjective(...) to keep
the cached CSR arrays and flag in sync with the new objective, and make sure the
getter methods getQuadraticObjectiveValues/Indices/Offsets() always reflect the
most recently set objective rather than old matrix data.
- Around line 413-500: The quadratic constraint naming logic in DataModel is
using insertion order, so after read() or parseMPS() leaves
quadraticConstraintNames empty, the next addQuadraticConstraint(...) can
overwrite an existing native constraint’s name. Fix this by initializing
quadraticConstraintNames from the loaded model in the parsing/load path, or by
associating names with the native row index instead of list position; update the
addQuadraticConstraint overloads and getQuadraticConstraints so they resolve
names using a stable identifier rather than append order.
In
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java`:
- Around line 393-414: `updateConstraint` is missing the same ownership
validation that `updateObjective` uses for coefficient variables. In
`Problem.updateConstraint`, add a check in the `coefficients` loop to নিশ্চিত
each `Variable` key belongs to this `Problem`’s `variables` set and throw
`IllegalArgumentException` if not, before merging it into the
`LinearExpression`; keep the rest of the update flow in `updateConstraint`
unchanged.
- Around line 416-456: The updateObjective method in Problem is leaving the
linearObjective field and objectiveSet flag unchanged when coefficients are
provided before any objective has been initialized and constant is null, so
getObjective() stays stale. Add a branch in updateObjective that rebuilds
linearObjective from the current variable objective coefficients (or otherwise
synchronizes linearObjective) and marks objectiveSet true for this
coefficients-only path, while keeping the existing ObjectiveSense handling and
compatibility with objectiveCoefficients() and getObjective().
- Around line 39-55: `addVariable(...)` and `addConstraint(...)` only mark the
model unsolved, leaving stale solve metadata behind. Update both methods in
`Problem` to also clear cached solved state the same way `setObjective(...)`
does by invoking `resetSolvedValues()` after mutating `variables` or
`constraints`, so `getStatus()`, `getObjectiveValue()`, `getSolveTime()`, and
cached variable/constraint values no longer reflect an older solve.
- Around line 222-235: The solve(SolverSettings) flow in Problem leaves the
native Solution open if populateSolution(...) throws after dataModel.solve(...)
returns. Update this method so the Solution returned from
dataModel.solve(actualSettings) is always closed on the failure path, e.g. by
wrapping populateSolution(solution) in its own try/catch/finally and calling
solution.close() before rethrowing, while preserving the existing closeSettings
handling for SolverSettings.
- Around line 278-310: The constraint reconstruction logic in
Problem.read/Problem.readMPS is flattening ranged rows into a single LE/GE/EQ
constraint based on rhs[row], which drops the original interval bounds. Update
the row handling around the ConstraintSense selection and switch so rows with
differing constraintLowerBounds[row] and constraintUpperBounds[row] are either
preserved via a range-aware representation or explicitly rejected with a clear
error instead of being converted. Keep the existing single-sided path for
non-ranged rows and make the behavior around rowNames and addConstraint
consistent with the chosen handling.
In `@java/cuopt/src/main/native/cuopt_jni.cpp`:
- Around line 261-289: The JNI callback in mip_get_solution_callback leaks a
local reference because GetObjectClass(context->callback) is never paired with a
DeleteLocalRef. Store the jclass returned by GetObjectClass, use it for
GetMethodID, and delete that local ref before returning; keep the rest of the
callback flow in cuopt_jni.cpp unchanged.
- Around line 291-327: mip_set_solution_callback is leaking several JNI local
references and silently ignoring a solution-size mismatch. In this callback,
delete the local refs created by GetObjectClass, CallObjectMethod,
GetObjectClass on callback_solution, and GetObjectField for the solution array
before returning, ideally using a consistent cleanup path in
mip_set_solution_callback. Also add explicit handling for the values.size() !=
context->num_variables case in the same function: surface an error via the
existing JNI/error-reporting mechanism or logging instead of doing nothing, so
callback sizing bugs are diagnosable.
---
Nitpick comments:
In @.github/workflows/build.yaml:
- Around line 72-95: The java-build workflow job is unnecessarily reserving a
GPU node even though it only runs ci/build_java.sh for build/package and does
not execute Java tests or GPU work. Update the java-build job’s node_type
setting in the custom-job invocation to use a CPU-only builder if available,
while keeping the rest of the matrix and artifact/upload configuration
unchanged.
In `@java/cuopt/CMakeLists.txt`:
- Around line 11-52: The CUDA runtime and include path resolution in
CMakeLists.txt is hand-rolled and should be replaced with CUDAToolkit discovery.
Update the cuopt_jni setup to use find_package(CUDAToolkit) and then reference
its imported targets/variables instead of hardcoding CONDA_PREFIX,
targets/x86_64-linux, and /usr/local/cuda paths. Keep the existing
cuOpt-specific checks, but switch target_link_libraries and include directories
to symbols from CUDAToolkit so the JNI build stays portable across
architectures.
In `@java/cuopt/scripts/build_native.sh`:
- Around line 12-23: The JAVA_HOME auto-detection logic in build_native.sh is
duplicated in test.sh and should be shared to avoid drift. Extract the
locate-derive-validate flow into a small sourced helper script (for example, a
reusable Java-home setup helper) and have both build_native.sh and test.sh
source it instead of maintaining separate copies; keep the existing
JAVAC_PATH/JAVA_HOME detection and validation behavior centralized in that
helper.
In
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/LinearExpression.java`:
- Around line 96-98: Add a fail-fast guard in LinearExpression.dividedBy so
scalar values of 0.0 are rejected before calling times(1.0 / scalar). Update the
LinearExpression.dividedBy method to validate the input and throw a clear
exception for zero (and any other invalid non-finite case if appropriate), so
invalid coefficients cannot propagate into the native solver.
In
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java`:
- Around line 165-216: The deprecated public toDataModel() method is still being
used as the internal implementation for solve(SolverSettings) and
writeMPS(String), which makes the deprecation message misleading. Extract the
shared conversion logic into a private/package-private builder method such as
buildDataModel(), have solve and writeMPS call that internal helper, and keep
toDataModel() as a thin deprecated wrapper so external callers still see the
deprecation while internal paths avoid depending on the deprecated API.
In
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/QuadraticExpression.java`:
- Around line 108-110: The QuadraticExpression.dividedBy method has the same
missing division-by-zero guard as LinearExpression.dividedBy. Update
QuadraticExpression.dividedBy(double scalar) to validate the scalar before
calling times(1.0 / scalar), and make its behavior consistent with the
corresponding linear expression method by rejecting zero (and any other
unsupported values if applicable) with the same error handling pattern.
In
`@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/DataModelIntegrationTest.java`:
- Around line 354-389: The qp_diagonal_objective CaseSpec is setting
expectSolutionValues and expectedObjective even though
DataModelIntegrationTest.verify() exits early for cases with
hasQuadraticObjective() before assertSolution() runs, so these values are
misleading at the call site. Update this test setup to either remove those
unused expectations from the CaseSpec builder chain or move the checks into the
quadratic-path assertions in verify()/assertSolution() using the CaseSpec and
its hasQuadraticObjective()/withQuadraticObjective() flow.
In
`@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java`:
- Around line 20-26: The native test setup helpers are duplicated between
NativeIntegrationTest and DataModelIntegrationTest, so extract the shared logic
into a single test-support helper (for example, a NativeTestSupport class) and
have both tests call it. Move assumeNativeLibrary() and
assumeCudaDriverAvailable() into that shared class, keep the existing behavior
consistent across both callers, and align the ProcessBuilder setup so the two
copies do not drift (including the redirectErrorStream(true) detail where
needed).
- Around line 169-184: The small QP test is building the quadratic objective
from variables owned by a separate Problem, which relies on incidental matching
indices and is brittle. Update solvesSmallQP to make the intent explicit by
either constructing the expression from the DataModel/tinyLP variable context or
adding a clear comment near the Problem/addVariable and QuadraticExpression.of
usage that this is intentional index-only wiring, so future changes to tinyLP()
don’t silently break the test.
🪄 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: 3ca4aa57-ac64-49cf-9f79-73f9782af4c5
📒 Files selected for processing (56)
.github/workflows/build.yaml.github/workflows/pr.yaml.github/workflows/test.yamlci/build_java.shci/release/update-version.shci/test_java.shdependencies.yamldocs/cuopt/source/cuopt-java/convex/convex-api.rstdocs/cuopt/source/cuopt-java/convex/convex-examples.rstdocs/cuopt/source/cuopt-java/convex/index.rstdocs/cuopt/source/cuopt-java/index.rstdocs/cuopt/source/cuopt-java/mip/index.rstdocs/cuopt/source/cuopt-java/mip/mip-api.rstdocs/cuopt/source/cuopt-java/mip/mip-examples.rstdocs/cuopt/source/cuopt-java/quick-start.rstdocs/cuopt/source/index.rstjava/.gitignorejava/cuopt/CMakeLists.txtjava/cuopt/README.mdjava/cuopt/TESTS.mdjava/cuopt/pom.xmljava/cuopt/scripts/build_native.shjava/cuopt/scripts/generate_constants.shjava/cuopt/scripts/test.shjava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/CSRMatrix.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Constraint.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/ConstraintSense.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/CuOptException.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/LPStats.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/LinearExpression.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/MIPCallback.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/MIPCallbackSolution.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/MIPSetSolutionCallback.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/MIPSolutionCallback.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/MIPStats.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/NativeCuOpt.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/ObjectiveExpression.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/ObjectiveSense.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/PDLPSolverMode.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/ProblemCategory.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/QuadraticConstraint.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/QuadraticExpression.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Solution.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/SolverMethod.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/SolverSettings.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/TerminationStatus.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Variable.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/VariableType.javajava/cuopt/src/main/native/cuopt_java_native_api.cppjava/cuopt/src/main/native/cuopt_java_native_api.hppjava/cuopt/src/main/native/cuopt_jni.cppjava/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/DataModelIntegrationTest.javajava/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.javajava/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemModelingTest.java
| java-build-matrix: | ||
| needs: cpp-build | ||
| permissions: | ||
| contents: read | ||
| uses: rapidsai/shared-workflows/.github/workflows/compute-matrix.yaml@main | ||
| with: | ||
| build_type: pull-request | ||
| matrix_name: conda-cpp-build | ||
| matrix_filter: map(select(.ARCH == "amd64")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workflow and the referenced shared workflow usage in context.
git ls-files .github/workflows/build.yaml .github/workflows/*.yaml .github/actions 2>/dev/null || true
printf '\n--- build.yaml (lines 1-220) ---\n'
nl -ba .github/workflows/build.yaml | sed -n '1,220p'
printf '\n--- search for build_type usage in workflows ---\n'
rg -n "build_type:\s*|inputs\.build_type|compute-matrix\.yaml|java-build-matrix|java-build" .github/workflows .github/actions -g '*.yml' -g '*.yaml' || trueRepository: NVIDIA/cuopt
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files .github/workflows/build.yaml .github/workflows/*.yaml .github/actions 2>/dev/null || true
printf '\n--- build.yaml (lines 1-220) ---\n'
nl -ba .github/workflows/build.yaml | sed -n '1,220p'
printf '\n--- search for build_type usage in workflows ---\n'
rg -n "build_type:\s*|inputs\.build_type|compute-matrix\.yaml|java-build-matrix|java-build" .github/workflows .github/actions -g '*.yml' -g '*.yaml' || trueRepository: NVIDIA/cuopt
Length of output: 713
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- .github/workflows/build.yaml (lines 1-220) ---\n'
awk '{printf("%5d %s\n", NR, $0)}' .github/workflows/build.yaml | sed -n '1,220p'
printf '\n--- search for workflow triggers / build_type in .github/workflows/build.yaml ---\n'
rg -n "on:|workflow_call|workflow_dispatch|schedule:|build_type:|java-build-matrix|java-build:" .github/workflows/build.yamlRepository: NVIDIA/cuopt
Length of output: 10559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- usages of compute-matrix.yaml across repo ---\n'
rg -n "compute-matrix\.yaml@main|matrix_name: conda-cpp-build|matrix_filter:" .github/workflows -g '*.yml' -g '*.yaml'
printf '\n--- relevant workflow context around any other compute-matrix callsites ---\n'
for f in .github/workflows/*.yaml .github/workflows/*.yml; do
[ -f "$f" ] || continue
if rg -q "compute-matrix\.yaml@main" "$f"; then
echo "### $f"
awk '{printf("%5d %s\n", NR, $0)}' "$f" | rg -n "compute-matrix\.yaml@main|matrix_name:|build_type:|needs:|strategy:|uses:"
echo
fi
doneRepository: NVIDIA/cuopt
Length of output: 9030
Use the workflow build_type for java-build-matrix.
.github/workflows/build.yaml:67-70 runs on push, tags, and manual nightly dispatch, but this matrix is still hardcoded to pull-request, so branch/nightly runs compute the wrong Java matrix.
Fix
- build_type: pull-request
+ build_type: ${{ inputs.build_type || 'branch' }}📝 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.
| java-build-matrix: | |
| needs: cpp-build | |
| permissions: | |
| contents: read | |
| uses: rapidsai/shared-workflows/.github/workflows/compute-matrix.yaml@main | |
| with: | |
| build_type: pull-request | |
| matrix_name: conda-cpp-build | |
| matrix_filter: map(select(.ARCH == "amd64")) | |
| java-build-matrix: | |
| needs: cpp-build | |
| permissions: | |
| contents: read | |
| uses: rapidsai/shared-workflows/.github/workflows/compute-matrix.yaml@main | |
| with: | |
| build_type: ${{ inputs.build_type || 'branch' }} | |
| matrix_name: conda-cpp-build | |
| matrix_filter: map(select(.ARCH == "amd64")) |
🤖 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 @.github/workflows/build.yaml around lines 63 - 71, The java-build-matrix job
is still hardcoded to use pull-request, which causes the wrong Java matrix for
non-PR runs. Update the workflow input in build.yaml so the uses:
compute-matrix.yaml call for java-build-matrix passes the same build_type used
by the workflow instead of a fixed value, keeping the matrix selection aligned
with the trigger type.
| set(CUOPT_CUDA_RUNTIME_DIR "${CUOPT_PREFIX}/targets/x86_64-linux/lib" | ||
| CACHE PATH "CUDA runtime library directory") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hardcoded x86_64-linux breaks aarch64 builds.
CUOPT_CUDA_RUNTIME_DIR and the include path both hardcode targets/x86_64-linux, but CONTRIBUTING.md explicitly lists aarch64 as a supported architecture. On an aarch64 conda environment this path won't exist, so CUOPT_CUDART_LIBRARY will fail the EXISTS check at line 36 with a misleading "not found" error rather than resolving correctly.
🛠️ Suggested fix: use `CMAKE_SYSTEM_PROCESSOR` or `find_package(CUDAToolkit)`
-set(CUOPT_CUDA_RUNTIME_DIR "${CUOPT_PREFIX}/targets/x86_64-linux/lib"
- CACHE PATH "CUDA runtime library directory")
+if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")
+ set(CUOPT_CUDA_TARGET_DIR "sbsa-linux")
+else()
+ set(CUOPT_CUDA_TARGET_DIR "x86_64-linux")
+endif()
+set(CUOPT_CUDA_RUNTIME_DIR "${CUOPT_PREFIX}/targets/${CUOPT_CUDA_TARGET_DIR}/lib"
+ CACHE PATH "CUDA runtime library directory")As per coding guidelines, "Only Linux is supported" with architectures "x86_64 (64-bit)" and "aarch64 (64-bit)" per CONTRIBUTING.md.
Also applies to: 47-49
🤖 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 `@java/cuopt/CMakeLists.txt` around lines 23 - 24, The CUOPT CUDA runtime and
include paths are hardcoded to x86_64 and will break supported aarch64 builds.
Update the CUOPT_CUDA_RUNTIME_DIR and related include-path logic in
CMakeLists.txt to derive the target architecture dynamically, using
CMAKE_SYSTEM_PROCESSOR or CUDA toolkit discovery instead of embedding
x86_64-linux, so CUOPT_CUDART_LIBRARY resolves correctly on both x86_64 and
aarch64.
Source: Coding guidelines
| public CSRMatrix(double[] values, int[] columnIndices, int[] rowOffsets) { | ||
| this.rowOffsets = Arrays.copyOf(rowOffsets, rowOffsets.length); | ||
| this.columnIndices = Arrays.copyOf(columnIndices, columnIndices.length); | ||
| this.values = Arrays.copyOf(values, values.length); | ||
| if (this.values.length != this.columnIndices.length) { | ||
| throw new IllegalArgumentException("CSR values and column indices must have the same length"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Missing null-safety and rowOffsets consistency validation.
values/columnIndices/rowOffsets being null throws an NPE from Arrays.copyOf rather than a clear IllegalArgumentException. More importantly, there's no check that rowOffsets is internally consistent with the nnz count (e.g. rowOffsets[rowOffsets.length - 1] == values.length). Since this type marshals directly into the native JNI layer (NativeCuOpt.createProblem, setConstraintMatrix, etc.), a malformed rowOffsets array could propagate to native code and cause out-of-bounds reads there.
🛡️ Proposed validation
public CSRMatrix(double[] values, int[] columnIndices, int[] rowOffsets) {
+ java.util.Objects.requireNonNull(values, "values");
+ java.util.Objects.requireNonNull(columnIndices, "columnIndices");
+ java.util.Objects.requireNonNull(rowOffsets, "rowOffsets");
this.rowOffsets = Arrays.copyOf(rowOffsets, rowOffsets.length);
this.columnIndices = Arrays.copyOf(columnIndices, columnIndices.length);
this.values = Arrays.copyOf(values, values.length);
if (this.values.length != this.columnIndices.length) {
throw new IllegalArgumentException("CSR values and column indices must have the same length");
}
+ if (this.rowOffsets.length == 0
+ || this.rowOffsets[this.rowOffsets.length - 1] != this.values.length) {
+ throw new IllegalArgumentException(
+ "CSR rowOffsets must be non-empty and end with the number of non-zero values");
+ }
}📝 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.
| public CSRMatrix(double[] values, int[] columnIndices, int[] rowOffsets) { | |
| this.rowOffsets = Arrays.copyOf(rowOffsets, rowOffsets.length); | |
| this.columnIndices = Arrays.copyOf(columnIndices, columnIndices.length); | |
| this.values = Arrays.copyOf(values, values.length); | |
| if (this.values.length != this.columnIndices.length) { | |
| throw new IllegalArgumentException("CSR values and column indices must have the same length"); | |
| } | |
| } | |
| public CSRMatrix(double[] values, int[] columnIndices, int[] rowOffsets) { | |
| java.util.Objects.requireNonNull(values, "values"); | |
| java.util.Objects.requireNonNull(columnIndices, "columnIndices"); | |
| java.util.Objects.requireNonNull(rowOffsets, "rowOffsets"); | |
| this.rowOffsets = Arrays.copyOf(rowOffsets, rowOffsets.length); | |
| this.columnIndices = Arrays.copyOf(columnIndices, columnIndices.length); | |
| this.values = Arrays.copyOf(values, values.length); | |
| if (this.values.length != this.columnIndices.length) { | |
| throw new IllegalArgumentException("CSR values and column indices must have the same length"); | |
| } | |
| if (this.rowOffsets.length == 0 | |
| || this.rowOffsets[this.rowOffsets.length - 1] != this.values.length) { | |
| throw new IllegalArgumentException( | |
| "CSR rowOffsets must be non-empty and end with the number of non-zero values"); | |
| } | |
| } |
🤖 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
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/CSRMatrix.java`
around lines 15 - 22, Add explicit validation in CSRMatrix’s constructor before
calling Arrays.copyOf: reject null values, columnIndices, and rowOffsets with a
clear IllegalArgumentException, then validate CSR consistency by checking that
the rowOffsets array is non-empty, starts at 0, is monotonic, and that its last
entry matches values.length (and therefore the nnz count). Keep the existing
values/columnIndices length check, and ensure the constructor fails fast with a
descriptive message so malformed matrices cannot reach NativeCuOpt.createProblem
or setConstraintMatrix.
| public DataModel setQuadraticObjectiveMatrix(double[] values, int[] indices, int[] offsets) { | ||
| double[] copiedValues = copy(values); | ||
| int[] copiedIndices = copy(indices); | ||
| int[] copiedOffsets = copy(offsets); | ||
| NativeCuOpt.setQuadraticObjectiveMatrix(handle(), copiedValues, copiedIndices, copiedOffsets); | ||
| quadraticObjectiveValues = copiedValues; | ||
| quadraticObjectiveIndices = copiedIndices; | ||
| quadraticObjectiveOffsets = copiedOffsets; | ||
| quadraticObjectiveMatrixSet = true; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stale cached quadratic-objective data after mixing setter APIs.
setQuadraticObjective(QuadraticExpression) writes the new objective to native but never updates quadraticObjectiveValues/Indices/Offsets or resets quadraticObjectiveMatrixSet. If a caller previously invoked setQuadraticObjectiveMatrix(...) (which sets the flag and caches the CSR arrays) and later calls setQuadraticObjective(...), subsequent getQuadraticObjectiveValues/Indices/Offsets() calls will silently return the earlier cached data instead of the objective that was just set.
🛡️ Proposed fix — keep cache in sync
public DataModel setQuadraticObjective(QuadraticExpression expression) {
NativeCuOpt.setQuadraticObjective(
handle(), quadraticRows(expression), quadraticColumns(expression), quadraticValues(expression));
+ quadraticObjectiveValues = quadraticValues(expression);
+ quadraticObjectiveIndices = quadraticColumns(expression);
+ // quadraticObjectiveOffsets requires row-offset construction consistent with expression's rows
+ quadraticObjectiveMatrixSet = true;
return this;
}Also applies to: 335-351, 407-411
🤖 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
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java`
around lines 253 - 263, The quadratic objective cache in DataModel becomes stale
when mixing setQuadraticObjectiveMatrix(...) and
setQuadraticObjective(QuadraticExpression), because the latter updates native
state but leaves quadraticObjectiveValues/Indices/Offsets and
quadraticObjectiveMatrixSet unchanged. Update setQuadraticObjective(...) to keep
the cached CSR arrays and flag in sync with the new objective, and make sure the
getter methods getQuadraticObjectiveValues/Indices/Offsets() always reflect the
most recently set objective rather than old matrix data.
| public DataModel addQuadraticConstraint(Constraint constraint) { | ||
| if (!constraint.isQuadratic()) { | ||
| throw new IllegalArgumentException("Quadratic constraint requires quadratic terms"); | ||
| } | ||
| if (constraint.getSense() == ConstraintSense.EQ) { | ||
| throw new IllegalArgumentException("Equality quadratic constraints are not supported"); | ||
| } | ||
| QuadraticExpression expression = constraint.getQuadraticExpression(); | ||
| LinearExpression linear = constraint.getLinearExpression(); | ||
| int[] linearIndices = new int[linear.getTerms().size()]; | ||
| double[] linearCoefficients = new double[linear.getTerms().size()]; | ||
| int i = 0; | ||
| for (var entry : linear.getTerms().entrySet()) { | ||
| linearIndices[i] = entry.getKey().getIndex(); | ||
| linearCoefficients[i] = entry.getValue(); | ||
| i++; | ||
| } | ||
| NativeCuOpt.addQuadraticConstraint( | ||
| handle(), | ||
| quadraticRows(expression), | ||
| quadraticColumns(expression), | ||
| quadraticValues(expression), | ||
| linearIndices, | ||
| linearCoefficients, | ||
| constraint.getSense().nativeValue(), | ||
| constraint.getRHS()); | ||
| quadraticConstraintNames.add(constraint.getConstraintName()); | ||
| return this; | ||
| } | ||
|
|
||
| public DataModel addQuadraticConstraint( | ||
| String rowName, | ||
| double[] linearValues, | ||
| int[] linearIndices, | ||
| double rhs, | ||
| double[] values, | ||
| int[] rows, | ||
| int[] columns, | ||
| ConstraintSense sense) { | ||
| if (sense == ConstraintSense.EQ) { | ||
| throw new IllegalArgumentException("Equality quadratic constraints are not supported"); | ||
| } | ||
| if (linearValues == null || linearIndices == null || linearValues.length != linearIndices.length) { | ||
| throw new IllegalArgumentException("linearValues and linearIndices must have the same length"); | ||
| } | ||
| if (values == null || rows == null || columns == null | ||
| || values.length != rows.length || values.length != columns.length) { | ||
| throw new IllegalArgumentException("quadratic COO arrays must have the same length"); | ||
| } | ||
| NativeCuOpt.addQuadraticConstraint( | ||
| handle(), | ||
| copy(rows), | ||
| copy(columns), | ||
| copy(values), | ||
| copy(linearIndices), | ||
| copy(linearValues), | ||
| sense.nativeValue(), | ||
| rhs); | ||
| quadraticConstraintNames.add(rowName == null ? "" : rowName); | ||
| return this; | ||
| } | ||
|
|
||
| public List<QuadraticConstraint> getQuadraticConstraints() { | ||
| Object[] nativeConstraints = NativeCuOpt.getQuadraticConstraints(handle()); | ||
| List<QuadraticConstraint> result = new ArrayList<>(nativeConstraints.length); | ||
| for (int i = 0; i < nativeConstraints.length; i++) { | ||
| Object[] entry = (Object[]) nativeConstraints[i]; | ||
| int rowIndex = ((int[]) entry[0])[0]; | ||
| String rowName = (String) entry[1]; | ||
| if (i < quadraticConstraintNames.size() && !quadraticConstraintNames.get(i).isEmpty()) { | ||
| rowName = quadraticConstraintNames.get(i); | ||
| } | ||
| ConstraintSense sense = ConstraintSense.fromNative(((byte[]) entry[2])[0]); | ||
| double rhs = ((double[]) entry[5])[0]; | ||
| result.add( | ||
| new QuadraticConstraint( | ||
| rowIndex, | ||
| rowName, | ||
| sense, | ||
| (double[]) entry[3], | ||
| (int[]) entry[4], | ||
| rhs, | ||
| (int[]) entry[6], | ||
| (int[]) entry[7], | ||
| (double[]) entry[8])); | ||
| } | ||
| return List.copyOf(result); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant Java sources and inspect the DataModel and parser entry points.
git ls-files 'java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/*.java' | sed -n '1,120p'
echo
echo "=== DataModel references ==="
rg -n "quadraticConstraintNames|addQuadraticConstraint|getQuadraticConstraints|read\\(|parseMPS\\(" \
java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java \
java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming -S
echo
echo "=== Parser / loader references ==="
rg -n "parseMPS|read\\(|quadratic constraint|QuadraticConstraint|QPS|MPS" \
java/cuopt/src/main/java -S
echo
echo "=== Relevant DataModel excerpt ==="
sed -n '1,260p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java
echo
echo "=== Later DataModel excerpt ==="
sed -n '260,560p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.javaRepository: NVIDIA/cuopt
Length of output: 33948
Quadratic constraint names can shift after parsing
read()/parseMPS() leave quadraticConstraintNames empty, so the first later addQuadraticConstraint(...) overrides the name of the first native quadratic constraint instead of the new one. Seed this list from the loaded model, or key names by native row index instead of insertion order.
🤖 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
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java`
around lines 413 - 500, The quadratic constraint naming logic in DataModel is
using insertion order, so after read() or parseMPS() leaves
quadraticConstraintNames empty, the next addQuadraticConstraint(...) can
overwrite an existing native constraint’s name. Fix this by initializing
quadraticConstraintNames from the loaded model in the parsing/load path, or by
associating names with the native row index instead of list position; update the
addQuadraticConstraint overloads and getQuadraticConstraints so they resolve
names using a stable identifier rather than append order.
| for (int row = 0; row < dataModel.getNumConstraints(); row++) { | ||
| LinearExpression expression = new LinearExpression(); | ||
| for (int p = rowOffsets[row]; p < rowOffsets[row + 1]; p++) { | ||
| expression = expression.plus(problem.getVariable(columnIndices[p]), values[p]); | ||
| } | ||
| ConstraintSense sense = ConstraintSense.fromNative(senses[row]); | ||
| if (constraintLowerBounds.length > row && constraintUpperBounds.length > row) { | ||
| if (Double.compare(constraintLowerBounds[row], constraintUpperBounds[row]) == 0) { | ||
| sense = ConstraintSense.EQ; | ||
| } else if (Double.compare(constraintLowerBounds[row], rhs[row]) == 0) { | ||
| sense = ConstraintSense.GE; | ||
| } else if (Double.compare(constraintUpperBounds[row], rhs[row]) == 0) { | ||
| sense = ConstraintSense.LE; | ||
| } | ||
| } | ||
| Constraint constraint; | ||
| switch (sense) { | ||
| case LE: | ||
| constraint = expression.le(rhs[row]); | ||
| break; | ||
| case GE: | ||
| constraint = expression.ge(rhs[row]); | ||
| break; | ||
| case EQ: | ||
| constraint = expression.eq(rhs[row]); | ||
| break; | ||
| default: | ||
| throw new IllegalStateException("Unsupported sense " + sense); | ||
| } | ||
| problem.addConstraint( | ||
| constraint, | ||
| rowNames.length > row && !rowNames[row].isEmpty() ? rowNames[row] : "c" + row); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "RANGE|ranged" java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java -i
rg -n "cuOptCreateRangedProblem|RANGES" cpp/include -iRepository: NVIDIA/cuopt
Length of output: 1126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Problem.java relevant ranges ==\n'
sed -n '1,220p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java | nl -ba | sed -n '1,220p'
printf '\n== DataModel.java ranged problem factory ==\n'
sed -n '1,180p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java | nl -ba | sed -n '1,180p'
printf '\n== Native API signatures ==\n'
sed -n '230,290p' cpp/include/cuopt/linear_programming/cuopt_c.h | nl -ba | sed -n '230,290p'Repository: NVIDIA/cuopt
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Problem.java around the cited lines ==\n'
sed -n '240,330p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java | nl -ba | sed -n '240,330p'
printf '\n== DataModel.java around createRangedProblem ==\n'
sed -n '1,130p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java | nl -ba | sed -n '1,130p'
printf '\n== Search for read/readMPS entry points ==\n'
rg -n "readMPS|read\(" java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming -g '*.java'Repository: NVIDIA/cuopt
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "enum ConstraintSense|fromNative\\(" java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming -g '*.java'
sed -n '1,220p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/ConstraintSense.java | nl -baRepository: NVIDIA/cuopt
Length of output: 1935
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Problem.java around the cited lines ==\n'
sed -n '268,312p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java
printf '\n== ConstraintSense.java ==\n'
sed -n '1,120p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/ConstraintSense.java
printf '\n== DataModel.java createRangedProblem and read path ==\n'
sed -n '60,120p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java
sed -n '440,520p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java
printf '\n== Problem.java read entry points ==\n'
rg -n "readMPS|read\\(" java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.javaRepository: NVIDIA/cuopt
Length of output: 9648
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Problem.read / readMPS implementation ==\n'
sed -n '235,390p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java
printf '\n== Constraint bound getters in DataModel ==\n'
rg -n "getConstraintLowerBounds|getConstraintUpperBounds|getConstraintSense|getConstraintRHS|constraintLowerBounds|constraintUpperBounds" java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.java
printf '\n== DataModel constraint metadata methods around getters ==\n'
sed -n '180,260p' java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/DataModel.javaRepository: NVIDIA/cuopt
Length of output: 10118
Handle ranged rows instead of flattening them. Rows whose lower and upper bounds differ are rebuilt as a single-sided LE/GE/EQ constraint from rhs[row], so read/readMPS lose the original interval and can’t round-trip RANGES-style input. Add a range-aware path or reject those rows explicitly.
🤖 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
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java`
around lines 278 - 310, The constraint reconstruction logic in
Problem.read/Problem.readMPS is flattening ranged rows into a single LE/GE/EQ
constraint based on rhs[row], which drops the original interval bounds. Update
the row handling around the ConstraintSense selection and switch so rows with
differing constraintLowerBounds[row] and constraintUpperBounds[row] are either
preserved via a range-aware representation or explicitly rejected with a clear
error instead of being converted. Keep the existing single-sided path for
non-ranged rows and make the behavior around rowNames and addConstraint
consistent with the chosen handling.
| public void updateConstraint(Constraint constraint, Map<Variable, Double> coefficients, Double rhs) { | ||
| if (!constraints.contains(constraint)) { | ||
| throw new IllegalArgumentException("Constraint does not belong to this problem"); | ||
| } | ||
| if (constraint.isQuadratic()) { | ||
| throw new IllegalArgumentException("updateConstraint applies to linear constraints only"); | ||
| } | ||
| LinearExpression expression = new LinearExpression(); | ||
| for (Map.Entry<Variable, Double> entry : constraint.getLinearExpression().getTerms().entrySet()) { | ||
| expression = expression.plus(entry.getKey(), entry.getValue()); | ||
| } | ||
| if (coefficients != null) { | ||
| for (Map.Entry<Variable, Double> entry : coefficients.entrySet()) { | ||
| expression = expression.plus(entry.getKey(), entry.getValue() - constraint.getCoefficient(entry.getKey())); | ||
| } | ||
| } | ||
| constraint.updateLinearExpression(expression); | ||
| if (rhs != null) { | ||
| constraint.updateRHS(rhs); | ||
| } | ||
| resetSolvedValues(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
updateConstraint doesn't validate that new coefficient variables belong to this Problem, unlike updateObjective.
updateObjective explicitly throws IllegalArgumentException when a coefficient's key isn't in variables (line 419-421), but updateConstraint's coefficients loop (lines 404-408) has no equivalent check. A Variable from a different Problem instance could be merged into this constraint's LinearExpression, later producing a wrong column index in buildLinearConstraintMatrix() (via entry.getKey().getIndex()) that silently corrupts the CSR matrix passed to the native solver.
🤖 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
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java`
around lines 393 - 414, `updateConstraint` is missing the same ownership
validation that `updateObjective` uses for coefficient variables. In
`Problem.updateConstraint`, add a check in the `coefficients` loop to নিশ্চিত
each `Variable` key belongs to this `Problem`’s `variables` set and throw
`IllegalArgumentException` if not, before merging it into the
`LinearExpression`; keep the rest of the update flow in `updateConstraint`
unchanged.
| public void updateObjective(Map<Variable, Double> coefficients, Double constant, ObjectiveSense sense) { | ||
| if (coefficients != null) { | ||
| for (Map.Entry<Variable, Double> entry : coefficients.entrySet()) { | ||
| if (!variables.contains(entry.getKey())) { | ||
| throw new IllegalArgumentException("Objective variable does not belong to this problem"); | ||
| } | ||
| entry.getKey().setObjectiveCoefficient(entry.getValue()); | ||
| } | ||
| if (objectiveSet) { | ||
| LinearExpression updated = | ||
| LinearExpression.ofConstant(constant == null ? linearObjective.getConstant() : constant); | ||
| for (Map.Entry<Variable, Double> entry : linearObjective.getTerms().entrySet()) { | ||
| updated = updated.plus(entry.getKey(), coefficients.getOrDefault(entry.getKey(), entry.getValue())); | ||
| } | ||
| for (Map.Entry<Variable, Double> entry : coefficients.entrySet()) { | ||
| if (!linearObjective.getTerms().containsKey(entry.getKey())) { | ||
| updated = updated.plus(entry.getKey(), entry.getValue()); | ||
| } | ||
| } | ||
| linearObjective = updated; | ||
| if (quadraticObjective != null) { | ||
| QuadraticExpression updatedQuadratic = | ||
| new QuadraticExpression().constant(linearObjective.getConstant()); | ||
| for (Map.Entry<Variable, Double> entry : linearObjective.getTerms().entrySet()) { | ||
| updatedQuadratic = updatedQuadratic.plus(entry.getKey(), entry.getValue()); | ||
| } | ||
| for (QuadraticExpression.QuadraticTerm term : quadraticObjective.getQuadraticTerms()) { | ||
| updatedQuadratic = | ||
| updatedQuadratic.plus(term.getFirst(), term.getSecond(), term.getCoefficient()); | ||
| } | ||
| quadraticObjective = updatedQuadratic; | ||
| } | ||
| } else if (constant != null) { | ||
| linearObjective = LinearExpression.ofConstant(constant); | ||
| for (Variable variable : variables) { | ||
| if (variable.getObjectiveCoefficient() != 0.0) { | ||
| linearObjective = linearObjective.plus(variable, variable.getObjectiveCoefficient()); | ||
| } | ||
| } | ||
| objectiveSet = true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
updateObjective leaves getObjective() stale when only per-variable coefficients are updated before any objective is set.
When coefficients != null, objectiveSet is false, and constant == null, the loop updates each Variable's own objectiveCoefficient (line 422), but neither linearObjective nor objectiveSet are touched — there's no corresponding branch for this case (only the objectiveSet == true branch and the objectiveSet == false && constant != null branch handle it). Since objectiveCoefficients() (used for solving, line 644) falls back to reading variable.getObjectiveCoefficient() when !objectiveSet, the actual solve is unaffected. However, getObjective() (line 470) returns the stale/empty linearObjective field, so introspection via problem.getObjective() will not reflect the coefficients that were just set — a real observable inconsistency for API consumers.
🤖 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
`@java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.java`
around lines 416 - 456, The updateObjective method in Problem is leaving the
linearObjective field and objectiveSet flag unchanged when coefficients are
provided before any objective has been initialized and constant is null, so
getObjective() stays stale. Add a branch in updateObjective that rebuilds
linearObjective from the current variable objective coefficients (or otherwise
synchronizes linearObjective) and marks objectiveSet true for this
coefficients-only path, while keeping the existing ObjectiveSense handling and
compatibility with objectiveCoefficients() and getObjective().
| void mip_get_solution_callback(const cuopt_float_t* solution, | ||
| const cuopt_float_t* objective_value, | ||
| const cuopt_float_t* solution_bound, | ||
| void* user_data) | ||
| { | ||
| auto* context = static_cast<java_callback_context_t*>(user_data); | ||
| if (context == nullptr || context->callback == nullptr) { return; } | ||
|
|
||
| bool detach = false; | ||
| JNIEnv* env = get_callback_env(detach); | ||
| if (env == nullptr) { return; } | ||
|
|
||
| jclass cls = env->GetObjectClass(context->callback); | ||
| jmethodID method = | ||
| env->GetMethodID(cls, "onSolution", "([DDDLjava/lang/Object;)V"); | ||
| if (method != nullptr) { | ||
| std::vector<cuopt_float_t> values(solution, solution + context->num_variables); | ||
| jdoubleArray solution_array = to_double_array(env, values); | ||
| env->CallVoidMethod(context->callback, | ||
| method, | ||
| solution_array, | ||
| static_cast<jdouble>(*objective_value), | ||
| static_cast<jdouble>(*solution_bound), | ||
| context->user_data); | ||
| env->DeleteLocalRef(solution_array); | ||
| } | ||
|
|
||
| if (detach) { g_jvm->DetachCurrentThread(); } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Local JNI ref leak in mip_get_solution_callback hot path.
GetObjectClass(context->callback) at Line 273 creates a local ref that is never released. If the callback is invoked repeatedly from the same already-attached thread within a single solve() call (i.e., detach == false, so no automatic cleanup on return), these refs accumulate in one native frame and can eventually overflow the JVM's local-reference table, crashing the process on long MIP solves.
🔒 Proposed fix
jclass cls = env->GetObjectClass(context->callback);
jmethodID method =
env->GetMethodID(cls, "onSolution", "([DDDLjava/lang/Object;)V");
if (method != nullptr) {
std::vector<cuopt_float_t> values(solution, solution + context->num_variables);
jdoubleArray solution_array = to_double_array(env, values);
env->CallVoidMethod(context->callback,
method,
solution_array,
static_cast<jdouble>(*objective_value),
static_cast<jdouble>(*solution_bound),
context->user_data);
env->DeleteLocalRef(solution_array);
}
+ env->DeleteLocalRef(cls);📝 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.
| void mip_get_solution_callback(const cuopt_float_t* solution, | |
| const cuopt_float_t* objective_value, | |
| const cuopt_float_t* solution_bound, | |
| void* user_data) | |
| { | |
| auto* context = static_cast<java_callback_context_t*>(user_data); | |
| if (context == nullptr || context->callback == nullptr) { return; } | |
| bool detach = false; | |
| JNIEnv* env = get_callback_env(detach); | |
| if (env == nullptr) { return; } | |
| jclass cls = env->GetObjectClass(context->callback); | |
| jmethodID method = | |
| env->GetMethodID(cls, "onSolution", "([DDDLjava/lang/Object;)V"); | |
| if (method != nullptr) { | |
| std::vector<cuopt_float_t> values(solution, solution + context->num_variables); | |
| jdoubleArray solution_array = to_double_array(env, values); | |
| env->CallVoidMethod(context->callback, | |
| method, | |
| solution_array, | |
| static_cast<jdouble>(*objective_value), | |
| static_cast<jdouble>(*solution_bound), | |
| context->user_data); | |
| env->DeleteLocalRef(solution_array); | |
| } | |
| if (detach) { g_jvm->DetachCurrentThread(); } | |
| } | |
| void mip_get_solution_callback(const cuopt_float_t* solution, | |
| const cuopt_float_t* objective_value, | |
| const cuopt_float_t* solution_bound, | |
| void* user_data) | |
| { | |
| auto* context = static_cast<java_callback_context_t*>(user_data); | |
| if (context == nullptr || context->callback == nullptr) { return; } | |
| bool detach = false; | |
| JNIEnv* env = get_callback_env(detach); | |
| if (env == nullptr) { return; } | |
| jclass cls = env->GetObjectClass(context->callback); | |
| jmethodID method = | |
| env->GetMethodID(cls, "onSolution", "([DDDLjava/lang/Object;)V"); | |
| if (method != nullptr) { | |
| std::vector<cuopt_float_t> values(solution, solution + context->num_variables); | |
| jdoubleArray solution_array = to_double_array(env, values); | |
| env->CallVoidMethod(context->callback, | |
| method, | |
| solution_array, | |
| static_cast<jdouble>(*objective_value), | |
| static_cast<jdouble>(*solution_bound), | |
| context->user_data); | |
| env->DeleteLocalRef(solution_array); | |
| } | |
| env->DeleteLocalRef(cls); | |
| if (detach) { g_jvm->DetachCurrentThread(); } | |
| } |
🤖 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 `@java/cuopt/src/main/native/cuopt_jni.cpp` around lines 261 - 289, The JNI
callback in mip_get_solution_callback leaks a local reference because
GetObjectClass(context->callback) is never paired with a DeleteLocalRef. Store
the jclass returned by GetObjectClass, use it for GetMethodID, and delete that
local ref before returning; keep the rest of the callback flow in cuopt_jni.cpp
unchanged.
| void mip_set_solution_callback(cuopt_float_t* solution, | ||
| cuopt_float_t* objective_value, | ||
| const cuopt_float_t* solution_bound, | ||
| void* user_data) | ||
| { | ||
| auto* context = static_cast<java_callback_context_t*>(user_data); | ||
| if (context == nullptr || context->callback == nullptr) { return; } | ||
|
|
||
| bool detach = false; | ||
| JNIEnv* env = get_callback_env(detach); | ||
| if (env == nullptr) { return; } | ||
|
|
||
| jclass cls = env->GetObjectClass(context->callback); | ||
| jmethodID method = env->GetMethodID( | ||
| cls, "getSolution", "(DLjava/lang/Object;)Lcom/nvidia/cuopt/mathematicalprogramming/MIPCallbackSolution;"); | ||
| if (method != nullptr) { | ||
| jobject callback_solution = | ||
| env->CallObjectMethod(context->callback, method, static_cast<jdouble>(*solution_bound), context->user_data); | ||
| if (callback_solution != nullptr) { | ||
| jclass result_cls = env->GetObjectClass(callback_solution); | ||
| jfieldID solution_field = env->GetFieldID(result_cls, "solution", "[D"); | ||
| jfieldID objective_field = env->GetFieldID(result_cls, "objectiveValue", "D"); | ||
| if (solution_field != nullptr && objective_field != nullptr) { | ||
| auto solution_array = | ||
| static_cast<jdoubleArray>(env->GetObjectField(callback_solution, solution_field)); | ||
| const auto values = get_double_array(env, solution_array); | ||
| if (values.size() == static_cast<size_t>(context->num_variables)) { | ||
| std::memcpy(solution, values.data(), values.size() * sizeof(cuopt_float_t)); | ||
| *objective_value = | ||
| static_cast<cuopt_float_t>(env->GetDoubleField(callback_solution, objective_field)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (detach) { g_jvm->DetachCurrentThread(); } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Multiple local JNI ref leaks and silent failure on size mismatch in mip_set_solution_callback.
cls (Line 303), callback_solution (Line 307-308), result_cls (Line 310), and the jdoubleArray returned by GetObjectField (Line 314-315) are all local refs that are never deleted — same reference-table-overflow risk as the sibling callback above, but worse since four refs leak per call in what is a solver hot path (repeatedly invoked to poll for injected incumbents).
Separately, when values.size() != context->num_variables (Line 317), the function silently does nothing — no exception, no log — making a user-side sizing bug in the callback impossible to diagnose.
🔒 Proposed fix
jclass cls = env->GetObjectClass(context->callback);
jmethodID method = env->GetMethodID(
cls, "getSolution", "(DLjava/lang/Object;)Lcom/nvidia/cuopt/mathematicalprogramming/MIPCallbackSolution;");
if (method != nullptr) {
jobject callback_solution =
env->CallObjectMethod(context->callback, method, static_cast<jdouble>(*solution_bound), context->user_data);
if (callback_solution != nullptr) {
jclass result_cls = env->GetObjectClass(callback_solution);
jfieldID solution_field = env->GetFieldID(result_cls, "solution", "[D");
jfieldID objective_field = env->GetFieldID(result_cls, "objectiveValue", "D");
if (solution_field != nullptr && objective_field != nullptr) {
auto solution_array =
static_cast<jdoubleArray>(env->GetObjectField(callback_solution, solution_field));
const auto values = get_double_array(env, solution_array);
if (values.size() == static_cast<size_t>(context->num_variables)) {
std::memcpy(solution, values.data(), values.size() * sizeof(cuopt_float_t));
*objective_value =
static_cast<cuopt_float_t>(env->GetDoubleField(callback_solution, objective_field));
}
+ env->DeleteLocalRef(solution_array);
}
+ env->DeleteLocalRef(result_cls);
+ env->DeleteLocalRef(callback_solution);
}
}
+ env->DeleteLocalRef(cls);🤖 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 `@java/cuopt/src/main/native/cuopt_jni.cpp` around lines 291 - 327,
mip_set_solution_callback is leaking several JNI local references and silently
ignoring a solution-size mismatch. In this callback, delete the local refs
created by GetObjectClass, CallObjectMethod, GetObjectClass on
callback_solution, and GetObjectField for the solution array before returning,
ideally using a consistent cleanup path in mip_set_solution_callback. Also add
explicit handling for the values.size() != context->num_variables case in the
same function: surface an error via the existing JNI/error-reporting mechanism
or logging instead of doing nothing, so callback sizing bugs are diagnosable.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java (1)
87-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
problemis never closed, and the objective relies on an implicit default sense.Every other test in this file wraps
Problemin try-with-resources; hereproblemis created via plain assignment and never closed, leaking whatever native/Java-side stateProblem.close()releases. Separately, nosetObjectivecall is made — the test relies onaddVariable's side-channel objective coefficient plus whateverObjectiveSenseProblemdefaults to, which is fragile if that default ever changes.🔧 Proposed fix
- Problem problem = new Problem("integer"); - Variable x = problem.addVariable(0, 10, 1.0, VariableType.INTEGER, "x"); - problem.addConstraint(LinearExpression.of(x).ge(1.0)); - - try (SolverSettings settings = new SolverSettings().setSetting(CuOptConstants.CUOPT_TIME_LIMIT, 10.0); - Solution solution = problem.solve(settings)) { - assertTrue(solution.isMIP()); - assertEquals(TerminationStatus.OPTIMAL, solution.getTerminationStatus()); - assertEquals(1.0, x.getValue(), 1e-6); - assertDoesNotThrow(solution::getMIPStats); - assertThrows(IllegalStateException.class, solution::getDualSolution); - solution.close(); - solution.close(); - } + try (Problem problem = new Problem("integer")) { + Variable x = problem.addVariable(0, 10, 1.0, VariableType.INTEGER, "x"); + problem.addConstraint(LinearExpression.of(x).ge(1.0)); + problem.setObjective(x, ObjectiveSense.MINIMIZE); + + try (SolverSettings settings = new SolverSettings().setSetting(CuOptConstants.CUOPT_TIME_LIMIT, 10.0); + Solution solution = problem.solve(settings)) { + assertTrue(solution.isMIP()); + assertEquals(TerminationStatus.OPTIMAL, solution.getTerminationStatus()); + assertEquals(1.0, x.getValue(), 1e-6); + assertDoesNotThrow(solution::getMIPStats); + assertThrows(IllegalStateException.class, solution::getDualSolution); + solution.close(); + solution.close(); + } + }🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java` around lines 87 - 105, Wrap the Problem instance in try-with-resources in solvesProblemApiMIPAndLifecycleCloseIsIdempotent so it is closed like the other tests, and make the objective explicit instead of relying on the default sense. Use the existing Problem, SolverSettings, and solve flow to set the objective with the intended ObjectiveSense before calling solve, then keep the current MIP and close-idempotence assertions unchanged.
🧹 Nitpick comments (3)
java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemIntegrationTest.java (3)
31-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEarly return for QP cases leaves two branches permanently dead.
verify()returns before callingcreateSettings/assertSolutionwheneverhasQuadraticObjective()is true. Since the only case exercisinghasQuadraticConstraint()(qp_diagonal_objective) also sets a quadratic objective, this means:
- the
else if (testCase.hasQuadraticObjective())branch increateSettings(Lines 147-148) is unreachable, and- the quadratic-constraint feasibility check in
CaseSpec.assertFeasible(Lines 707-729) is unreachable.As a result, quadratic-constraint satisfaction is only checked structurally (construction), never against actual solved primal values. Consider either removing the now-dead branches, or exercising them with a solvable QP+quadratic-constraint case (or a dedicated lower-iteration-limit-tolerant assertion) so this coverage isn't silently lost.
🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemIntegrationTest.java` around lines 31 - 47, The early return in verify() for hasQuadraticObjective() makes the quadratic-objective branch in createSettings() and the quadratic-constraint feasibility path in CaseSpec.assertFeasible() unreachable. Update verify(), createSettings(), and/or CaseSpec so quadratic-constraint behavior is actually exercised by a solvable QP case, or remove the dead branches if they are no longer needed; use the existing symbols hasQuadraticObjective(), hasQuadraticConstraint(), createSettings(), and assertFeasible() to locate the affected logic.
162-180: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
assumeNativeLibrary/assumeCudaDriverAvailableacross test classes.Both helpers are copy-pasted verbatim from
NativeIntegrationTest, and they've already drifted slightly: this copy addsredirectErrorStream(true)to thenvidia-smiprocess whileNativeIntegrationTest's copy doesn't. Consider extracting a shared test-support base class/utility to avoid future divergence.🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemIntegrationTest.java` around lines 162 - 180, The helper methods assumeNativeLibrary and assumeCudaDriverAvailable are duplicated from NativeIntegrationTest and have already started to diverge, so extract them into a shared test-support base class or utility and update ProblemIntegrationTest to call the shared implementation. Keep the native library check and the CUDA driver probe in one place so both test classes use the same behavior, and ensure the nvidia-smi ProcessBuilder setup is centralized in the shared method.
612-618: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConstraint-count formula assumes ranged rows always produce exactly 2 constraints.
linearProblemConstraintCount()returnsnumConstraints * 2for any ranged case, butcreateProblem()(Lines 499-505) only adds a lower/upper row when the corresponding bound is!Double.isInfinite(...). No current case has an actually-infinite ranged bound, so this passes today, but the formula and the construction logic will silently disagree the moment such a case is added. Separately,qp_diagonal_objective's upper bound is1.0e20— a large but finite double, notDouble.POSITIVE_INFINITY— so it's unclear whether that value was meant to represent "effectively unbounded" (in which case it should useDouble.POSITIVE_INFINITYto actually hit the skip branch) or a deliberately large finite bound.🔧 Suggested fix to make the count formula match the construction logic
- private int linearProblemConstraintCount() { - return isRanged() ? numConstraints * 2 : numConstraints; - } + private int linearProblemConstraintCount() { + if (!isRanged()) { + return numConstraints; + } + int count = 0; + for (int row = 0; row < numConstraints; row++) { + if (!Double.isInfinite(constraintLowerBounds[row])) { + count++; + } + if (!Double.isInfinite(constraintUpperBounds[row])) { + count++; + } + } + return count; + }🤖 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 `@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemIntegrationTest.java` around lines 612 - 618, The constraint-count logic in linearProblemConstraintCount() is assuming every ranged row always contributes two constraints, but createProblem() only adds each side when the bound is not infinite. Update the counting helper to mirror the actual construction in createProblem() by counting lower and upper contributions based on the same bound checks, and review the qp_diagonal_objective upper bound to make sure it intentionally uses a large finite value or is changed to a true unbounded value if that was the intent.
🤖 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.
Outside diff comments:
In
`@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.java`:
- Around line 87-105: Wrap the Problem instance in try-with-resources in
solvesProblemApiMIPAndLifecycleCloseIsIdempotent so it is closed like the other
tests, and make the objective explicit instead of relying on the default sense.
Use the existing Problem, SolverSettings, and solve flow to set the objective
with the intended ObjectiveSense before calling solve, then keep the current MIP
and close-idempotence assertions unchanged.
---
Nitpick comments:
In
`@java/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemIntegrationTest.java`:
- Around line 31-47: The early return in verify() for hasQuadraticObjective()
makes the quadratic-objective branch in createSettings() and the
quadratic-constraint feasibility path in CaseSpec.assertFeasible() unreachable.
Update verify(), createSettings(), and/or CaseSpec so quadratic-constraint
behavior is actually exercised by a solvable QP case, or remove the dead
branches if they are no longer needed; use the existing symbols
hasQuadraticObjective(), hasQuadraticConstraint(), createSettings(), and
assertFeasible() to locate the affected logic.
- Around line 162-180: The helper methods assumeNativeLibrary and
assumeCudaDriverAvailable are duplicated from NativeIntegrationTest and have
already started to diverge, so extract them into a shared test-support base
class or utility and update ProblemIntegrationTest to call the shared
implementation. Keep the native library check and the CUDA driver probe in one
place so both test classes use the same behavior, and ensure the nvidia-smi
ProcessBuilder setup is centralized in the shared method.
- Around line 612-618: The constraint-count logic in
linearProblemConstraintCount() is assuming every ranged row always contributes
two constraints, but createProblem() only adds each side when the bound is not
infinite. Update the counting helper to mirror the actual construction in
createProblem() by counting lower and upper contributions based on the same
bound checks, and review the qp_diagonal_objective upper bound to make sure it
intentionally uses a large finite value or is changed to a true unbounded value
if that was the intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 594299eb-4f80-4963-96fe-fd88c2b6acd4
📒 Files selected for processing (12)
.github/workflows/pr.yamldocs/cuopt/source/cuopt-java/convex/convex-api.rstdocs/cuopt/source/cuopt-java/convex/convex-examples.rstdocs/cuopt/source/cuopt-java/convex/index.rstdocs/cuopt/source/cuopt-java/quick-start.rstjava/cuopt/TESTS.mdjava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/NativeCuOpt.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/NativeProblem.javajava/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/Problem.javajava/cuopt/src/main/native/cuopt_jni.cppjava/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/NativeIntegrationTest.javajava/cuopt/src/test/java/com/nvidia/cuopt/mathematicalprogramming/ProblemIntegrationTest.java
💤 Files with no reviewable changes (3)
- docs/cuopt/source/cuopt-java/convex/convex-examples.rst
- java/cuopt/src/main/java/com/nvidia/cuopt/mathematicalprogramming/NativeCuOpt.java
- java/cuopt/src/main/native/cuopt_jni.cpp
✅ Files skipped from review due to trivial changes (2)
- java/cuopt/TESTS.md
- docs/cuopt/source/cuopt-java/quick-start.rst
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/cuopt/source/cuopt-java/convex/index.rst
- .github/workflows/pr.yaml
4 participants
Description
Added Java bindings at parity with LP/MIP/QP along with some basic tests for functionality. Currently it is fully decoupled from other build components and must be built on its own.
Checklist