Merge pull request #398 from khaeru/hotfix/model-name

Fix writing filenames with invalid characters

.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: |

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.
 

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 #"),
 }
 

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",

ixmp/model/base.py

@@ -1,11 +1,17 @@
 import logging
+import os
+import re
 from abc import ABC, abstractmethod
 
 from ixmp.utils import maybe_check_out, maybe_commit
 
 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.

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 <https://gams.com/>`_.
 
@@ -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 <https://www.gams.com/latest/docs/UG_GamsCall.html#UG_GamsCall_ListOfCommandLineParameters>`_.
+        Additional arguments passed directly to GAMS without formatting, e.g. to
+        control solver options or behaviour. See the `GAMS Documentation <https://www.gams.com/latest/docs/UG_GamsCall.html#UG_GamsCall_ListOfCommandLineParameters>`_.
         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()

ixmp/tests/data/_abort.gms

@@ -0,0 +1 @@
+$abort Test abort message.