Repository: acorg/slurm-pipeline

Issue:
Improve error output when running step scripts
Catch a subprocess error and print the stdout and stderr of the offending script.

Discussion:

Patch:
diff --git a/setup.py b/setup.py
index 68cd7fc..e20f87c 100644
--- a/setup.py
+++ b/setup.py
@@ -3,7 +3,7 @@
 from setuptools import setup
 
 setup(name='slurm-pipeline',
-      version='1.1.8',
+      version='1.1.9',
       packages=['slurm_pipeline'],
       include_package_data=True,
       url='https://github.com/acorg/slurm-pipeline',
diff --git a/slurm_pipeline/pipeline.py b/slurm_pipeline/pipeline.py
index e8c6eaf..443c1ae 100644
--- a/slurm_pipeline/pipeline.py
+++ b/slurm_pipeline/pipeline.py
@@ -281,11 +281,28 @@ class SlurmPipeline(SlurmPipelineBase):
         @param step: A C{dict} with a job specification.
         @param env: A C{str} key to C{str} value environment for the script.
         @param args: A C{list} of command-line arguments.
-        @raise SchedulingError: If a script outputs a task name more than once.
+        @raise SchedulingError: If a script outputs a task name more than once
+            or if the step script cannot be executed.
         """
-        step['stdout'] = subprocess.check_output(
-            [step['script']] + args, cwd=step.get('cwd', '.'), env=env,
-            stdin=DEVNULL, universal_newlines=True)
+        try:
+            step['stdout'] = subprocess.check_output(
+                [step['script']] + args, cwd=step.get('cwd', '.'), env=env,
+                stdin=DEVNULL, universal_newlines=True)
+        except subprocess.CalledProcessError as e:
+            import sys
+            if sys.version_info >= (3, 5):
+                raise SchedulingError(
+                    "Could not execute step '%s' script '%s' in directory "
+                    "'%s'. Attempted command: '%s'. Exit status: %s. Standard "
+                    "output: '%s'. Standard error: '%s'." % (
+                        step['name'], step['script'], step.get('cwd', '.'),
+                        e.cmd, e.returncode, e.output, e.stderr))
+            else:
+                raise SchedulingError(
+                    "Could not execute step '%s' script '%s' in directory "
+                    "'%s'. Attempted command: '%s'. Exit status: %s."
+                    % (step['name'], step['script'],  step.get('cwd', '.'),
+                       e.cmd, e.returncode))
 
         # Look at all output lines for task names and SLURM job ids created
         # (if any) by this script. Ignore any non-matching output.


Test patch:
diff --git a/test/test_pipeline.py b/test/test_pipeline.py
index 4c85b14..a392015 100644
--- a/test/test_pipeline.py
+++ b/test/test_pipeline.py
@@ -3,6 +3,8 @@ from unittest import TestCase
 from six import assertRaisesRegex
 from json import dumps
 import platform
+from subprocess import CalledProcessError
+from sys import version_info
 
 from slurm_pipeline.pipeline import SlurmPipeline, DEVNULL
 from slurm_pipeline.error import SchedulingError, SpecificationError
@@ -797,7 +799,8 @@ class TestSlurmPipeline(TestCase):
     @patch('subprocess.check_output')
     @patch('os.access')
     @patch('os.path.exists')
-    def testCwdWithRelativeScriptPath(self, existsMock, accessMock,
+    @patch('os.path.isdir')
+    def testCwdWithRelativeScriptPath(self, isdirMock, existsMock, accessMock,
                                       subprocessMock):
         """
         If a step has a cwd set and its script is a relative path, the path of
@@ -1682,3 +1685,41 @@ class TestSlurmPipeline(TestCase):
                  "\[-10000, 10000\] range$")
         assertRaisesRegex(self, SchedulingError, error, sp.schedule,
                           nice=-10001)
+
+    @patch('subprocess.check_output')
+    @patch('os.access')
+    @patch('os.path.exists')
+    @patch('os.path.isdir')
+    def testSubprocessExecRaises(self, isdirMock, existsMock, accessMock,
+                                 subprocessMock):
+        """
+        If subprocess.check_output raises CalledProcessError, we must
+        raise a corresponding SchedulingError.
+        """
+        sp = SlurmPipeline(
+            {
+                'steps': [
+                    {
+                        'cwd': 'dir',
+                        'name': 'name1',
+                        'script': 'script1',
+                    },
+                ],
+            })
+
+        if version_info >= (3, 5):
+            subprocessMock.side_effect = CalledProcessError(
+                3, 'command.sh', output='the stdout', stderr='the stderr')
+
+            error = ("^Could not execute step 'name1' script 'script1' in "
+                     "directory 'dir'\. Attempted command: 'command.sh'\. "
+                     "Exit status: 3\. Standard output: 'the stdout'\. "
+                     "Standard error: 'the stderr'\.$")
+        else:
+            subprocessMock.side_effect = CalledProcessError(3, 'command.sh')
+
+            error = ("^Could not execute step 'name1' script 'script1' in "
+                     "directory 'dir'\. Attempted command: 'command.sh'\. "
+                     "Exit status: 3\.$")
+
+        assertRaisesRegex(self, SchedulingError, error, sp.schedule)