test(mip): Update incumbent callback API test to use a bit more complex problem - #1649
Conversation
…PI test On fast hardware (GB300), presolve fixes both variables of the 2-variable MIP directly, so branch-and-bound never runs and the incumbent callback never fires. The assertion n_callbacks > 0 tests solver behavior, not API shape. Whether callbacks fire is covered by test_incumbent_callbacks.py with real MPS files; this test covers registration, argument types, and no crash. Fixes #1647
On fast hardware (GB300), presolve fixes both variables of the 2-variable MIP directly and no branch-and-bound runs, so the incumbent callback never fires. Use the same pytest.skip pattern as test_incumbent_callbacks.py instead of failing hard on n_callbacks == 0. Fixes #1647
CI Test Summary✅ All 31 test job(s) passed. |
|
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:
📝 WalkthroughWalkthroughThe incumbent-solution callback test now uses an eight-item binary knapsack model. It validates binary values, capacity feasibility, and objective consistency for each callback solution. Coding conventions prohibit volatile identifiers and hardware names in comments and skip messages. ChangesIncumbent callback test
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py`:
- Around line 448-449: Update the callback-registration test around
settings.set_mip_callback to assert that the registered callback is the same
object as get_callback after registration. Keep zero callback invocations valid
and retain the existing API-shape checks, but do not rely on callback-count
assertions to verify registration.
- Around line 446-448: Generalize the explanatory comment in the test around
_run_incumbent_solutions to state that n_callbacks may be zero when the solver
produces no intermediate incumbent, without attributing this specifically to
presolve or branch-and-bound behavior. Keep zero callback invocations valid and
retain the API-shape focus of the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9cbeadc0-ef53-4c93-85b7-4e6885206b84
📒 Files selected for processing (1)
python/cuopt/cuopt/tests/linear_programming/test_python_API.py
| # n_callbacks may be 0. This test covers API shape only: registration, | ||
| # argument types, and no crash. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify callback registration independently of invocation.
When the solver emits no incumbent, the solution-validation loop is empty. The test can then pass if settings.set_mip_callback silently stops retaining get_callback. Add an identity check after registration, but keep zero callback invocations valid.
Proposed assertion
settings.set_mip_callback(get_callback, user_data)
+ assert any(
+ callback is get_callback
+ for callback in settings.get_mip_callbacks()
+ )As per path instructions, removing the callback-count assertion must still leave meaningful callback-registration coverage.
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py` around lines
448 - 449, Update the callback-registration test around
settings.set_mip_callback to assert that the registered callback is the same
object as get_callback after registration. Keep zero callback invocations valid
and retain the existing API-shape checks, but do not rely on callback-count
assertions to verify registration.
Source: Path instructions
|
This doesn't feel like the right fix to me. Either we set up the code path so it deterministically calls the callback or we test something else. |
|
Why does presolve behavior on a two-variable problem vary by GPU? |
…er pointer Add keywords to the conventions.md pointer so searches for 'stale', 'hardware', 'issue number', or 'skip message' hit the pointer and redirect to references/conventions.md where the rule lives.
This happens intermittently and may not be connected to GPU exactly. But I agree with the other comment, it is better to add an example which will for sure get incumbent detail rather than skipping it. |
| ## Coding Conventions | ||
|
|
||
| For C++ naming (`snake_case`, `d_`/`h_` prefixes, `_t` suffix), file extensions (`.hpp`/`.cpp`/`.cu`/`.cuh` and which compiler each uses), include order, Python style, error handling (`CUOPT_EXPECTS`, `RAFT_CUDA_TRY`), memory management (RMM patterns, no raw `new`/`delete`), and test-impact rules, see [references/conventions.md](references/conventions.md). | ||
| For C++ naming (`snake_case`, `d_`/`h_` prefixes, `_t` suffix), file extensions (`.hpp`/`.cpp`/`.cu`/`.cuh` and which compiler each uses), include order, Python style, error handling (`CUOPT_EXPECTS`, `RAFT_CUDA_TRY`), memory management (RMM patterns, no raw `new`/`delete`), test-impact rules, and volatile-comment rules (no PR numbers, commit hashes, issue links, or hardware names in comments or skip messages — these go stale), see [references/conventions.md](references/conventions.md). |
There was a problem hiding this comment.
PR numbers, commit hashes, and issue links don't go stale.
There was a problem hiding this comment.
Stale in the sense out of context or doesn't have valid information. But I agree with issue number though. But claude use it in another way where it added issue number for which it was adding this PR, which doesn't add any value.
ramakrishnap-nv
added
non-breaking
Why does presolve behave nondeterministically on a two variable problem? |
Unsure, I have seen this problem happen in other test as well, where I had to skip in case it couldn't find one. https://github.com/NVIDIA/cuopt/pull/1512/changes#diff-3db7ef937cb0c65153bfa4aa4378af5326fb873b9fdd29fb240e141b1f9c68de |
The original 2-variable MIP was solved without branch-and-bound on some hardware, so the incumbent callback never fired. Replace it with an 8-item 0-1 knapsack whose LP relaxation is fractional, requiring B&B and guaranteeing at least one incumbent callback.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py`:
- Around line 423-425: Remove the n_callbacks > 0 assertion from the callback
test and delete or correct the inaccurate claim that the highest-ratio item does
not fit exactly. Keep callback registration and callback argument-shape
validation independent of whether GetSolutionCallback is invoked, without adding
reliance on nondeterministic solver behavior.
- Around line 454-465: Strengthen the validation loop over
get_callback.solutions by asserting each sol["solution"] vector has the expected
item count and that every entry is numerically close to either 0 or 1 before
checking capacity and cost. Keep the existing tolerance-based capacity and
objective assertions, using the test’s n, tol, and established numerical
assertion conventions.
🪄 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: 26b3090a-ab2b-42b1-b803-d2c573ab3fa8
📒 Files selected for processing (1)
python/cuopt/cuopt/tests/linear_programming/test_python_API.py
| # (the highest-ratio item doesn't fit exactly), so B&B is required and | ||
| # the incumbent callback is guaranteed to fire when a feasible integer | ||
| # solution is found. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
Remove the callback-count requirement.
Line 452 reintroduces the flaky n_callbacks > 0 assertion. A fractional LP relaxation proves an integrality gap, but it does not guarantee branch-and-bound or a GetSolutionCallback invocation. Root cuts or heuristics can complete the solve without an intermediate incumbent. Lines 423-425 also contain a factual error: the highest-ratio item has weight 1 and fits exactly.
Remove the count assertion, or use a solver-documented deterministic callback trigger. Keep callback registration and argument-shape checks independent of invocation count.
As per path instructions, Python callback tests must not rely on nondeterministic callback invocation.
Also applies to: 452-452
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py` around lines
423 - 425, Remove the n_callbacks > 0 assertion from the callback test and
delete or correct the inaccurate claim that the highest-ratio item does not fit
exactly. Keep callback registration and callback argument-shape validation
independent of whether GetSolutionCallback is invoked, without adding reliance
on nondeterministic solver behavior.
Source: Path instructions
- Correct inaccurate comment about 'highest-ratio item' (greedy fill leaves a partial item, not necessarily the highest-ratio one) - Add len check and binary-value assertion (each entry must be 0 or 1) to the incumbent solution validation loop
… tracking issues are fine
ramakrishnap-nv
changed the title
Iroy30
left a comment
Iroy30
left a comment
There was a problem hiding this comment.
Thanks for adding a robust callback test!
|
/merge |
GB300 has been fixed by #1649. This reverts commit 1637fb0. ## Issue #1647 Authors: - Kyle Edwards (https://github.com/KyleFromNVIDIA) Approvers: - Bradley Dice (https://github.com/bdice) URL: #1651
3 participants
Fixes #1647
Root cause
The 2-variable MIP in
_run_incumbent_solutionscan be fully solved by presolve on GB300:y=39is determined by bound tightening from the two constraints, thenx=37follows uniquely. No branch-and-bound runs, so the incumbent callback never fires.Whether the callback fires for a given problem on a given GPU is tested in
test_incumbent_callbacks.py.