-
Notifications
You must be signed in to change notification settings - Fork 112
Fix assert failure on MIP problem reducing to LP after presolve #235
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
Conversation
akifcorduk
left a comment
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 not run MIP at all if the number of integers is zero but run LP with it and exit. For now you could just run PDLP.
…ables after presolve" This reverts commit 32c057e.
Authors: - Robert Maynard (https://github.com/robertmaynard) - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Ramakrishnap (https://github.com/rgsl888prabhu) URL: #58
Bumps the version of actions/checkout in nightly.yaml to the latest, v4 - https://github.com/actions/checkout?tab=readme-ov-file#checkout-v4 Authors: - Scott Brenner (https://github.com/ScottBrenner) Approvers: - Bradley Dice (https://github.com/bdice) URL: #230
The helm chart adds an option to users to deploy cuOpt in kubernetes with basic setting. Users can always modify or create a helm chart as per their requirement. ## Issue closes #124 Authors: - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Trevor McKay (https://github.com/tmckayus) URL: #224
Adds workflow for container build, test and push for nightly and release. Nightly container would have the tag for example 25.8.0a-cuda12.8-py3.12 And these will be published to https://hub.docker.com/r/nvidia/cuopt/tags and ngc internal registry. This PR also removes several unused workflows and also update license header. ## Issue closes #123 Authors: - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Trevor McKay (https://github.com/tmckayus) - Gil Forsyth (https://github.com/gforsyth) URL: #180
| settings.time_limit = timer_.remaining_time(); | ||
| settings.method = method_t::Concurrent; | ||
|
|
||
| auto opt_sol = solve_lp_with_method<i_t, f_t>( |
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.
Calling LP with solve_lp_with_method instead of solve_lp means we would not get the initial LP problem checks and presolve being added in #234.
Should we use it instead? the rest looks good to me!
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 also think we should be calling solve_lp and pass down the corresponding settings to use concurrent unless I missed something
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.
Problem checks should be superfluous as they're already performed on the presolved problem. Additionally, the checks in solve_lp are only done on the original problem structure (which has already been checked at the time of calling solve_mip)
solve_lp requires passing either a mps_data_model or the original optimization_problem_t instead of the presolved problem, I used solve_lp_with_method to avoid making changes to solve_lp. Do you think I should add an overload instead?
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 another overload would over complexify things at this point. I prefer sticking with this approach. What we could do is move the try/catch and presolve in solve_lp_with_method in some other PR.
|
Lets try to merge this PR, since few other PRs are waiting on this. |
hlinsen
left a comment
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.
LGTM, thanks Alice!
| settings.time_limit = timer_.remaining_time(); | ||
| settings.method = method_t::Concurrent; | ||
|
|
||
| auto opt_sol = solve_lp_with_method<i_t, f_t>( |
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 another overload would over complexify things at this point. I prefer sticking with this approach. What we could do is move the try/catch and presolve in solve_lp_with_method in some other PR.
|
/merge |
|
Thank you @aliceb-nv @hlinsen |
This is a simple smoketest to look for breakage and any abnormal behavior with respect to runtime from cuopt Python API changes. It does not do any exhaustive performance testing or building. depends on #235 Authors: - Trevor McKay (https://github.com/tmckayus) - James Lamb (https://github.com/jameslamb) - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - James Lamb (https://github.com/jameslamb) URL: #199
The MIP codepath assumes the current problem possesses a non-zero number of integer variables.
This is normally never the case as LP problems are directed to the LP codepath, however some trivial MIP instances lose their integer variables after presolve.
In those cases the problem is solved at the root relaxation, this PR simply fixes assertions and checks to ensure such instances don't trigger failures on the MIP heuristics side.
Also fixes an issue where initial solutions were scaled regardless of the mip scaling setting
Fixes #233