-
Notifications
You must be signed in to change notification settings - Fork 83
Concurrent LP in get_relaxed_lp (used in FP) and lay foundations for future PDLP warm start #173
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: branch-25.08
Are you sure you want to change the base?
Conversation
* @return optimization_problem_solution_t<i_t, f_t> owning container for the solver solution | ||
*/ | ||
template <typename i_t, typename f_t> | ||
optimization_problem_solution_t<i_t, f_t> solve_lp( |
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.
If this is used purely in MIP context. It is cleaner to rename this to solve_relaxed_lp
or solve_lp_relaxation
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 agree that for now this is just used in the context of MIP but some people might want to solve an LP using the detail::problem representation instead of the optimization_problem one. What do you think?
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.
@akifcorduk do you have an opinion there?
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 renaming it kind of pins it to MIP use-cases, I agree with Nicolas that it is just another LP interface that accepts problem_t
. In the MIP context, the naming doesn't matter too much because we have other functions like run_lp_with_vars_fixed
std::tuple<dual_simplex::lp_solution_t<i_t, f_t>, dual_simplex::lp_status_t, f_t, f_t, f_t> | ||
run_dual_simplex(dual_simplex::user_problem_t<i_t, f_t>& user_problem, | ||
pdlp_solver_settings_t<i_t, f_t> const& settings) | ||
pdlp_solver_settings_t<i_t, f_t> const& settings, |
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.
We should consider changing pdlp_solver_settings
to lp_settings
} | ||
|
||
template <typename i_t, typename f_t> | ||
optimization_problem_solution_t<i_t, f_t> solve_lp(optimization_problem_t<i_t, f_t>& op_problem, |
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.
Is this for pure LP?
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.
Same comment as above, yes currently this used in the context LP as its using the optimization_problem_t representation.
detail::problem_t<i_t, f_t>& problem, | ||
pdlp_solver_settings_t<i_t, f_t> const& settings) | ||
pdlp_solver_settings_t<i_t, f_t> const& settings, | ||
bool inside_mip) |
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.
Is inside_mip currently used anywhere? or is it for future purposes?
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.
Yes I'm passing it to both PDLP and Dual Simplex
* @return optimization_problem_solution_t<i_t, f_t> owning container for the solver solution | ||
*/ | ||
template <typename i_t, typename f_t> | ||
optimization_problem_solution_t<i_t, f_t> solve_lp( |
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 renaming it kind of pins it to MIP use-cases, I agree with Nicolas that it is just another LP interface that accepts problem_t
. In the MIP context, the naming doesn't matter too much because we have other functions like run_lp_with_vars_fixed
cpp/src/linear_programming/pdlp.cu
Outdated
if (settings.has_pdlp_warm_start_data()) { | ||
const auto& warm_start_data = settings.get_pdlp_warm_start_data(); | ||
if (!warm_start_data.solved_by_pdlp_) { | ||
CUOPT_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.
I wouldn't put INFO logs here, it would bloat the regular solution logs. DEBUG would be more appropiate.
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.
Agreed, changed
cpp/src/linear_programming/pdlp.cu
Outdated
} else if (pdlp_hyper_params::restart_strategy == | ||
static_cast<int>( | ||
pdlp_restart_strategy_t<i_t, f_t>::restart_strategy_t::TRUST_REGION_RESTART)) { | ||
CUOPT_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.
Same: INFO is only for solution progress info to be conveyed to the user.
CUOPT_LOG_INFO("Solved with dual simplex"); | ||
sol_pdlp.copy_from(op_problem.get_handle_ptr(), sol_dual_simplex); | ||
// TODO confirm with Akif that using the problem handle ptr is not a problem | ||
sol_pdlp.copy_from(problem.handle_ptr, sol_dual_simplex); |
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 using problem handle_ptr should be okay given that there are no races across host threads. Even a different handle is used, the thread is default_per_thread
so it should be fine.
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.
Thanks, removed the comment
cpp/src/mip/relaxed_lp/relaxed_lp.cu
Outdated
lp_solver.set_inside_mip(true); | ||
auto solver_response = lp_solver.run_solver(start_time); | ||
// TODO check that we do want to do problem checking here | ||
auto solver_response = solve_lp(op_problem, settings, true, true, true); |
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 name the boolean argument, so that it is easier to read.
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 agree. But regarding the comment, should we do problem checking here? Maybe just when we are in assert/debug mode
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.
At this point we don't need to do problem checks because we are not modifiying the problem. It is done in places where we modify the problem.
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.
It did help me in the debug process, will add it in only assert/debug mode
}); | ||
lp_solver.set_initial_primal_solution(lp_state.prev_primal); | ||
lp_solver.set_initial_dual_solution(lp_state.prev_dual); | ||
settings.set_initial_primal_solution(lp_state.prev_primal.data(), |
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.
So currently, we are not using the full warm start data yet and this code is a base/skeleton for adding the future warm start data?
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.
Excatly, because we could need to add the warm start data to the lp_state which is also tied to the mip problem
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.
Needs changes in the server warmstart population
pdlpwarmstart_data = { |
cpp/src/linear_programming/solve.cu
Outdated
problem_checking_t<i_t, f_t>::check_initial_solution_representation( | ||
*problem.original_problem_ptr, settings); | ||
} else { | ||
problem.check_problem_representation(true, true); |
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 this might be unnecessary as we are doing the problem check in the problem cstr. We could maybe convert it to an assert? I think we should do the runtime problem check only at the beginning from the user, later problem checks should be asserts.
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 agree we should do it in as assert. But I believe we should keep the checks for two reasons:
- We also check the initial solutions which is not done in the problem constructor
- Even if we check in the problem constructor this is more for the case where the problem has been modified in an invalid way, we then need to check the problem again
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 just looked at the changes that touched the code for starting dual simplex. They look fine. It is fine to add info about inside mip to the dual simplex solve. Right now this is only used for changing how optimality information is printed.
I did not review the other parts of this PR. (Sorry ran out of time.) I think ideally we want to use primal simplex warm starting inside FP. I'm not sure how this PR relates to that.
Approving so this can be merged while I am out of office.
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Set to 25.10 milestone when it is created |
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.
Greptile Overview
Greptile Summary
This PR introduces concurrent LP solving capabilities to the MIP (Mixed Integer Programming) solver, specifically targeting the Feasibility Pump (FP) algorithm. The changes add a new solve_lp
interface that accepts MIP problem representations directly (detail::problem_t
) instead of requiring conversion to LP representations (optimization_problem_t
), eliminating unnecessary overhead.
The core architectural improvement enables both PDLP and Dual Simplex solvers to run concurrently in get_relaxed_lp
, with proper handling of the inside_mip
flag for optimized behavior within MIP contexts. A significant addition is the solved_by_pdlp
boolean field throughout the codebase that tracks which solver was used for previous solves, enabling proper warm start data handling when switching between solvers.
The PR also adds NVTX profiling ranges to improve performance debugging capabilities and lays the foundation for future PDLP warm start functionality. The changes maintain backward compatibility while providing a more efficient path for MIP algorithms to solve LP subproblems repeatedly, which is crucial for the Feasibility Pump's performance.
Important Files Changed
Changed Files
Filename | Score | Overview |
---|---|---|
cpp/src/linear_programming/solve.cu |
3/5 | Major refactor adding new solve_lp overload with concurrent solving and inside_mip flag handling - critical solver infrastructure changes |
python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx |
4/5 | Added conditional warm start validation based on solved_by_pdlp flag with proper warning handling for concurrent solver scenarios |
cpp/src/linear_programming/pdlp.cu |
4/5 | Enhanced PDLP warm start validation with solver type checking and trust region compatibility - affects core PDLP solver behavior |
cpp/src/mip/relaxed_lp/relaxed_lp.cu |
4/5 | Refactored to use new unified solve_lp interface replacing direct solver instantiation - key change for concurrent solving in FP |
cpp/include/cuopt/linear_programming/solve.hpp |
4/5 | Added new solve_lp overload declaration accepting detail::problem_t with inside_mip parameter - core API expansion |
cpp/include/cuopt/linear_programming/pdlp/solver_settings.hpp |
4/5 | Changed pdlp_warm_start_data_ to std::optional with new safety methods - significant API safety improvement |
cpp/src/linear_programming/solver_settings.cu |
4/5 | Implemented optional warm start data handling with proper memory safety and new solved_by_pdlp parameter |
cpp/src/math_optimization/solver_settings.cu |
4/5 | Added solved_by_pdlp parameter to set_pdlp_warm_start_data method for concurrent solving support |
cpp/src/linear_programming/utilities/problem_checking.cu |
4/5 | Refactored validation functions to accept device vectors directly and added detail::problem_t overload |
cpp/include/cuopt/linear_programming/solver_settings.hpp |
4/5 | Added solved_by_pdlp parameter to set_pdlp_warm_start_data method signature for warm start tracking |
cpp/include/cuopt/linear_programming/pdlp/pdlp_warm_start_data.hpp |
4/5 | Added solved_by_pdlp_ boolean field to both pdlp_warm_start_data_t struct and its view counterpart |
cpp/src/linear_programming/pdlp_warm_start_data.cu |
5/5 | Added solved_by_pdlp field initialization to constructors and copy operations - clean implementation |
python/cuopt/cuopt/linear_programming/solution/solution.py |
5/5 | Added solved_by_pdlp parameter to Solution class and PDLPWarmStartData with getter method |
python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py |
5/5 | Improved documentation and examples for PDLP warm start with concurrent solving constraints |
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py |
4/5 | Added test_solved_by_pdlp() function to verify solver type tracking and updated documentation accuracy |
cpp/tests/linear_programming/unit_tests/solver_settings_test.cu |
5/5 | Updated unit tests to accommodate new solved_by_pdlp_ field with proper test coverage |
python/cuopt/cuopt/linear_programming/solver/solver.pxd |
5/5 | Added solved_by_pdlp_ parameter to Cython declarations for set_pdlp_warm_start_data method |
cpp/include/cuopt/linear_programming/utilities/cython_solve.hpp |
5/5 | Added comprehensive documentation comment explaining dual-purpose nature of solved_by_pdlp_ field |
python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py |
5/5 | Added solved_by_pdlp boolean field to WarmStartData class with backward compatibility |
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py |
5/5 | Added solved_by_pdlp field extraction to PDLP warm start data dictionary |
cpp/src/linear_programming/translate.hpp |
5/5 | Added inside_mip parameter to skip row/column name copying for MIP context optimization |
cpp/src/mip/local_search/feasibility_pump/feasibility_pump.cu |
5/5 | Added NVTX profiling range to linear_project_onto_polytope function for performance monitoring |
cpp/src/mip/diversity/diversity_manager.cu |
5/5 | Added NVTX profiling range to generate_quick_feasible_solution method for performance analysis |
cpp/src/linear_programming/pdlp.cuh |
4/5 | Added solved_by_pdlp parameter to get_filled_warmed_start_data method declaration |
cpp/src/linear_programming/utilities/problem_checking.cuh |
4/5 | Modified method signatures to accept rmm::device_uvector and added detail::problem_t overload |
Confidence score: 4/5
- This PR makes significant architectural changes to enable concurrent LP solving, but the implementation appears well-structured and maintains backward compatibility
- Score reflects the complexity of changes affecting critical solver infrastructure and concurrent execution paths, requiring careful review of solver state management
- Pay close attention to
cpp/src/linear_programming/solve.cu
,cpp/src/linear_programming/pdlp.cu
, andcpp/src/mip/relaxed_lp/relaxed_lp.cu
for proper concurrent solver behavior and warm start handling
Sequence Diagram
sequenceDiagram
participant User
participant Python_API
participant Cython
participant solve_lp
participant problem_t
participant solve_lp_with_method
participant pdlp_solver
participant dual_simplex_solver
participant concurrent_solver
User->>Python_API: "Call solve() with data_model and settings"
Python_API->>Cython: "Call Solve() with data_model_obj and settings"
Cython->>solve_lp: "call_solve(data_model_view, solver_settings)"
solve_lp->>problem_t: "Create problem_t from mps_data_model"
solve_lp->>solve_lp: "Convert mps_data_model to optimization_problem"
solve_lp->>solve_lp: "solve_lp(detail::problem_t, settings, use_pdlp_solver_mode, inside_mip=true)"
solve_lp->>solve_lp_with_method: "solve_lp_with_method(problem, settings, inside_mip)"
alt method == Concurrent
solve_lp_with_method->>concurrent_solver: "run_concurrent(problem, settings, inside_mip)"
concurrent_solver->>dual_simplex_solver: "Start dual simplex thread"
concurrent_solver->>pdlp_solver: "Run PDLP in main thread"
concurrent_solver-->>solve_lp_with_method: "Return best solution"
else method == PDLP
solve_lp_with_method->>pdlp_solver: "run_pdlp(problem, settings, inside_mip)"
pdlp_solver-->>solve_lp_with_method: "Return PDLP solution"
else method == DualSimplex
solve_lp_with_method->>dual_simplex_solver: "run_dual_simplex(problem, settings, inside_mip)"
dual_simplex_solver-->>solve_lp_with_method: "Return dual simplex solution"
end
solve_lp_with_method-->>solve_lp: "Return optimization_problem_solution"
solve_lp-->>Cython: "Return solver_ret_t"
Cython-->>Python_API: "Create Solution object with warm start data"
Python_API-->>User: "Return Solution object"
Additional Comments (1)
-
cpp/src/linear_programming/solver_settings.cu
, line 32-33 (link)style: Duplicate include of thrust/scatter.h - already included on line 29
25 files reviewed, 8 comments
* @tparam i_t Data type of indexes | ||
* @tparam f_t Data type of the variables and their weights in the equations | ||
* | ||
* @param[in] op_problem An optimization_problem_t<i_t, f_t> object with a |
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.
syntax: Parameter documentation refers to 'op_problem' but the actual parameter is 'problem' - should be consistent
Prompt To Fix With AI
This is a comment left during a code review.
Path: cpp/include/cuopt/linear_programming/solve.hpp
Line: 70:70
Comment:
**syntax:** Parameter documentation refers to 'op_problem' but the actual parameter is 'problem' - should be consistent
How can I resolve this? If you propose a fix, please make it concise.
* @param[in] settings A pdlp_solver_settings_t<i_t, f_t> object with the settings for the PDLP | ||
* solver. | ||
* @param[in] use_pdlp_solver_modes If true, the PDLP hyperparameters coming from the | ||
* pdlp_solver_mode are used (instead of the ones comming from a potential hyper-params file). |
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.
syntax: Typo: 'comming' should be 'coming'
Prompt To Fix With AI
This is a comment left during a code review.
Path: cpp/include/cuopt/linear_programming/solve.hpp
Line: 75:75
Comment:
**syntax:** Typo: 'comming' should be 'coming'
How can I resolve this? If you propose a fix, please make it concise.
} | ||
if (settings.initial_dual_solution_.get() != nullptr) { | ||
check_initial_dual_representation(op_problem, settings.get_initial_dual_solution()); | ||
check_initial_dual_representation(problem.constraint_lower_bounds, |
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.
style: Direct field access assumes problem.constraint_lower_bounds is always valid - verify this is consistent with the constraint selection logic in the optimization_problem_t overload
Prompt To Fix With AI
This is a comment left during a code review.
Path: cpp/src/linear_programming/utilities/problem_checking.cu
Line: 121:121
Comment:
**style:** Direct field access assumes problem.constraint_lower_bounds is always valid - verify this is consistent with the constraint selection logic in the optimization_problem_t overload
How can I resolve this? If you propose a fix, please make it concise.
assert solution.get_solved_by_pdlp() | ||
assert solution.get_pdlp_warm_start_data().solved_by_pdlp |
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.
style: Both assertions test the same property - solved_by_pdlp
should be true when PDLP method is used. Consider adding a comment explaining why both checks are needed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
Line: 525:526
Comment:
**style:** Both assertions test the same property - `solved_by_pdlp` should be true when PDLP method is used. Consider adding a comment explaining why both checks are needed.
How can I resolve this? If you propose a fix, please make it concise.
"Invalid PDLPWarmStart data. Passed problem and PDLPWarmStart " # noqa | ||
"data should have the same amount of constraints." | ||
if not settings.get_pdlp_warm_start_data().solved_by_pdlp: | ||
warnings.warn("PDLPWarmStart data was passed to the solver, but the problem was solved by Dual Simplex. This data will be ignored.") # noqa |
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.
style: Warning message should use proper formatting for long lines to match project style
Prompt To Fix With AI
This is a comment left during a code review.
Path: python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
Line: 348:348
Comment:
**style:** Warning message should use proper formatting for long lines to match project style
How can I resolve this? If you propose a fix, please make it concise.
pdhg_solver_.get_primal_solution(), | ||
pdhg_solver_.get_dual_solution(), | ||
get_filled_warmed_start_data(), | ||
get_filled_warmed_start_data(false), |
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.
logic: Missing the true
parameter that was added in other calls to get_filled_warmed_start_data
Prompt To Fix With AI
This is a comment left during a code review.
Path: cpp/src/linear_programming/pdlp.cu
Line: 335:335
Comment:
**logic:** Missing the `true` parameter that was added in other calls to `get_filled_warmed_start_data`
How can I resolve this? If you propose a fix, please make it concise.
stream_view_); | ||
if (settings.has_pdlp_warm_start_data()) { | ||
const auto& warm_start_data = settings.get_pdlp_warm_start_data(); | ||
if (!warm_start_data.solved_by_pdlp_) { |
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.
style: Should verify the logic: warm start data from non-PDLP solvers might still be partially useful for initialization
Prompt To Fix With AI
This is a comment left during a code review.
Path: cpp/src/linear_programming/pdlp.cu
Line: 133:133
Comment:
**style:** Should verify the logic: warm start data from non-PDLP solvers might still be partially useful for initialization
How can I resolve this? If you propose a fix, please make it concise.
This PR aims at multiple improvements in MIP:
Eventually we want to add plug warm start in FP for both PDLP and Dual Simplex.