Java bindings for LP, MIP and 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.
|
/ok to test dc8b4b0 |
The public enum constant is gone, so the paragraph documented an API that no longer exists. The only remaining mention is a comment in ProblemCategory explaining why the native value is folded into MIP, which has to stay while the engine still reports it. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
@chris-maes before I work through the rest of your review, I'd like to check the direction, because I think the remaining comments share one underlying question and answering it settles most of them at once. What's already done
On "fluent": @mlubin noted it is standard terminology for this style of API, so I have left the wording unless you would still prefer it changed. The questionThe Java surface was built for parity with the Python API — that was its design goal, and it is why the shape looks the way it does. Your review is asking it to be consistent with the C API instead, which took the attribute direction in #1549. Both are reasonable, but they point different ways, and five of the six items you have asked to remove exist in the Python API today:
So the choice is roughly: (a) Java aligns with the C API and diverges from Python. Java becomes the model the Python API moves toward later. Users moving between the two see different shapes until Python follows. (b) Java matches Python for now, and both move to an attribute model together as a separate piece of work.
What each remaining item would mean
"Attributes instead of stats" — this is the one I would most like pinned down, since it subsumes several of the above. Do you mean:
Option 2 is a much larger change and would be unusual for Java, where typed accessors are the norm and an untyped selector API gives up compile-time checking. Option 1 I can do now. Last thingThis PR is around 6,700 lines and has been open since July. Which of these do you consider blocking for merge, and which could land as follow-ups against the Java module once it is in? If most can follow, the bindings become reviewable on their own merits and the API refinement can proceed without holding up CI coverage and the build integration. |
getStatus already answers the question: it stays NO_TERMINATION until a solve populates it, so isSolved was a second way to ask something the API already told you. This is the one item from the review that needs no decision on the wider API direction, since the Python API has no equivalent either. The private solved field went with it, as isSolved was its only reader. Verified: 26/26 Java tests. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 8b178d9 |
|
I agree with Chris on pushing against matching Python as the design goal. I don't believe that was a goal that we specified. Keep the surface functional and minimal, we should not copy over historical cruft from the Python API.
The right flow would be to trim down the PR to what's needed and unobjectionable and then add onto it later as needed. |
Following mlubin: Python parity was never a stated goal, the surface should be functional and minimal, and the right flow is to trim the PR down and add back later as needed rather than defer removals to follow-ups. Removed as redundant with something the API already provides: - Solution.getProblemCategory, since isMIP answers the only distinction that remains once the all-integer category folds into MIP - Solution.getSolvedBy and getSolvedByPDLP, both derivable from LPStats - Solution.getTerminationReason, the string form of getTerminationStatus - Solution.getVars and Problem.getIncumbentValues, wrappers over the accessors - Problem.update, whose body was a call to resetSolvedValues - SolverSettings' three static getSolverSetting overloads, which built and destroyed a native settings object per call to read a default Removed because solve rebuilds the native problem on every call, so there is no incremental re-solve for them to support, and setObjective with the Variable setters already covers the same ground: - Problem.updateConstraint and Problem.updateObjective - Problem.relax, which duplicated model-copy logic; setting VariableType on the variables achieves the same thing Narrowed rather than deleted, because the bindings themselves need them: - Solution.getPrimalSolution, getDualSolution, getReducedCost and Solution.getSolveTime are package-private. Problem distributes them onto Variable and Constraint after each solve, so callers read values from the model. The bulk path is unchanged internally, so this costs no performance. - Constraint.computeSlack is package-private. Deleting it outright would remove slack from Java entirely until the engine provides it, which is a separate C++ change. ObjectiveExpression is gone. It existed only as a supertype for the two expression classes, which stand on their own. getObjective now returns LinearExpression, and the quadratic part is read as a matrix from getQuadraticObjectiveMatrix. isQuadratic went with it from both expression classes, where it was a constant. Public members across the five main classes go from 109 to 90. Verified: 24/24 Java tests, 59 JNI symbols matched, all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test ce50c37 |
They belong to NVIDIA#1734 and were swept in by a git add -A while that fix was copied into this worktree to build against. This branch should carry only the Java module; the C API arrives from main once NVIDIA#1734 merges. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test d1ebd46 |
java-build failed with: /opt/conda/envs/java/include/cuopt/error.hpp:14:10: fatal error: raft/core/error.hpp: No such file or directory The JNI layer compiles against libcuopt's public headers, and those include raft, rmm and rapids_logger headers transitively. The java file-key pulled in libcuopt itself but none of those, so the conda environment CI builds in had the headers that include them and not the headers they include. Add the same three dependency groups py_build_libcuopt already uses. Local builds did not catch this because they compile against the cuOpt build tree, where build.sh passes the _deps include paths explicitly. Only the CI path, which builds against the conda prefix, exercises this. Verified: rapids-dependency-file-generator now resolves libraft-headers, librmm and rapids-logger into the java environment. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 8a06bf2 |
# Conflicts: # build.sh
Fixes #1706. `cuOptGetPrimalSolution`, `cuOptGetDualSolution` and `cuOptGetReducedCosts` copy into a caller-allocated buffer and report no length. When the underlying vector is empty the `memcpy` copies nothing, the function returns `CUOPT_SUCCESS`, and the caller's buffer keeps whatever it already held. So "this solve produced no values" is indistinguishable from "the values are all zero". A caller that zeroes its buffer first reads zeros and believes them. ### Confirmed, not theoretical Against an infeasible LP (`x >= 2` and `x <= 1`), with each buffer pre-filled with a `-12345` sentinel: ``` termination_status = 2 (INFEASIBLE) cuOptGetPrimalSolution -> 0 (CUOPT_SUCCESS), buffer still -12345 cuOptGetDualSolution -> 0 (CUOPT_SUCCESS), buffer still -12345 cuOptGetReducedCosts -> 0 (CUOPT_SUCCESS), buffer still -12345 ``` Note this affects **`cuOptGetPrimalSolution` as well**, which the original issue did not mention — it was found while reproducing. This is also what forced a revert in #1524: moving the Java bindings onto `cuOptGetReducedCosts` turned "no reduced costs" into "all reduced costs are zero" for an infeasible LP, caught by `ProblemIntegrationTest.problemsBuildAndSolve[10]`. Those getters had to stay on the internal C++ interface, which does report the real length. ### The change Return `CUOPT_INVALID_ARGUMENT` when there are no values to copy, and leave the output buffer untouched. `CUOPT_INVALID_ARGUMENT` rather than a new status code because `cuOptGetDualSolution` **already returns it** when the underlying call throws `std::logic_error`. The empty-vector case was simply slipping past that guard, so the two paths now agree rather than one silently succeeding. Happy to use a distinct code instead if reviewers would prefer one — that would be a new public constant, which seemed like more surface than this warrants. `cuOptGetPrimalSolution` had no exception guard at all; it now has the same one as the other two. ### Compatibility A solve that produced values is unaffected. The behaviour only changes where the function previously reported success without writing anything. I checked the in-tree callers: the C examples under `docs/cuopt/source/cuopt-c/` and `skills/cuopt-numerical-optimization-api/assets/c/` all branch on the status code, so they now report the failure instead of printing uninitialised memory. Python does not go through these entry points; it binds to the C++ structs via Cython. ### Tests Two tests, covering both directions, since a fix that only checks the failing case could pass by breaking the working one: - `solution_accessors_report_absent_values` — infeasible solve; all three must report the absence and leave the sentinel intact - `solution_accessors_return_values_when_present` — solved LP; all three must still return values Verified locally: **71/71** `C_API_TEST` cases pass. ### Not included The issue also notes that `cuOptGetProblemStringArrayAttribute` has a related shape problem in the other direction — it requires `count` to match exactly, so "no names set" is indistinguishable from "bad argument", with no size query to ask first. That one needs a small API addition rather than a behaviour fix, so I have left it out of this PR and it can be handled separately. Authors: - Ramakrishna Prabhu (https://github.com/ramakrishnap-nv) Approvers: - Miles Lubin (https://github.com/mlubin) URL: #1734
|
/ok to test acd9be0 |
LPStats and MIPStats are removed, along with Solution.getLPStats and getMIPStats, their two native declarations and the two JNI entry points that packed attribute reads into a double[]. Nothing needed to build, solve or read a result goes with them. Solution keeps the termination status, objectives, MIP gap and solution bound, and variable and constraint values continue to come from the model. What is dropped is diagnostics: residuals, iteration and node counts, presolve time, violation magnitudes, and which method solved the problem. The capability is not lost, only unexposed. The statistics are scalar solution attributes in the C API, and every selector already reaches Java through the generated CuOptConstants, so re-adding them later is a change to this module alone. This also removes a fragile contract. LPStats and MIPStats read a positional double[] whose ordering was defined in the JNI selector lists and consumed by array index in the constructors, with nothing tying the two together and no test asserting any statistic's value. Transposing two selectors would have misreported every field silently. Docs updated: the API references say where the statistics now live, and the examples that printed them no longer do. The convex "Inspecting Solutions" section existed only to demonstrate LPStats and is gone. Public members across the module go from 195 to 182, and the JNI from 59 native methods to 57. Verified: 24/24 Java tests, 57 JNI symbols matched, all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test d757259 |
ramakrishnap-nv
changed the title
Restores access to the statistics, in the shape chris-maes asked for: the generic getter model already used for problem attributes, applied to solutions. Solution gains getIntAttribute and getFloatAttribute, keyed by a CUOPT_SOLUTION_ATTR_* constant, over the C API accessors that merged in NVIDIA#1715. This is two methods rather than the two classes and eleven getters that were here before, and a statistic added later is a new constant rather than new Java, since the selectors are generated into CuOptConstants from constants.h. It also drops the positional double[] contract the previous LPStats and MIPStats relied on, where the ordering lived in the JNI selector lists and was consumed by array index with nothing tying the two together. Tests assert values rather than that a call returns. An optimal LP has residuals and a gap within tolerance; a solved MIP has a non-negative node count and a constraint violation near zero; a float selector through the integer accessor, and a selector belonging to the other solver, both raise CuOptException. One thing the value assertions turned up: requesting a method does not mean that method is credited with the solve. The small LP here reports CUOPT_METHOD_UNSET even when PDLP is requested, so the test asserts that solved-by is a method the API defines rather than the one that was asked for. Verified: 26/26 Java tests, 59 JNI symbols matched, all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 84a9af6 |
Two tests, both asserting behaviour rather than that a call returns. mutatingTheModelChangesTheSolve walks a problem through the Variable setters and re-solves after each change, checking the answer moves as predicted: tightening the upper bound moves the optimum, pinning both bounds fixes the variable, doubling the objective coefficient doubles the objective, and making the variable integral turns the solve into a MIP so the LP-only accessors start rejecting. Those setters mutate state that Problem reads at solve time, and only a changed answer shows the change reached the solver. This matters more now that relaxing a MIP is documented as setting VariableType rather than calling relax. mipStartsAndMIPOnlySolutionFields seeds a knapsack through Variable.setMIPStart and solves. Problem.addMIPStarts collects those by variable index, so a wrong index would seed the wrong variable; the test pins the optimum and the values of both variables. It also covers getMIPGap and getSolutionBound, which an optimal solve determines exactly, and the callback payload, which is a value type and so can be checked without waiting for the solver to produce an incumbent. Public API coverage goes from 75% to 86%. What is left is expression arithmetic and the QuadraticConstraint accessors, which are only reachable through a file round trip. Verified: 28/28 Java tests, all pre-commit hooks. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
# Conflicts: # cpp/tests/linear_programming/c_api_tests/c_api_tests.cpp
|
/ok to test 785faec |
The java-build job resolves plugins and dependencies from Maven Central with no warm local repository. A 429 from Central failed the build outright, since Maven does not retry rate-limited or unavailable responses by default. Both mvn invocations now carry retry settings for the wagon and native resolver transports, so the build survives a transient Central failure. Signed-off-by: Ramakrishna Prabhu <ramakrishnap@nvidia.com>
|
/ok to test 86cd996c8a51d02a3e70dd21a2cbf65e94b6a1b1 |
@ramakrishnap-nv, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 86cd996 |
5 participants
Description
Java bindings for LP, MIP and QP, built as hand-written JNI over the public C API, with build and CI integration. Contributes to #1535 and #860.
The API is deliberately small. Following review, it covers building a problem, solving it, and reading the result — nothing beyond that. Anything further can be added later with a case for it.
API
Problembuilds and solves;VariableandConstraintcarry the model and the values from the last solve;SolverSettingsconfigures the solver;Solutionreports the outcome.Solution values are read from the model —
Variable.getValue,Variable.getReducedCost,Constraint.getDualValue,Constraint.getSlack— rather than as bulk arrays.Solver statistics are read as scalar solution attributes, keyed by a
CuOptConstants.CUOPT_SOLUTION_ATTR_*value:This mirrors the problem attribute accessors, following the model @chris-maes asked for and @tmckayus clarified. Because the selectors are generated into
CuOptConstantsfromconstants.h, a statistic added later is a new constant rather than new Java. A selector that does not apply to the solution, or that does not have the requested value type, raisesCuOptException.Build integration
javais an opt-in target of the top-levelbuild.sh:It prefers the
cpp/buildtree so 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.The rmm and raft headers must be the ones
libcuoptwas compiled against — rmm carries its version in an inline namespace, so a mismatched copy links cleanly and then fails atdlopen.build.shpasses the right include and library paths when it targets a build tree.CI
pr.yamljava-buildgpu-l4-latest-1ci/test_java.shbuild.yamljava-buildcpu4ci/build_java.shtest.yamlconda-java-testsgpu-l4-latest-1ci/test_java.shAll three are in their workflow's aggregator job, so a Java failure fails the PR. The PR job is gated on the
test_javachanged-files group (java/**,ci/build_java.sh,ci/test_java.sh) and ontest_cpp, so a C++ change that could break the bindings still exercises them.target/is uploaded as thecuopt-javaartifact.The
javadependency file-key pullslibcuoptplus thelibraft-headers,librmmandrapids-loggerheaders thatlibcuopt's public headers include transitively.ci/release/update-version.shbumps the POM through a sentinel comment; the artifact version drops the zero-padded RAPIDS patch field, since Maven has no notion of it.JNI symbol check
The bindings are hand-written, so a
static nativedeclaration and its entry point can drift apart. JNI resolves lazily, so the library still loads and the failure appears only when something calls the method.scripts/check_jni_symbols.shdiffs the prototypesjavac -hderives from the Java sources against the symbols the built library exports, and fails on a mismatch either way. It reads the built artifact rather than parsing source, so the macro-generated entry points need no special casing.build_native.shruns it after every native build, which covers./build.sh javaand both CI jobs. It takes about a second.Packaging
Version
26.10.0. Sources and javadoc jars are attached, and the POM carries theurl,licenses,scmanddevelopersmetadata Maven Central requires. Signing and upload are not wired up; that is tracked by the "Publishing & support" item in #1535.Known gap
cuopt_jni.cppstill includespdlp/cuopt_c_internal.hppfor parts of the problem path the C API does not cover — setting names, the quadratic objective and quadratic constraint getters, and the problem category. That coupleslibcuopt_jni.soto a specificlibcuoptbuild rather than to a stable ABI, which matters before this ships as a binary artifact. Tracked in #1703. The related copy-out behaviour was #1706, fixed by #1734 and now inmain.Checklist