Repository: acorg/slurm-pipeline

Issue:
Use the cwd attribute when checking for the existence of step scripts
The checking to see if scripts exist does not use the cwd attribute. So it's not possible to have a specification that uses cwd and a relative path to a script.

Discussion:

Patch:
diff --git a/bin/slurm-pipeline-status.py b/bin/slurm-pipeline-status.py
index c0392d4..bd65b37 100755
--- a/bin/slurm-pipeline-status.py
+++ b/bin/slurm-pipeline-status.py
@@ -25,23 +25,23 @@ parser.add_argument(
           'in JSON format.'))
 
 parser.add_argument(
-    '--squeueArgs', nargs='*', default=None,
+    '--squeueArgs', nargs='*',
     help=('A list of arguments to pass to squeue (including the squeue '
-          "command itself). If not specified, the user's login name will "
-          'be appended to squeue -u.'))
+          'command itself). If not specified, "squeue -u USERNAME" is '
+          "used, where USERNAME is the user's login name."))
 
 parser.add_argument(
     '--printUnfinished', default=False, action='store_true',
     help=('If specified, print a list of job ids that have not yet finished. '
-          'This can easily be used to cancel a job, via e.g., '
-          '%s --printUnfinished -s spec.json | xargs scancel' % sys.argv[0]))
+          'This can be used to cancel a job, via e.g., '
+          '%s --printUnfinished -s status.json | xargs scancel' % sys.argv[0]))
 
 parser.add_argument(
     '--printFinal', default=False, action='store_true',
     help=('If specified, print a list of job ids issued by the final steps '
           'of a specification. This can be used with the --startAfter option '
           'to slurm-pipeline.py to make it schedule a different specification '
-          'to run only after the given specification finishes.'))
+          'to run after the given specification is completely finished.'))
 
 args = parser.parse_args()
 
diff --git a/setup.py b/setup.py
index b25b5cb..79394e7 100644
--- a/setup.py
+++ b/setup.py
@@ -3,7 +3,7 @@
 from setuptools import setup
 
 setup(name='slurm-pipeline',
-      version='1.1.5',
+      version='1.1.6',
       packages=['slurm_pipeline'],
       include_package_data=True,
       url='https://github.com/acorg/slurm-pipeline',
diff --git a/slurm_pipeline/base.py b/slurm_pipeline/base.py
index 83e586b..d9d2452 100644
--- a/slurm_pipeline/base.py
+++ b/slurm_pipeline/base.py
@@ -79,7 +79,7 @@ class SlurmPipelineBase(object):
                     'The name %r of step %d was already used in '
                     'an earlier step' % (stepName, count))
 
-            if 'collect' in step and not step.get('dependencies', None):
+            if 'collect' in step and not step.get('dependencies'):
                 raise SpecificationError(
                     "Step %d (%r) is a 'collect' step but does not have any "
                     "dependencies" %
diff --git a/slurm_pipeline/pipeline.py b/slurm_pipeline/pipeline.py
index 817c9ae..503d51e 100644
--- a/slurm_pipeline/pipeline.py
+++ b/slurm_pipeline/pipeline.py
@@ -40,23 +40,28 @@ class SlurmPipeline(SlurmPipelineBase):
         @raise SpecificationError: if there is anything wrong with the
             specification.
         """
-        if 'scheduledAt' in specification:
-            raise SpecificationError(
-                "The specification has a top-level 'scheduledAt' key, "
-                'but was not passed as a status specification')
+        SlurmPipelineBase.checkSpecification(specification)
 
         for count, step in enumerate(specification['steps'], start=1):
-            if not path.exists(step['script']):
+            try:
+                cwd = step['cwd']
+            except KeyError:
+                script = step['script']
+            else:
+                script = step['script']
+                if not path.isabs(script):
+                    if cwd:
+                        script = path.join(cwd, script)
+
+            if not path.exists(script):
                 raise SpecificationError(
                     'The script %r in step %d does not exist' %
                     (step['script'], count))
 
-            if not os.access(step['script'], os.X_OK):
+            if not os.access(script, os.X_OK):
                 raise SpecificationError(
                     'The script %r in step %d is not executable' %
-                    (step['script'], count))
-
-        SlurmPipelineBase.checkSpecification(specification)
+                    (script, count))
 
     def schedule(self, force=False, firstStep=None, lastStep=None, sleep=0.0,
                  scriptArgs=None, skip=None, startAfter=None, nice=None):


Test patch:
diff --git a/test/test_pipeline.py b/test/test_pipeline.py
index a665d01..fb2ccfc 100644
--- a/test/test_pipeline.py
+++ b/test/test_pipeline.py
@@ -1,4 +1,4 @@
-from os import X_OK
+from os import X_OK, path
 from unittest import TestCase
 from six import assertRaisesRegex
 from json import dumps
@@ -22,7 +22,7 @@ class TestSlurmPipeline(TestCase):
 
     @patch('os.access')
     @patch('os.path.exists')
-    def testAccessFails(self, existsMock, accessMock):
+    def testNonexecutableScript(self, existsMock, accessMock):
         """
         If os.access fails, a SpecificationError must be raised.
         """
@@ -39,6 +39,25 @@ class TestSlurmPipeline(TestCase):
                               ]
                           })
 
+    @patch('os.access')
+    @patch('os.path.exists')
+    def testNonexistentScript(self, existsMock, accessMock):
+        """
+        If a step has a 'script' key that mentions a non-existent file, a
+        SpecificationError must be raised.
+        """
+        existsMock.return_value = False
+        error = "^The script 'script' in step 1 does not exist$"
+        assertRaisesRegex(self, SpecificationError, error, SlurmPipeline,
+                          {
+                              'steps': [
+                                  {
+                                      'name': 'name',
+                                      'script': 'script',
+                                  },
+                              ]
+                          })
+
     @patch('os.access')
     @patch('os.path.exists')
     def testAccessAndExistsAreCalled(self, existsMock, accessMock):
@@ -58,20 +77,55 @@ class TestSlurmPipeline(TestCase):
         existsMock.assert_called_once_with('script')
         accessMock.assert_called_once_with('script', X_OK)
 
-    def testNonexistentScript(self):
+    @patch('os.access')
+    @patch('os.path.exists')
+    @patch('os.path.isabs')
+    def testAccessAndExistsAreCalledWithCwd(self, isabsMock, existsMock,
+                                            accessMock):
         """
-        If a step has a 'script' key that mentions a non-existent file, a
-        SpecificationError must be raised.
+        os.access, os.path.exists, and os.path.isabs must all be called as
+        expected, including the cwd from the step, as the specification is
+        checked.
         """
-        error = "^The script 'script' in step 1 does not exist$"
-        assertRaisesRegex(self, SpecificationError, error, SlurmPipeline,
-                          {
-                              'steps': [
-                                  {
-                                      'script': 'script',
-                                  },
-                              ]
-                          })
+        isabsMock.return_value = False
+        SlurmPipeline(
+            {
+                'steps': [
+                    {
+                        'cwd': 'dir',
+                        'name': 'name',
+                        'script': 'script',
+                    },
+                ]
+            })
+        script = path.join('dir', 'script')
+        isabsMock.assert_called_once_with('script')
+        existsMock.assert_called_once_with(script)
+        accessMock.assert_called_once_with(script, X_OK)
+
+    @patch('os.access')
+    @patch('os.path.exists')
+    @patch('os.path.isabs')
+    def testAccessAndExistsAreCalledWithAbsolutePathScript(
+            self, isabsMock, existsMock, accessMock):
+        """
+        os.access, os.path.exists, and os.path.isabs must all be called as
+        expected when the script path is absolute, as the specification is
+        checked.
+        """
+        SlurmPipeline(
+            {
+                'steps': [
+                    {
+                        'cwd': 'dir',
+                        'name': 'name',
+                        'script': '/bin/script',
+                    },
+                ]
+            })
+        isabsMock.assert_called_once_with('/bin/script')
+        existsMock.assert_called_once_with('/bin/script')
+        accessMock.assert_called_once_with('/bin/script', X_OK)
 
     @patch('os.access')
     @patch('os.path.exists')
@@ -200,18 +254,6 @@ class TestSlurmPipeline(TestCase):
         specification = sp.schedule()
         self.assertIsInstance(specification['scheduledAt'], float)
 
-    def testAlreadyScheduled(self):
-        """
-        If a specification with a top-level 'scheduledAt' key is passed to
-        SlurmPipeline, a SpecificationError must be raised.
-        """
-        error = ("^The specification has a top-level 'scheduledAt' key, but "
-                 'was not passed as a status specification$')
-        assertRaisesRegex(self, SpecificationError, error, SlurmPipeline, {
-            'scheduledAt': None,
-            'steps': [],
-        })
-
     @patch('subprocess.check_output')
     @patch('os.access')
     @patch('os.path.exists')