-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
Add SAS API interface #729
base: master
Are you sure you want to change the base?
Conversation
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 for the contribution. Please find some comments. I haven't checked everything but this should already be a good start.
I've also seen that the two python packages are available through pypi.
- Can you add the solver to the github CI? example:
pulp/.github/workflows/pythonpackage.yml
Line 32 in 6e18ecb
- name: Install gurobipy - Can you add the solver to the testing suite? example:
Line 1576 in 6e18ecb
class MOSEKTest(BaseSolverTest.PuLPTest):
pulp/apis/sas_api.py
Outdated
msg=msg, | ||
) | ||
self._sas = saspy.SASsession() | ||
self.warmStart=warmStart |
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.
warmstart
can be passed to LpSolver
and then becomes available inside self.optionsDict
. See how it's used in other solvers. No need to assign it to the object.
self.warmStart=warmStart | ||
# Only named options are allowed in SAS solvers. | ||
self.solverOptions = {key:value for key, value in solverParams.items()} | ||
def available(self): |
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 believe you need to run the black linter on the code.
https://coin-or.github.io/pulp/develop/contribute.html#applying-the-black-linter-formatter
pulp/apis/sas_api.py
Outdated
|
||
return (f"{name}-pulp.{n}" for n in args) | ||
|
||
def _delete_tmp_files(self, *args): |
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.
there already exists a delete_tmp_files
method that does almost exactly this. No need to create a new one. The same goes for _create_tmp_files
pulp/apis/sas_api.py
Outdated
decompsubprob_str = self._create_statement_str("decompsubprob") | ||
rootnode_str = self._create_statement_str("rootnode") | ||
|
||
if lp.isMIP(): |
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.
simpler:
if lp.isMIP() and not self.mip:
warnings.warn("SAS94 will solve the relaxed problem of the MILP instance.")
pulp/apis/sas_api.py
Outdated
primalin=tmpMst, | ||
postfix=postfix, | ||
) | ||
self.solverOptions["primalin"] = f"primalin{postfix}" |
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 not a good practice to edit the solverOptions
attribute. You should avoid editing during solve. It should be stateless.
pulp/apis/sas_api.py
Outdated
self._delete_tmp_files(tmpMps, tmpMst) | ||
|
||
# Store log and ODS output | ||
self._log = res["LOG"] |
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 do not see this property being defined anywhere: _log
. Why do we need to store it inside self
?
self._log = res["LOG"] | ||
if self.msg: | ||
print(self._log) | ||
self._macro = dict( |
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 goes for _macro
. is it needed after the solving? why?
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.
_macro is used to store some information returned by SAS solvers. It has information like primal and dual infeasibility, and SAS solution status. It might be interesting to SAS users.
pulp/apis/sas_api.py
Outdated
raise PulpSolverError("PuLP: Error ({err_name}) \ | ||
while trying to solve the instance: {name}".format( | ||
err_name=self._macro.get("STATUS", "ERROR"), name=lp.name)) | ||
status = self._read_solution(lp, primal_out, dual_out) |
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.
what if there are no values available? should we read the soluton still?
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 no values available, the solution will be NaN.
pulp/apis/sas_api.py
Outdated
except FileNotFoundError: | ||
pass | ||
|
||
def _write_sol(self, filename, vs): |
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.
there are many methods that are common between the two solvers. Could we just inherit one from the other and get all those methods?
raise PulpSolverError("SASCAS : Objective sense should be min or max.") | ||
|
||
status = None | ||
with redirect_stdout(SASLogWriter(self.msg)) as self._log_writer: |
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 part has many indentations and ifs. Can it be refactored so that it's easier to read? for example breaking it into other methods. re-using the code with different arguments, etc.
Hello, I solved the conflicts with master. Now the tests are failing because you need to run the black linter as I previously said: https://coin-or.github.io/pulp/develop/contribute.html#applying-the-black-linter-formatter |
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.
I've left more comments again. You need to get your code to run in the pulp CI, so fix the black linter first.
If someone else from SAS can help you reviewing the python code before asking for a review here, that would be awesome. I do not have too much time to give the amount of feedback/ improvements that is needed for this PR.
pulp/apis/sas_api.py
Outdated
|
||
def _create_statement_str(self, statement): | ||
"""Helper function to create the strings for the statements of the proc OPTLP/OPTMILP code.""" | ||
stmt = self.solverOptions.pop(statement, 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.
please do not edit this property, it should be static. There are many places where it's edited in the code.
pulp/apis/sas_api.py
Outdated
except: | ||
return False | ||
|
||
def toDict(self): |
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.
(1) you have this function duplicated.
(2) you should not just copy the inherit method, you should call the LpSolver_CMD.toDict
method and then add the functionality that you need.
(3) What is cas object and how do you expect to serialize it?
pulp/apis/sas_api.py
Outdated
return True | ||
|
||
def sasAvailable(self): | ||
if self.cas == 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.
self.cas is None
instead
I improved the PuLP interface with lessons learned from other interfaces, added the "with" options that other interfaces support (and a test for it) and improved performance of reading the output. @bolyu: Please review and test the changes. |
I have reviewed and tested the changes. The updates look good to me. |
thanks to both. The CI tests are still failing. see below. Are you sure the code works when there's no SAS solver around?
|
My bad, I assumed your machines would not have the SAS packages installed. I'll come up with another way of doing this next week. |
@bolyu I updated the interface classes, please review and verify the code on your side. |
@bolyu I adjust the code as you suggested. |
Add API interface of SAS solvers.