Merged
Conversation
…pt_public into upgrade_skills
rgsl888prabhu
added
non-breaking
|
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:
📝 WalkthroughWalkthroughAdds and reorganizes repository documentation: introduces many domain- and interface-specific SKILLs, example assets (Python, C, CLI, MPS), pointer files to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
.github/skills/cuopt-user-rules/SKILL.md-137-137 (1)
137-137:⚠️ Potential issue | 🟡 MinorFix typo in installation prompt sentence.
Line 137 reads “do you have access another way?”; this appears to be missing “it” and can confuse users. Suggested: “...or do you have access to it another way?”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-user-rules/SKILL.md at line 137, Update the prompt sentence in SKILL.md by correcting the typo in the quoted string "Would you like help installing cuOpt, or do you have access another way?" to include the missing words—change it to "Would you like help installing cuOpt, or do you have access to it another way?" so the user-facing prompt (the quoted sentence in the file) reads clearly..github/skills/cuopt-qp-api-python/assets/portfolio/model.py-33-35 (1)
33-35:⚠️ Potential issue | 🟡 MinorGuard standard deviation computation against tiny negative variance.
If numerical noise returns a slightly negative objective, Line 34 can produce invalid output formatting. Clamp before sqrt.
💡 Proposed fix
if problem.Status.name in ["Optimal", "PrimalFeasible"]: print(f"Portfolio variance: {problem.ObjValue:.6f}") - print(f"Std dev: {problem.ObjValue**0.5:.4f}") + variance = max(problem.ObjValue, 0.0) + print(f"Std dev: {variance**0.5:.4f}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-qp-api-python/assets/portfolio/model.py around lines 33 - 35, The Std dev print uses problem.ObjValue directly which can be slightly negative due to numerical noise; before taking the square root in the Std dev calculation (around where problem.ObjValue is used for printing and alongside x1.getValue()), clamp the variance with something like var = max(problem.ObjValue, 0.0) and then compute sqrt(var) for the Std dev print, ensuring the formatted output uses the clamped value while leaving the original problem.ObjValue unmodified where you still want to display the raw variance..github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py-40-40 (1)
40-40:⚠️ Potential issue | 🟡 MinorGuard objective printing by solver status.
On line 40,
problem.ObjValueis printed unconditionally. The incumbent callback can terminate with various statuses (Optimal, FeasibleFound, TimeLimit, NumericalIssues, etc.), and accessingObjValuewithout checking status first may be unsafe or misleading. This pattern is inconsistent with standard cuOpt examples.Proposed change
- print(f"Status: {problem.Status.name}, Objective: {problem.ObjValue}") + if problem.Status.name in ["Optimal", "FeasibleFound"]: + print(f"Status: {problem.Status.name}, Objective: {problem.ObjValue}") + else: + print(f"Status: {problem.Status.name}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py at line 40, The print unconditionally reads problem.ObjValue even when the solver status may not have a valid incumbent; update the incumbent callback (the function/class that contains the print of "Status: {problem.Status.name}, Objective: {problem.ObjValue}") to first check problem.Status for a feasible/optimal state (e.g., Optimal or FeasibleFound/Feasible) before accessing problem.ObjValue, and if not feasible just print the status (or a placeholder) without reading ObjValue; replace the single unconditional print with a guarded conditional that only reads ObjValue when the status indicates a valid incumbent..github/skills/cuopt-lp-milp-formulation/SKILL.md-49-57 (1)
49-57:⚠️ Potential issue | 🟡 MinorHeading/text mismatch: “Three labels” but four labels are defined.
Please rename the section title (e.g., “Four labels”) to match the table content.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-formulation/SKILL.md around lines 49 - 57, The section heading "Three labels" in SKILL.md is inconsistent with the table that defines four labels; update the heading text from "Three labels" to "Four labels" (or another accurate heading) so the title matches the table content, making sure to update the exact heading string "Three labels" in the markdown file..github/skills/cuopt-lp-milp-api-python/SKILL.md-205-217 (1)
205-217:⚠️ Potential issue | 🟡 MinorAdd blank lines around markdown tables to satisfy lint.
MD058is triggered around the "Minimal / canonical examples" and "Other reference" tables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/SKILL.md around lines 205 - 217, The markdown tables under the headings "Minimal / canonical examples (LP & MILP)" and "Other reference" are missing blank lines before and after them (triggering MD058); edit SKILL.md to insert a blank line between each heading/paragraph and its following table and also a blank line after each table so the tables are separated from subsequent content (ensure the tables for lp_basic, lp_duals, lp_warmstart, milp_basic, milp_production_planning and mps_solver are each surrounded by an empty line to satisfy the MD058 lint rule)..github/skills/cuopt-lp-milp-api-python/SKILL.md-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorFix malformed table row (column count mismatch).
The
Unclearrow has 2 cells while the table defines 3 columns (MD056).Suggested fix
-| **Unclear** | Prefer **INTEGER** if the noun is a countable thing (a worker, a car); prefer **CONTINUOUS** if it's a measure (amount of steel, hours worked). If the problem says "whole" or "integer" or "number of", use INTEGER. | +| **Unclear** | Prefer **INTEGER** if the noun is a countable thing (a worker, a car); prefer **CONTINUOUS** if it's a measure (amount of steel, hours worked). If the problem says "whole" or "integer" or "number of", use INTEGER. | Ask a clarifying question if domain wording remains ambiguous. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/SKILL.md at line 33, The markdown table row for "Unclear" has only 2 cells while the table defines 3 columns; update the row in SKILL.md by adding the missing third cell (either a brief explanatory note or an empty cell placeholder) so the "Unclear" row matches the header's three-column structure and resolves the MD056 column count mismatch..github/skills/cuopt-routing-api-python/SKILL.md-44-46 (1)
44-46:⚠️ Potential issue | 🟡 Minor
np.arrayis used without importing NumPy in the snippet.This code block is non-runnable as-is when copied. Add the import to make the example complete and functional.
Suggested fix
```python +import numpy as np # Pickup-delivery pairs dm.set_pickup_delivery_pairs(pickup_indices, delivery_indices) # Precedence dm.add_order_precedence(node_id=2, preceding_nodes=np.array([0, 1]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-routing-api-python/SKILL.md around lines 44 - 46, The snippet uses np.array in the example for dm.add_order_precedence but NumPy is not imported; add an import line (import numpy as np) at the top of the code example so np.array is defined and the example using dm.add_order_precedence(node_id=2, preceding_nodes=np.array([0, 1])) is runnable..github/skills/cuopt-lp-milp-api-python/SKILL.md-73-93 (1)
73-93:⚠️ Potential issue | 🟡 MinorMILP snippet is missing
SolverSettingsimport.This code block will fail with
NameError: name 'SolverSettings' is not definedif copied independently.Suggested fix
from cuopt.linear_programming.problem import Problem, CONTINUOUS, INTEGER, MINIMIZE +from cuopt.linear_programming.solver_settings import SolverSettings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/SKILL.md around lines 73 - 93, The snippet uses SolverSettings but never imports it, causing a NameError; add an import for SolverSettings (e.g., add "from cuopt.linear_programming.solver import SolverSettings") at the top of the example so the symbol SolverSettings referenced when creating settings is defined and the MILP settings block (settings.set_parameter(...)) works..github/skills/cuopt-routing-api-python/SKILL.md-84-84 (1)
84-84:⚠️ Potential issue | 🟡 MinorAsymmetric cost matrices are valid; remove strict symmetry check.
The original guidance incorrectly implies that cost matrices must be symmetric. Per official NVIDIA documentation, asymmetric matrices are explicitly supported in cuOpt. Real-world routing scenarios frequently require asymmetric costs (one-way routes, directional preferences, time-dependent travel costs).
Suggested fix
-| Wrong cost | Check cost_matrix is symmetric | +| Wrong cost | Check matrix indexing, units, and directionality assumptions (asymmetric costs may be valid) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-routing-api-python/SKILL.md at line 84, Update the guidance that enforces or suggests cost matrices must be symmetric: remove the "Check cost_matrix is symmetric" assertion and replace it with a note that asymmetric cost matrices are supported by cuOpt (directional/one-way costs are valid), referencing the table entry "Wrong cost" and the existing "Check cost_matrix is symmetric" text so you change that cell to something like "Verify cost_matrix values and indexing; asymmetric matrices are supported; ensure costs correspond to directed routes." Ensure the revised wording clarifies asymmetry is allowed and suggests validating matrix indexing and units rather than enforcing symmetry..github/skills/cuopt-lp-milp-api-python/assets/lp_duals/model.py-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorUse enum comparison instead of fragile string matching against enum names.
Line 25 compares
problem.Status.nameagainst hardcoded strings, which breaks if enum member names drift. Prefer direct enum comparison:Recommended fix
+from cuopt.linear_programming.solver.solver_wrapper import LPTerminationStatus + def main(): problem = Problem("min_dual_rc") ... problem.solve() - if problem.Status.name in ["Optimal", "PrimalFeasible"]: + if problem.Status in (LPTerminationStatus.Optimal, LPTerminationStatus.PrimalFeasible):This eliminates fragility by comparing enum values directly rather than relying on string name stability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/lp_duals/model.py at line 25, Replace the fragile string check "if problem.Status.name in ['Optimal', 'PrimalFeasible']" with direct enum comparisons against the solver's status constants (e.g., use gp.GRB.OPTIMAL and gp.GRB.PRIMAL_FEASIBLE or the appropriate Status enum in scope). For example, change the condition to check problem.Status == gp.GRB.OPTIMAL or problem.Status in {gp.GRB.OPTIMAL, gp.GRB.PRIMAL_FEASIBLE} so that the code compares enum values directly (referencing problem.Status and the GRB status constants)..github/skills/cuopt-installation-api-python/resources/verification_examples.md-94-95 (1)
94-95:⚠️ Potential issue | 🟡 MinorAvoid false negatives when
CONDA_PREFIXis unset.This compile check drops the fallback used above; on non-conda setups it points to
/includeand can report a misleading failure.Suggested patch
-gcc -I${CONDA_PREFIX}/include -c /tmp/test_cuopt.c -o /tmp/test_cuopt.o && \ +gcc -I${CONDA_PREFIX:-/usr}/include -c /tmp/test_cuopt.c -o /tmp/test_cuopt.o && \ echo "C API headers OK" || echo "C API headers not found"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-installation-api-python/resources/verification_examples.md around lines 94 - 95, The gcc compile check uses -I${CONDA_PREFIX}/include unguarded which expands to /include when CONDA_PREFIX is empty and causes false negatives; change the one-line check so the -I flag is only added when CONDA_PREFIX is set (e.g. use the parameter expansion form ${CONDA_PREFIX:+-I${CONDA_PREFIX}/include} or wrap the gcc invocation in a conditional that tests CONDA_PREFIX), update the echoed messages accordingly, and keep the same gcc ... /tmp/test_cuopt.c -o /tmp/test_cuopt.o && echo ... || echo ... structure..github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c-64-88 (1)
64-88:⚠️ Potential issue | 🟡 MinorStatus variable reuse may cause incorrect exit code.
The
statusvariable is overwritten bycuOptGetDualSolution(line 66) andcuOptGetReducedCosts(line 76). The final return statement (line 88) checksstatus, which would reflect the last API call's status rather than the solve status. If the solve succeeds butcuOptGetReducedCostsfails, the program returns 1 even though the main computation succeeded.Proposed fix: use separate variables for retrieval status
cuopt_float_t *dual = malloc((size_t)num_constraints * sizeof(cuopt_float_t)); if (dual) { - status = cuOptGetDualSolution(solution, dual); - if (status == CUOPT_SUCCESS) { + cuopt_int_t dual_status = cuOptGetDualSolution(solution, dual); + if (dual_status == CUOPT_SUCCESS) { printf("Constraint c1 DualValue = %f\n", dual[0]); printf("Constraint c2 DualValue = %f\n", dual[1]); } free(dual); } cuopt_float_t *reduced = malloc((size_t)num_variables * sizeof(cuopt_float_t)); if (reduced) { - status = cuOptGetReducedCosts(solution, reduced); - if (status == CUOPT_SUCCESS) { + cuopt_int_t reduced_status = cuOptGetReducedCosts(solution, reduced); + if (reduced_status == CUOPT_SUCCESS) { printf("x ReducedCost = %f, y ReducedCost = %f, z ReducedCost = %f\n", reduced[0], reduced[1], reduced[2]); } free(reduced); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c around lines 64 - 88, The code reuses the `status` variable for API calls `cuOptGetDualSolution` and `cuOptGetReducedCosts`, which can overwrite the original solve result; change to separate result variables (e.g., `solve_status`, `dual_status`, `reduced_status`) so `solve_status` holds the solver outcome and only the retrieval calls assign to their own variables before logging; ensure the final return checks `solve_status == CUOPT_SUCCESS` and not the last retrieval status, and update references to `status` around `cuOptGetDualSolution`, `cuOptGetReducedCosts`, and the final `return` accordingly while still freeing `dual`/`reduced` and performing cleanup..github/skills/cuopt-lp-milp-api-python/assets/lp_warmstart/model.py-34-35 (1)
34-35:⚠️ Potential issue | 🟡 MinorAdd status check before accessing objective value.
The first problem's solve result is accessed without checking the status, unlike the second problem (line 47). If the solve fails,
problem.ObjValuemay raise an exception or return invalid data.Proposed fix
problem.solve(settings) - print(f"Objective: {problem.ObjValue}") + if problem.Status.name in ["Optimal", "PrimalFeasible"]: + print(f"Objective: {problem.ObjValue}") + else: + print(f"Problem 1 Status: {problem.Status.name}") + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/lp_warmstart/model.py around lines 34 - 35, The code prints problem.ObjValue immediately after calling problem.solve(settings); update it to first check the solver status (match the pattern used for the second problem around line 47) by inspecting the problem's status property or SolveStatus after problem.solve(settings), handle non-optimal/failure cases (log or raise) and only access/print problem.ObjValue when the status indicates a valid optimal/feasible solution; ensure you modify the block that calls problem.solve and the subsequent print so it mirrors the guarded check used for the second problem..github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md-62-64 (1)
62-64:⚠️ Potential issue | 🟡 MinorClarify execution directory for Line 63 command.
model.pyis not in thisdata/directory, sopython model.py ...is ambiguous/broken when run from here.Suggested docs fix
-```bash -python model.py --file path/to/your/problem.mps -``` +From `assets/mps_solver/`: +```bash +python model.py --file data/your_problem.mps +``` + +From `assets/mps_solver/data/`: +```bash +python ../model.py --file path/to/your/problem.mps +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md around lines 62 - 64, The README shows an ambiguous command "python model.py --file ..." that fails when run from the data/ directory; update the README (lines around the code block) to clarify execution contexts by adding two explicit examples: one showing how to run model.py from the assets/mps_solver/ directory using data/your_problem.mps as the file, and another showing how to run it from inside assets/mps_solver/data/ by invoking ../model.py with the correct relative path to the .mps file; reference the existing model.py script and the assets/mps_solver/data/ directory in the updated text so users know which working directory to use..github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py-150-155 (1)
150-155:⚠️ Potential issue | 🟡 MinorPrefix unused variable with underscore.
The
problemvariable from unpacking is never used.Proposed fix
- problem, solution = solve_mps( + _, solution = solve_mps(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 150 - 155, The unpacked variable problem returned by solve_mps is unused; update the unpacking at the call site (the solve_mps invocation) to prefix the unused variable with an underscore (e.g., _problem, solution = solve_mps(...)) or use a single underscore to ignore it, so that the linter/compiler no longer flags an unused variable..github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorRemove extraneous
fprefix from string literal.This f-string has no placeholders, so the
fprefix is unnecessary.Proposed fix
- print(f"Downloading air05.mps from MIPLIB...") + print("Downloading air05.mps from MIPLIB...")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py at line 34, The print statement uses an unnecessary f-string; locate the print call that contains the literal "Downloading air05.mps from MIPLIB..." and remove the leading f so it becomes a normal string literal (replace print(f"Downloading air05.mps from MIPLIB...") with print("Downloading air05.mps from MIPLIB...")).
🧹 Nitpick comments (8)
.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/results.md (1)
27-30: Clarify the two different gap metrics to avoid misinterpretation.
Gap to Optimalin the table (vs best-known objective) and solverrelative_mip_gapin logs are not the same metric. Please add a brief note so readers don’t compare them directly.Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/results.md around lines 27 - 30, Add a short clarifying note beneath the results table explaining that the table column "Gap to Optimal" is computed as the percentage gap between the reported Objective and a best-known (external) optimal value, while the solver's "relative_mip_gap" (mentioned in logs) is the solver's internal stopping tolerance; explicitly state they use different baselines and should not be compared directly (also add the same note near the other instance at lines 74-77). Reference the table header "Gap to Optimal" and the solver metric name "relative_mip_gap" so readers know exactly which values are being contrasted..github/skills/cuopt-user-rules/SKILL.md (1)
16-19: Polish repeated question stems for readability.These bullets currently start with “What” repeatedly. Consider varying phrasing (e.g., “Which language/interface...”, “Required output format...”) to improve flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-user-rules/SKILL.md around lines 16 - 19, Edit the bullet list in SKILL.md to remove repetitive "What" at the start of each item for better readability: replace "What **language/interface**?" with e.g. "Which language/interface is required?", change "What problem type?" to "Problem type:", change "What constraints matter?" to "Relevant constraints:", and change "What output format?" to "Required output format:" (or similar varied phrasing) so each list item reads distinctly while preserving the original meaning..github/skills/cuopt-qp-api-c/SKILL.md (1)
3-10: Clarify “C only” vs “C/C++” wording to avoid routing ambiguity.Line 3 implies dual-language scope, while Line 10 narrows to C only. Suggest explicitly saying this is the C API skill and it can be called from C/C++ codebases.
Suggested wording diff
-description: Quadratic Programming (QP) with cuOpt — C API. Use when the user is embedding QP in C/C++. +description: Quadratic Programming (QP) with cuOpt — C API. Use when the user needs the cuOpt C API (from C or C++ codebases). @@ -This skill is **C only**. +This skill covers the **cuOpt C API**.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-qp-api-c/SKILL.md around lines 3 - 10, Clarify the language to avoid routing ambiguity: update the SKILL.md header and the sentence "This skill is **C only**" to explicitly state this is the "C API skill" and that the C API can be invoked from both C and C++ codebases (e.g., change wording around "cuOpt QP — C API" and the line asserting "This skill is **C only**" to something like "This skill exposes the cuOpt QP C API; it is intended for C projects and can also be called from C++ codebases"), ensuring the document still warns that the API surface is C-based and QP must be minimization..github/skills/cuopt-qp-formulation/SKILL.md (1)
12-13: Add one sentence clarifying coefficient interpretation inxᵀQx.A brief note on how coefficients map to terms (especially diagonals vs cross terms) would reduce formulation mistakes.
Based on learnings: In cuOPT's quadratic programming API, the user-facing convention is
x^T Q x, with diagonalq_imapping toq_i * x_i^2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-qp-formulation/SKILL.md around lines 12 - 13, Add one clarifying sentence to the quadratic objective description stating that the API follows the xᵀQx convention where diagonal elements Q_ii (or q_i) correspond to coefficients of x_i^2 (i.e., Q_ii * x_i^2) and off-diagonal elements Q_ij contribute to cross terms (e.g., Q_ij * x_i * x_j), so authors should supply Q with those mappings in mind when forming the objective xᵀQx..github/skills/cuopt-server-api-python/SKILL.md (1)
46-48: Add error handling for HTTP status and safe JSON parsing in the example.Lines 46–48 assume success and directly read
reqIdwithout checking the HTTP status code. If the server returns a 4xx or 5xx error,r.json()["reqId"]will produce a misleading JSON parsing error instead of surfacing the HTTP failure. Additionally, a timeout should be specified to prevent indefinite hangs.Suggested doc snippet update
-r = requests.post(f"{SERVER}/cuopt/request", json=payload, headers=HEADERS) -req_id = r.json()["reqId"] +r = requests.post(f"{SERVER}/cuopt/request", json=payload, headers=HEADERS, timeout=30) +r.raise_for_status() +body = r.json() +req_id = body.get("reqId") +if not req_id: + raise ValueError(f"Missing reqId in response: {body}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-server-api-python/SKILL.md around lines 46 - 48, The example POST call assigns the response to r and immediately indexes r.json()["reqId"] without checking HTTP status or handling JSON errors and lacks a timeout; update the call that posts to f"{SERVER}/cuopt/request" (using variables SERVER, payload, HEADERS) to include a timeout parameter, verify success via r.raise_for_status() or by inspecting r.status_code, then parse JSON in a try/except to surface HTTP errors or JSONDecodeError and only then extract req_id (req_id = ...), returning or logging clear error messages on failure..github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py (1)
36-36: Keep a strong reference to the callback object.On line 36, passing
IncumbentCallback()inline may lead to garbage collection before or duringproblem.solve(). The official cuOpt documentation explicitly advises against this pattern. Keep the callback instance alive for the duration of the solve by assigning it to a named variable.Proposed change
- settings.set_mip_callback(IncumbentCallback()) + incumbent_callback = IncumbentCallback() + settings.set_mip_callback(incumbent_callback)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py at line 36, The callback instance passed inline to settings.set_mip_callback may be garbage-collected during problem.solve() — create and retain a strong reference by instantiating IncumbentCallback() into a variable (e.g., callback = IncumbentCallback()) and then pass that variable to settings.set_mip_callback(callback); ensure the variable outlives the solve call so the callback object remains alive for the duration of the solve..github/skills/cuopt-lp-milp-api-python/assets/milp_production_planning/model.py (1)
16-18:Materialis redundant withMachine_Time.Line 18 is a scaled duplicate of Line 16, so it does not constrain the model further. Consider making material coefficients/RHS independent to keep the example formulation meaningful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/milp_production_planning/model.py around lines 16 - 18, The Material constraint is a scaled duplicate of Machine_Time and thus redundant; update the coefficients or RHS for the constraint added via problem.addConstraint with name "Material" (currently the 4 * x1 + 2 * x2 <= 200 line) so it becomes independent (e.g., change coefficients or RHS to reflect distinct material limits) ensuring the "Material" constraint meaningfully differs from the "Machine_Time" constraint..github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py (1)
116-118: Silent exception handling obscures errors.Catching a broad
Exceptionand passing silently makes debugging difficult. Consider logging or at least narrowing the exception type.Proposed improvement
- except (AttributeError, Exception): - # For MPS problems, variable access may be limited - pass + except AttributeError: + # For MPS problems, variable access may be limited + if verbose: + print("Note: Variable values not accessible for this problem type")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 116 - 118, The current except block "except (AttributeError, Exception): pass" swallows all errors; change it to catch only the specific expected error (e.g., "except AttributeError:") and add logging of the exception (use logging.exception or logger.exception with a clear message about variable access in MPS parsing) so unexpected errors are visible; if there are other known, narrower exceptions to handle, list them explicitly rather than using Exception, and re-raise any unknown exceptions after logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/cuopt-installation-api-python/SKILL.md:
- Around line 17-20: The two pip install lines ("pip install
--extra-index-url=https://pypi.nvidia.com cuopt-cu13" and "pip install
--extra-index-url=https://pypi.nvidia.com 'cuopt-cu12==26.2.*'") are presented
sequentially but are mutually exclusive; update the SKILL.md snippet to clearly
state "Choose one" and show the two commands as alternatives (one for cuopt-cu13
and one for cuopt-cu12 pinned example) so users won't run both and cause a
CUDA/package mismatch.
In @.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c:
- Around line 43-46: The calls to cuOptCreateSolverSettings and
cuOptSetFloatParameter (for CUOPT_MIP_ABSOLUTE_TOLERANCE,
CUOPT_MIP_RELATIVE_GAP, CUOPT_TIME_LIMIT) currently ignore return statuses;
update them to check each function's return value (like the existing checks for
cuOptCreateRangedProblem and cuOptSolve), and on non-success log an error with
context (which function/parameter failed) and exit/return appropriately.
Specifically, capture the return from cuOptCreateSolverSettings into a status
variable and validate it before proceeding, and do the same for each
cuOptSetFloatParameter call so invalid parameter names or failures are handled
consistently.
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c:
- Around line 36-38: Capture and check each cuOpt C API return code before using
results: assign the return values of cuOptCreateSolverSettings,
cuOptSetFloatParameter (both calls), cuOptGetObjectiveValue, cuOptGetSolveTime,
cuOptGetTerminationStatus, and cuOptGetPrimalSolution to a status variable
(e.g., status) and verify it equals success (CUOPT_STATUS_SUCCESS) before
proceeding; if any call fails, handle the error (log and exit or cleanup)
instead of consuming uninitialized data — update the code paths around functions
named cuOptCreateSolverSettings, cuOptSetFloatParameter, cuOptGetObjectiveValue,
cuOptGetSolveTime, cuOptGetTerminationStatus, and cuOptGetPrimalSolution to
perform these checks.
In @.github/skills/cuopt-lp-milp-api-cli/assets/milp_facility/facility.mps:
- Around line 10-19: The CAP rows currently have inverted signs (e.g., CAP1
lists OPEN1 with +50 and SHIP11 with -1) which allows shipping without opening a
facility; change each CAPi row to enforce SHIPij <= CAPi * OPENi by giving SHIP
variables a +1 coefficient and the OPEN variable a -CAPi coefficient (e.g., in
row CAP1 set SHIP11 coefficient to 1.0 and OPEN1 coefficient to -50.0; similarly
for CAP2 set SHIP21 to 1.0 and OPEN2 to -70.0).
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 80-82: The printed diagnostic is showing bound method objects
because NumVariables, NumConstraints, and IsMIP are methods on Problem; update
the prints to call these methods (e.g., call Problem.NumVariables(),
Problem.NumConstraints(), Problem.IsMIP()) so the actual values are returned;
locate the calls in model.py (the print statements referencing
problem.NumVariables, problem.NumConstraints, problem.IsMIP) and add the
parentheses to invoke each method.
- Around line 157-162: The code always computes 'gap_to_optimal' using the
hardcoded AIR05_OPTIMAL inside the block that builds results (when solution is
present), which is wrong for arbitrary MPS files; update the function that
constructs results to accept an optional parameter (e.g., optimal_value=None)
and only compute and include 'gap_to_optimal' in results[gap] when optimal_value
is provided (compute as (solution['objective'] - optimal_value) / optimal_value
* 100); remove the unconditional reliance on AIR05_OPTIMAL and keep existing
behavior when optimal_value is None by omitting 'gap_to_optimal'.
In @.github/skills/cuopt-qp-api-python/SKILL.md:
- Around line 24-39: The portfolio example uses undefined variables r1, r2, r3
in the constraint on the Problem instance; add definitions for the return
coefficients (e.g., r1, r2, r3 as numeric values) before they are used or
replace them inline with numeric literals so the constraint
problem.addConstraint(r1*x1 + r2*x2 + r3*x3 >= 0.08, name="min_return")
references defined symbols; update the SKILL.md snippet to declare the return
coefficients above the constraint (or document that they should be provided) so
Problem, x1/x2/x3, and the min_return constraint are executable.
In @.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py:
- Line 19: The call to dm.set_order_locations uses cudf.Series constructed from
a Python list which defaults to int64; change the series creation to explicitly
use int32 dtype (e.g., pass dtype=cudf.int32 or dtype='int32' to cudf.Series) so
dm.set_order_locations receives int32 as required by the cuOpt routing API.
---
Minor comments:
In
@.github/skills/cuopt-installation-api-python/resources/verification_examples.md:
- Around line 94-95: The gcc compile check uses -I${CONDA_PREFIX}/include
unguarded which expands to /include when CONDA_PREFIX is empty and causes false
negatives; change the one-line check so the -I flag is only added when
CONDA_PREFIX is set (e.g. use the parameter expansion form
${CONDA_PREFIX:+-I${CONDA_PREFIX}/include} or wrap the gcc invocation in a
conditional that tests CONDA_PREFIX), update the echoed messages accordingly,
and keep the same gcc ... /tmp/test_cuopt.c -o /tmp/test_cuopt.o && echo ... ||
echo ... structure.
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c:
- Around line 64-88: The code reuses the `status` variable for API calls
`cuOptGetDualSolution` and `cuOptGetReducedCosts`, which can overwrite the
original solve result; change to separate result variables (e.g.,
`solve_status`, `dual_status`, `reduced_status`) so `solve_status` holds the
solver outcome and only the retrieval calls assign to their own variables before
logging; ensure the final return checks `solve_status == CUOPT_SUCCESS` and not
the last retrieval status, and update references to `status` around
`cuOptGetDualSolution`, `cuOptGetReducedCosts`, and the final `return`
accordingly while still freeing `dual`/`reduced` and performing cleanup.
In @.github/skills/cuopt-lp-milp-api-python/assets/lp_duals/model.py:
- Line 25: Replace the fragile string check "if problem.Status.name in
['Optimal', 'PrimalFeasible']" with direct enum comparisons against the solver's
status constants (e.g., use gp.GRB.OPTIMAL and gp.GRB.PRIMAL_FEASIBLE or the
appropriate Status enum in scope). For example, change the condition to check
problem.Status == gp.GRB.OPTIMAL or problem.Status in {gp.GRB.OPTIMAL,
gp.GRB.PRIMAL_FEASIBLE} so that the code compares enum values directly
(referencing problem.Status and the GRB status constants).
In @.github/skills/cuopt-lp-milp-api-python/assets/lp_warmstart/model.py:
- Around line 34-35: The code prints problem.ObjValue immediately after calling
problem.solve(settings); update it to first check the solver status (match the
pattern used for the second problem around line 47) by inspecting the problem's
status property or SolveStatus after problem.solve(settings), handle
non-optimal/failure cases (log or raise) and only access/print problem.ObjValue
when the status indicates a valid optimal/feasible solution; ensure you modify
the block that calls problem.solve and the subsequent print so it mirrors the
guarded check used for the second problem.
In
@.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py:
- Line 40: The print unconditionally reads problem.ObjValue even when the solver
status may not have a valid incumbent; update the incumbent callback (the
function/class that contains the print of "Status: {problem.Status.name},
Objective: {problem.ObjValue}") to first check problem.Status for a
feasible/optimal state (e.g., Optimal or FeasibleFound/Feasible) before
accessing problem.ObjValue, and if not feasible just print the status (or a
placeholder) without reading ObjValue; replace the single unconditional print
with a guarded conditional that only reads ObjValue when the status indicates a
valid incumbent.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md:
- Around line 62-64: The README shows an ambiguous command "python model.py
--file ..." that fails when run from the data/ directory; update the README
(lines around the code block) to clarify execution contexts by adding two
explicit examples: one showing how to run model.py from the assets/mps_solver/
directory using data/your_problem.mps as the file, and another showing how to
run it from inside assets/mps_solver/data/ by invoking ../model.py with the
correct relative path to the .mps file; reference the existing model.py script
and the assets/mps_solver/data/ directory in the updated text so users know
which working directory to use.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 150-155: The unpacked variable problem returned by solve_mps is
unused; update the unpacking at the call site (the solve_mps invocation) to
prefix the unused variable with an underscore (e.g., _problem, solution =
solve_mps(...)) or use a single underscore to ignore it, so that the
linter/compiler no longer flags an unused variable.
- Line 34: The print statement uses an unnecessary f-string; locate the print
call that contains the literal "Downloading air05.mps from MIPLIB..." and remove
the leading f so it becomes a normal string literal (replace print(f"Downloading
air05.mps from MIPLIB...") with print("Downloading air05.mps from MIPLIB...")).
In @.github/skills/cuopt-lp-milp-api-python/SKILL.md:
- Around line 205-217: The markdown tables under the headings "Minimal /
canonical examples (LP & MILP)" and "Other reference" are missing blank lines
before and after them (triggering MD058); edit SKILL.md to insert a blank line
between each heading/paragraph and its following table and also a blank line
after each table so the tables are separated from subsequent content (ensure the
tables for lp_basic, lp_duals, lp_warmstart, milp_basic,
milp_production_planning and mps_solver are each surrounded by an empty line to
satisfy the MD058 lint rule).
- Line 33: The markdown table row for "Unclear" has only 2 cells while the table
defines 3 columns; update the row in SKILL.md by adding the missing third cell
(either a brief explanatory note or an empty cell placeholder) so the "Unclear"
row matches the header's three-column structure and resolves the MD056 column
count mismatch.
- Around line 73-93: The snippet uses SolverSettings but never imports it,
causing a NameError; add an import for SolverSettings (e.g., add "from
cuopt.linear_programming.solver import SolverSettings") at the top of the
example so the symbol SolverSettings referenced when creating settings is
defined and the MILP settings block (settings.set_parameter(...)) works.
In @.github/skills/cuopt-lp-milp-formulation/SKILL.md:
- Around line 49-57: The section heading "Three labels" in SKILL.md is
inconsistent with the table that defines four labels; update the heading text
from "Three labels" to "Four labels" (or another accurate heading) so the title
matches the table content, making sure to update the exact heading string "Three
labels" in the markdown file.
In @.github/skills/cuopt-qp-api-python/assets/portfolio/model.py:
- Around line 33-35: The Std dev print uses problem.ObjValue directly which can
be slightly negative due to numerical noise; before taking the square root in
the Std dev calculation (around where problem.ObjValue is used for printing and
alongside x1.getValue()), clamp the variance with something like var =
max(problem.ObjValue, 0.0) and then compute sqrt(var) for the Std dev print,
ensuring the formatted output uses the clamped value while leaving the original
problem.ObjValue unmodified where you still want to display the raw variance.
In @.github/skills/cuopt-routing-api-python/SKILL.md:
- Around line 44-46: The snippet uses np.array in the example for
dm.add_order_precedence but NumPy is not imported; add an import line (import
numpy as np) at the top of the code example so np.array is defined and the
example using dm.add_order_precedence(node_id=2, preceding_nodes=np.array([0,
1])) is runnable.
- Line 84: Update the guidance that enforces or suggests cost matrices must be
symmetric: remove the "Check cost_matrix is symmetric" assertion and replace it
with a note that asymmetric cost matrices are supported by cuOpt
(directional/one-way costs are valid), referencing the table entry "Wrong cost"
and the existing "Check cost_matrix is symmetric" text so you change that cell
to something like "Verify cost_matrix values and indexing; asymmetric matrices
are supported; ensure costs correspond to directed routes." Ensure the revised
wording clarifies asymmetry is allowed and suggests validating matrix indexing
and units rather than enforcing symmetry.
In @.github/skills/cuopt-user-rules/SKILL.md:
- Line 137: Update the prompt sentence in SKILL.md by correcting the typo in the
quoted string "Would you like help installing cuOpt, or do you have access
another way?" to include the missing words—change it to "Would you like help
installing cuOpt, or do you have access to it another way?" so the user-facing
prompt (the quoted sentence in the file) reads clearly.
---
Nitpick comments:
In
@.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py:
- Line 36: The callback instance passed inline to settings.set_mip_callback may
be garbage-collected during problem.solve() — create and retain a strong
reference by instantiating IncumbentCallback() into a variable (e.g., callback =
IncumbentCallback()) and then pass that variable to
settings.set_mip_callback(callback); ensure the variable outlives the solve call
so the callback object remains alive for the duration of the solve.
In
@.github/skills/cuopt-lp-milp-api-python/assets/milp_production_planning/model.py:
- Around line 16-18: The Material constraint is a scaled duplicate of
Machine_Time and thus redundant; update the coefficients or RHS for the
constraint added via problem.addConstraint with name "Material" (currently the 4
* x1 + 2 * x2 <= 200 line) so it becomes independent (e.g., change coefficients
or RHS to reflect distinct material limits) ensuring the "Material" constraint
meaningfully differs from the "Machine_Time" constraint.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 116-118: The current except block "except (AttributeError,
Exception): pass" swallows all errors; change it to catch only the specific
expected error (e.g., "except AttributeError:") and add logging of the exception
(use logging.exception or logger.exception with a clear message about variable
access in MPS parsing) so unexpected errors are visible; if there are other
known, narrower exceptions to handle, list them explicitly rather than using
Exception, and re-raise any unknown exceptions after logging.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/results.md:
- Around line 27-30: Add a short clarifying note beneath the results table
explaining that the table column "Gap to Optimal" is computed as the percentage
gap between the reported Objective and a best-known (external) optimal value,
while the solver's "relative_mip_gap" (mentioned in logs) is the solver's
internal stopping tolerance; explicitly state they use different baselines and
should not be compared directly (also add the same note near the other instance
at lines 74-77). Reference the table header "Gap to Optimal" and the solver
metric name "relative_mip_gap" so readers know exactly which values are being
contrasted.
In @.github/skills/cuopt-qp-api-c/SKILL.md:
- Around line 3-10: Clarify the language to avoid routing ambiguity: update the
SKILL.md header and the sentence "This skill is **C only**" to explicitly state
this is the "C API skill" and that the C API can be invoked from both C and C++
codebases (e.g., change wording around "cuOpt QP — C API" and the line asserting
"This skill is **C only**" to something like "This skill exposes the cuOpt QP C
API; it is intended for C projects and can also be called from C++ codebases"),
ensuring the document still warns that the API surface is C-based and QP must be
minimization.
In @.github/skills/cuopt-qp-formulation/SKILL.md:
- Around line 12-13: Add one clarifying sentence to the quadratic objective
description stating that the API follows the xᵀQx convention where diagonal
elements Q_ii (or q_i) correspond to coefficients of x_i^2 (i.e., Q_ii * x_i^2)
and off-diagonal elements Q_ij contribute to cross terms (e.g., Q_ij * x_i *
x_j), so authors should supply Q with those mappings in mind when forming the
objective xᵀQx.
In @.github/skills/cuopt-server-api-python/SKILL.md:
- Around line 46-48: The example POST call assigns the response to r and
immediately indexes r.json()["reqId"] without checking HTTP status or handling
JSON errors and lacks a timeout; update the call that posts to
f"{SERVER}/cuopt/request" (using variables SERVER, payload, HEADERS) to include
a timeout parameter, verify success via r.raise_for_status() or by inspecting
r.status_code, then parse JSON in a try/except to surface HTTP errors or
JSONDecodeError and only then extract req_id (req_id = ...), returning or
logging clear error messages on failure.
In @.github/skills/cuopt-user-rules/SKILL.md:
- Around line 16-19: Edit the bullet list in SKILL.md to remove repetitive
"What" at the start of each item for better readability: replace "What
**language/interface**?" with e.g. "Which language/interface is required?",
change "What problem type?" to "Problem type:", change "What constraints
matter?" to "Relevant constraints:", and change "What output format?" to
"Required output format:" (or similar varied phrasing) so each list item reads
distinctly while preserving the original meaning.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (95)
.claude/AGENTS.md.claude/CLAUDE.md.claude/skills.cursor/AGENTS.md.cursor/skills.github/AGENTS.md.github/skills/README.md.github/skills/cuopt-debugging/SKILL.md.github/skills/cuopt-debugging/resources/diagnostic_snippets.md.github/skills/cuopt-installation-api-c/SKILL.md.github/skills/cuopt-installation-api-c/resources/verification_examples.md.github/skills/cuopt-installation-api-python/SKILL.md.github/skills/cuopt-installation-api-python/resources/verification_examples.md.github/skills/cuopt-installation-common/SKILL.md.github/skills/cuopt-installation-developer/SKILL.md.github/skills/cuopt-installation/SKILL.md.github/skills/cuopt-lp-milp-api-c/SKILL.md.github/skills/cuopt-lp-milp-api-c/assets/README.md.github/skills/cuopt-lp-milp-api-c/assets/lp_basic/README.md.github/skills/cuopt-lp-milp-api-c/assets/lp_basic/lp_simple.c.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/README.md.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c.github/skills/cuopt-lp-milp-api-c/assets/lp_warmstart/README.md.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/README.md.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/README.md.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/README.md.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/data/sample.mps.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c.github/skills/cuopt-lp-milp-api-c/resources/examples.md.github/skills/cuopt-lp-milp-api-cli/SKILL.md.github/skills/cuopt-lp-milp-api-cli/assets/README.md.github/skills/cuopt-lp-milp-api-cli/assets/lp_production/README.md.github/skills/cuopt-lp-milp-api-cli/assets/lp_production/production.mps.github/skills/cuopt-lp-milp-api-cli/assets/lp_simple/README.md.github/skills/cuopt-lp-milp-api-cli/assets/lp_simple/sample.mps.github/skills/cuopt-lp-milp-api-cli/assets/milp_facility/README.md.github/skills/cuopt-lp-milp-api-cli/assets/milp_facility/facility.mps.github/skills/cuopt-lp-milp-api-python/SKILL.md.github/skills/cuopt-lp-milp-api-python/assets/README.md.github/skills/cuopt-lp-milp-api-python/assets/lp_basic/README.md.github/skills/cuopt-lp-milp-api-python/assets/lp_basic/model.py.github/skills/cuopt-lp-milp-api-python/assets/lp_duals/README.md.github/skills/cuopt-lp-milp-api-python/assets/lp_duals/model.py.github/skills/cuopt-lp-milp-api-python/assets/lp_warmstart/README.md.github/skills/cuopt-lp-milp-api-python/assets/lp_warmstart/model.py.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/README.md.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/model.py.github/skills/cuopt-lp-milp-api-python/assets/milp_production_planning/README.md.github/skills/cuopt-lp-milp-api-python/assets/milp_production_planning/model.py.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/README.md.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/sample.mps.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/results.md.github/skills/cuopt-lp-milp-formulation/SKILL.md.github/skills/cuopt-lp-milp/SKILL.md.github/skills/cuopt-lp-milp/resources/cli_examples.md.github/skills/cuopt-lp-milp/resources/python_examples.md.github/skills/cuopt-lp-milp/resources/server_examples.md.github/skills/cuopt-qp-api-c/SKILL.md.github/skills/cuopt-qp-api-c/assets/README.md.github/skills/cuopt-qp-api-cli/SKILL.md.github/skills/cuopt-qp-api-cli/assets/README.md.github/skills/cuopt-qp-api-python/SKILL.md.github/skills/cuopt-qp-api-python/assets/README.md.github/skills/cuopt-qp-api-python/assets/least_squares/README.md.github/skills/cuopt-qp-api-python/assets/least_squares/model.py.github/skills/cuopt-qp-api-python/assets/maximization_workaround/README.md.github/skills/cuopt-qp-api-python/assets/maximization_workaround/model.py.github/skills/cuopt-qp-api-python/assets/portfolio/README.md.github/skills/cuopt-qp-api-python/assets/portfolio/model.py.github/skills/cuopt-qp-api-python/resources/examples.md.github/skills/cuopt-qp-formulation/SKILL.md.github/skills/cuopt-qp/SKILL.md.github/skills/cuopt-routing-api-python/SKILL.md.github/skills/cuopt-routing-api-python/assets/README.md.github/skills/cuopt-routing-api-python/assets/pdp_basic/README.md.github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py.github/skills/cuopt-routing-api-python/assets/vrp_basic/README.md.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py.github/skills/cuopt-routing-api-python/resources/examples.md.github/skills/cuopt-routing-api-python/resources/server_examples.md.github/skills/cuopt-routing-formulation/SKILL.md.github/skills/cuopt-routing/SKILL.md.github/skills/cuopt-server-api-python/SKILL.md.github/skills/cuopt-server-api-python/resources/lp_milp_examples.md.github/skills/cuopt-server-api-python/resources/routing_examples.md.github/skills/cuopt-server-common/SKILL.md.github/skills/cuopt-server/SKILL.md.github/skills/cuopt-user-rules/SKILL.md.opencode/AGENTS.md.opencode/skills
💤 Files with no reviewable changes (11)
- .github/skills/cuopt-lp-milp/resources/cli_examples.md
- .github/skills/cuopt-lp-milp/resources/server_examples.md
- .github/skills/cuopt-debugging/SKILL.md
- .github/skills/cuopt-debugging/resources/diagnostic_snippets.md
- .github/skills/cuopt-lp-milp/SKILL.md
- .github/skills/cuopt-routing/SKILL.md
- .github/skills/cuopt-server/SKILL.md
- .github/skills/cuopt-qp/SKILL.md
- .github/skills/cuopt-lp-milp/resources/python_examples.md
- .github/skills/cuopt-installation/SKILL.md
- .github/skills/README.md
.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c
Outdated
Show resolved
Hide resolved
.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c
Outdated
Show resolved
Hide resolved
.github/skills/cuopt-lp-milp-api-cli/assets/milp_facility/facility.mps
Outdated
Show resolved
Hide resolved
.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/skills/cuopt-user-rules/SKILL.md (1)
174-177: 🛠️ Refactor suggestion | 🟠 MajorResolve policy contradiction for package installs
Line 174-177 says commands may be run without asking in some exceptions, but Line 194 says package changes have “No exceptions.” This creates ambiguous behavior when users explicitly ask to run install commands. Make package install/upgrade commands explicitly excluded from all exceptions in Line 174-177.
Suggested wording adjustment
**Exceptions (okay without asking):** - Read-only commands the user explicitly requested - Commands the user just provided and asked you to run + - Except any package install/upgrade/modify command (always user-executed only)Also applies to: 194-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-user-rules/SKILL.md around lines 174 - 177, The "Exceptions (okay without asking)" section currently permits running some user-requested or read-only commands, which contradicts the later "No exceptions" rule for package changes; update the SKILL.md text by explicitly excluding package install/upgrade commands from the exceptions list and clarifying that "package install", "package upgrade", "apt-get install", "pip install", and equivalent package manager commands are never allowed to run without explicit confirmation; adjust the wording in the "Exceptions (okay without asking)" paragraph and the package-change "No exceptions." paragraph so they consistently state that package installation or upgrade commands are always prohibited from automatic execution even if the user requests them.
♻️ Duplicate comments (5)
.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py (2)
83-85:⚠️ Potential issue | 🔴 CriticalCall cuOpt
Problemaccessors; current code likely stores bound methods instead of values.At Line 83-85 and Line 105-107,
problem.NumVariables,problem.NumConstraints, andproblem.IsMIPshould be invoked if they are methods; otherwise logs/JSON will contain method objects.Proposed fix
- print(f"Variables: {problem.NumVariables}") - print(f"Constraints: {problem.NumConstraints}") - print(f"Is MIP: {problem.IsMIP}") + print(f"Variables: {problem.NumVariables()}") + print(f"Constraints: {problem.NumConstraints()}") + print(f"Is MIP: {problem.IsMIP()}") @@ - "num_variables": problem.NumVariables, - "num_constraints": problem.NumConstraints, - "is_mip": problem.IsMIP, + "num_variables": problem.NumVariables(), + "num_constraints": problem.NumConstraints(), + "is_mip": problem.IsMIP(),#!/bin/bash set -euo pipefail # Verify whether these are methods or properties in the repository version. fd 'problem.py|problem.pyi' | xargs -r rg -n -C3 'class Problem|def NumVariables|def NumConstraints|def IsMIP|@property'Also applies to: 105-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 83 - 85, The prints/logs are capturing bound method objects instead of their return values; call/invoke the cuOpt Problem accessors rather than referencing them. Replace uses of problem.NumVariables, problem.NumConstraints, and problem.IsMIP (also the occurrences around the later prints at the other block) with problem.NumVariables(), problem.NumConstraints(), and problem.IsMIP() if those are methods (or leave as properties only if inspection of Problem shows `@property`), and update any JSON/log serialization to use the returned values from NumVariables(), NumConstraints(), and IsMIP() rather than the method objects.
129-129:⚠️ Potential issue | 🟠 Major
gap_to_optimalis incorrectly tied to AIR05 even for non-AIR05 input files.Line 157-164 (and related CLI output at Line 206-207, 214-215, 249-252) produces misleading metrics when
--fileis notair05.mps.Proposed fix (parameterize known optimum)
-def compare_gaps(filepath: str, time_limit: float = 120.0) -> dict: +def compare_gaps( + filepath: str, + time_limit: float = 120.0, + known_optimal: float | None = None, +) -> dict: @@ - if solution: - results[gap] = { + if solution: + result = { "objective": solution["objective"], "status": solution["status"], - "gap_to_optimal": (solution["objective"] - AIR05_OPTIMAL) - / AIR05_OPTIMAL - * 100, } + if known_optimal: + result["gap_to_optimal"] = ( + (solution["objective"] - known_optimal) / known_optimal * 100 + ) + results[gap] = resultAlso applies to: 157-164, 206-207, 214-215, 249-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py at line 129, The compare_gaps function currently computes gap_to_optimal using a hardcoded AIR05 optimum regardless of the input file, causing misleading metrics; update compare_gaps to determine the correct known_optimum per filepath (e.g., map filenames or accept a known_optimum parameter) and use that value when computing gap_to_optimal and related CLI output; locate references to gap_to_optimal, known_optimum and the compare_gaps signature and either add a function parameter (known_optimum: Optional[float]) or a filename→optimum lookup, ensure CLI code that calls compare_gaps passes the correct optimum when provided, and update the printed metrics to use the resolved optimum..github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c (1)
41-43:⚠️ Potential issue | 🟠 MajorCheck every cuOpt API return code before consuming outputs.
At Line 41-43, Line 53-55, and Line 63, cuOpt calls are currently unchecked. If any fails, subsequent values (
objective_value,time,termination_status,primal) may be invalid while the program can still exit as success.Proposed fix
- cuOptCreateSolverSettings(&settings); - cuOptSetFloatParameter(settings, CUOPT_TIME_LIMIT, 60.0); - cuOptSetFloatParameter(settings, CUOPT_MIP_RELATIVE_GAP, 0.01); + status = cuOptCreateSolverSettings(&settings); + if (status != CUOPT_SUCCESS) { + printf("Error creating solver settings: %d\n", status); + goto cleanup; + } + status = cuOptSetFloatParameter(settings, CUOPT_TIME_LIMIT, 60.0); + if (status != CUOPT_SUCCESS) { + printf("Error setting CUOPT_TIME_LIMIT: %d\n", status); + goto cleanup; + } + status = cuOptSetFloatParameter(settings, CUOPT_MIP_RELATIVE_GAP, 0.01); + if (status != CUOPT_SUCCESS) { + printf("Error setting CUOPT_MIP_RELATIVE_GAP: %d\n", status); + goto cleanup; + } @@ - cuOptGetObjectiveValue(solution, &objective_value); - cuOptGetSolveTime(solution, &time); - cuOptGetTerminationStatus(solution, &termination_status); + status = cuOptGetObjectiveValue(solution, &objective_value); + if (status != CUOPT_SUCCESS) { + printf("Error getting objective value: %d\n", status); + goto cleanup; + } + status = cuOptGetSolveTime(solution, &time); + if (status != CUOPT_SUCCESS) { + printf("Error getting solve time: %d\n", status); + goto cleanup; + } + status = cuOptGetTerminationStatus(solution, &termination_status); + if (status != CUOPT_SUCCESS) { + printf("Error getting termination status: %d\n", status); + goto cleanup; + } @@ - cuOptGetPrimalSolution(solution, primal); + status = cuOptGetPrimalSolution(solution, primal); + if (status != CUOPT_SUCCESS) { + printf("Error getting primal solution: %d\n", status); + goto cleanup; + } @@ - free(primal); + free(primal); + primal = NULL; @@ cleanup: + free(primal); cuOptDestroyProblem(&problem); cuOptDestroySolverSettings(&settings); cuOptDestroySolution(&solution);Also applies to: 53-55, 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c around lines 41 - 43, All cuOpt API calls must have their return codes checked before using outputs: wrap calls like cuOptCreateSolverSettings, cuOptSetFloatParameter, cuOptSolveModel, and cuOptGetSolutionObjectiveValue/cuOptGetSolutionPrimal/cuOptGetSolveInfo with error checks and bail out (log error and return/fail) if the returned status is not success; do this for the calls at the sites where settings are created/parameters set (cuOptCreateSolverSettings, cuOptSetFloatParameter), when solving (cuOptSolveModel), and when retrieving outputs (cuOptGetSolutionObjectiveValue, cuOptGetSolutionPrimal, cuOptGetSolveInfo), so that invalid outputs are never used after a failed API call..github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py (1)
23-23:⚠️ Potential issue | 🟠 MajorPreviously flagged issue remains at Line 23:
order_locationsshould beint32.
cudf.Series([1, 2, 3])can default toint64; cast explicitly before passing toset_order_locations.Required fix
-dm.set_order_locations(cudf.Series([1, 2, 3])) +dm.set_order_locations(cudf.Series([1, 2, 3], dtype="int32"))#!/bin/bash set -euo pipefail # Confirm API dtype requirement and unresolved call in this file. rg -n -C2 'order_locations : cudf.Series dtype - int32|def set_order_locations' python/cuopt/cuopt/routing/vehicle_routing.py rg -n -C2 'set_order_locations\(' .github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py at line 23, The call dm.set_order_locations(cudf.Series([1, 2, 3])) uses a default integer dtype (likely int64); change it to explicitly cast the series to int32 before passing to set_order_locations so order_locations meets the API dtype requirement—e.g., construct or cast the cudf.Series to dtype="int32" so the argument passed to set_order_locations is int32..github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c (1)
48-51:⚠️ Potential issue | 🟠 MajorUnresolved: solver-settings return values are still unchecked.
At Lines 48-51, failures from
cuOptCreateSolverSettings/cuOptSetFloatParametercan be silently ignored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c around lines 48 - 51, The calls to cuOptCreateSolverSettings and cuOptSetFloatParameter (for CUOPT_MIP_ABSOLUTE_TOLERANCE, CUOPT_MIP_RELATIVE_GAP, CUOPT_TIME_LIMIT) ignore return values and can fail silently; update the code to check each function's return status (the result of cuOptCreateSolverSettings and each cuOptSetFloatParameter call), and on error log the status and perform appropriate cleanup/exit (e.g., free settings if created and return non-zero or exit) so failures are not ignored—use the same settings variable and the three parameter calls to locate where to add the checks and error handling.
🧹 Nitpick comments (4)
.github/skills/cuopt-user-rules/SKILL.md (1)
16-19: Tighten repeated bullet phrasing for readabilityLine 16-19 repeats “What …?” in consecutive bullets. Minor rewording would read cleaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-user-rules/SKILL.md around lines 16 - 19, The consecutive bullets repeating “What …?” are repetitive; edit the SKILL.md bullets to use parallel, concise phrasing instead (e.g., "Language/interface", "Problem type", "Relevant constraints", "Desired output format") so they read cleaner and avoid repeating the word "What" while preserving the original intent and order of the four questions..github/skills/cuopt-lp-milp-api-cli/SKILL.md (1)
49-49: Clarify LP vs MILP behavior when integer markers are absent.At Line 49, add one explicit sentence: if
INTORG/INTENDmarkers are not present, the model is treated as LP (not MILP). This avoids formulation ambiguity.Based on learnings, in MIP problems at least one integer variable must exist; otherwise the model is LP by definition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-cli/SKILL.md at line 49, Add a single clarifying sentence after the line containing "Integer variables: use 'MARKER' 'INTORG' before and 'MARKER' 'INTEND' after the integer columns" stating that if the 'INTORG'/'INTEND' markers are not present the model will be treated as an LP (not an MILP), and remind that a MIP requires at least one integer variable to be considered MILP; reference the 'INTORG' and 'INTEND' markers and the MARKER usage in that sentence..github/skills/cuopt-qp-api-python/assets/maximization_workaround/model.py (1)
10-22: Wrap module-level code inmain()withif __name__ == "__main__":guard.Lines 10-22 execute
problem.solve()at module import time. While this module is not imported elsewhere in the codebase, wrapping the code in amain()function with the standard guard is a best practice for documentation examples and makes the intent explicit (this code is meant to run standalone, not be imported).♻️ Proposed refactor
-problem = Problem("MaxWorkaround") - -x = problem.addVariable(lb=0, ub=10, vtype=CONTINUOUS, name="x") -problem.setObjective(x * x - 4 * x, sense=MINIMIZE) - -problem.solve() - -if problem.Status.name in ["Optimal", "PrimalFeasible"]: - print(f"x = {x.getValue():.4f}") - print(f"Minimized value = {problem.ObjValue:.4f}") - print(f"Original maximum = {-problem.ObjValue:.4f}") -else: - print(f"Status: {problem.Status.name}") +def main(): + problem = Problem("MaxWorkaround") + x = problem.addVariable(lb=0, ub=10, vtype=CONTINUOUS, name="x") + problem.setObjective(x * x - 4 * x, sense=MINIMIZE) + + problem.solve() + + if problem.Status.name in ["Optimal", "PrimalFeasible"]: + print(f"x = {x.getValue():.4f}") + print(f"Minimized value = {problem.ObjValue:.4f}") + print(f"Original maximum = {-problem.ObjValue:.4f}") + else: + print(f"Status: {problem.Status.name}") + + +if __name__ == "__main__": + main()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-qp-api-python/assets/maximization_workaround/model.py around lines 10 - 22, The module currently runs solver code at import time (creating Problem, calling problem.addVariable, problem.setObjective and problem.solve and printing results); refactor by moving that sequence into a new main() function (e.g., def main(): create Problem("MaxWorkaround"), addVariable, setObjective, call problem.solve and print results) and add the standard guard if __name__ == "__main__": main() so importing the module no longer executes problem.solve() automatically..github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py (1)
42-44: Stream decompression instead of reading the full gzip file into memory.Line 42-44 uses
f_in.read()which buffers the full file; use streamed copy to reduce memory pressure.Proposed refactor
+import shutil @@ - with gzip.open(gz_file, "rb") as f_in: - with open(mps_file, "wb") as f_out: - f_out.write(f_in.read()) + with gzip.open(gz_file, "rb") as f_in: + with open(mps_file, "wb") as f_out: + shutil.copyfileobj(f_in, f_out)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 42 - 44, The current decompression reads the entire gzip into memory via f_in.read(); replace that with a streamed copy to avoid buffering the whole file—inside the with gzip.open(gz_file, "rb") as f_in: with open(mps_file, "wb") as f_out: block, use a streamed transfer (e.g., shutil.copyfileobj(f_in, f_out) or a small-chunk read/write loop) so gz_file is decompressed to mps_file incrementally rather than all at once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_basic/lp_simple.c:
- Around line 48-50: The calls to cuOptCreateSolverSettings and
cuOptSetFloatParameter (for CUOPT_ABSOLUTE_PRIMAL_TOLERANCE and
CUOPT_TIME_LIMIT) must have their return status checked and handled before
proceeding to cuOptSolve; update the code to capture the return values from
cuOptCreateSolverSettings and each cuOptSetFloatParameter call, verify they
equal success, and on failure clean up any allocated settings (or context) and
return/exit with an error instead of calling cuOptSolve with an invalid
configuration. Ensure you reference and check the statuses returned by
cuOptCreateSolverSettings, cuOptSetFloatParameter (for
CUOPT_ABSOLUTE_PRIMAL_TOLERANCE and CUOPT_TIME_LIMIT) and only call cuOptSolve
when those checks succeed.
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c:
- Around line 71-83: The code overwrites the solver result stored in status when
calling cuOptGetDualSolution and cuOptGetReducedCosts; preserve the original
solve result (e.g., store it as solveStatus) and use separate local statuses
(e.g., status_dual, status_reduced) for the post-solve queries so failures in
queries don't clobber the solve result. Specifically, keep the value returned by
the solver in status/solveStatus before allocating dual/reduced, call
cuOptGetDualSolution into status_dual and cuOptGetReducedCosts into
status_reduced (checking each independently before printing), and ensure the
function returns the original solveStatus rather than the last query status;
reference symbols: status, cuOptGetDualSolution, cuOptGetReducedCosts, solution,
dual, reduced, num_variables.
- Around line 48-50: The calls to cuOptCreateSolverSettings and
cuOptSetFloatParameter (for CUOPT_ABSOLUTE_PRIMAL_TOLERANCE and
CUOPT_TIME_LIMIT) currently ignore return status; update the code that creates
and configures settings (symbol: settings) to check each function's return
value, handle non-success results by logging the error and cleaning up resources
(free settings and any allocated solver objects) and exiting or returning an
error before calling the solve routine; follow the pattern used in
simple_lp_example.c: capture the status from cuOptCreateSolverSettings and each
cuOptSetFloatParameter, verify it indicates success, and on failure call an
appropriate error handler (log message including the failing function and
parameter) and perform necessary teardown.
In
@.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c:
- Around line 65-70: Allocation of the solution buffer (sol = malloc(...)) is
not handled on failure; if malloc returns NULL the code silently skips output
and may still indicate success—fix by checking for NULL after malloc and
handling it explicitly: when sol == NULL, emit an error message (e.g., fprintf
to stderr or use existing logger) that includes the attempted allocation
size/num_variables, and return a non-success status or call exit(1) so the
caller knows the operation failed; otherwise call
cuOptGetPrimalSolution(solution, sol), print the values with printf, and
free(sol) as currently done. Ensure references: sol, malloc, num_variables,
cuOptGetPrimalSolution, printf, free.
- Around line 50-53: The cuOpt calls are unchecked and may leave settings NULL
or other state invalid; update the calls to check each API return code
(cuOptCreateSolverSettings, cuOptSetFloatParameter for CUOPT_TIME_LIMIT and
CUOPT_MIP_RELATIVE_GAP) and handle failures by logging the error and
aborting/returning early so execution does not continue with an invalid settings
pointer; also ensure any variables used later (e.g., objective_value) are
initialized or only used after successful API returns.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md:
- Around line 60-64: Update the README example to clarify the custom-file
workflow: explain that users can either pass an absolute or relative path to any
.mps file via the model.py --file flag (mention model.py and the --file option)
or copy the .mps into this data directory and reference it directly (mention the
data/ directory and the file name pattern like your_problem.mps); change the
single generic command into two explicit options showing both a full/relative
path usage and a data-directory usage so readers know both workflows are
supported.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 119-121: The except (AttributeError, Exception): pass block in the
variable extraction loop is swallowing all errors and should be fixed to surface
real failures; replace the blanket except with specific handling (e.g., catch
AttributeError/KeyError when a variable property is legitimately missing), log a
clear warning including context (problem id/name and variable identifier) via
the module logger, and re-raise or propagate any other unexpected exceptions
instead of ignoring them so true bugs are not hidden; update the block around
the variable extraction code where the except (AttributeError, Exception)
currently appears to implement this targeted catch+log+rethrow behavior.
In @.github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py:
- Around line 26-28: The Series order_locations, pickup_indices, and
delivery_indices are created without an explicit dtype and default to int64,
violating the cuOpt API requirement for int32; update their construction so each
is an int32 cudf.Series (e.g., use cudf.Series(..., dtype="int32") or
.astype("int32")) before they are passed to set_order_locations and
set_pickup_delivery_pairs to match the other int32 usages (demand,
vehicle_capacity, set_vehicle_locations).
---
Outside diff comments:
In @.github/skills/cuopt-user-rules/SKILL.md:
- Around line 174-177: The "Exceptions (okay without asking)" section currently
permits running some user-requested or read-only commands, which contradicts the
later "No exceptions" rule for package changes; update the SKILL.md text by
explicitly excluding package install/upgrade commands from the exceptions list
and clarifying that "package install", "package upgrade", "apt-get install",
"pip install", and equivalent package manager commands are never allowed to run
without explicit confirmation; adjust the wording in the "Exceptions (okay
without asking)" paragraph and the package-change "No exceptions." paragraph so
they consistently state that package installation or upgrade commands are always
prohibited from automatic execution even if the user requests them.
---
Duplicate comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c:
- Around line 48-51: The calls to cuOptCreateSolverSettings and
cuOptSetFloatParameter (for CUOPT_MIP_ABSOLUTE_TOLERANCE,
CUOPT_MIP_RELATIVE_GAP, CUOPT_TIME_LIMIT) ignore return values and can fail
silently; update the code to check each function's return status (the result of
cuOptCreateSolverSettings and each cuOptSetFloatParameter call), and on error
log the status and perform appropriate cleanup/exit (e.g., free settings if
created and return non-zero or exit) so failures are not ignored—use the same
settings variable and the three parameter calls to locate where to add the
checks and error handling.
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c:
- Around line 41-43: All cuOpt API calls must have their return codes checked
before using outputs: wrap calls like cuOptCreateSolverSettings,
cuOptSetFloatParameter, cuOptSolveModel, and
cuOptGetSolutionObjectiveValue/cuOptGetSolutionPrimal/cuOptGetSolveInfo with
error checks and bail out (log error and return/fail) if the returned status is
not success; do this for the calls at the sites where settings are
created/parameters set (cuOptCreateSolverSettings, cuOptSetFloatParameter), when
solving (cuOptSolveModel), and when retrieving outputs
(cuOptGetSolutionObjectiveValue, cuOptGetSolutionPrimal, cuOptGetSolveInfo), so
that invalid outputs are never used after a failed API call.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 83-85: The prints/logs are capturing bound method objects instead
of their return values; call/invoke the cuOpt Problem accessors rather than
referencing them. Replace uses of problem.NumVariables, problem.NumConstraints,
and problem.IsMIP (also the occurrences around the later prints at the other
block) with problem.NumVariables(), problem.NumConstraints(), and
problem.IsMIP() if those are methods (or leave as properties only if inspection
of Problem shows `@property`), and update any JSON/log serialization to use the
returned values from NumVariables(), NumConstraints(), and IsMIP() rather than
the method objects.
- Line 129: The compare_gaps function currently computes gap_to_optimal using a
hardcoded AIR05 optimum regardless of the input file, causing misleading
metrics; update compare_gaps to determine the correct known_optimum per filepath
(e.g., map filenames or accept a known_optimum parameter) and use that value
when computing gap_to_optimal and related CLI output; locate references to
gap_to_optimal, known_optimum and the compare_gaps signature and either add a
function parameter (known_optimum: Optional[float]) or a filename→optimum
lookup, ensure CLI code that calls compare_gaps passes the correct optimum when
provided, and update the printed metrics to use the resolved optimum.
In @.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py:
- Line 23: The call dm.set_order_locations(cudf.Series([1, 2, 3])) uses a
default integer dtype (likely int64); change it to explicitly cast the series to
int32 before passing to set_order_locations so order_locations meets the API
dtype requirement—e.g., construct or cast the cudf.Series to dtype="int32" so
the argument passed to set_order_locations is int32.
---
Nitpick comments:
In @.github/skills/cuopt-lp-milp-api-cli/SKILL.md:
- Line 49: Add a single clarifying sentence after the line containing "Integer
variables: use 'MARKER' 'INTORG' before and 'MARKER' 'INTEND' after the integer
columns" stating that if the 'INTORG'/'INTEND' markers are not present the model
will be treated as an LP (not an MILP), and remind that a MIP requires at least
one integer variable to be considered MILP; reference the 'INTORG' and 'INTEND'
markers and the MARKER usage in that sentence.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 42-44: The current decompression reads the entire gzip into memory
via f_in.read(); replace that with a streamed copy to avoid buffering the whole
file—inside the with gzip.open(gz_file, "rb") as f_in: with open(mps_file, "wb")
as f_out: block, use a streamed transfer (e.g., shutil.copyfileobj(f_in, f_out)
or a small-chunk read/write loop) so gz_file is decompressed to mps_file
incrementally rather than all at once.
In @.github/skills/cuopt-qp-api-python/assets/maximization_workaround/model.py:
- Around line 10-22: The module currently runs solver code at import time
(creating Problem, calling problem.addVariable, problem.setObjective and
problem.solve and printing results); refactor by moving that sequence into a new
main() function (e.g., def main(): create Problem("MaxWorkaround"), addVariable,
setObjective, call problem.solve and print results) and add the standard guard
if __name__ == "__main__": main() so importing the module no longer executes
problem.solve() automatically.
In @.github/skills/cuopt-user-rules/SKILL.md:
- Around line 16-19: The consecutive bullets repeating “What …?” are repetitive;
edit the SKILL.md bullets to use parallel, concise phrasing instead (e.g.,
"Language/interface", "Problem type", "Relevant constraints", "Desired output
format") so they read cleaner and avoid repeating the word "What" while
preserving the original intent and order of the four questions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/skills/cuopt-installation-developer/SKILL.md.github/skills/cuopt-lp-milp-api-c/assets/lp_basic/lp_simple.c.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c.github/skills/cuopt-lp-milp-api-cli/SKILL.md.github/skills/cuopt-lp-milp-api-python/assets/lp_duals/model.py.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py.github/skills/cuopt-qp-api-python/assets/least_squares/model.py.github/skills/cuopt-qp-api-python/assets/maximization_workaround/model.py.github/skills/cuopt-qp-api-python/assets/portfolio/model.py.github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py.github/skills/cuopt-user-rules/SKILL.md
✅ Files skipped from review due to trivial changes (1)
- .github/skills/cuopt-installation-developer/SKILL.md
Comment on lines
+71
to
+83
| status = cuOptGetDualSolution(solution, dual); | ||
| if (status == CUOPT_SUCCESS) { | ||
| printf("Constraint c1 DualValue = %f\n", dual[0]); | ||
| printf("Constraint c2 DualValue = %f\n", dual[1]); | ||
| } | ||
| free(dual); | ||
| } | ||
|
|
||
| cuopt_float_t *reduced = malloc((size_t)num_variables * sizeof(cuopt_float_t)); | ||
| if (reduced) { | ||
| status = cuOptGetReducedCosts(solution, reduced); | ||
| if (status == CUOPT_SUCCESS) { | ||
| printf("x ReducedCost = %f, y ReducedCost = %f, z ReducedCost = %f\n", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd .github/skills/cuopt-lp-milp-api-c/assets/lp_duals && wc -l lp_duals.cRepository: NVIDIA/cuopt
Length of output: 69
🏁 Script executed:
cd .github/skills/cuopt-lp-milp-api-c/assets/lp_duals && cat -n lp_duals.c | sed -n '60,100p'Repository: NVIDIA/cuopt
Length of output: 1513
🏁 Script executed:
cd .github/skills/cuopt-lp-milp-api-c/assets/lp_duals && cat -n lp_duals.c | sed -n '1,70p'Repository: NVIDIA/cuopt
Length of output: 3069
Do not overwrite global status with post-solve query calls.
At lines 71 and 81, status is reused for dual and reduced-cost retrieval. If the dual solution retrieval fails but reduced costs retrieval succeeds, the function returns success while masking the dual solution failure. Conversely, if only reduced costs retrieval fails, the return signals failure despite successfully obtaining dual values. Use separate status variables for each query and preserve the solve result.
Suggested fix
- cuopt_float_t *dual = malloc((size_t)num_constraints * sizeof(cuopt_float_t));
+ cuopt_float_t *dual = malloc((size_t)num_constraints * sizeof(cuopt_float_t));
if (dual) {
- status = cuOptGetDualSolution(solution, dual);
- if (status == CUOPT_SUCCESS) {
+ cuopt_int_t dual_status = cuOptGetDualSolution(solution, dual);
+ if (dual_status == CUOPT_SUCCESS) {
printf("Constraint c1 DualValue = %f\n", dual[0]);
printf("Constraint c2 DualValue = %f\n", dual[1]);
+ } else {
+ printf("Error getting dual solution: %d\n", dual_status);
+ if (status == CUOPT_SUCCESS) status = dual_status;
}
free(dual);
}
@@
- cuopt_float_t *reduced = malloc((size_t)num_variables * sizeof(cuopt_float_t));
+ cuopt_float_t *reduced = malloc((size_t)num_variables * sizeof(cuopt_float_t));
if (reduced) {
- status = cuOptGetReducedCosts(solution, reduced);
- if (status == CUOPT_SUCCESS) {
+ cuopt_int_t reduced_status = cuOptGetReducedCosts(solution, reduced);
+ if (reduced_status == CUOPT_SUCCESS) {
printf("x ReducedCost = %f, y ReducedCost = %f, z ReducedCost = %f\n",
reduced[0], reduced[1], reduced[2]);
+ } else {
+ printf("Error getting reduced costs: %d\n", reduced_status);
+ if (status == CUOPT_SUCCESS) status = reduced_status;
}
free(reduced);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c around lines
71 - 83, The code overwrites the solver result stored in status when calling
cuOptGetDualSolution and cuOptGetReducedCosts; preserve the original solve
result (e.g., store it as solveStatus) and use separate local statuses (e.g.,
status_dual, status_reduced) for the post-solve queries so failures in queries
don't clobber the solve result. Specifically, keep the value returned by the
solver in status/solveStatus before allocating dual/reduced, call
cuOptGetDualSolution into status_dual and cuOptGetReducedCosts into
status_reduced (checking each independently before printing), and ensure the
function returns the original solveStatus rather than the last query status;
reference symbols: status, cuOptGetDualSolution, cuOptGetReducedCosts, solution,
dual, reduced, num_variables.
.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c
Outdated
Show resolved
Hide resolved
Comment on lines
+65
to
+70
| cuopt_float_t *sol = malloc((size_t)num_variables * sizeof(cuopt_float_t)); | ||
| if (sol) { | ||
| cuOptGetPrimalSolution(solution, sol); | ||
| printf("Product_A: %f, Product_B: %f\n", sol[0], sol[1]); | ||
| free(sol); | ||
| } |
There was a problem hiding this comment.
Don’t silently ignore solution-buffer allocation failure.
If malloc fails, the program currently suppresses variable output and may still return success, which hides the failure.
Suggested fix
cuopt_float_t *sol = malloc((size_t)num_variables * sizeof(cuopt_float_t));
- if (sol) {
+ if (sol) {
cuOptGetPrimalSolution(solution, sol);
printf("Product_A: %f, Product_B: %f\n", sol[0], sol[1]);
free(sol);
+ } else {
+ printf("Error: failed to allocate primal solution buffer\n");
+ status = -1;
+ goto cleanup;
}📝 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.
Suggested change
| cuopt_float_t *sol = malloc((size_t)num_variables * sizeof(cuopt_float_t)); | |
| if (sol) { | |
| cuOptGetPrimalSolution(solution, sol); | |
| printf("Product_A: %f, Product_B: %f\n", sol[0], sol[1]); | |
| free(sol); | |
| } | |
| cuopt_float_t *sol = malloc((size_t)num_variables * sizeof(cuopt_float_t)); | |
| if (sol) { | |
| cuOptGetPrimalSolution(solution, sol); | |
| printf("Product_A: %f, Product_B: %f\n", sol[0], sol[1]); | |
| free(sol); | |
| } else { | |
| printf("Error: failed to allocate primal solution buffer\n"); | |
| return -1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c
around lines 65 - 70, Allocation of the solution buffer (sol = malloc(...)) is
not handled on failure; if malloc returns NULL the code silently skips output
and may still indicate success—fix by checking for NULL after malloc and
handling it explicitly: when sol == NULL, emit an error message (e.g., fprintf
to stderr or use existing logger) that includes the attempted allocation
size/num_variables, and return a non-success status or call exit(1) so the
caller knows the operation failed; otherwise call
cuOptGetPrimalSolution(solution, sol), print the values with printf, and
free(sol) as currently done. Ensure references: sol, malloc, num_variables,
cuOptGetPrimalSolution, printf, free.
Comment on lines
+60
to
+64
| ## Adding Custom MPS Files | ||
|
|
||
| ```bash | ||
| python model.py --file path/to/your/problem.mps | ||
| ``` |
There was a problem hiding this comment.
Clarify the custom-file workflow command.
Line 63 currently looks like a generic run command, not an “add custom MPS files” workflow. Please specify whether users should place files in this directory, pass absolute paths, or both.
Proposed doc clarification
## Adding Custom MPS Files
```bash
-python model.py --file path/to/your/problem.mps
+# Option 1: keep your file anywhere and pass its path
+python model.py --file /absolute/or/relative/path/to/your/problem.mps
+
+# Option 2: copy into this directory and reference it directly
+python model.py --file .github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/your_problem.mps</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/data/README.md
around lines 60 - 64, Update the README example to clarify the custom-file
workflow: explain that users can either pass an absolute or relative path to any
.mps file via the model.py --file flag (mention model.py and the --file option)
or copy the .mps into this data directory and reference it directly (mention the
data/ directory and the file name pattern like your_problem.mps); change the
single generic command into two explicit options showing both a full/relative
path usage and a data-directory usage so readers know both workflows are
supported.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+119
to
+121
| except (AttributeError, Exception): | ||
| # For MPS problems, variable access may be limited | ||
| pass |
There was a problem hiding this comment.
Avoid blind except ...: pass during variable extraction.
Line 119-121 swallows all failures and hides real issues; this makes bad outputs hard to diagnose.
Proposed fix
- except (AttributeError, Exception):
- # For MPS problems, variable access may be limited
- pass
+ except AttributeError:
+ if verbose:
+ print("Variable values are unavailable for this MPS-loaded problem.")🧰 Tools
🪛 Ruff (0.15.2)
[error] 119-121: try-except-pass detected, consider logging the exception
(S110)
[warning] 119-119: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around
lines 119 - 121, The except (AttributeError, Exception): pass block in the
variable extraction loop is swallowing all errors and should be fixed to surface
real failures; replace the blanket except with specific handling (e.g., catch
AttributeError/KeyError when a variable property is legitimately missing), log a
clear warning including context (problem id/name and variable identifier) via
the module logger, and re-raise or propagate any other unexpected exceptions
instead of ignoring them so true bugs are not hidden; update the block around
the variable extraction code where the except (AttributeError, Exception)
currently appears to implement this targeted catch+log+rethrow behavior.
Comment on lines
+26
to
+28
| order_locations = cudf.Series([1, 2, 3, 4]) | ||
| pickup_indices = cudf.Series([0, 2]) | ||
| delivery_indices = cudf.Series([1, 3]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and check if it exists
find .github/skills/cuopt-routing-api-python -name "model.py" -type fRepository: NVIDIA/cuopt
Length of output: 187
🏁 Script executed:
# Check the content at the specified lines
head -45 .github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py | tail -25Repository: NVIDIA/cuopt
Length of output: 824
🏁 Script executed:
# Search for set_order_locations to understand the dtype contract
rg -n "set_order_locations|int32" --type py | head -30Repository: NVIDIA/cuopt
Length of output: 3710
🏁 Script executed:
# Search for any cuopt routing Python API documentation or type hints
find . -name "*.py" -path "*/cuopt*" -type f | head -20Repository: NVIDIA/cuopt
Length of output: 1493
🏁 Script executed:
# Search for DataModel class and set_order_locations method definition
find . -name "*.py" -type f -exec grep -l "class DataModel\|def set_order_locations" {} \; | head -20Repository: NVIDIA/cuopt
Length of output: 168
🏁 Script executed:
# Look for routing module to find API definitions
find . -path "*/routing/*" -name "*.py" -type f | grep -v __pycache__ | head -20Repository: NVIDIA/cuopt
Length of output: 1164
🏁 Script executed:
# Check if there's API documentation or type hints
rg -n "set_order_locations|set_pickup_delivery_pairs" -A5 --type py | head -50Repository: NVIDIA/cuopt
Length of output: 4322
🏁 Script executed:
# Extract the set_order_locations and set_pickup_delivery_pairs function signatures and documentation
sed -n '1,500p' ./python/cuopt/cuopt/routing/vehicle_routing.py | grep -A20 "def set_order_locations\|def set_pickup_delivery_pairs"Repository: NVIDIA/cuopt
Length of output: 873
🏁 Script executed:
# Look at the test_pickup_delivery.py to understand the dtype expectations
cat -n ./python/cuopt/cuopt/tests/routing/test_pickup_delivery.py | head -120Repository: NVIDIA/cuopt
Length of output: 4457
🏁 Script executed:
# Check if there are any dtype conversions or validation in vehicle_routing.py
rg -n "int32|dtype" ./python/cuopt/cuopt/routing/vehicle_routing.py | head -30Repository: NVIDIA/cuopt
Length of output: 1664
Cast routing ID/index series to int32 explicitly.
The cuOpt API documentation for set_order_locations and set_pickup_delivery_pairs explicitly requires cudf.Series dtype - int32, but Lines 26–28 create these Series from Python lists without specifying dtype, which defaults to int64. This violates the API contract and is inconsistent with the same file's treatment of demand, vehicle_capacity, and set_vehicle_locations (which all use explicit dtype="int32").
Proposed fix
-order_locations = cudf.Series([1, 2, 3, 4])
-pickup_indices = cudf.Series([0, 2])
-delivery_indices = cudf.Series([1, 3])
+order_locations = cudf.Series([1, 2, 3, 4], dtype="int32")
+pickup_indices = cudf.Series([0, 2], dtype="int32")
+delivery_indices = cudf.Series([1, 3], dtype="int32")📝 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.
Suggested change
| order_locations = cudf.Series([1, 2, 3, 4]) | |
| pickup_indices = cudf.Series([0, 2]) | |
| delivery_indices = cudf.Series([1, 3]) | |
| order_locations = cudf.Series([1, 2, 3, 4], dtype="int32") | |
| pickup_indices = cudf.Series([0, 2], dtype="int32") | |
| delivery_indices = cudf.Series([1, 3], dtype="int32") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py around
lines 26 - 28, The Series order_locations, pickup_indices, and delivery_indices
are created without an explicit dtype and default to int64, violating the cuOpt
API requirement for int32; update their construction so each is an int32
cudf.Series (e.g., use cudf.Series(..., dtype="int32") or .astype("int32"))
before they are passed to set_order_locations and set_pickup_delivery_pairs to
match the other int32 usages (demand, vehicle_capacity, set_vehicle_locations).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c (1)
76-86:⚠️ Potential issue | 🟡 MinorHandle solution-buffer allocation failure explicitly.
On Line 76, if
mallocreturnsNULL, the code currently skips primal extraction and can still exit successfully. Please set a non-successstatusand go to cleanup in that branch.Suggested fix
cuopt_float_t *sol = malloc((size_t)num_variables * sizeof(cuopt_float_t)); if (sol) { status = cuOptGetPrimalSolution(solution, sol); if (status != CUOPT_SUCCESS) { printf("Error getting primal solution: %d\n", status); free(sol); goto cleanup; } printf("Product_A: %f, Product_B: %f\n", sol[0], sol[1]); free(sol); + } else { + fprintf(stderr, "Error: failed to allocate primal solution buffer\n"); + status = -1; + goto cleanup; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c around lines 76 - 86, If malloc for sol returns NULL, the code currently continues as if successful; update the malloc failure branch in the block that allocates cuopt_float_t *sol so it sets a non-success status (e.g. status = CUOPT_ERROR or another appropriate non-success constant) and jumps to the existing cleanup label (goto cleanup) instead of silently skipping extraction; adjust the branch around sol, cuOptGetPrimalSolution, and cleanup to ensure allocation failures are handled consistently..github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c (1)
85-109:⚠️ Potential issue | 🟠 MajorStatus variable overwrite masks potential failures.
Lines 87 and 97 overwrite
statuswith post-solve query results. IfcuOptGetDualSolutionfails butcuOptGetReducedCostssucceeds, the return value incorrectly indicates success while masking the dual retrieval failure. Use separate status variables for these optional queries to preserve the solve result.Suggested fix
cuopt_float_t *dual = malloc((size_t)num_constraints * sizeof(cuopt_float_t)); if (dual) { - status = cuOptGetDualSolution(solution, dual); - if (status == CUOPT_SUCCESS) { + cuopt_int_t dual_status = cuOptGetDualSolution(solution, dual); + if (dual_status == CUOPT_SUCCESS) { printf("Constraint c1 DualValue = %f\n", dual[0]); printf("Constraint c2 DualValue = %f\n", dual[1]); + } else { + printf("Warning: could not retrieve dual solution: %d\n", dual_status); } free(dual); } cuopt_float_t *reduced = malloc((size_t)num_variables * sizeof(cuopt_float_t)); if (reduced) { - status = cuOptGetReducedCosts(solution, reduced); - if (status == CUOPT_SUCCESS) { + cuopt_int_t reduced_status = cuOptGetReducedCosts(solution, reduced); + if (reduced_status == CUOPT_SUCCESS) { printf("x ReducedCost = %f, y ReducedCost = %f, z ReducedCost = %f\n", reduced[0], reduced[1], reduced[2]); + } else { + printf("Warning: could not retrieve reduced costs: %d\n", reduced_status); } free(reduced); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c around lines 85 - 109, The code overwrites the solver `status` when calling cuOptGetDualSolution and cuOptGetReducedCosts, masking earlier failures; change to use separate local status variables (e.g., status_dual and status_reduced) for those post-solve queries, only log or handle their errors independently, and keep the original `status` returned by the solver (used in the final return expression). Update the calls to cuOptGetDualSolution and cuOptGetReducedCosts to store results in status_dual/status_reduced and conditionally print based on those, leaving `status` (from the solve) unchanged so the function return reflects the actual solve result.
🧹 Nitpick comments (4)
.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py (1)
38-39: Consider adding a timeout to the HTTP download.
urllib.request.urlretrievecan hang indefinitely on network issues. For robustness, consider usingurllib.request.urlopenwith a timeout.♻️ Suggested improvement
- print("Downloading air05.mps from MIPLIB...") - urllib.request.urlretrieve(AIR05_URL, gz_file) + print("Downloading air05.mps from MIPLIB...") + with urllib.request.urlopen(AIR05_URL, timeout=60) as response: + with open(gz_file, "wb") as out_file: + out_file.write(response.read())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 38 - 39, Replace the blocking urllib.request.urlretrieve call with a bounded download using urllib.request.urlopen(AIR05_URL, timeout=...) and stream the response into gz_file; locate the call where AIR05_URL is used and gz_file is written (the print + urllib.request.urlretrieve lines) and change it to open the URL with a timeout, read in chunks and write to gz_file, ensuring the response and file are properly closed (use context managers) and surface/handle timeout or HTTP errors appropriately..github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c (1)
1-4: Missing SPDX license header for consistency.Unlike
lp_simple.c, this file is missing the SPDX license header. For consistency across all example assets and compliance, add the standard NVIDIA copyright and Apache-2.0 license header.Suggested fix
/* + * SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +/* * LP with dual values and reduced costs (C API). * Problem: Minimize 3x + 2y + 5z subject to x + y + z = 4, 2x + y + z = 5, x,y,z >= 0. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c around lines 1 - 4, Add the missing SPDX license header at the top of lp_duals.c to match lp_simple.c: insert the standard NVIDIA copyright and Apache-2.0 SPDX header as the first lines of the file (above the existing block comment "LP with dual values and reduced costs (C API).") so the file begins with the same SPDX header text used across the example assets for consistency and compliance..github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c (1)
80-95: Consider reporting malloc failure.If
mallocreturnsNULL, the primal solution is silently skipped. For a more informative example, consider logging the failure.Optional improvement
primal = malloc((size_t)num_variables * sizeof(cuopt_float_t)); - if (primal) { + if (!primal) { + fprintf(stderr, "Warning: Failed to allocate memory for primal solution\n"); + } else { status = cuOptGetPrimalSolution(solution, primal);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c around lines 80 - 95, The code currently skips reporting when malloc((size_t)num_variables * sizeof(cuopt_float_t)) returns NULL; update the block around primal allocation (variable primal) to check for NULL and log an error (e.g., fprintf(stderr, "Failed to allocate primal buffer of %zu bytes\n", (size_t)num_variables * sizeof(cuopt_float_t))) and then handle the failure consistently (free nothing and goto cleanup or set an error status) before attempting cuOptGetPrimalSolution; keep the existing cuOptGetPrimalSolution, printing and free logic unchanged for the non-NULL case..github/skills/cuopt-routing-api-python/resources/examples.md (1)
31-31: Consider adding a brief comment explaining dtype specification.Since this is a documentation file meant to teach users, consider adding a brief inline comment explaining why
dtype="int32"is specified, especially if it's required by the API or recommended for performance reasons.📝 Suggested documentation enhancement
# Order data (customers 1-5) -order_locations = cudf.Series([1, 2, 3, 4, 5], dtype="int32") # Location indices for orders +order_locations = cudf.Series([1, 2, 3, 4, 5], dtype="int32") # Location indices (int32 required by API)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-routing-api-python/resources/examples.md at line 31, Add a short inline comment next to the order_locations = cudf.Series(..., dtype="int32") example explaining why dtype="int32" is chosen (e.g., required by the API for index matching and/or recommended for memory/performance/compatibility with downstream C++/GPU operations), so readers understand the dtype requirement or recommendation when creating order location series.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c:
- Around line 23-27: The code returns early on cuOptReadProblem failure,
skipping the existing cleanup logic; change the direct "return 1" after checking
status != CUOPT_SUCCESS to "goto cleanup" so the cleanup label executes and
frees any partially allocated resources associated with problem, and ensure the
cleanup block (label cleanup) safely handles a partially-initialized
cuOptProblem object (check for non-NULL handles or flags before freeing) so no
double-free or use-after-free occurs.
---
Duplicate comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c:
- Around line 85-109: The code overwrites the solver `status` when calling
cuOptGetDualSolution and cuOptGetReducedCosts, masking earlier failures; change
to use separate local status variables (e.g., status_dual and status_reduced)
for those post-solve queries, only log or handle their errors independently, and
keep the original `status` returned by the solver (used in the final return
expression). Update the calls to cuOptGetDualSolution and cuOptGetReducedCosts
to store results in status_dual/status_reduced and conditionally print based on
those, leaving `status` (from the solve) unchanged so the function return
reflects the actual solve result.
In
@.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c:
- Around line 76-86: If malloc for sol returns NULL, the code currently
continues as if successful; update the malloc failure branch in the block that
allocates cuopt_float_t *sol so it sets a non-success status (e.g. status =
CUOPT_ERROR or another appropriate non-success constant) and jumps to the
existing cleanup label (goto cleanup) instead of silently skipping extraction;
adjust the branch around sol, cuOptGetPrimalSolution, and cleanup to ensure
allocation failures are handled consistently.
---
Nitpick comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c:
- Around line 1-4: Add the missing SPDX license header at the top of lp_duals.c
to match lp_simple.c: insert the standard NVIDIA copyright and Apache-2.0 SPDX
header as the first lines of the file (above the existing block comment "LP with
dual values and reduced costs (C API).") so the file begins with the same SPDX
header text used across the example assets for consistency and compliance.
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c:
- Around line 80-95: The code currently skips reporting when
malloc((size_t)num_variables * sizeof(cuopt_float_t)) returns NULL; update the
block around primal allocation (variable primal) to check for NULL and log an
error (e.g., fprintf(stderr, "Failed to allocate primal buffer of %zu bytes\n",
(size_t)num_variables * sizeof(cuopt_float_t))) and then handle the failure
consistently (free nothing and goto cleanup or set an error status) before
attempting cuOptGetPrimalSolution; keep the existing cuOptGetPrimalSolution,
printing and free logic unchanged for the non-NULL case.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 38-39: Replace the blocking urllib.request.urlretrieve call with a
bounded download using urllib.request.urlopen(AIR05_URL, timeout=...) and stream
the response into gz_file; locate the call where AIR05_URL is used and gz_file
is written (the print + urllib.request.urlretrieve lines) and change it to open
the URL with a timeout, read in chunks and write to gz_file, ensuring the
response and file are properly closed (use context managers) and surface/handle
timeout or HTTP errors appropriately.
In @.github/skills/cuopt-routing-api-python/resources/examples.md:
- Line 31: Add a short inline comment next to the order_locations =
cudf.Series(..., dtype="int32") example explaining why dtype="int32" is chosen
(e.g., required by the API for index matching and/or recommended for
memory/performance/compatibility with downstream C++/GPU operations), so readers
understand the dtype requirement or recommendation when creating order location
series.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/skills/cuopt-installation-api-c/resources/verification_examples.md.github/skills/cuopt-installation-api-python/SKILL.md.github/skills/cuopt-installation-api-python/resources/verification_examples.md.github/skills/cuopt-lp-milp-api-c/assets/lp_basic/lp_simple.c.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c.github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c.github/skills/cuopt-lp-milp-api-cli/assets/milp_facility/facility.mps.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py.github/skills/cuopt-qp-api-python/SKILL.md.github/skills/cuopt-routing-api-python/SKILL.md.github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py.github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py.github/skills/cuopt-routing-api-python/resources/examples.md
✅ Files skipped from review due to trivial changes (1)
- .github/skills/cuopt-installation-api-python/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/skills/cuopt-lp-milp-api-c/assets/milp_basic/milp_simple.c
- .github/skills/cuopt-routing-api-python/SKILL.md
- .github/skills/cuopt-installation-api-python/resources/verification_examples.md
- .github/skills/cuopt-routing-api-python/assets/pdp_basic/model.py
- .github/skills/cuopt-routing-api-python/assets/vrp_basic/model.py
Comment on lines
+23
to
+27
| cuopt_int_t status = cuOptReadProblem(filename, &problem); | ||
| if (status != CUOPT_SUCCESS) { | ||
| printf("Error reading MPS file: %d\n", status); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Early return bypasses cleanup.
If cuOptReadProblem fails after partially allocating internal resources, the direct return 1 skips the cleanup block. For consistency with the rest of the file and to ensure proper resource release, use goto cleanup instead.
Proposed fix
cuopt_int_t status = cuOptReadProblem(filename, &problem);
if (status != CUOPT_SUCCESS) {
- printf("Error reading MPS file: %d\n", status);
- return 1;
+ fprintf(stderr, "Error reading MPS file: %d\n", status);
+ goto cleanup;
}📝 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.
Suggested change
| cuopt_int_t status = cuOptReadProblem(filename, &problem); | |
| if (status != CUOPT_SUCCESS) { | |
| printf("Error reading MPS file: %d\n", status); | |
| return 1; | |
| } | |
| cuopt_int_t status = cuOptReadProblem(filename, &problem); | |
| if (status != CUOPT_SUCCESS) { | |
| fprintf(stderr, "Error reading MPS file: %d\n", status); | |
| goto cleanup; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c around
lines 23 - 27, The code returns early on cuOptReadProblem failure, skipping the
existing cleanup logic; change the direct "return 1" after checking status !=
CUOPT_SUCCESS to "goto cleanup" so the cleanup label executes and frees any
partially allocated resources associated with problem, and ensure the cleanup
block (label cleanup) safely handles a partially-initialized cuOptProblem object
(check for non-NULL handles or flags before freeing) so no double-free or
use-after-free occurs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py (2)
84-86:⚠️ Potential issue | 🔴 CriticalInvoke
Problemmethods instead of storing/printing bound method objects.Line 84, Line 85, Line 86, and Line 105-Line 108 currently use method references, not values. This can break at Line 171-Line 175 and Line 276-Line 281 when arithmetic/formatting expects numbers/bools.
Proposed fix
- print(f"Variables: {problem.NumVariables}") - print(f"Constraints: {problem.NumConstraints}") - print(f"Is MIP: {problem.IsMIP}") + print(f"Variables: {problem.NumVariables()}") + print(f"Constraints: {problem.NumConstraints()}") + print(f"Is MIP: {problem.IsMIP()}") @@ - "objective": problem.ObjValue, - "num_variables": problem.NumVariables, - "num_constraints": problem.NumConstraints, - "is_mip": problem.IsMIP, + "objective": problem.ObjValue(), + "num_variables": problem.NumVariables(), + "num_constraints": problem.NumConstraints(), + "is_mip": problem.IsMIP(),#!/bin/bash set -euo pipefail # Verify cuOpt Problem API definitions are methods rg -n -C2 'def (NumVariables|NumConstraints|IsMIP|ObjValue)\(' python/cuopt/cuopt/linear_programming/problem.py # Verify callsites in this skill file (should use parentheses) rg -n -C2 'NumVariables|NumConstraints|IsMIP|ObjValue' .github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.pyAlso applies to: 105-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 84 - 86, The prints and later uses are referencing bound method objects instead of calling them — update all occurrences to call the Problem accessor methods (e.g., use problem.NumVariables(), problem.NumConstraints(), problem.IsMIP(), and problem.ObjValue() where referenced) so numeric/bool values are used in arithmetic/formatting; search for usages of NumVariables, NumConstraints, IsMIP, and ObjValue in model.py and replace the property references with method invocations (retain existing formatting/log messages).
120-122:⚠️ Potential issue | 🟠 MajorAvoid swallowing all variable-extraction failures.
Line 120-Line 122 catches
Exceptionand silently ignores it, which hides real bugs and produces silently incomplete solutions.Proposed fix
- except (AttributeError, Exception): - # For MPS problems, variable access may be limited - pass + except AttributeError: + if verbose: + print("Variable values are unavailable for this MPS-loaded problem.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 120 - 122, The current try/except block in model.py that uses "except (AttributeError, Exception): pass" swallows all errors; change it so only AttributeError is silently ignored (keep "except AttributeError: pass") and handle other exceptions explicitly by capturing them (e.g., "except Exception as e:") and logging the exception with context via the module/class logger (use logger.exception or logger.error with the exception) and then re-raise the exception so real bugs are not hidden; update the try/except surrounding the variable-extraction logic (the block currently using "except (AttributeError, Exception): pass") accordingly..github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c (1)
28-32:⚠️ Potential issue | 🟡 MinorRoute
cuOptReadProblemfailures through shared cleanup.Line 31 returns early and skips the
cleanupblock used everywhere else. Keep failure handling consistent and jump tocleanuphere too.Suggested patch
cuopt_int_t status = cuOptReadProblem(filename, &problem); if (status != CUOPT_SUCCESS) { - printf("Error reading MPS file: %d\n", status); - return 1; + fprintf(stderr, "Error reading MPS file: %d\n", status); + goto cleanup; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c around lines 28 - 32, The early return after calling cuOptReadProblem(...) skips the shared cleanup block; replace the direct return with a jump to the common cleanup path: capture the error in the existing status variable (or set an int retval = 1), log the error as currently done, then goto cleanup so that the problem object and other resources are freed consistently; update cleanup to return retval (or status) at the end. Ensure changes reference cuOptReadProblem, status, problem, filename and the existing cleanup label/block.
🧹 Nitpick comments (2)
.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c (1)
78-79: Consider adding error messages for malloc failures.When
mallocreturnsNULLat lines 78, 90, or 100, the code silently continues without any diagnostic message. This is inconsistent with the error handling pattern elsewhere in the file. For an example/educational asset, explicit failure messages help users diagnose issues.Optional: Add malloc failure messages
cuopt_float_t *primal = malloc((size_t)num_variables * sizeof(cuopt_float_t)); - if (primal) { + if (!primal) { + printf("Warning: could not allocate memory for primal solution\n"); + } else { status = cuOptGetPrimalSolution(solution, primal);Apply similar pattern to
dualandreducedallocations.Also applies to: 90-91, 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c around lines 78 - 79, When malloc for primal, dual, or reduced returns NULL (checks around the pointers primal, dual, reduced), add explicit error messages and proper cleanup/return like the other error paths in lp_duals.c: log a descriptive error (including which allocation failed and the requested size/num variables if available) and free any previously allocated buffers before returning an error code or NULL; update the malloc checks for primal, dual, and reduced to follow this pattern so failures are not silent..github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py (1)
161-163: Use_for the unused tuple item incompare_gaps.
problemis unpacked but not used in this block.Proposed fix
- problem, solution = solve_mps( + _, solution = solve_mps( filepath=filepath, time_limit=time_limit, mip_gap=gap, verbose=True )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around lines 161 - 163, The tuple returned by solve_mps is being unpacked as "problem, solution" in compare_gaps but "problem" is never used; change the unpack to "_, solution = solve_mps(...)" inside the compare_gaps function (the call that passes filepath, time_limit, mip_gap=gap, verbose=True) so the unused value is clearly ignored and lint warnings are avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c:
- Around line 85-100: The malloc for primal can fail silently; after calling
malloc((size_t)num_variables * sizeof(cuopt_float_t)) check if primal == NULL
when num_variables > 0 and on failure set a non-success status (e.g., status =
CUOPT_ERROR or another non-zero sentinel), print an explicit error message about
allocation failure (including num_variables), and jump to cleanup (or return)
instead of continuing; ensure references to primal, num_variables, status, and
cuOptGetPrimalSolution are used to locate and modify the logic so failure is
handled consistently.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 170-175: The current computation of results[gap]["gap_to_optimal"]
(using solution["objective"] and known_optimal) can divide by zero when
known_optimal == 0; update the blocks around the computation (the if
known_optimal is not None checks that set results[gap]["gap_to_optimal"] and the
similar block near lines handling the second gap) to explicitly guard against
known_optimal == 0 and handle that case by either setting gap_to_optimal to None
(or math.inf) or computing an absolute difference instead, so replace the
unconditional division with a conditional that checks known_optimal != 0 before
dividing and provides the chosen fallback when it is zero.
---
Duplicate comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c:
- Around line 28-32: The early return after calling cuOptReadProblem(...) skips
the shared cleanup block; replace the direct return with a jump to the common
cleanup path: capture the error in the existing status variable (or set an int
retval = 1), log the error as currently done, then goto cleanup so that the
problem object and other resources are freed consistently; update cleanup to
return retval (or status) at the end. Ensure changes reference cuOptReadProblem,
status, problem, filename and the existing cleanup label/block.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 84-86: The prints and later uses are referencing bound method
objects instead of calling them — update all occurrences to call the Problem
accessor methods (e.g., use problem.NumVariables(), problem.NumConstraints(),
problem.IsMIP(), and problem.ObjValue() where referenced) so numeric/bool values
are used in arithmetic/formatting; search for usages of NumVariables,
NumConstraints, IsMIP, and ObjValue in model.py and replace the property
references with method invocations (retain existing formatting/log messages).
- Around line 120-122: The current try/except block in model.py that uses
"except (AttributeError, Exception): pass" swallows all errors; change it so
only AttributeError is silently ignored (keep "except AttributeError: pass") and
handle other exceptions explicitly by capturing them (e.g., "except Exception as
e:") and logging the exception with context via the module/class logger (use
logger.exception or logger.error with the exception) and then re-raise the
exception so real bugs are not hidden; update the try/except surrounding the
variable-extraction logic (the block currently using "except (AttributeError,
Exception): pass") accordingly.
---
Nitpick comments:
In @.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c:
- Around line 78-79: When malloc for primal, dual, or reduced returns NULL
(checks around the pointers primal, dual, reduced), add explicit error messages
and proper cleanup/return like the other error paths in lp_duals.c: log a
descriptive error (including which allocation failed and the requested size/num
variables if available) and free any previously allocated buffers before
returning an error code or NULL; update the malloc checks for primal, dual, and
reduced to follow this pattern so failures are not silent.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py:
- Around line 161-163: The tuple returned by solve_mps is being unpacked as
"problem, solution" in compare_gaps but "problem" is never used; change the
unpack to "_, solution = solve_mps(...)" inside the compare_gaps function (the
call that passes filepath, time_limit, mip_gap=gap, verbose=True) so the unused
value is clearly ignored and lint warnings are avoided.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/skills/cuopt-lp-milp-api-c/assets/README.md.github/skills/cuopt-lp-milp-api-c/assets/lp_duals/lp_duals.c.github/skills/cuopt-lp-milp-api-c/assets/milp_production_planning/milp_production.c.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c.github/skills/cuopt-lp-milp-api-cli/assets/README.md.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py
✅ Files skipped from review due to trivial changes (1)
- .github/skills/cuopt-lp-milp-api-c/assets/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/skills/cuopt-lp-milp-api-cli/assets/README.md
Comment on lines
+85
to
+100
| primal = malloc((size_t)num_variables * sizeof(cuopt_float_t)); | ||
| if (primal) { | ||
| status = cuOptGetPrimalSolution(solution, primal); | ||
| if (status != CUOPT_SUCCESS) { | ||
| printf("Error getting primal solution: %d\n", status); | ||
| free(primal); | ||
| primal = NULL; | ||
| goto cleanup; | ||
| } | ||
| printf("Primal (first 10): "); | ||
| for (cuopt_int_t i = 0; i < (num_variables < 10 ? num_variables : 10); i++) | ||
| printf("%f ", primal[i]); | ||
| if (num_variables > 10) printf("... (%d total)", (int)num_variables); | ||
| printf("\n"); | ||
| free(primal); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "mps_solver.c" --type fileRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
find .github -name "*mps_solver*" -o -name "*.c" 2>/dev/null | head -20Repository: NVIDIA/cuopt
Length of output: 509
🏁 Script executed:
wc -l .github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.cRepository: NVIDIA/cuopt
Length of output: 125
🏁 Script executed:
cat -n .github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.cRepository: NVIDIA/cuopt
Length of output: 4283
Handle malloc failure for primal buffer—prevent silent success on allocation error.
When malloc fails at line 85, the code silently skips primal retrieval and can return CUOPT_SUCCESS (status is never updated). If num_variables > 0 and allocation fails, the program should report failure. Add an explicit check after malloc to detect and handle allocation failure with a non-zero exit code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-lp-milp-api-c/assets/mps_solver/mps_solver.c around
lines 85 - 100, The malloc for primal can fail silently; after calling
malloc((size_t)num_variables * sizeof(cuopt_float_t)) check if primal == NULL
when num_variables > 0 and on failure set a non-success status (e.g., status =
CUOPT_ERROR or another non-zero sentinel), print an explicit error message about
allocation failure (including num_variables), and jump to cleanup (or return)
instead of continuing; ensure references to primal, num_variables, status, and
cuOptGetPrimalSolution are used to locate and modify the logic so failure is
handled consistently.
Comment on lines
+170
to
+175
| if known_optimal is not None: | ||
| results[gap]["gap_to_optimal"] = ( | ||
| (solution["objective"] - known_optimal) | ||
| / known_optimal | ||
| * 100 | ||
| ) |
There was a problem hiding this comment.
Validate known_optimal before percentage division.
If --known-optimal 0 is passed, both gap calculations divide by zero.
Proposed fix
def compare_gaps(
@@
) -> dict:
@@
+ if known_optimal is not None and known_optimal == 0:
+ raise ValueError("known_optimal must be non-zero for gap percentage calculations")
@@
if known_optimal is not None:
results[gap]["gap_to_optimal"] = (
(solution["objective"] - known_optimal)
/ known_optimal
* 100
) known_optimal = args.known_optimal
+ if known_optimal == 0:
+ parser.error("--known-optimal must be non-zero")Also applies to: 279-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/skills/cuopt-lp-milp-api-python/assets/mps_solver/model.py around
lines 170 - 175, The current computation of results[gap]["gap_to_optimal"]
(using solution["objective"] and known_optimal) can divide by zero when
known_optimal == 0; update the blocks around the computation (the if
known_optimal is not None checks that set results[gap]["gap_to_optimal"] and the
similar block near lines handling the second gap) to explicitly guard against
known_optimal == 0 and handle that case by either setting gap_to_optimal to None
(or math.inf) or computing an absolute difference instead, so replace the
unconditional division with a conditional that checks known_optimal != 0 before
dividing and provides the chosen fallback when it is zero.
tmckayus
approved these changes
Mar 2, 2026
Contributor
tmckayus
left a comment
tmckayus
left a comment
There was a problem hiding this comment.
lgtm! we should think about a test suite for the agent examples to make sure they are up to date and accurate
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci/test_skills_assets.sh (1)
54-54: Remove dead assignment (binary) to keep lint clean.
binaryis set at line 54 but never used, triggering ShellCheck SC2034. The assignment can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci/test_skills_assets.sh` at line 54, The assignment to the variable "binary" (binary="${dir}/${base}") is unused and triggers ShellCheck SC2034; remove that line from the script so the temporary variable is not set, leaving any subsequent logic to use "dir" and "base" directly or their existing computed values; ensure no other references to "binary" exist elsewhere (e.g., in functions or later commands) before removing..github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py (1)
21-25: Mark unused callback arguments to satisfy linting.The
solution_boundanduser_dataparameters (line 21) are required by theGetSolutionCallbackinterface but not used in this implementation. Prefix them with_to make this intentional and avoid linter warnings.Suggested fix
- def get_solution(self, solution, solution_cost, solution_bound, user_data): + def get_solution(self, solution, solution_cost, _solution_bound, _user_data):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py around lines 21 - 25, Rename the unused parameters in get_solution to mark them intentionally unused to satisfy linters: change the parameter names solution_bound and user_data to _solution_bound and _user_data in the get_solution method signature (keeping the method name get_solution and other parameter names unchanged) so the callback still matches the GetSolutionCallback interface while avoiding unused-argument warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
@.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py:
- Line 42: The print of problem.ObjValue in incumbent_callback.py is
unconditional and can fail if the solver finishes without a feasible incumbent;
update the printing logic in the incumbent callback to first check
problem.Status.name (e.g., if problem.Status.name in ["Optimal",
"FeasibleFound"]) and only then print the objective value, otherwise print or
log a status-only message; reference the existing print call that uses
problem.Status.name and problem.ObjValue to locate and replace with this guarded
conditional.
---
Nitpick comments:
In
@.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py:
- Around line 21-25: Rename the unused parameters in get_solution to mark them
intentionally unused to satisfy linters: change the parameter names
solution_bound and user_data to _solution_bound and _user_data in the
get_solution method signature (keeping the method name get_solution and other
parameter names unchanged) so the callback still matches the GetSolutionCallback
interface while avoiding unused-argument warnings.
In `@ci/test_skills_assets.sh`:
- Line 54: The assignment to the variable "binary" (binary="${dir}/${base}") is
unused and triggers ShellCheck SC2034; remove that line from the script so the
temporary variable is not set, leaving any subsequent logic to use "dir" and
"base" directly or their existing computed values; ensure no other references to
"binary" exist elsewhere (e.g., in functions or later commands) before removing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.pyci/test_python.shci/test_skills_assets.sh
| settings.set_parameter(CUOPT_TIME_LIMIT, 30) | ||
| problem.solve(settings) | ||
|
|
||
| print(f"Status: {problem.Status.name}, Objective: {problem.ObjValue}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the specific file
fd "incumbent_callback.py" .github/skills/ --type fRepository: NVIDIA/cuopt
Length of output: 135
🏁 Script executed:
# Once located, read the file to see the actual code around lines 39-42
cat -n .github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.pyRepository: NVIDIA/cuopt
Length of output: 2048
🏁 Script executed:
# Search for ObjValue access patterns in similar examples to understand best practices
rg -n "ObjValue|Status\.name" .github/skills/ --type py -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 9228
Guard objective printing when solve finishes without a feasible incumbent.
At Line 42, problem.ObjValue is printed unconditionally. With the time limit set at Line 39, the solver can finish without a feasible incumbent depending on status. All other examples in the codebase guard ObjValue access with a status check before printing (e.g., if problem.Status.name in ["Optimal", "FeasibleFound"]). Apply the same pattern here to avoid fragile example behavior:
Proposed patch
- print(f"Status: {problem.Status.name}, Objective: {problem.ObjValue}")
+ print(f"Status: {problem.Status.name}")
+ if problem.Status.name in ["Optimal", "FeasibleFound"]:
+ print(f"Objective: {problem.ObjValue}")📝 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.
Suggested change
| print(f"Status: {problem.Status.name}, Objective: {problem.ObjValue}") | |
| print(f"Status: {problem.Status.name}") | |
| if problem.Status.name in ["Optimal", "FeasibleFound"]: | |
| print(f"Objective: {problem.ObjValue}") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
@.github/skills/cuopt-lp-milp-api-python/assets/milp_basic/incumbent_callback.py
at line 42, The print of problem.ObjValue in incumbent_callback.py is
unconditional and can fail if the solver finishes without a feasible incumbent;
update the printing logic in the incumbent callback to first check
problem.Status.name (e.g., if problem.Status.name in ["Optimal",
"FeasibleFound"]) and only then print the objective value, otherwise print or
log a status-only message; reference the existing print call that uses
problem.Status.name and problem.ObjValue to locate and replace with this guarded
conditional.
Collaborator
Author
Added a test suite in conda test python workflow to test all the assets to make sure all are working. |
Collaborator
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds upgrades to skills,
Checklist