Correct dual-value scope to LP/QP in numerical-optimization API skills#1408
Conversation
cuOpt's barrier solver is primal-dual (SOCP barrier NVIDIA#1290, general convex quadratic constraints NVIDIA#1361), so a QP with a quadratic objective and linear constraints returns dual values and reduced costs — not LP only. The Python and C API skills still described duals as LP-only, wording that predates the QP/barrier work (NVIDIA#1183). - Python SKILL.md: retitle "Getting Dual Values (LP only)" -> "(LP / QP)" and rewrite the prose; add a "Reading Duals from a QP" example to references/qp_examples.md. - C SKILL.md: add a "Dual values (LP / QP)" section; add a duals note to assets/README.md. Boundary documented in all four edits: cuOpt returns no dual variables for problems with quadratic constraints (the dual/reduced-cost arrays come back filled with NaN), so the constraints whose duals you read must be linear. This matches docs/cuopt/source/cuopt-c/convex/convex-examples.rst and the QCQP path in cpp/src/pdlp/solve.cu. Existing lp_duals assets are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: cafzal <cameron.afzal@gmail.com>
…vior cpp/src/pdlp/solve.cu (has_quadratic_constraints -> thrust::fill the entire dual_solution + reduced_cost with quiet_NaN) shows a single quadratic constraint suppresses duals for the *whole* solve, not just that constraint's row. Reword "no dual for a quadratic constraint" -> "any quadratic constraint makes cuOpt return no duals for the whole solve" in Step 3 and the QP eval, for consistency with PRs NVIDIA#1407 (formulation) and NVIDIA#1408 (api skills, "NaN-filled"). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: cafzal <cameron.afzal@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates documentation across cuOpt's C and Python API skill guides to clarify when dual values and reduced costs are available. The C API gains a new section documenting ChangesDual Values Availability Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/cuopt-numerical-optimization-api-c/SKILL.md`:
- Around line 38-44: Branch has DCO sign-off but lacks the NVSkills bot
signature commit (no commit titled "Attach NVSkills validation signatures"), so
NVSkills CI gating won’t allow merge; ask a maintainer/admin to trigger the
NVSkills validation by commenting `/nvskills-ci` and confirm that the resulting
NVSkills bot signature commit is appended to the branch before merging,
re-checking that a commit from the NVSkills bot is present in the branch
history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61de0c4e-d2d6-4469-8bb3-c33bc48d627b
📒 Files selected for processing (4)
skills/cuopt-numerical-optimization-api-c/SKILL.mdskills/cuopt-numerical-optimization-api-c/assets/README.mdskills/cuopt-numerical-optimization-api-python/SKILL.mdskills/cuopt-numerical-optimization-api-python/references/qp_examples.md
| ## Dual values (LP / QP) | ||
|
|
||
| `cuOptGetDualSolution` (shadow prices) and `cuOptGetReducedCosts` (reduced costs) return values | ||
| for **LP and QP with linear constraints** — the barrier solver is primal-dual, so a quadratic | ||
| objective still yields duals. cuOpt returns **no duals for a problem with quadratic constraints** | ||
| (the returned arrays are filled with `NaN`). See [assets/lp_duals](assets/lp_duals/) for the call | ||
| sequence — it is an LP, but the same calls apply to a QP whose constraints are all linear. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Commits in PR branch missing DCO sign-off =="
git log --format='%H%n%B%n==END==' origin/main..HEAD \
| awk '
BEGIN { RS="==END==\n"; ORS="\n\n" }
{
if ($0 !~ /Signed-off-by:[[:space:]]+/) {
split($0, lines, "\n")
print lines[1]
}
}
' | sed '/^$/d' || true
echo
echo "== Check for NVSkills signature commit in PR branch =="
git log --oneline origin/main..HEAD | rg -n "Attach NVSkills validation signatures" || true
echo
echo "Expected:"
echo "- No commit hashes printed in DCO section."
echo "- At least one match for 'Attach NVSkills validation signatures' if NVSkills CI has run."Repository: NVIDIA/cuopt
Length of output: 303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DCO check (commit bodies) =="
git log --format='%H%n%B%n==END==' origin/main..HEAD \
| awk '
BEGIN { RS="==END==\n"; ORS="\n\n" }
{
if ($0 !~ /Signed-off-by:[[:space:]]+/) {
split($0, lines, "\n")
print lines[1]
}
}
' | sed '/^$/d' || true
echo
echo "== NVSkills signature phrase search (commit bodies) =="
git log --format='%H %s%n%B%n---END---\n' origin/main..HEAD \
| rg -n "Attach NVSkills validation signatures" || true
echo
echo "== NVSkills bot presence (commit subjects/bodies) =="
git log --format='%H %s%n%B%n---END---\n' origin/main..HEAD \
| rg -n "nv-nvskill-ci\\[bot\\]|NVSkills CI|NVSkills validation|validation signatures" || trueRepository: NVIDIA/cuopt
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Git remotes/branch state =="
git remote -v || true
git branch -a || true
git rev-parse --verify origin/main >/dev/null 2>&1 && echo "origin/main exists" || echo "origin/main missing"
echo
echo "== Commit range size (origin/main..HEAD) =="
git log --oneline origin/main..HEAD --max-count=20 || true
echo
git log --oneline origin/main..HEAD | wc -l
echo
echo "== Any Signed-off-by present in range? (case-insens) =="
git log --format='%H%n%B%n==END==' origin/main..HEAD \
| rg -ni "signed-off-by:" || true
echo
echo "== Any nvskills indicators present in commit messages/bodies? (case-insens) =="
git log --format='%H%n%s%n%B%n==END==' origin/main..HEAD \
| rg -ni "nvskills|nv-nvskill-ci|/nvskills-ci|validation signatures|attach nvskills" || true
echo
echo "== Look for NVSkills attach phrase variants =="
git log --format='%H%n%s%n%B%n==END==' origin/main..HEAD \
| rg -ni "attach.*nvskills|nvskills.*attach|validation.*signature|signature.*validation" || trueRepository: NVIDIA/cuopt
Length of output: 680
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Commits in origin/main..HEAD (subject) =="
git log --format='%H %s' origin/main..HEAD
echo
echo "== Full commit body(s) (to confirm DCO) =="
git log --format='---\ncommit %H\nsubject: %s\nbody:\n%B' origin/main..HEAD
echo
echo "== Check for NVSkills signature phrase across the same range (body+subject) =="
git log --format='%H%n%s%n%B%n==END==' origin/main..HEAD \
| rg -ni "attach nvskills|nvskills|nv-nvskill-ci\\[bot\\]|validation signatures" || trueRepository: NVIDIA/cuopt
Length of output: 1639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Authors/committers in origin/main..HEAD =="
git log --format='commit %H%n author: %an <%ae>%n committer: %cn <%ce>%n subject: %s%n%n' origin/main..HEAD
echo "== Look for any nvskills/nv-nvskill-ci references by committer/author =="
git log --format='%H%n%an%n%ae%n%cn%n%ce%n%s%n%B==END==' origin/main..HEAD \
| rg -ni "nvskills|nv-nvskill-ci\\[bot\\]|nv-nvskill-ci|/nvskills-ci|validation signatures|attach nvskills" || true
echo "== Show any commits touching skills/ in this range =="
git log --name-only --format='---\ncommit %H %s' origin/main..HEAD -- skills/ | sed -n '1,200p' || trueRepository: NVIDIA/cuopt
Length of output: 8682
Fix NVSkills CI gate for skills/ before merge (DCO present, NVSkills signature missing).
DCO sign-off is present in the updated skills commit. No commit in the branch contains Attach NVSkills validation signatures (or other NVSkills indicators), so NVSkills validation/signature flow still needs to run—ask for /nvskills-ci from a maintainer/admin and re-check that the NVSkills bot signature commit is attached before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/cuopt-numerical-optimization-api-c/SKILL.md` around lines 38 - 44,
Branch has DCO sign-off but lacks the NVSkills bot signature commit (no commit
titled "Attach NVSkills validation signatures"), so NVSkills CI gating won’t
allow merge; ask a maintainer/admin to trigger the NVSkills validation by
commenting `/nvskills-ci` and confirm that the resulting NVSkills bot signature
commit is appended to the branch before merging, re-checking that a commit from
the NVSkills bot is present in the branch history.
Source: Coding guidelines
| | [milp_production_planning](milp_production_planning/) | MILP | Production planning with resource constraints | | ||
| | [mps_solver](mps_solver/) | LP/MILP | Solve from MPS file via `cuOptReadProblem` | | ||
|
|
||
| > **Duals:** `lp_duals` is an LP, but `cuOptGetDualSolution` / `cuOptGetReducedCosts` also return |
There was a problem hiding this comment.
| > **Duals:** `lp_duals` is an LP, but `cuOptGetDualSolution` / `cuOptGetReducedCosts` also return | |
| **Duals:** `lp_duals` is an LP, but `cuOptGetDualSolution` / `cuOptGetReducedCosts` also return |
same for other lines as well
There was a problem hiding this comment.
If we have similar workding in skills, do we need to update in README again?
There was a problem hiding this comment.
Removed the note entirely instead of reformatting — it duplicated the Dual values (LP / QP) section now in SKILL.md. Thanks for catching it.
There was a problem hiding this comment.
Good call — no need to state it in both. Removed it from the README; the SKILL.md section is the single source.
The Dual values (LP/QP) scope lives in the api-c SKILL.md "Dual values (LP / QP)" section; the assets/README note restated it. Per review feedback, remove the duplicate so the skill body is the single source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: cafzal <cameron.afzal@gmail.com>
|
/ok to test 708f5d1 |
mlubin
left a comment
There was a problem hiding this comment.
How much of the new content is useful compared with the more minimal change of updating the incorrect "LP only" in skills/cuopt-numerical-optimization-api-python/SKILL.md?
|
|
||
| ## Dual values (LP / QP) | ||
|
|
||
| `cuOptGetDualSolution` (shadow prices) and `cuOptGetReducedCosts` (reduced costs) return values |
There was a problem hiding this comment.
I'm going to continue my push against "shadow prices". I don't think the parentheticals are necessary here. cuOptGetDualSolution returns duals and cuOptGetReducedCosts returns reduces costs. Readers can look at the function documentation for more information.
There was a problem hiding this comment.
Good point. Removed. I commit to no more 'shadow prices' too 😆
| ## Dual values (LP / QP) | ||
|
|
||
| `cuOptGetDualSolution` (shadow prices) and `cuOptGetReducedCosts` (reduced costs) return values | ||
| for **LP and QP with linear constraints** — the barrier solver is primal-dual, so a quadratic |
There was a problem hiding this comment.
No need to say anything about the barrier solver. The solve method isn't part of the API contract for duals.
| problem.addConstraint(x + y + z == 10, name="budget") | ||
| problem.solve() | ||
|
|
||
| if problem.Status.name in ["Optimal", "PrimalFeasible"]: |
There was a problem hiding this comment.
I don't think it's safe or correct to check the dual if the status is PrimalFeasible
| ## Getting Dual Values (LP only) | ||
| ## Getting Dual Values (LP / QP) | ||
|
|
||
| Shadow prices (`DualValue`) and reduced costs (`ReducedCost`) are returned for **LP and QP** — |
There was a problem hiding this comment.
Duals and reduced costs (no parentheticals)
- Drop "shadow prices"/"reduced costs" parenthetical glosses (api-c, api-python). - Remove the barrier-solver mention — the solve method isn't part of the dual contract for the API. - Remove the standalone "Reading Duals from a QP" example; lp_duals already shows the call sequence and the SKILL.md sections state it applies to QP (this also resolves reading duals on PrimalFeasible status). Reduces the change to the minimal LP-only -> LP/QP scope correction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: cafzal <cameron.afzal@gmail.com>
…barrier solver" - Step 5 cross-ref: "defines shadow price ..." -> "covers what duals mean and which problem types expose them". - Step 3 quadratic-constraint note: drop "the barrier solver" (the solve method isn't part of the dual contract); keep the second-order-cone constraint form. Consistent with the trim applied to the api-* skills in NVIDIA#1408. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: cafzal <cameron.afzal@gmail.com>
Replace "shadow prices" with "dual values" in the LP/MILP/QP sensitivity row
and the post-solve note, keeping the decision meaning ("where to invest").
Consistent with the api-* skill trim in NVIDIA#1408.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: cafzal <cameron.afzal@gmail.com>
|
/ok to test 1048880 |
@rgsl888prabhu, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 1048880 |
@rgsl888prabhu, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test 224030d |
|
/merge |
Description
Corrects the dual-value scope in the
cuopt-numerical-optimization-api-pythonand-api-cskills from "LP only" to LP/QP: cuOpt's barrier solver is primal-dual, so a QP with linear constraints returns shadow prices and reduced costs (the "LP only" wording predates the 26.06 QP/barrier work). Documents the boundary — problems with quadratic constraints return no duals (NaN-filled), so the constraints you read duals from must be linear — and adds a short QP-duals example to the Python references.Verified in source (
cuopt_c.h,pdlp/solve.cu) and empirically on a 26.08 nightly (budgetdual ≈ 6.667; quadratic constraint →NaN). Follows #1393; companion to #1407 (formulation). Existinglp_dualsassets unchanged;validate_skills.shpasses.Checklist