-
Notifications
You must be signed in to change notification settings - Fork 546
Enable parallel big-M calculation for gdp.mbigm transformation #3641
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: main
Are you sure you want to change the base?
Conversation
…adlock with gurobi
@@ -23,7 +27,7 @@ def _convert_M_to_tuple(M, constraint, disjunct=None): | |||
else: | |||
try: | |||
M = (-M, M) | |||
except: | |||
except Exception: |
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.
You touched this line so now I get to snark. It's good practice, if catching a general exception, to raise the original exception as well so folks can directly inspect it.
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.
Don't we already do this? There is a raise
statement after we log.
(transBlock, algebraic_constraint) = self._setup_transform_disjunctionData( | ||
obj, root_disjunct | ||
) | ||
def _transform_disjunctionDatas( |
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 it possible to break this up? This function is huge.
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's even worse than it looks, because _setup_jobs_for_disjunction is basically just the inner body of this function that I transposed out so it would be less offensively indented (notice it mutates like 4 of its parameters). The problem is that the whole transformation is basically a big ball of state until it's done and I don't know if it can really not be that way. Emma's version was a lot nicer because it did the disjunctions one by one, but I can't do that if I want it to use threads effectively.
All that said, I will look and see if I can separate any more of this out in a reasonably clean way.
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 moved the multiprocessing pool setup to its own instance method, slightly improving this situation
After discussion today, I have reverted the change to the LegacySolverWrapper class names, and switched from sending the solver name to recreate the solver to sending the solver class to recreate the solver. This makes the assumption that all solver classes (besides contrib without the wrapper, which we reject for other reasons) can be correctly constructed with the single named argument Also, using this on windows now depends on |
Fixes # (n/a)
Summary/Motivation:
The multiple big-M transformation tends to slow down as model size increases due to linear or quadratic growth in the number of subsolver runs required. This change parallelizes M calculation using Python's multiprocessing module. The threading module was tried at first, but due to previously discussed issues was switched to multiprocessing.
Changes proposed in this PR:
dill
in this case as models often contain nested functions and in my testing do not reliably pickle without it. I think this code leads to a nested pickle, but I'm not sure if anything can be done about it.name
class attribute of LegacySolverWrapper derived class to the solver's legacy_name. This is so that code like this:will behave as intended (as solvers do not reliably pickle even with dill). This does not affect code that gets the original contrib Solver directly from the pyomo.contrib.solver.common.factory.SolverFactory.
test_calculated_Ms_correct
with parameters to the solve() call altered.Since this is a performance change, I ran a test on the medium-sized instance

gdp_col
from GDPlib, using baron as the subsolver.It kind of looks like
f(x) = 1/x
, if you squint in such a way that you cannot see the bottom of the chart. This instance transformed in 145 seconds on the currentmain
branch, so this is not a regression in the single-threaded case. Naturally, things are slightly slower when using 'spawn', but I do at least make sure we only pickle the model once per thread (and hopefully only once total? I'm not sure how multiprocessing works on the inside, but it really should cache these).I also tested the small instance

jobshop
, to ensure nothing horrible happened.On the current main branch, this model transforms in 0.36 seconds so again there is no regression.
Finally, there seems to be a bug when using this transformation with
gurobi_direct
v1. It works fine with the other solvers I've tried, so I suspect it's a bug in that interface, but I haven't tracked it down yet. This combination also has errors on the current main branch, but they're different errors, so it's hard to know if I've changed anything in that regard.Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: