Skip to content
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

Fix logical errors in Gurobi check #149

Merged
merged 2 commits into from
Oct 25, 2020

Conversation

lpsinger
Copy link
Contributor

@lpsinger lpsinger commented Oct 9, 2020

  • Fix circular import (ImportError: cannot import name 'Model' from partially initialized module 'mip' (most likely due to a circular import))
  • Fix logic for setting mip.gurobi.found flag

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2020

CLA assistant check
All committers have signed the CLA.

@jurasofish
Copy link
Contributor

Very clean, a couple of thoughts:

  • Seems FileNotFoundError (as raised here) would be appropriate to catch instead of a ~~ wide~~ catch Exception, and perhaps log.info the message so it doesn't disappear into the void.

  • If the user specifies Gurobi as the solver and it can't be imported they get a very unhelpful exception (previously that FileNotFoundError was produced)

>>> import mip
>>> m = mip.Model(solver_name='GRB')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/michael/git/python-mip/mip/model.py", line 84, in __init__
    self.solver = mip.gurobi.SolverGurobi(self, name, sense)
AttributeError: module 'mip.gurobi' has no attribute 'SolverGurobi'

@lpsinger
Copy link
Contributor Author

Due to the bare except Exception: in the original code, that nice error message would never have been printed. The latest version I just pushed does away with the except Exception: clause.

@jurasofish
Copy link
Contributor

Due to the bare except Exception: in the original code, that nice error message would never have been printed. The latest version I just pushed does away with the except Exception: clause.

i didn't notice that, thanks for pointing out. good to fix it :)

new changes result in a bunch of types and default values of the methods of the classes being defined under the else. should be super easy to fix by just moving a couple of things above the if lib_path is None.

import mip
m = mip.Model('GRB')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/Users/michael/git/python-mip/mip/model.py", line 90, in __init__
    import mip.gurobi
  File "/Applications/PyCharm.app/Contents/plugins/python/helpers/pydev/_pydev_bundle/pydev_import_hook.py", line 21, in do_import
    module = self._system_import(name, *args, **kwargs)
  File "/Users/michael/git/python-mip/mip/gurobi.py", line 325, in <module>
    class SolverGurobi(Solver):
  File "/Users/michael/git/python-mip/mip/gurobi.py", line 326, in SolverGurobi
    def __init__(self, model: Model, name: str, sense: str, modelp: CData = ffi.NULL):
NameError: name 'CData' is not defined

@h-g-s
Copy link
Contributor

h-g-s commented Oct 17, 2020

Hi @lpsinger , thanks for your contribution ! I`m reviewing the patches. In the last modification (October 14) there are some large modifications in guroby.py in several methods. Are they necessary in your patch or it happened due to an automatic format ? If they are, it would be good perhaps to split in more patches with comments.

@lpsinger
Copy link
Contributor Author

@h-g-s, try suppressing whitespace changes in your diff viewer. It's not as big as a change as it looks.

@lpsinger
Copy link
Contributor Author

@h-g-s, the large diff is due to indenting a large portion of gurobi.py to be inside an if statement.

@lpsinger
Copy link
Contributor Author

The Black formatting errors were there before my PR. Would you like me to open a separate PR to clean up those formatting errors first?

@h-g-s
Copy link
Contributor

h-g-s commented Oct 23, 2020 via email

* Fix circular import (`ImportError: cannot import name 'Model'
  from partially initialized module 'mip' (most likely due to a
  circular import)`)
* Fix logic for setting `mip.gurobi.found` flag
@lpsinger
Copy link
Contributor Author

Actually, the Black errors were introduced by me. Now fixed.

@h-g-s h-g-s merged commit 4288864 into coin-or:master Oct 25, 2020
pabloazurduy pushed a commit that referenced this pull request Mar 1, 2021
* Fix logical errors in Gurobi check

* Fix circular import (`ImportError: cannot import name 'Model'
  from partially initialized module 'mip' (most likely due to a
  circular import)`)
* Fix logic for setting `mip.gurobi.found` flag

* Provide useful exception if gurobi is specifically requested but cannot be found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants