feat: container error handling

spras/containers.py

@@ -3,9 +3,11 @@
 import re
 import subprocess
 from pathlib import Path, PurePath, PurePosixPath
+import textwrap
 from typing import Any, Dict, List, Optional, Tuple, Union
 
 import docker
+import docker.errors
 
 import spras.config as config
 from spras.logging import indent
@@ -120,6 +122,45 @@ def prepare_dsub_cmd(flags: dict):
     print(f"dsub command: {dsub_command}")
     return dsub_command
 
+class ContainerError(RuntimeError):
+    """Raises when anything goes wrong inside a container"""
+
+    error_code: int
+    stdout: Optional[str]
+    stderr: Optional[str]
+
+    def __init__(self, message: str, error_code: int, stdout: Optional[str], stderr: Optional[str], *args):
+        """
+        Constructs a new ContainerError.
+
+        @param message: The message to display to the user. This should usually refer to the indent call to differentriate between
+        general logging done by Snakemake/logging calls.
+        @param error_code: Also known as exit status; this should generally be non-zero for ContainerErrors.
+        @param stdout: The standard output stream. If the origin of the stream is unknown, leave it in stdout.
+        @param stderr: The standard error stream.
+        """
+
+        # https://stackoverflow.com/a/26938914/7589775
+        self.message = message
+
+        self.error_code = error_code
+        self.stdout = stdout
+        self.stderr = stderr
+
+        super(ContainerError, self).__init__(message, error_code, stdout, stderr, *args) 
+
+    def streams_contain(self, snippet: str):
+        stdout = self.stdout if self.stdout else ''
+        stderr = self.stderr if self.stderr else ''
+
+        return snippet in stdout or snippet in stderr
+
+    # Due to
+    # https://github.com/snakemake/snakemake/blob/d4890b4da691506b6a258f7534ac41fdb7ef5ab4/src/snakemake/exceptions.py#L18
+    # this overrides the tostr implementation to have nicer container errors
+    def __str__(self):
+        return self.message
+
 
 # TODO consider a better default environment variable
 # TODO environment currently a single string (e.g. 'TMPDIR=/OmicsIntegrator1'), should it be a list?
@@ -171,22 +212,29 @@ def run_container_and_log(name: str, framework: str, container_suffix: str, comm
                 if 'message' in out:
                     # This is the format of a singularity message.
                     # See https://singularityhub.github.io/singularity-cli/api/source/spython.main.html?highlight=execute#spython.main.execute.execute.
-                    if 'return_code' in out and not out['return_code'] == 0:
-                        print(f"(Program exited with non-zero exit code '{out['return_code']}')")
+                    exit_status = int(out['return_code']) if 'return_code' in out else 0
                     out = ''.join(out['message'])
+                    if exit_status != 0:
+                        message = f'An unexpected non-zero exit status ({exit_status}) occured while running this singularity container:\n' + indent(out)
+                        raise ContainerError(message, exit_status, out, None)
                 else:
-                    print("Note: This is an unknown message format - if you want this pretty printed, please file an issue at https://github.com/Reed-CompBio/spras/issues/new.")
+                    print("Note: The following output is an unknown message format which should be properly handled.")
+                    print("Please file an issue at https://github.com/Reed-CompBio/spras/issues/new with this output.")
                     out = str(out)
             elif not isinstance(out, str):
                 out = str(out, "utf-8")
             print(indent(out))
     except docker.errors.ContainerError as err:
-        print(f"(Command formatted as list: `{err.command}`)")
-        print(f"An unexpected non-zero exit status ({err.exit_status}) inside the docker image {err.image} occurred:")
-        err = str(err.stderr if err.stderr is not None else "", "utf-8")
-        print(indent(err))
-    except Exception as err:
-        raise err
+        # TODO: does this lose us any information provided by stdout while the container was running?
+        # ContainerError doesn't expose any stdout property.
+
+        stderr = err.stderr if err.stderr else ''
+        stderr = str(stderr, 'utf-8') if isinstance(stderr, bytes) else stderr
+
+        message = textwrap.dedent(f'''\
+                                  (Command formatted as list: `{err.command}`)
+                                  An unexpected non-zero exit status ({err.exit_status}) inside the docker image {err.image} occurred:\n''') + indent(stderr)
+        raise ContainerError(message, err.exit_status, None, stderr)
 
 # TODO any issue with creating a new client each time inside this function?
 def run_container_docker(container: str, command: List[str], volumes: List[Tuple[PurePath, PurePath]], working_dir: str, environment: str = 'SPRAS=True'):

spras/domino.py

@@ -3,7 +3,7 @@
 
 import pandas as pd
 
-from spras.containers import prepare_volume, run_container_and_log
+from spras.containers import ContainerError, prepare_volume, run_container_and_log
 from spras.interactome import (
     add_constant,
     reinsert_direction_col_undirected,
@@ -137,12 +137,19 @@ def run(network=None, active_genes=None, output_file=None, slice_threshold=None,
         if module_threshold is not None:
             domino_command.extend(['--module_threshold', str(module_threshold)])
 
-        run_container_and_log('DOMINO',
-                             container_framework,
-                             container_suffix,
-                             domino_command,
-                             volumes,
-                             work_dir)
+        try:
+            run_container_and_log('DOMINO',
+                                container_framework,
+                                container_suffix,
+                                domino_command,
+                                volumes,
+                                work_dir)
+        except ContainerError as err:
+            # TODO: can we more appropiately handle this case? Here, DOMINO
+            # still outputs to our output folder with a still viable HTML output.
+            # https://github.com/Reed-CompBio/spras/pull/103#issuecomment-1681526958
+            if not err.streams_contain("ValueError: cannot apply union_all to an empty list"):
+                raise err
 
         # DOMINO creates a new folder in out_dir to output its modules HTML files into called active_genes
         # The filename is determined by the input active_genes and cannot be configured

spras/omicsintegrator2.py

@@ -2,7 +2,7 @@
 
 import pandas as pd
 
-from spras.containers import prepare_volume, run_container_and_log
+from spras.containers import ContainerError, prepare_volume, run_container_and_log
 from spras.dataset import Dataset
 from spras.interactome import reinsert_direction_col_undirected
 from spras.prm import PRM
@@ -119,12 +119,27 @@ def run(edges=None, prizes=None, output_file=None, w=None, b=None, g=None, noise
             command.extend(['--seed', str(seed)])
 
         container_suffix = "omics-integrator-2:v2"
-        run_container_and_log('Omics Integrator 2',
-                             container_framework,
-                             container_suffix,
-                             command,
-                             volumes,
-                             work_dir)
+
+        # We use this later either if we encounter unrecoverable OI2 errors
+        # or as the main output file we want to post-process in parse_output.
+        output_tsv = Path(out_dir, 'oi2.tsv')
+
+        try:
+            run_container_and_log('Omics Integrator 2',
+                                container_framework,
+                                container_suffix,
+                                command,
+                                volumes,
+                                work_dir)
+        except ContainerError as err:
+            needle = "all the input arrays must have same number of dimensions, but the array at index 0 has 2 dimension(s) and the array at index 1 has 1 dimension(s)"
+            if not err.streams_contain(needle):
+                raise err
+            else:
+                # https://github.com/Reed-CompBio/spras/issues/218
+                # This error occurs when we have an empty dataframe passed to OI2.
+                Path(output_tsv).write_text("protein1\tprotein2\tcost\n")
+
 
         # TODO do we want to retain other output files?
         # TODO if deleting other output files, write them all to a tmp directory and copy

test/OmicsIntegrator2/input/empty/oi2-edges.txt

@@ -0,0 +1 @@
+protein1	protein2	cost

test/OmicsIntegrator2/input/empty/oi2-prizes.txt

@@ -0,0 +1 @@
+name	 prize

test/OmicsIntegrator2/test_oi2.py

@@ -8,11 +8,14 @@
 
 config.init_from_file("config/config.yaml")
 
-TEST_DIR = 'test/OmicsIntegrator2/'
-EDGE_FILE = TEST_DIR+'input/oi2-edges.txt'
-PRIZE_FILE = TEST_DIR+'input/oi2-prizes.txt'
+TEST_DIR = Path('test', 'OmicsIntegrator2')
 OUT_FILE = Path(TEST_DIR, 'output', 'test.tsv')
 
+EDGE_FILE = TEST_DIR / 'input' / 'simple' / 'oi2-edges.txt'
+PRIZE_FILE = TEST_DIR / 'input' / 'simple' / 'oi2-prizes.txt'
+
+EDGE_FILE_EMPTY = TEST_DIR / 'input' / 'empty' / 'oi2-edges.txt'
+PRIZE_FILE_EMPTY = TEST_DIR / 'input' / 'empty' / 'oi2-prizes.txt'
 
 class TestOmicsIntegrator2:
     """
@@ -58,6 +61,13 @@ def test_oi2_missing(self):
             OmicsIntegrator2.run(edges=EDGE_FILE,
                                  prizes=PRIZE_FILE)
 
+    def test_oi2_empty(self):
+        OUT_FILE.unlink(missing_ok=True)
+        OmicsIntegrator2.run(edges=EDGE_FILE_EMPTY,
+                             prizes=PRIZE_FILE_EMPTY,
+                             output_file=OUT_FILE)
+        assert OUT_FILE.exists()
+
     # Only run Singularity test if the binary is available on the system
     # spython is only available on Unix, but do not explicitly skip non-Unix platforms
     @pytest.mark.skipif(not shutil.which('singularity'), reason='Singularity not found on system')