Improvements to MOSEK Direct interface and a new persistent interface #1686
Conversation
1.) The direct interface is now registered as 'mosek_direct' in the SolverFactory
(formerly called just 'mosek').
2.) License check is performed in the apply_solver method, instead of having a dedicated method
for the purpose.
3.) 'apply_solver' method now records a termination code, which also provides a return code
if mosek has a significant failure.
4.) (DISCUSS) '_get_cone_data' always sets the continuity relaxation to True.
5.) _get_expr_from_pyomo_repn method now has list comprehensions and map functions
instead of the for loops that were used formerly. Time-it to see if it speeds things up
significantly.
|
Could someone (maybe @jsiirola) give me a hint as to why the Jenkins CI /inspection has not started yet? I am still new to the world of CI/CD and any help would be greatly appreciated. |
|
@Utkarsh-Detha someone on the core development team needs to do a preinspection on the PR before the Jenkins tests are launched. Our weekly developers meeting is on Tuesday so you should see some movement on this PR later today. |
Codecov Report
@@ Coverage Diff @@
## master #1686 +/- ##
==========================================
- Coverage 72.99% 72.74% -0.26%
==========================================
Files 630 631 +1
Lines 90948 90131 -817
==========================================
- Hits 66388 65565 -823
- Misses 24560 24566 +6
Continue to review full report at Codecov.
|
|
Thanks for your response, @blnicho ! One more thing: I see that the codecov/patch test has failed and the culprit seems to be the foqus_graph.py file (I say that simply because the delta is negative only in that file's case). However, I have not tampered with this file, and therefore do not understand how this could happen. Any advice would be much appreciated. |
|
@Utkarsh-Detha, You always have to take codecov results with a grain of salt: there are frequently inexplicable changes in coverage that are unrelated to the PR. That said, |
|
Regarding testing you can use http://solve.mosek.com if you set it up appropriately: locally you would only need the interface, which is not licensed, and you would somehow have to hack the Pyomo interface to enable remote optimization. See for example what happens in Julia: https://github.com/MOSEK/Mosek.jl/blob/112e0d7d7f8bb753360abc3c3693f9dccd5c6fb7/test/apitest.jl#L43 |
|
@jsiirola am I right in thinking that the PR will not be considered for merging as it fails the codecov/patch test? If so, I can close this PR and open another one with @aszekMosek's suggestion incorporated in it. How would you suggest we proceed? |
|
No - I do not think this PR needs to be closed, although an extension that supports the remote interface that @aszekMosek highlights would be great. (I am most of the way through my review, so hopefully I can get that finished and posted soon) |
pyomo/pysp/tests/convert/TestConvertDDSIPSimple.test_bad_objective_var/core.lp
Outdated
Show resolved
Hide resolved
| Runs a check for a valid Mosek license. Returns False | ||
| if Mosek fails to run on a trivial test case. | ||
| Runs a check for a valid MOSEK license. Returns False if MOSEK fails | ||
| to run on a trivial test case. |
There was a problem hiding this comment.
Question (unrelated to this PR), is it better/faster/more efficient to use checkoutlicense()/checkinlicense(), or the call to optimize() below?
There was a problem hiding this comment.
I never thought about this before, but it is in fact more efficient to use the checkoutlicense methods! I ran a loop checking license availability 10**4 times, and found that if a license is available, then optimze() method will take about 69 seconds (84% CPU usage) and the checkoutlicense() method method will take about 58 seconds (with 94% CPU usage). I will make the necessary changes in the file.
|
Ah, I am sorry for the inconvenience. I will do as you suggested. |
|
@jsiirola I have tried to follow the suggestions you made. For each suggestion that I implemented, I marked its corresponding comment as resolved. The comments that I did not mark as resolved are the ones that could use more of your attention. I was confused about:
Thank you for your immensely helpful review. |
There was a problem hiding this comment.
@Utkarsh-Detha: Thank you for all your work on this. I think we are very close. As to your two outstanding questions, a simple wrapper class can be:
@SolverFactory.register('mosek', doc='The MOSEK LP/MIP/QP solver')
class MOSEK(OptSolver):
"""The MOSEK LP/MIP/QP solver
"""
def __new__(cls, *args, **kwds):
mode = kwds.pop('solver_io', 'direct')
#
if mode in {'python', 'direct'}:
opt = SolverFactory('mosek_direct', **kwds)
if opt is None:
logger.error('Python API for MOSEK is not installed')
return opt
if mode == 'persistent':
opt = SolverFactory('mosek_persistent', **kwds)
if opt is None:
logger.error('Python API for MOSEK is not installed')
return opt
else:
logger.error('Unknown/unsupported solver interface: %s' % (mode,))
return NoneAs for using the "_linear_canonical_form" attribute, I don't have a good suggestion. We should probably file this as an issue to consider when we finally get around to updating how we generate / manage canonical representation caches (and more generally, constraints that embed special storage structures (and not let that hold up this PR).
| else: | ||
| self._solver_model.putintparam(eval(param), option) | ||
| self._solver_model.putintparam(param, option) |
There was a problem hiding this comment.
Error checking: should you add an:
else:
raise ValueError("unrecognized MOSEX option name: %s" % (key,))There was a problem hiding this comment.
In case of an invalid parameter name, an AttributeError will be raised when I use the getattr method to set the value for param. This exception will be caught in the except statement just below, and then re-raised. So this scenario should be covered.
|
@michaelbynum: I think this PR is close, and we could really use your eyes on it (particularly around the persistent interface). |
Merging upstream master into forked master.
| "bindings for Mosek?\n\n\t" + | ||
| "Error message: {0}".format(e)) | ||
| raise Exception(msg) | ||
| def add_vars(self, var_seq): |
There was a problem hiding this comment.
Should these methods be private with public wrappers on the persistent interface? Users should never call this method on the direct interface, right?
There was a problem hiding this comment.
Agreed, I will make the change.
However, I have a (sort-of) unrelated question about the wrapper method for adding constraints: if a user decides to add a cone, but one that is constructed as a domain, then they would be essentially adding a block. Do you think, then, that it would be a good idea to raise an error telling the user to switch to the add_block method, or would it be better to just call the add_block method from within the add_constraints wrapper method?
There was a problem hiding this comment.
That is an excellent question. I would lean toward calling add_block within the add_constraints wrapper, but I don't have a strong preference.
| :return: mosek.variabletype.type_int or mosek.variabletype.type_cont | ||
| self.add_constraints((con,)) | ||
|
|
||
| def add_constraints(self, con_seq): |
There was a problem hiding this comment.
Same as above about private vs public.
|
I would avoid calling add_block within add_constraint. I think that will just lead to confusion in the long term.
… On Nov 20, 2020, at 2:18 PM, Michael Bynum ***@***.***> wrote:
@michaelbynum commented on this pull request.
In pyomo/solvers/plugins/solvers/mosek_direct.py:
>
- try:
- self._solver_model = self._mosek_env.Task(0, 0)
- except Exception:
- e = sys.exc_info()[1]
- msg = ("Unable to create Mosek Task. "
- "Have you installed the Python "
- "bindings for Mosek?\n\n\t" +
- "Error message: {0}".format(e))
- raise Exception(msg)
+ def add_vars(self, var_seq):
That is an excellent question. I would lean toward calling add_block within the add_constraints wrapper, but I don't have a strong preference.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Yeah, @ghackebeil is probably right... |
|
As the changes have been approved, I was wondering if there is something that needs to be done before the PR can be merged? Any hints would be appreciated. Thank you. |
Fixes #1513
Motivation:
While investigating why the direct interface was slow to pass the problem to the solver (see google group discussion), @aszekMosek found that
_add_varand_add_constraintmethods pass one variable/constraint to the MOSEK task per call, thereby making it a bit slow. Furthermore, @michaelbynum stated that this was a design choice made to accommodate persistent interfaces. This pull request is therefore motivated by a need to add some performance improvements to the MOSEK direct interface and making a new persistent interface for MOSEK.Major changes proposed in this PR:
add_varsandadd_constraintsmethods are included. These take a list of variables/constraints (single data objects), extract the necessary information to be passed to the MOSEK task object, and make one call to the appropriate MOSEK Optimizer API methods. Naturally,_add_blockhas been overridden to use these methods._add_varand_add_constraintmethods are now simply calls to theadd_varsandadd_constraintsmethods."mosek_persistent"is now available. Methods are available to remove variable(s) and constraint(s), modify variable(s), add a column etc.Minor changes proposed in the PR:
MosekDirectclass is now calledMOSEKDirectand the direct interface is registered in the SolverFactory as"mosek_direct".apply_solvermethod in the direct interface now prints a termination code in case of a significant failure._get_cone_datamethod always sets the continuity relaxation to True.remove_vars,remove_constraintsandupdate_varsmethods in the persistent interface will accept multiple singleton var/constraint objects, passed as individual arguments. Analogous methods that accept a single object are also available.Benchmarks (direct interface) [updated]
A performance improvement of about ~30 % was observed across the following benchmarks:
NOTE: Suggestions made by @jsiirola further improved the performance of the interface, and the benchmark results provided above were updated accordingly.
I ran the benchmarks on an Intel i7 10875h, with 32 GB RAM.
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: