-
Notifications
You must be signed in to change notification settings - Fork 115
Several bug fixes #786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/26.02
Are you sure you want to change the base?
Several bug fixes #786
Conversation
📝 WalkthroughWalkthroughTreats PDLP objective as a user-space value and adds a user→solver objective conversion API; passes solver- and user-space objectives plus solve time to B&B callbacks; makes presolve threading configurable with OpenMP; records which solver solved the root; and suppresses LP solver logs during MIP solves via a conditional logging macro. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…ed concurrent solution when the B&B is already running. moved number of threads in the probing cache to the setting struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip/presolve/probing_cache.cu (1)
859-910: Add defensive validation fornum_threadsbefore pool sizing and parallel launchThe code assumes
settings.num_threadsis always positive, but sincesettings_tis a public struct, external code can setnum_threadsto 0 (or modify it between initialization and use). If 0 is assigned:
modification_vector_poolandsubstitution_vector_poolwould have size 0#pragma omp parallel num_threads(0)produces undefined behavior (OpenMP spec)- If threads spawn despite 0,
thread_idxfromomp_get_thread_num()will exceed pool bounds, causing out-of-bounds accessAdd a defensive clamp before sizing the pools:
Proposed fix
- const size_t num_threads = bound_presolve.settings.num_threads; + const i_t requested_threads = bound_presolve.settings.num_threads; + size_t num_threads = + requested_threads > 0 ? static_cast<size_t>(requested_threads) + : static_cast<size_t>(omp_get_max_threads()); + if (num_threads == 0) { num_threads = 1; }Aligns with multithreading safety guidelines to prevent resource exhaustion and thread safety violations.
cpp/src/linear_programming/solve.cu
Outdated
|
|
||
| #include <thread> // For std::thread | ||
|
|
||
| #define CUOPT_LP_SOLVER_LOG_INFO(...) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we undef the previous macro and still use CUOPT_LOG_INFO here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to add another macro, since there are some methods where we do not pass the setting as argument, hence they need to use the old version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we should add logger_t to the settings rather than CUOPT_LOG_INFO or rapids::logger_t.
CMS750_4 + Fixed hard-coded number of threads| root_crossover_soln_.user_objective = user_objective; | ||
| root_crossover_soln_.iterations = iterations; | ||
| root_crossover_solution_set_.store(true, std::memory_order_release); | ||
| if (root_solver_type_ == root_solver_type_t::NONE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this printing to inside solve_root_relaxation and only print when crossover is successful.
Otherwise we could tell the user we have found a solution with PDLP/Barrier. And then the user could wait a long time only to have us use the dual simplex solution because we either: a) failed in crossover, or b) crossover took a very long time.
| class branch_and_bound_t { | ||
| public: | ||
| // Specify which solver was used for solving the root LP relaxation | ||
| enum class root_solver_type_t { NONE = 0, CROSSOVER = 1, DUAL_SIMPLEX = 2 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? Can we just determine who won in solve_root_relaxation?
|
|
||
| // Check if crossover was stopped by dual simplex | ||
| if (crossover_status == crossover_status_t::OPTIMAL) { | ||
| settings_.log.printf("\nCrossover found an optimal solution for the root relaxation\n\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we should indicate that PDLP/Barrier won. Can you use the is_pdlp_solution to determine which of them won and print it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not find a way to get the winner solver from the optimization_problem_solution_t unless we change the API (#787). This object is user-facing, so it needs to be reflect on the Python side as well.
| } | ||
|
|
||
| if (root_crossover_solution_set_.load(std::memory_order_acquire)) { | ||
| settings_.log.printf("\nRunning crossover\n\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this print. I don't think we want to interrupt the dual simplex logs for the root solve, unless we know that crossover was successful.
| std::atomic<bool> root_crossover_solution_set_{false}; | ||
| bool enable_concurrent_lp_root_solve_{false}; | ||
| std::atomic<int> root_concurrent_halt_{0}; | ||
| std::atomic<root_solver_type_t> root_solver_type_{root_solver_type_t::NONE}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an atomic? I think we can never have a race condition because we write to this var only when one f the solve is optimal which means the other solver returned with halt status.
CMS750_4due to an incorrect root objective from the PDLP. It provides an objective in user space and B&B expects the objective in the solver space.cuopt_cli. Now the number of threads in the probing cache can be controlled by thebounds_update::settings_t.Checklist
Summary by CodeRabbit
New Features
Optimizations
Bug Fixes
Behavior Changes
Quality of Life
✏️ Tip: You can customize this high-level summary in your review settings.