Added node-limit argument to cuopt_cli#1234
Open
nguidotti wants to merge 5 commits into
Open
Conversation
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
nguidotti
added
doc
📝 WalkthroughWalkthroughAdds a new MIP solver parameter ChangesNode Limit Feature for MIP Solver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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 `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1496-1502: The loop currently checks
exploration_stats_.nodes_explored against settings_.node_limit and breaks before
calling update_tree(...), which skips processing the just-solved node; move or
defer the node-limit break so update_tree(node_ptr, search_tree_, worker,
lp_status, settings_.log) always runs for the solved node, then after
update_tree set solver_status_ = mip_status_t::NODE_LIMIT (or check
nodes_explored again) and break; modify the control flow around the
nodes_explored check to ensure update_tree is invoked with
node_ptr/search_tree_/worker/lp_status/settings_.log before exiting due to node
limit.
- Around line 1613-1623: The dive node-limit check is off-by-one and can abort
before applying the last solved dive node; in the loop where
dive_stats.nodes_explored is incremented, change the condition that compares
against diving_node_limit from a strict greater-than to greater-than-or-equal
(i.e., use >=) or move the check to after the call to update_tree(...) so that
update_tree(...) always runs for the terminal node; specifically update the
logic around dive_stats.nodes_explored, diving_node_limit, the lp_status checks
(dual::status_t::TIME_LIMIT / CONCURRENT_LIMIT / ITERATION_LIMIT), and the
solver_status_ assignment to ensure the final solved dive node is processed by
update_tree(...) before breaking.
In `@cpp/tests/mip/miplib_test.cu`:
- Line 117: Replace the symmetric EXPECT_NEAR used on settings.node_limit with
one-sided checks: assert solution.get_num_nodes() is at least
settings.node_limit (EXPECT_GE or ASSERT_GE) and at most settings.node_limit
plus the documented overshoot bound (EXPECT_LE/ASSERT_LE); reference the
existing overshoot constant (e.g., kNodeOvershoot or
settings.node_limit_overshoot) if available, or introduce a named constant for
the documented overshoot instead of hardcoding 8, and use
solution.get_num_nodes() and settings.node_limit in the two assertions to
enforce the one-sided range.
🪄 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: d2caac2f-d59c-437f-94c2-5e4a3aa7e065
📒 Files selected for processing (4)
cpp/include/cuopt/linear_programming/constants.hcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/math_optimization/solver_settings.cucpp/tests/mip/miplib_test.cu
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/cuopt/source/lp-qp-milp-settings.rst (1)
454-456: ⚡ Quick winMake the node-limit overshoot bound explicit.
Line 455 says a “small number” of extra nodes may be explored; please document the concrete bound (
at most b - 1, wherebis the number of active best-first workers) so the contract is precise and consistent across docs.As per coding guidelines: “Consistency: version numbers, parameter types, and terminology match code”.
🤖 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 `@docs/cuopt/source/lp-qp-milp-settings.rst` around lines 454 - 456, Replace the vague phrase "a small number of additional nodes" with a precise bound: state that the solver may explore at most b - 1 extra nodes past the limit, where b is defined as the number of active best-first workers; update the sentence around "If set along with the time limit..." to mention this explicit overshoot bound and ensure the term "best-first workers" and the symbol b match the codebase terminology and other docs for consistency.
🤖 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 `@docs/cuopt/source/milp-features.rst`:
- Around line 87-89: Update the documentation for the node_limit description to
note that in parallel branch-and-bound the solver may slightly exceed the
requested node_limit before stopping; specifically modify the paragraph that
mentions ``node_limit`` and ``time_limit`` so it states the solver attempts to
stop when the node_limit is reached but parallel solving can allow the count to
slightly exceed the limit, and clarify that if both ``time_limit`` and
``node_limit`` are set the solver stops at whichever limit is hit first (subject
to the same slight-exceed behavior for ``node_limit`` in parallel mode).
---
Nitpick comments:
In `@docs/cuopt/source/lp-qp-milp-settings.rst`:
- Around line 454-456: Replace the vague phrase "a small number of additional
nodes" with a precise bound: state that the solver may explore at most b - 1
extra nodes past the limit, where b is defined as the number of active
best-first workers; update the sentence around "If set along with the time
limit..." to mention this explicit overshoot bound and ensure the term
"best-first workers" and the symbol b match the codebase terminology and other
docs for consistency.
🪄 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: dd55c152-5011-4566-bf68-3aa328c74032
📒 Files selected for processing (3)
docs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rstdocs/cuopt/source/lp-qp-milp-settings.rstdocs/cuopt/source/milp-features.rst
✅ Files skipped from review due to trivial changes (1)
- docs/cuopt/source/cuopt-c/lp-qp-milp/lp-qp-milp-c-api.rst
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1479-1499: The deterministic solve path still increments
exploration_stats_.nodes_explored without honoring settings_.node_limit; update
the deterministic branch in branch_and_bound::plunge_with / the deterministic
loop that calls solve_node_lp (look for exploration_stats_.nodes_explored,
exploration_stats_.nodes_unexplored, solve_node_lp and solver_status_ usage) to
check settings_.node_limit and set solver_status_ = mip_status_t::NODE_LIMIT and
break when exploration_stats_.nodes_explored >= settings_.node_limit (or check
before incrementing), ensuring the same node-limit logic used in the
non-deterministic path is applied to the deterministic path as well.
- Around line 1479-1487: The guard that checks exploration_stats_.nodes_explored
against settings_.node_limit should use >= to prevent one extra node from being
processed; update the condition before calling solve_node_lp(node_ptr, worker,
...) so that if exploration_stats_.nodes_explored >= settings_.node_limit you
set solver_status_ = mip_status_t::NODE_LIMIT and break, preventing the
off-by-one overshoot (and ensuring node_limit == 0 explores zero nodes).
In `@cpp/tests/mip/miplib_test.cu`:
- Around line 111-114: The test must assert that the solver actually terminated
due to the node limit rather than accepting Optimal; replace the current loose
EXPECT_TRUE on solution.get_termination_status() with a deterministic assertion
that the returned mip_termination_status_t equals the node-limit termination
enum value exposed by the API (e.g., mip_termination_status_t::NodeLimitReached
or the equivalent value used in your codebase), and explicitly fail the test if
status == mip_termination_status_t::Optimal; update the assertion around
mip_solution_t<int,double> solution = solve_mip(&handle_, problem, settings) /
solution.get_termination_status() accordingly.
🪄 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: e048d965-f354-41f1-8b41-246b8d410281
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/tests/mip/miplib_test.cudocs/cuopt/source/milp-features.rst
✅ Files skipped from review due to trivial changes (1)
- docs/cuopt/source/milp-features.rst
rgsl888prabhu
approved these changes
May 18, 2026
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
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 `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 1476-1488: In plunge_with, the early-break paths for
time/node/concurrent limits currently leave pending nodes in the local stack and
miscount nodes_unexplored when requeuing; modify the break paths in the blocks
that set solver_status_ to TIME_LIMIT, NODE_LIMIT and CONCURRENT_LIMIT so that
before breaking you drain the local stack into node_queue_ (preserving order),
adjust exploration_stats_.nodes_unexplored to account for any nodes you requeue
(i.e., defer decrement/increment pairing around requeue operations), and ensure
you decrement exploration_stats_.nodes_being_solved exactly once for the
node_ptr being returned to node_queue_; apply the same fix for the analogous
block at the later occurrence mentioned (the other early-exit block).
🪄 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: 70a5a73c-b5d3-44c7-a662-77f48cc7a249
📒 Files selected for processing (5)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/worker.hppcpp/tests/mip/miplib_test.cudocs/cuopt/source/lp-qp-milp-settings.rstdocs/cuopt/source/milp-features.rst
✅ Files skipped from review due to trivial changes (2)
- docs/cuopt/source/milp-features.rst
- docs/cuopt/source/lp-qp-milp-settings.rst
Comment on lines
1476
to
1488
| if (toc(exploration_stats_.start_time) > settings_.time_limit) { | ||
| solver_status_ = mip_status_t::TIME_LIMIT; | ||
| stack.push_front(node_ptr); | ||
| --exploration_stats_.nodes_being_solved; | ||
| break; | ||
| } | ||
|
|
||
| if (exploration_stats_.nodes_explored >= settings_.node_limit) { | ||
| if (exploration_stats_.nodes_explored + exploration_stats_.nodes_being_solved > | ||
| settings_.node_limit) { | ||
| solver_status_ = mip_status_t::NODE_LIMIT; | ||
| stack.push_front(node_ptr); | ||
| --exploration_stats_.nodes_being_solved; | ||
| break; |
There was a problem hiding this comment.
Preserve pending nodes when breaking out of plunge_with.
These break paths leave node_ptr (and any siblings already in stack) only in the local deque, but the only handoff back to node_queue_ is the gap-converged block at Lines 1558-1568. Exiting here for node/time/concurrent limits can therefore drop pending nodes from the final B&B state, and the CONCURRENT_LIMIT branch also requeues a node after --exploration_stats_.nodes_unexplored, so the unexplored count is off by one. Drain the remaining stack on every early exit, and restore/defer nodes_unexplored when a node is requeued.
As per coding guidelines, “Focus review on … branch-and-bound control-flow changes … treat B&B logic as CRITICAL.”
Also applies to: 1492-1504
🤖 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 `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 1476 - 1488, In
plunge_with, the early-break paths for time/node/concurrent limits currently
leave pending nodes in the local stack and miscount nodes_unexplored when
requeuing; modify the break paths in the blocks that set solver_status_ to
TIME_LIMIT, NODE_LIMIT and CONCURRENT_LIMIT so that before breaking you drain
the local stack into node_queue_ (preserving order), adjust
exploration_stats_.nodes_unexplored to account for any nodes you requeue (i.e.,
defer decrement/increment pairing around requeue operations), and ensure you
decrement exploration_stats_.nodes_being_solved exactly once for the node_ptr
being returned to node_queue_; apply the same fix for the analogous block at the
later occurrence mentioned (the other early-exit block).
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
2 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.
This PR adds the
node-limitargument to thecuopt_cli, which causes the solver to stop when the number of explored nodes reaches the specified limit.Checklist