Java bindings for LP/MIP/QP - #1524
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesJava bindings
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 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.
|
|
||
| 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.
| @@ -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
|
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.
| } | ||
|
|
||
| /** 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.)
| 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.
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.
| * @return A status code indicating success or failure. Returns CUOPT_INVALID_ARGUMENT if the | ||
| * solution is a MIP solution. | ||
| */ | ||
| cuopt_int_t cuOptGetLPSolverStats(cuOptSolution solution, |
There was a problem hiding this comment.
I believe these stats are already exposed through attributes. And if not, they should be. Rather than adding a new C API call.
| * @return A status code indicating success or failure. Returns CUOPT_INVALID_ARGUMENT if the | ||
| * solution is an LP solution. | ||
| */ | ||
| cuopt_int_t cuOptGetMIPSolverStats(cuOptSolution solution, |
There was a problem hiding this comment.
See previous comment about trying to access these via attributes.
| * @return A status code indicating success or failure. Returns CUOPT_INVALID_ARGUMENT if the file | ||
| * is missing or malformed. | ||
| */ | ||
| cuopt_int_t cuOptLoadParametersFromFile(cuOptSolverSettings settings, const char* path); |
There was a problem hiding this comment.
What's the use case for these parameter API calls?
| * | ||
| * @param[in] path - Path of the file to write. | ||
| * | ||
| * @param[in] hyperparameters_only - Non-zero to write only hyperparameters, zero to write every |
There was a problem hiding this comment.
We should not expose hyperparameters via a public API. Parameters are marked as hyperparameters, precisely because we do not want to broadly advertise them.
| * - ``solve()`` / ``solve(SolverSettings)`` | ||
| - Solve the problem and return a ``Solution``. | ||
| * - ``getCSR()`` / ``getQCSR()`` | ||
| - Inspect the linear or quadratic objective matrix in CSR form. |
There was a problem hiding this comment.
What is a linear objective matrix? Does this return the constraint matrix? Or the objective?
I'd suggest renaming these functions to make what they do more clear.
| - Solve the problem and return a ``Solution``. | ||
| * - ``getCSR()`` / ``getQCSR()`` | ||
| - Inspect the linear or quadratic objective matrix in CSR form. | ||
| * - ``writeMPS(String)`` / ``read(String)`` / ``readMPS(String)`` |
There was a problem hiding this comment.
writeMPS and readMPS are deprecated. We should only support read and write and then dispatch based on file type.
|
|
||
| ``Problem`` also exposes ``getVariables``, ``getVariable``, ``getConstraints``, | ||
| ``getConstraint``, ``getNumVariables``, ``getNumConstraints``, | ||
| ``getNumNonZeros``, ``isMIP``, ``isSolved``, ``getStatus``, |
There was a problem hiding this comment.
Remove isSolved, users should use getStatus to determine the status. of the solves.
Also instead of having individual accessor functions, we should probably move to an attribute based model. See changes to C API.
| ``ge``, and ``eq``. Comparisons return a ``Constraint``. | ||
|
|
||
| ``QuadraticExpression`` supports quadratic terms through | ||
| ``QuadraticExpression.of(first, second, coefficient)`` and the same fluent |
There was a problem hiding this comment.
Nit: what does "fluent" mean in this context? Maybe reword for clarity?
There was a problem hiding this comment.
FWIW fluent is a standard term for describing APIs: https://en.wikipedia.org/wiki/Fluent_interface
| ``le`` and ``ge`` methods return quadratic constraints. It does not expose an | ||
| ``eq`` method because equality quadratic constraints are not supported. | ||
|
|
||
| Both expression classes implement ``ObjectiveExpression``, which exposes the |
There was a problem hiding this comment.
Why do we need an ObjectiveExpression? We should be able to use LinearExpressions and QuadraticExpression for both objectives and constraints.
| ``VariableType.SEMI_CONTINUOUS``; and | ||
| * ``ProblemCategory`` for the native problem classification. | ||
|
|
||
| ``ProblemCategory.IP`` is deprecated. Java normalizes the legacy native IP |
There was a problem hiding this comment.
No need to include or mention deprecated API. Remove all references to IP.
| * ``ConstraintSense.LE``, ``ConstraintSense.GE``, and ``ConstraintSense.EQ``; | ||
| * ``VariableType.CONTINUOUS``, ``VariableType.INTEGER``, and | ||
| ``VariableType.SEMI_CONTINUOUS``; and | ||
| * ``ProblemCategory`` for the native problem classification. |
There was a problem hiding this comment.
What is ProblemCategory used for? Could we get away with just isMIP()?
|
|
||
| ``Constraint`` provides ``getSense``, ``getRHS``, ``getCoefficient``, | ||
| ``getLinearExpression``, ``getQuadraticExpression``, ``isQuadratic``, | ||
| ``computeSlack``, ``getSlack``, and ``getDualValue``. |
There was a problem hiding this comment.
Is computeSlack needed. Let's provide the slack inside the C++ engine.
|
|
||
| ``Solution`` implements ``AutoCloseable`` and exposes: | ||
|
|
||
| * ``getPrimalSolution``, ``getDualSolution``, and ``getReducedCost``; |
There was a problem hiding this comment.
I'm not sure we want getPrimalSolution, getDualSolution and getReducedCost? These should be accessed via the constraints and variables.
| * ``getSolveTime`` and ``getProblemCategory``; and | ||
| * ``getVars`` when variable names are available. | ||
|
|
||
| LP solutions additionally expose ``getLPStats``. ``LPStats`` contains primal |
There was a problem hiding this comment.
Let's add attributes instead of stats.
| MPS and Errors | ||
| -------------- | ||
|
|
||
| ``Problem.read`` and ``Problem.readMPS`` support MPS/QPS parsing, including a |
|
@chris-maes thanks for the review — replying to the C API comments in one place since they're connected. Two facts first, because I think one of them changes the pictureThere is no solution-scoped attribute API today. All five attribute getters are None of these stats were reachable by any route before this PR. Checking Why we think they're worth supportingThe data already exists on
Two more general points. Python already exposes all of this by binding to the C++ structs through Cython, so today the C ABI is a second-class path — that asymmetry is what #1202 was filed about. And users arriving from Gurobi or CPLEX expect Where we agree, and what we'd proposeAttributes are the right shape for these. All eleven are scalar ints and floats, which is exactly what the attribute model is for. There's also a maintenance argument specific to the bindings: cuOpt already generates What we'd suggest is a shape-based rule rather than attributes everywhere, since it's the rule the C API already follows:
The third case is why Happy to convert the two bulk Conceded
The One question backOn the parameter calls ( |
Reworks the C API surface this PR adds, following review. Seven functions become two. Statistics are now read with cuOptGetSolutionIntAttribute and cuOptGetSolutionFloatAttribute using CUOPT_SOLUTION_ATTR_* selectors, in place of cuOptGetLPSolverStats and cuOptGetMIPSolverStats. This matches how problem data is already read, and it means a future statistic is a new constant rather than a new exported symbol, so existing callers keep working without a relink. It also costs the bindings nothing: CuOptConstants.java is generated from constants.h, so a new selector reaches Java with no hand-written code, where a new function needs a JNI entry point, a native declaration, and a wrapper. Selectors are numbered in their own range so a problem selector passed to a solution accessor is rejected rather than silently read, and LP selectors require an LP solution while MIP selectors require a MIP solution. cuOptSolutionIsMIP is removed as redundant. Solution is constructed with the category of the problem it was solved for, so it already knows whether it came from the MIP solver, and a C caller holding the problem handle has cuOptIsMIP. The four parameter-file and parameter-enumeration entry points are removed rather than promoted. Nothing needs them to build, solve, or read a solution, and one of them exposed the hyperparameters concept through a public header, which it should not. The capability is recorded in NVIDIA#1705 so it is deferred rather than lost. On the Java side this drops getSolverSettingNames, toDict, and the settings-file methods; setOptimalityTolerance now names the six LP tolerances explicitly instead of discovering them by matching parameter names, which is also less fragile than the previous filter. Verified: 69/69 C API gtests, including new tests that the accessors reject a mismatched value type, the wrong solver's selectors, unknown selectors, and null arguments; 26/26 Java tests; all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
clang-format widens the value column to the longest macro name in the block. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 5972293 |
|
IMO changes to the C API should be in a separate PR, not combined with a 7000 line diff here. |
NVIDIA#1705 tracked the removed parameter enumeration and settings-file entry points as deferred work. mlubin pointed out that bindings can be generated from constants.h, which makes runtime enumeration unnecessary. That holds structurally, not just in practice: every one of the 117 parameter registrations in solver_settings.cu is written as a CUOPT_* macro from constants.h, and none of the 109 distinct registered names is absent from the header. The list get_parameter_names() returns is therefore derived from the same header the bindings generate from, and cannot contain anything extra. Loading and dumping a settings file is expressible on top of cuOptSetParameter and cuOptGetParameter, so that half needs no new C surface either. NVIDIA#1705 is closed. Point the README at the reasoning rather than describing the capability as pending. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 4eb70d8 |
|
@mlubin agreed — the C API changes are now #1715, off This PR is stacked on it. The Two related issues were also split out of the earlier discussion, so each is reviewable on its own:
#1705 is closed: you were right that runtime parameter enumeration is unnecessary. Every one of the 117 parameter registrations is written as a |
Documentation wording and the scoped solution guard in the C API tests, kept identical to the branch behind NVIDIA#1715 so this stack stays consistent until that PR merges and these files drop out of this diff. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test d981aec |
Keeps the cpp/ files in this stack identical to the branch behind NVIDIA#1715. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 6a04604 |
5 participants
Description
Java bindings at parity with the LP/MIP/QP surface, plus tests and docs.
The bindings are hand-written JNI over the public C API. Contributes to #1535 and #860.
Native surface
The bindings originally carried
cuopt_java_native_api.{hpp,cpp}, a Java-local shim declaring seven entry points thatlibcuoptdid not export. It reimplemented the memory layout of the opaquecuOptSolverSettingshandle in order to reach the underlyingsolver_settings_t, so any change tosolver_settings_handle_twould have silently corrupted it.All seven are now part of the public C API, implemented in
cuopt_c.cppbeside the functions they belong with:cuOptLoadParametersFromFile,cuOptDumpParametersToFile,cuOptGetNumSolverParameters,cuOptGetSolverParameterNamecuOptSolutionIsMIP,cuOptGetLPSolverStats,cuOptGetMIPSolverStatsThis covers the fields requested in #1202. Note for reviewers: #1202 proposed 11 individual getters and explicitly weighed that against a bulk call. This PR ships the bulk shape (two functions with nullable out-pointers), inherited from the original shim. If the individual-getter shape is preferred, say so and I will split them — flagging rather than silently closing #1202.
Build and CI
./build.sh javaand./build.sh java --run-java-tests— an opt-in target, matching how cuvs exposes its Java build. Preferscpp/buildso it works without--install, and falls back to the conda prefix, which is what CI uses.ci/build_java.shcallsbuild.shrather than duplicating the invocation.java/cuopt/scripts/test.shcarries aCUOPT_PRELOAD_LIBSescape hatch to work around [BUG] Build-tree libcuopt.so resolves librmm from the conda prefix instead of its own _deps, failing at dlopen #1704; it can be removed once that is fixed.release/26.08shared-workflow pins to@mainwithci-conda:26.10-latest, and the redundantcompute-matrixjob was dropped.Guard against JNI symbol drift
The bindings are hand-written, so a Java
static nativedeclaration and itsJava_com_nvidia_..._nameentry point can drift apart with nothing to catch it. JNI resolves methods lazily, so the library still loads and the failure only appears when something calls the method.java/cuopt/scripts/check_jni_symbols.shdiffs the prototypesjavac -hderives from the Java sources against the symbols the built library exports, and fails on a mismatch in either direction. It runs after every native build. It reads the built artifact rather than parsing source, so the macro-generated entry points need no special casing.Packaging
Version is
26.10.0— Maven has no notion of the zero-padded RAPIDS patch field, and26.10.00would sort oddly against a later26.10.1.update-version.shderives it. Sources and javadoc jars are attached, and the POM carries theurl,licenses,scm, anddevelopersmetadata Maven Central requires.Known gaps
These are deliberate, not oversights.
pdlp/cuopt_c_internal.hppfor the problem path — name setters, quadratic objective and quadratic constraint getters, and length reporting for duals and reduced costs. That coupleslibcuopt_jni.soto a specificlibcuoptbuild rather than to a stable ABI, which matters before this is published as a binary artifact. Tracked in [FEA] Expand cuopt_c.h with problem-model accessors for non-Python language bindings #1703.Checklist