diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index d47901dee..77dd5041e 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -133,7 +133,7 @@ jobs: shell: Rscript {0} - name: Run test suite using pytest - run: pytest ixmp -m "not performance" --verbose --cov-report=xml --color=yes + run: pytest ixmp -m "not performance" --verbose -rA --cov-report=xml --color=yes - name: Run R CMD check run: | diff --git a/RELEASE_NOTES.rst b/RELEASE_NOTES.rst index 495b2381d..59419ff62 100644 --- a/RELEASE_NOTES.rst +++ b/RELEASE_NOTES.rst @@ -4,6 +4,10 @@ Next release All changes ----------- +- :pull:`398`: + + - Fix :class:`.GAMSModel` would try to write GDX data to filenames containing invalid characters on Windows. + - Format user-friendly exceptions when GAMSModel errors (:issue:`383`). - :pull:`397`: Adjust :mod:`ixmp.reporting` to use :mod:`genno`. - :pull:`396`: Fix two minor bugs in reporting. diff --git a/doc/conf.py b/doc/conf.py index 283121c85..c528c1caa 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -91,6 +91,7 @@ # -- Options for sphinx.ext.extlinks --------------------------------------------------- extlinks = { + "issue": ("https://github.com/iiasa/ixmp/issue/%s", "#"), "pull": ("https://github.com/iiasa/ixmp/pull/%s", "PR #"), } diff --git a/ixmp/__init__.py b/ixmp/__init__.py index 70da8d9c6..c9118e096 100644 --- a/ixmp/__init__.py +++ b/ixmp/__init__.py @@ -7,6 +7,7 @@ from .backend.jdbc import JDBCBackend from .core import IAMC_IDX, Platform, Scenario, TimeSeries from .model import MODELS +from .model.base import ModelError from .model.dantzig import DantzigModel from .model.gams import GAMSModel from .reporting import Reporter @@ -15,6 +16,7 @@ __all__ = [ "IAMC_IDX", "ItemType", + "ModelError", "Platform", "Reporter", "Scenario", diff --git a/ixmp/model/base.py b/ixmp/model/base.py index 84e2b0b5f..6af4c038d 100644 --- a/ixmp/model/base.py +++ b/ixmp/model/base.py @@ -1,4 +1,6 @@ import logging +import os +import re from abc import ABC, abstractmethod from ixmp.utils import maybe_check_out, maybe_commit @@ -6,6 +8,10 @@ log = logging.getLogger(__name__) +class ModelError(Exception): + """Error in model code, e.g. :meth:`.Model.run`.""" + + class Model(ABC): #: Name of the model. name = "base" @@ -22,6 +28,12 @@ def __init__(self, name, **kwargs): Model subclasses MUST document acceptable option values. """ + @classmethod + def clean_path(cls, value: str) -> str: + """Subtitute invalid characters in `value` with "_".""" + chars = r'<>"/\|?*' + (":" if os.name == "nt" else "") + return re.sub("[{}]+".format(re.escape(chars)), "_", value) + @classmethod def initialize(cls, scenario): """Set up *scenario* with required items. diff --git a/ixmp/model/gams.py b/ixmp/model/gams.py index dd553c760..10a4751b7 100644 --- a/ixmp/model/gams.py +++ b/ixmp/model/gams.py @@ -1,14 +1,18 @@ +import logging import os import re +import shutil +import tempfile from pathlib import Path -from subprocess import check_call -from tempfile import TemporaryDirectory +from subprocess import CalledProcessError, check_call from typing import Mapping from ixmp.backend import ItemType -from ixmp.model.base import Model +from ixmp.model.base import Model, ModelError from ixmp.utils import as_str_list +log = logging.getLogger(__name__) + def gams_version(): """Return the GAMS version as a string, e.g. '24.7.4'.""" @@ -43,6 +47,50 @@ def gams_version(): return re.search(pattern, output, re.MULTILINE).groups()[0] +#: Return codes used by GAMS, from +#: https://www.gams.com/latest/docs/UG_GAMSReturnCodes.html . Values over 256 are only +#: valid on Windows, and are returned modulo 256 on other platforms. +RETURN_CODE = { + 0: "Normal return", + 1: "Solver is to be called, the system should never return this number", + 2: "There was a compilation error", + 3: "There was an execution error", + 4: "System limits were reached", + 5: "There was a file error", + 6: "There was a parameter error", + 7: "There was a licensing error", + 8: "There was a GAMS system error", + 9: "GAMS could not be started", + 10: "Out of memory", + 11: "Out of disk", + 109: "Could not create process/scratch directory", + 110: "Too many process/scratch directories", + 112: "Could not delete the process/scratch directory", + 113: "Could not write the script gamsnext", + 114: "Could not write the parameter file", + 115: "Could not read environment variable", + 400: "Could not spawn the GAMS language compiler (gamscmex)", + 401: "Current directory (curdir) does not exist", + 402: "Cannot set current directory (curdir)", + 404: "Blank in system directory", + 405: "Blank in current directory", + 406: "Blank in scratch extension (scrext)", + 407: "Unexpected cmexRC", + 408: "Could not find the process directory (procdir)", + 409: "CMEX library not be found (experimental)", + 410: "Entry point in CMEX library could not be found (experimental)", + 411: "Blank in process directory", + 412: "Blank in scratch directory", + 909: "Cannot add path / unknown UNIX environment / cannot set environment variable", + 1000: "Driver error: incorrect command line parameters for gams", + 2000: "Driver error: internal error: cannot install interrupt handler", + 3000: "Driver error: problems getting current directory", + 4000: "Driver error: internal error: GAMS compile and execute module not found", + 5000: "Driver error: internal error: cannot load option handling library", +} +RETURN_CODE = {key % 256: value for key, value in RETURN_CODE.items()} + + class GAMSModel(Model): """General class for ixmp models using `GAMS `_. @@ -64,40 +112,38 @@ class GAMSModel(Model): Override the :attr:`name` attribute to provide the `model_name` for format strings. model_file : str, optional - Path to GAMS file, including '.gms' extension. - Default: ``'{model_name}.gms'`` (in the current directory). + Path to GAMS file, including '.gms' extension. Default: ``'{model_name}.gms'`` + in the current directory. case : str, optional Run or case identifier to use in GDX file names. Default: ``'{scenario.model}_{scenario.name}'``, where `scenario` is the - :class:`.Scenario` object passed to :meth:`run`. - Formatted using `model_name` and `scenario`. + :class:`.Scenario` object passed to :meth:`run`. Formatted using `model_name` + and `scenario`. in_file : str, optional - Path to write GDX input file. Default: ``'{model_name}_in.gdx'``. - Formatted using `model_name`, `scenario`, and `case`. + Path to write GDX input file. Default: ``'{model_name}_in.gdx'``. Formatted + using `model_name`, `scenario`, and `case`. out_file : str, optional - Path to read GDX output file. Default: ``'{model_name}_out.gdx'``. - Formatted using `model_name`, `scenario`, and `case`. + Path to read GDX output file. Default: ``'{model_name}_out.gdx'``. Formatted + using `model_name`, `scenario`, and `case`. solve_args : list of str, optional - Arguments to be passed to GAMS, e.g. to identify the model input and - output files. Each formatted using `model_file`, `scenario`, `case`, - `in_file`, and `out_file`. Default: + Arguments to be passed to GAMS, e.g. to identify the model input and output + files. Each formatted using `model_file`, `scenario`, `case`, `in_file`, and + `out_file`. Default: - ``'--in="{in_file}"'`` - ``'--out="{out_file}"'`` gams_args : list of str, optional - Additional arguments passed directly to GAMS without formatting, e.g. - to control solver options or behaviour. See the `GAMS - Documentation `_. + Additional arguments passed directly to GAMS without formatting, e.g. to + control solver options or behaviour. See the `GAMS Documentation `_. For example: - - ``'LogOption=4'`` prints output to stdout (not console) and the log - file. + - ``'LogOption=4'`` prints output to stdout (not console) and the log file. check_solution : bool, optional - If :obj:`True`, raise an exception if the GAMS solver did not reach - optimality. (Only for MESSAGE-scheme Scenarios.) + If :obj:`True`, raise an exception if the GAMS solver did not reach optimality. + (Only for MESSAGE-scheme Scenarios.) comment : str, optional - Comment added to Scenario when importing the solution. If omitted, no - comment is added. + Comment added to Scenario when importing the solution. If omitted, no comment is + added. equ_list : list of str, optional Equations to be imported from the `out_file`. Default: all. var_list : list of str, optional @@ -107,12 +153,12 @@ class GAMSModel(Model): #: Model name. name = "default" - #: Default model options. + #: Default model options: defaults: Mapping[str, object] = { "model_file": "{model_name}.gms", "case": "{scenario.model}_{scenario.scenario}", - "in_file": str(Path("{temp_dir}", "{model_name}_in.gdx")), - "out_file": str(Path("{temp_dir}", "{model_name}_out.gdx")), + "in_file": str(Path("{cwd}", "{model_name}_in.gdx")), + "out_file": str(Path("{cwd}", "{model_name}_out.gdx")), "solve_args": ['--in="{in_file}"', '--out="{out_file}"'], # Not formatted "gams_args": ["LogOption=4"], @@ -123,66 +169,122 @@ class GAMSModel(Model): "use_temp_dir": True, } - def __init__(self, name=None, **model_options): - self.model_name = name or self.name + def __init__(self, name_=None, **model_options): + self.model_name = self.clean_path(name_ or self.name) + + # Store options from `model_options`, otherwise from `defaults` for arg_name, default in self.defaults.items(): setattr(self, arg_name, model_options.get(arg_name, default)) - def run(self, scenario): - """Execute the model.""" - backend = scenario.platform._backend + def format_exception(self, exc, model_file): + """Format a user-friendly exception when GAMS errors.""" + msg = [ + f"GAMS errored with return code {exc.returncode}:", + # Convert a Windows return code >256 to its equivalent on *nix platforms + f" {RETURN_CODE[exc.returncode % 256]}", + "", + "For details, see the terminal output above, plus:", + f"Input data: {self.in_file}", + ] - self.scenario = scenario + # Add a reference to the listing file, if it exists + lst_file = Path(self.cwd).joinpath(model_file.name).with_suffix(".lst") + if lst_file.exists(): + msg.insert(-1, f"Listing : {lst_file}") - if self.use_temp_dir: - # Create a temporary directory; automatically deleted at the end of - # the context - _temp_dir = TemporaryDirectory() - self.temp_dir = _temp_dir.name + return ModelError("\n".join(msg)) - def format(key): - value = getattr(self, key) - try: - return value.format(**self.__dict__) - except AttributeError: - # Something like a Path; don't format it - return value + def format_option(self, name): + """Retrieve the option `name` and format it.""" + return self.format(getattr(self, name)) - # Process args in order - command = ["gams"] + def format(self, value): + """Helper for recursive formatting of model options. - model_file = Path(format("model_file")) - command.append('"{}"'.format(model_file)) + `value` is formatted with replacements from the attributes of `self`. + """ + try: + return value.format(**self.__dict__) + except AttributeError: + # Something like a Path; don't format it + return value - self.case = format("case").replace(" ", "_") - self.in_file = Path(format("in_file")) - self.out_file = Path(format("out_file")) + def remove_temp_dir(self, msg="after run()"): + """Remove the temporary directory, if any.""" + try: + if self.use_temp_dir and self.cwd.exists(): + shutil.rmtree(self.cwd) + except AttributeError: + pass # No .cwd, e.g. in a subclass + except PermissionError as e: + log.debug(f"Could not delete {repr(self.cwd)} {msg}") + log.debug(str(e)) - for arg in self.solve_args: - command.append(arg.format(**self.__dict__)) + def __del__(self): + # Try once more to remove the temporary directory. + # This appears to still fail on Windows. + self.remove_temp_dir("at GAMSModel teardown") - command.extend(self.gams_args) + def run(self, scenario): + """Execute the model.""" + # Store the scenario so its attributes can be referenced by format() + self.scenario = scenario + + # Format or retrieve the model file option + model_file = Path(self.format_option("model_file")) + + # Determine working directory for the GAMS call, possibly a temporary directory + self.cwd = Path(tempfile.mkdtemp()) if self.use_temp_dir else model_file.parent + # The "case" name + self.case = self.format_option("case").replace(" ", "_") + # Input and output file names + self.in_file = Path(self.format_option("in_file")) + self.out_file = Path(self.format_option("out_file")) + + # Assemble the full command: executable, model file, model-specific arguments, + # and general GAMS arguments + command = ( + ["gams", f'"{model_file}"'] + + [self.format(arg) for arg in self.solve_args] + + self.gams_args + ) if os.name == "nt": + # Windows: join the commands to a single string command = " ".join(command) + # Remove stored reference to the Scenario to allow it to be GC'd later + delattr(self, "scenario") + + # Common argument for write_file and read_file s_arg = dict(filters=dict(scenario=scenario)) + try: # Write model data to file - backend.write_file(self.in_file, ItemType.SET | ItemType.PAR, **s_arg) + scenario.platform._backend.write_file( + self.in_file, ItemType.SET | ItemType.PAR, **s_arg + ) except NotImplementedError: # pragma: no cover - # Currently there is no such Backend + # No coverage because there currently is no such Backend that doesn't + # support GDX + + # Remove the temporary directory, which should be empty + self.remove_temp_dir() + raise NotImplementedError( - "GAMSModel requires a Backend that can " - "write to GDX files, e.g. JDBCBackend" + "GAMSModel requires a Backend that can write to GDX files, e.g. " + "JDBCBackend" ) - # Invoke GAMS - cwd = self.temp_dir if self.use_temp_dir else model_file.parent - check_call(command, shell=os.name == "nt", cwd=cwd) + try: + # Invoke GAMS + check_call(command, shell=os.name == "nt", cwd=self.cwd) + except CalledProcessError as exc: + # Do not remove self.temp_dir; the user may want to inspect the GDX file + raise self.format_exception(exc, model_file) from None # Read model solution - backend.read_file( + scenario.platform._backend.read_file( self.out_file, ItemType.MODEL, **s_arg, @@ -191,3 +293,6 @@ def format(key): equ_list=as_str_list(self.equ_list) or [], var_list=as_str_list(self.var_list) or [], ) + + # Finished: remove the temporary directory, if any + self.remove_temp_dir() diff --git a/ixmp/tests/data/_abort.gms b/ixmp/tests/data/_abort.gms new file mode 100644 index 000000000..93a434d30 --- /dev/null +++ b/ixmp/tests/data/_abort.gms @@ -0,0 +1 @@ +$abort Test abort message. diff --git a/ixmp/tests/test_model.py b/ixmp/tests/test_model.py index 5cc3d26f0..a2d3e5a3b 100644 --- a/ixmp/tests/test_model.py +++ b/ixmp/tests/test_model.py @@ -1,9 +1,10 @@ import logging +import re import pytest from ixmp import Scenario -from ixmp.model.base import Model +from ixmp.model.base import Model, ModelError from ixmp.model.dantzig import DantzigModel from ixmp.model.gams import gams_version from ixmp.testing import assert_logs, make_dantzig @@ -20,20 +21,6 @@ class M1(Model): M1() -@pytest.mark.parametrize( - "kwargs", - [ - dict(comment=None), - dict(equ_list=None, var_list=["x"]), - dict(equ_list=["demand", "supply"], var_list=[]), - ], - ids=["null-comment", "null-list", "empty-list"], -) -def test_GAMSModel(test_mp, test_data_path, kwargs): - s = make_dantzig(test_mp) - s.solve(model="dantzig", **kwargs) - - def test_model_initialize(test_mp, caplog): # Model.initialize runs on an empty Scenario s = make_dantzig(test_mp) @@ -111,3 +98,57 @@ def test_model_initialize(test_mp, caplog): def test_gams_version(): # Returns a version string like X.Y.Z assert len(gams_version().split(".")) == 3 + + +class TestGAMSModel: + @pytest.fixture(scope="class") + def dantzig(self, test_mp): + yield make_dantzig(test_mp) + + @pytest.mark.parametrize("char", r'<>"/\|?*') + def test_filename_invalid_char(self, dantzig, char): + """Model can be solved with invalid character names.""" + name = f"foo{char}bar" + s = dantzig.clone(model=name, scenario=name) + + # Indirectly test backend.write_file("….gdx") + # This name_ keyword argument ends up received to GAMSModel.__init__ and sets + # the GAMSModel.model_name attribute, and in turn the GDX file names used. + s.solve(name_=name) + + @pytest.mark.parametrize( + "kwargs", + [ + dict(comment=None), + dict(equ_list=None, var_list=["x"]), + dict(equ_list=["demand", "supply"], var_list=[]), + ], + ids=["null-comment", "null-list", "empty-list"], + ) + def test_GAMSModel_solve(test_data_path, dantzig, kwargs): + dantzig.clone().solve(**kwargs) + + def test_error_message(self, test_data_path, test_mp): + """GAMSModel.solve() displays a user-friendly message on error.""" + # Empty Scenario + s = Scenario(test_mp, model="foo", scenario="bar", version="new") + s.commit("Initial commit") + + # Expected paths for error message + paths = map( + lambda name: re.escape(str(test_data_path.joinpath(name))), + ["_abort.lst", "default_in.gdx"], + ) + + with pytest.raises( + ModelError, + match="""GAMS errored with return code 2: + There was a compilation error + +For details, see the terminal output above, plus: +Listing : {} +Input data: {}""".format( + *paths + ), + ): + s.solve(model_file=test_data_path / "_abort.gms", use_temp_dir=False)