-
Notifications
You must be signed in to change notification settings - Fork 553
New GAMS solver interface, writer, and solution loader. #3683
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
…ittest and coverage test
@AnhTran01 - Please make sure to run |
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.
Immediate piece of feedback, recognizing that I have not thoroughly looked over anything - there are no tests anywhere, and there absolutely will need to be some.
Thanks for this, though, I am so excited!!
…riting process. Added WIP unittest for the new solver interface
… and writing to gms.
@@ -518,6 +518,11 @@ def write(self, model): | |||
con_list[label] = declaration + definition + bounds | |||
self.var_symbol_map.addSymbol(obj, label) | |||
|
|||
# Write the GAMS model | |||
ostream.write("$offlisting\n") | |||
# $offdigit ignores extra precise digits instead of erroring |
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 was the error that you were seeing that prompted this addition? maybe something was being set above 1e300
?
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.
in the previous commit test (b0c3450), the listing file output error message is "Too many digits in number $offdigit can be used to ignore trailing digits"
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.
but do you know what triggered this? which test was causing this 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.
I went back to check the failed test. The following tests that triggered the "Too many digits error" are:
- TestSolvers.test_equality
- TestSolvers.test_linear_expression
- TestSolvers.test_mutable_param_with_range
- TestSolvers.test_param_changes
- TestSolvers.test_no_objective
- TestSolvers.test_time_limit
- TestLegacySolverInterface.test_param_updates
By testing locally, I see there are trailing decimal places and not the cases of 1e300
or large numbers.
… constraint). Fixed bug of load_solution that contains constraint id in addition to variable id
…arsing solver_options
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.
(Some initial comments, mostly on configuration)
pyomo/contrib/solver/solvers/gams.py
Outdated
self.writer_config: ConfigDict = self.declare( | ||
'writer_config', GAMSWriter.CONFIG() | ||
) | ||
# Share the same config as the writer, passes at the function call write |
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.
Here and below: this should just insert a copy of the CONFIG from the writer. See Ipopt here for an example.
pyomo/contrib/solver/solvers/gams.py
Outdated
n, | ||
) | ||
|
||
def version(self, config=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.
In our review of the new solver interfaces, we changed the API to:
def version(self, rehash: bool=False) -> Tuple:
Much like available()
, this should leverage a cache (class dict attribute). You can look at the ipopt implementation for an idea (although that hasn't been updated to the new API).
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.
Could you clarify the purpose of using rehash and whether the user can call rehash
from the solve statement?
I found the documentation here: https://pyomo.readthedocs.io/en/latest/api/pyomo.common.fileutils.PathManager.html
Would the implementation be Executable(config.executable.path()).rehash()
?
pyomo/contrib/solver/solvers/gams.py
Outdated
if config.logfile is not None: | ||
config.logfile = os.path.abspath(config.logfile) | ||
|
||
config.writer_config.put_results_format = 'gdx' if gdxcc_available else 'dat' |
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 should probably only overwrite the configuration if it is still None (that way users can set / override the interface)
…lable() and version()
…are no longer valid.
…etting default to gdx that caused failed test. Implemented raise infeasible
… rehash handling with Executable.rehash()
…age for infeasible model when calling load_var, dual, reduced_cost
…ded handling solvestat and modelstat when solver is interrupted
@mrmundt is the |
# NOTE: Taken from the lp_writer | ||
self.declare( | ||
'row_order', | ||
ConfigValue( | ||
default=None, | ||
description='Preferred constraint ordering', | ||
doc=""" | ||
To use with ordered_active_constraints function.""", | ||
), | ||
) |
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 don't quite understand why this config option has been added. Shouldn't that option live under the writer?
EDIT: Aha, I see it also lives under the writer! Why is it in both places? I suspect this was a mistake.
class GAMSResults(Results): | ||
def __init__(self): | ||
super().__init__() | ||
self.return_code: ConfigDict = self.declare( | ||
'return_code', | ||
ConfigValue(default=None, description="Return code from the GAMS solver."), | ||
) | ||
self.gams_solver_termination_condition: ConfigDict = self.declare( | ||
'gams_solver_termination_condition', | ||
ConfigValue( | ||
default=None, | ||
description="Include additional TerminationCondition domain." | ||
"Take precedence over model_termination_condition if interruption occur", | ||
), | ||
) | ||
self.gams_model_termination_condition: ConfigDict = self.declare( | ||
'gams_model_termination_condition', | ||
ConfigValue( | ||
default=None, | ||
description="Include additional TerminationCondition domain.", | ||
), | ||
) | ||
self.gams_solver_status: ConfigDict = self.declare( | ||
'gams_solver_status', | ||
ConfigValue( | ||
default=None, description="Include additional SolverStatus domain." | ||
), | ||
) |
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.
@jsiirola and @michaelbynum - I think I would prefer that all of these items live under Results.extra_info
rather than as distinct items / having a "custom" GAMSResults
object. Thoughts?
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 have no preference.
Keyword Arguments | ||
----------------- | ||
output_filename: str | ||
Name of file to write GAMS model to. Optionally pass a file-like | ||
stream and the model will be written to that instead. | ||
io_options: str | ||
- warmstart=True | ||
Warmstart by initializing model's variables to their values. | ||
- symbolic_solver_labels=False | ||
Use full Pyomo component names rather than | ||
shortened symbols (slower, but useful for debugging). | ||
- labeler=None | ||
Custom labeler. Incompatible with symbolic_solver_labels. | ||
- solver=None | ||
If None, GAMS will use default solver for model type. | ||
- mtype=None | ||
Model type. If None, will chose from lp, nlp, mip, and minlp. | ||
- add_options=None | ||
List of additional lines to write directly | ||
into model file before the solve statement. | ||
For model attributes, <model name> is GAMS_MODEL. | ||
- skip_trivial_constraints=False | ||
Skip writing constraints whose body section is fixed. | ||
- output_fixed_variables=False | ||
If True, output fixed variables as variables; otherwise, | ||
output numeric value. | ||
- file_determinism=1 | ||
| How much effort do we want to put into ensuring the | ||
| GAMS file is written deterministically for a Pyomo model: | ||
- NONE (0) : None | ||
- ORDERED (10): rely on underlying component ordering (default) | ||
- SORT_INDICES (20) : sort keys of indexed components | ||
- SORT_SYMBOLS (30) : sort keys AND sort names (not declaration order) | ||
- put_results='results' | ||
Filename for optionally writing solution values and | ||
marginals. If put_results_format is 'gdx', then GAMS | ||
will write solution values and marginals to | ||
GAMS_MODEL_p.gdx and solver statuses to | ||
{put_results}_s.gdx. If put_results_format is 'dat', | ||
then solution values and marginals are written to | ||
(put_results).dat, and solver statuses to (put_results + | ||
'stat').dat. | ||
- put_results_format='gdx' | ||
Format used for put_results, one of 'gdx', 'dat'. |
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 going to sound weird coming from me but you do not need all of this extra documentation. The autodoc will handle it from the ConfigValue descriptions/docs. I would encourage you to delete this.
def __init__(self): | ||
self.config = self.CONFIG() |
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.
def __init__(self): | |
self.config = self.CONFIG() | |
def __init__(self): | |
#: Instance configuration; | |
#: see :ref:`pyomo.repn.plugins.gams_writer_v2.GAMSWriter::CONFIG`. | |
self.config = self.CONFIG() |
|
||
def __call__(self, model, filename, solver_capability, io_options): | ||
if filename is None: | ||
filename = 'GAMS_MODEL' + ".gms" |
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.
filename = 'GAMS_MODEL' + ".gms" | |
filename = model.name + ".gms" |
This is more standard.
'Solution loader does not currently have a valid solution. Please ' | ||
'check results.termination_condition and/or results.solution_status.' |
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.
You don't need the text: https://github.com/Pyomo/pyomo/blob/main/pyomo/contrib/solver/common/util.py#L34
raise RuntimeError( | ||
'Solution loader does not currently have a valid solution. Please ' | ||
'check results.termination_condition and/or results.solution_status.' | ||
) |
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 suspect this is supposed to be a different error from those in here: github.com/Pyomo/pyomo/blob/main/pyomo/contrib/solver/common/util.py
'Solution loader does not currently have valid duals. Please ' | ||
'check results.termination_condition and/or results.solution_status.' |
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.
You don't need the text: https://github.com/Pyomo/pyomo/blob/main/pyomo/contrib/solver/common/util.py#L41
'Solution loader does not currently have valid duals. Please ' | ||
'check results.termination_condition and/or results.solution_status.' |
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.
You don't need the text: https://github.com/Pyomo/pyomo/blob/main/pyomo/contrib/solver/common/util.py#L41
raise NoReducedCostsError( | ||
'Solution loader does not currently have valid reduced costs. Please ' | ||
'check results.termination_condition and/or results.solution_status.' | ||
) | ||
if self._gdx_data is None: | ||
raise NoReducedCostsError( | ||
'Solution loader does not currently have valid reduced costs. Please ' | ||
'check results.termination_condition and/or results.solution_status.' | ||
) |
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.
You don't need the text: https://github.com/Pyomo/pyomo/blob/main/pyomo/contrib/solver/common/util.py#L48
Fixes NA
Summary/Motivation:
This PR introduced a new GAMS solver interface that uses the
LinearRepnVisitor
to enable coefficient gathering when writing out the scalar GAMS model ingms
format.Changes proposed in this PR:
pyomo.contrib.solver.solvers
pyomo.repn.plugins
pyomo.contrib.solver.solvers
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: