Solver Refactor Pt. 2: Restructure for Future Redesign #3476
Conversation
| def __format__(self, format_spec): | ||
| return format(self.name, format_spec) | ||
|
|
||
| def __str__(self): | ||
| return self.name |
There was a problem hiding this comment.
When I saw this, I thought, "I'm surprised we need this". Then I looked at the version that was removed from within SolverBase and saw that we dropped some useful comments. Why not port those comments? Also, I still find it odd that we need to override __str__ and __format__. Should we use a different type of enum?
There was a problem hiding this comment.
Oh, we need IntEnum for __bool__...
| :class:`BranchAndBoundConfig<pyomo.contrib.solver.config.BranchAndBoundConfig>`, | ||
| :class:`PersistentSolverConfig<pyomo.contrib.solver.config.PersistentSolverConfig>`, or | ||
| :class:`PersistentBranchAndBoundConfig<pyomo.contrib.solver.config.PersistentBranchAndBoundConfig>`. | ||
| Additionally, solvers should have a :attr:`CONFIG<SolverBase.CONFIG>` attribute that |
There was a problem hiding this comment.
Can we specify that this is a class attribute rather than an instance attribute?
| Nominally, this will return True if the solver interface is | ||
| valid and can be used to solve problems and False if it cannot. | ||
|
|
||
| Note that for licensed solvers there are a number of "levels" of | ||
| available: depending on the license, the solver may be available | ||
| with limitations on problem size or runtime (e.g., 'demo' | ||
| vs. 'community' vs. 'full'). In these cases, the solver may | ||
| return a subclass of enum.IntEnum, with members that resolve to | ||
| True if the solver is available (possibly with limitations). | ||
| The Enum may also have multiple members that all resolve to | ||
| False indicating the reason why the interface is not available | ||
| (not found, bad license, unsupported version, etc). |
There was a problem hiding this comment.
I think these are useful comments? Is this captured in the documentation? If so, I 'm fine with removing it.
| Note that the enum can be cast to bool, which will | ||
| be True if the solver is runable at all and False | ||
| otherwise. |
|
|
||
| def __init__(self, **kwds) -> None: | ||
| super().__init__(**kwds) | ||
| self._active_config = self.config |
There was a problem hiding this comment.
I think this is also set in PersistentSolverUtils. I'm not sure where it makes the most sense to keep it.
| def _load_vars(self, vars_to_load: Optional[Sequence[VarData]] = None) -> NoReturn: | ||
| def _load_vars(self, vars_to_load: Optional[Sequence[VarData]] = None) -> None: |
There was a problem hiding this comment.
Is there a difference between these two? Just curious - it does not really matter.
| @@ -10,28 +10,153 @@ | |||
| # ___________________________________________________________________________ | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Should this file go in the common directory?
| raise RuntimeError( | ||
| 'Solver does not currently have a valid solution. Please ' | ||
| 'check the termination condition.' | ||
| ) | ||
| raise NoSolutionError() |
There was a problem hiding this comment.
I love these changes.
|
|
||
| class GurobiDirect(GurobiSolverMixin, SolverBase): | ||
| """ | ||
| Interface to Gurobi direct (not persistent) |
There was a problem hiding this comment.
I would change this to something like "Interface to Gurobi using gurobipy"
| Gurobi._num_instances -= 1 | ||
| if Gurobi._num_instances == 0: | ||
| GurobiPersistent._num_instances -= 1 | ||
| if GurobiPersistent._num_instances == 0: |
There was a problem hiding this comment.
should _num_instances live on the mixin class and be shared by direct and persistent???
| # NOTE: This seems like an unnecessary local variable because it's | ||
| # never used or called again. But when removed, it causes KeyErrors. |
There was a problem hiding this comment.
Haha! The line self._symbol_map.removeSymbol(con) in _remove_sos_constraints has to be removed with it.
Fixes #1030 (doesn't actually fix it; just the next step)
Summary/Motivation:
This is meant to be a "mid-point" for the ongoing solver refactor. It is currently working in this form, so this PR is intended to merge changes up to now so we can start making more fine-tuned changes that are easier to track.
Changes proposed in this PR:
pylint)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: