-
Notifications
You must be signed in to change notification settings - Fork 557
Separate solver_available
and license_available
#3730
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
) | ||
return self._version_cache | ||
|
||
def license_available( |
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.
Should we separate acquire_license
and license_available
?
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.
Probably. What is the right way to acquire a license in gurobi? Could you help with that?
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.
For anyone following along, we are discussing this offline.
available
and license_available
solver_available
and license_available
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.
Overall, I think this looks good. I am happy with the design. I have a few questions/suggestions on the details/implementation.
try: | ||
env = self.acquire_license(timeout=timeout) | ||
if env is None: | ||
self._license_cache = LicenseAvailability.Timeout |
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 is hard to distinguish between a timeout and no license... It looks like it is possible to return Timeout when the result should actually be NotAvailable?
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 messed with this one a lot and am 1000% not sure what the right thing to do is. So basically, if acquire_license times out, it is guaranteed to return None
. I think otherwise, it will error. But I was having a hard time testing to ensure that that did actually happen.
if env is None: | ||
self._license_cache = LicenseAvailability.Timeout | ||
return self._license_cache | ||
except gurobipy.GurobiError as acquire_error: |
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 hit this exception? It looks like acquire_license
is guarded with try
statements in a way that this code cannot be reached?
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.
Ooo. That's a good point. What about this instead?
if not timeout:
try:
cls._gurobipy_env = gurobipy.Env()
except gurobipy.GurobiError:
# Re-raise so license_available can inspect further
# or so users can explicitly view the error
raise
return cls._gurobipy_env
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.
That looks good, but I'm not sure there is a point to the try
statement at this point???
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.
The only thing I see that might be useful here is, "What if there is some other type of exception that isn't a GurobiError?" - In that case, we are just returning None
. But do we want to do that? Is it better if we just let it explode?
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.
Sorry - I was not clear. I mean the try
statement in acquire_license
, not the try
statement here.
except gurobipy.GurobiError as large_error: | ||
msg = str(large_error).lower() | ||
status = getattr(large_error, "errno", None) | ||
if "too large" in msg or status in (10010,): | ||
self._license_cache = LicenseAvailability.LimitedLicense | ||
elif "queue" in msg or "timeout" in msg: | ||
self._license_cache = LicenseAvailability.Timeout | ||
elif ( | ||
"no gurobi license" in msg | ||
or "not licensed" in msg | ||
or status in (10009,) | ||
): | ||
self._license_cache = LicenseAvailability.NotAvailable | ||
else: | ||
self._license_cache = LicenseAvailability.BadLicense |
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.
Once we have gotten here, don't we know that the result should be LimitedLicense? Is anything else possible?
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 a timeout or "some other unknown thing" is possible. But not NotAvailable
, you're right. What about this?
try:
# We try a 'big' model (more than 2000 vars).
# This should give us all the information we need
# about the license status.
large_model = gurobipy.Model(env=env)
large_model.addVars(range(2001))
large_model.optimize()
self._license_cache = LicenseAvailability.FullLicense
except gurobipy.GurobiError as large_error:
msg = str(large_error).lower()
status = getattr(large_error, "errno", None)
if "too large" in msg or status in (10010,):
self._license_cache = LicenseAvailability.LimitedLicense
elif "queue" in msg or "timeout" in msg:
self._license_cache = LicenseAvailability.Timeout
else:
self._license_cache = LicenseAvailability.Unknown
finally:
large_model.dispose()
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.
Looks good, I think.
self._vars_added_since_update = ComponentSet() | ||
self._last_results_object: Optional[Results] = None | ||
|
||
def close(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.
What motivated this new method?
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.
See the comment below on release_license
- because I made a classmethod version, there was a naming clash, so I needed a bit of a hack to get around it.
try: | ||
cls._gurobipy_env = gurobipy.Env() | ||
except gurobipy.GurobiError: | ||
# Re-raise so license_available can inspect further | ||
# or so users can explicitly view the error | ||
raise |
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 is what I am talking about. This try
statement does not seem to do anything except raise any exception that gets raised???
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.
My remaining comments don't really matter. I'm happy at this point.
Fixes None (but is part of #3688)
Summary/Motivation:
As part of the solver redesign, we decided to separate the checks for
solver_available
(is the solver findable, e.g., through importing, on the path, etc.) andlicense_is_valid
(does the user have a valid license to use the solver).This PR introduces a fundamental change in which each derived class is now expected to create both
solver_available
andlicense_available
. The combination of these two results is then propagated toavailable
(which overall returns a bool stating whether a solver is both findable and licensed).Changes proposed in this PR:
pyomo.contrib.solver.common.availability
forSolverAvailability
andLicenseAvailability
enumslicense_available
andsolver_available
toSolverBase
license_available
andsolver_available
(and properly do caching for version and availability)config
as an argument in IPOPTLegal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: