Fix the issues with CLI (#60)

* Fix an issue that can stop local file from being normally processed. * Fix the failing test by cleaning up the Dockerfile that relies on Oracle Java. * Take the retry policy out from the cromwell-tools for clarity.
broadinstitute · Jan 17, 2019 · 5206d04 · 5206d04
1 parent 67abfbb
commit 5206d04
Show file tree

Hide file tree

Showing 7 changed files with 99 additions and 65 deletions.
diff --git a/Dockerfile b/Dockerfile
@@ -12,22 +12,9 @@ RUN apt-get update && apt-get upgrade -y && apt-get -y install --no-install-reco
     git
 
 # Install java 8
-ENV DEBIAN_FRONTEND noninteractive
-ENV JAVA_HOME       /usr/lib/jvm/java-8-oracle
-ENV LANG            en_US.UTF-8
-ENV LC_ALL          en_US.UTF-8
-
 RUN apt-get update && \
-  apt-get install -y --no-install-recommends locales && \
-  locale-gen en_US.UTF-8 && \
-  apt-get dist-upgrade -y && \
-  apt-get --purge remove openjdk* && \
-  echo "oracle-java8-installer shared/accepted-oracle-license-v1-1 select true" | debconf-set-selections && \
-  echo "deb http://ppa.launchpad.net/webupd8team/java/ubuntu xenial main" > /etc/apt/sources.list.d/webupd8team-java-trusty.list && \
-  apt-key adv --keyserver keyserver.ubuntu.com --recv-keys EEA14886 && \
-  apt-get update && \
-  apt-get install -y --no-install-recommends oracle-java8-installer oracle-java8-set-default && \
-  apt-get clean all
+    apt-get install -y openjdk-8-jdk && \
+    apt-get clean all
 
 # Download and expose womtool
 ADD https://github.com/broadinstitute/cromwell/releases/download/35/womtool-35.jar /usr/local/bin/womtool/womtool-35.jar

diff --git a/cromwell_tools/cli.py b/cromwell_tools/cli.py
@@ -5,11 +5,12 @@
 
 
 def parser(arguments=None):
-    # TODO: dynamically walk through the commands and automatcially create parsers here
+    # TODO: dynamically walk through the commands and automatcally create parsers here
 
     main_parser = argparse.ArgumentParser()
 
     subparsers = main_parser.add_subparsers(help='sub-command help', dest='command')
+
     # sub-commands of cromwell-tools
     submit = subparsers.add_parser(
             'submit', help='submit help', description='Submit a WDL workflow on Cromwell.')
@@ -42,59 +43,115 @@ def parser(arguments=None):
     def add_auth_args(subcommand_parser):
         for arg_dest, help_text in auth_args.items():
             subcommand_parser.add_argument('--{arg}'.format(arg=arg_dest.replace('_', '-')),
-                                           dest=arg_dest, default=None, type=str, help=help_text)
+                                           dest=arg_dest,
+                                           default=None,
+                                           type=str,
+                                           help=help_text)
     # TODO: this should be a group which is called authentication
     for p in cromwell_sub_commands:
         add_auth_args(p)
 
     # submit arguments
-    # TODO: add short flags for arguments
-    submit.add_argument('--wdl-file', dest='wdl_file', type=argparse.FileType('r'), required=True,
-                        help='The workflow source file to submit for execution.')
-    submit.add_argument('--inputs-file', dest='inputs_file', type=argparse.FileType('r'), required=True,
-                        help='File-like object containing input data in JSON format.')
-    submit.add_argument('--zip-file', dest='zip_file', type=argparse.FileType('r'),
-                        help='Zip file containing dependencies.')
-    submit.add_argument('--inputs-file2', dest='inputs_file2', type=argparse.FileType('r'),
-                        help='Inputs file 2.')
-    submit.add_argument('--options-file', dest='options_file', type=argparse.FileType('r'),
-                        help='Cromwell configs file.')
-
-    submit.add_argument('--collection-name', dest='collection_name', type=str, default=None,
+    submit.add_argument('-w',
+                        '--wdl-file',
+                        dest='wdl_file',
+                        type=str,
+                        required=True,
+                        help='Path to the workflow source file to submit for execution.')
+    submit.add_argument('-i',
+                        '--inputs_files',
+                        dest='inputs_files',
+                        nargs='+',
+                        type=str,
+                        required=True,
+                        help='Path(s) to the input file(s) containing input data in JSON format, separated by space.')
+    submit.add_argument('-d',
+                        '--deps-file',
+                        dest='dependencies',
+                        nargs='+',
+                        type=str,
+                        help='Path to the Zip file containing dependencies, or a list of raw dependency files to '
+                             'be zipped together separated by space.')
+    submit.add_argument('-o',
+                        '--options-file',
+                        dest='options_file',
+                        type=str,
+                        help='Path to the Cromwell configs JSON file.')
+    # TODO: add a mutually exclusive group to make it easy to add labels for users
+    submit.add_argument('-l',
+                        '--label-file',
+                        dest='label_file',
+                        type=str,
+                        default=None,
+                        help='Path to the JSON file containing a collection of key/value pairs for workflow labels.')
+    submit.add_argument('-c',
+                        '--collection-name',
+                        dest='collection_name',
+                        type=str,
+                        default=None,
                         help='Collection in SAM that the workflow should belong to, if use CaaS.')
-    submit.add_argument('--label', dest='label', type=argparse.FileType('r'), default=None,
-                        help='JSON file containing a collection of key/value pairs for workflow labels.')
-    submit.add_argument('--validate-labels', dest='validate_labels', type=bool, default=False,
-                        help='Whether to validate cromwell labels.')
-    submit.add_argument('--on-hold', dest='on_hold', type=bool, default=False,
+    submit.add_argument('--on-hold',
+                        dest='on_hold',
+                        type=bool,
+                        default=False,
                         help='Whether to submit the workflow in "On Hold" status.')
+    submit.add_argument('--validate-labels',
+                        dest='validate_labels',
+                        type=bool,
+                        default=False,
+                        help='Whether to validate cromwell labels.')
 
     # wait arguments
-    wait.add_argument('workflow_ids', nargs='+')
-    wait.add_argument('--timeout-minutes', dest='timeout_minutes', type=int, default=120,
+    wait.add_argument('workflow_ids',
+                      nargs='+')
+    wait.add_argument('--timeout-minutes',
+                      dest='timeout_minutes',
+                      type=int,
+                      default=120,
                       help='number of minutes to wait before timeout')
-    wait.add_argument('--poll-interval-seconds', dest='poll_interval_seconds', type=int, default=30,
+    wait.add_argument('--poll-interval-seconds',
+                      dest='poll_interval_seconds',
+                      type=int,
+                      default=30,
                       help='seconds between polling cromwell for workflow status')
 
     # status arguments
-    status.add_argument('--uuid', required=True, help='A Cromwell workflow UUID, which is the workflow identifier.')
+    status.add_argument('--uuid',
+                        required=True,
+                        help='A Cromwell workflow UUID, which is the workflow identifier.')
 
     # abort arguments
-    abort.add_argument('--uuid', required=True, help='A Cromwell workflow UUID, which is the workflow identifier.')
+    abort.add_argument('--uuid',
+                       required=True,
+                       help='A Cromwell workflow UUID, which is the workflow identifier.')
 
     # release_hold arguments
-    release_hold.add_argument('--uuid', required=True, help='A Cromwell workflow UUID, which is the workflow identifier.')
+    release_hold.add_argument('--uuid',
+                              required=True,
+                              help='A Cromwell workflow UUID, which is the workflow identifier.')
 
     # query arguments
     # TODO: implement CLI entry for query API.
 
     # validate arguments
-    validate.add_argument('--wdl-file', dest='wdl', type=str, required=True)
-    validate.add_argument('--womtool-path', dest='womtool_path', type=str, required=True,
+    validate.add_argument('-w',
+                          '--wdl-file',
+                          dest='wdl',
+                          type=str,
+                          required=True)
+    validate.add_argument('--womtool-path',
+                          dest='womtool_path',
+                          type=str,
+                          required=True,
                           help='path to cromwell womtool jar')
-    validate.add_argument('--dependencies-json', dest='dependencies_json', type=str, default=None)
+    validate.add_argument('--dependencies-json',
+                          dest='dependencies_json',
+                          type=str,
+                          default=None)
 
+    # group all of the arguments
     args = vars(main_parser.parse_args(arguments))
+
     # TODO: see if this can be moved or if the commands can be populated from above
     if args['command'] in ('submit', 'wait', 'status', 'abort', 'release_hold', 'health', 'validate'):
         if args['command'] == 'validate': args['command'] = 'validate_workflow'

diff --git a/cromwell_tools/cromwell_api.py b/cromwell_tools/cromwell_api.py
@@ -11,7 +11,6 @@
 import tempfile
 from datetime import datetime, timedelta
 from subprocess import PIPE, Popen
-from tenacity import retry, stop_after_delay, wait_exponential
 
 from cromwell_tools import utilities
 from cromwell_tools.utilities import _localize_file, validate_cromwell_label
@@ -128,11 +127,9 @@ def health(cls, auth, raise_for_status=False):
         return response
 
     @classmethod
-    @retry(reraise=True, wait=wait_exponential(multiplier=1, max=10), stop=stop_after_delay(20))
     def submit(cls, auth, wdl_file, inputs_files=None, options_file=None, dependencies=None,
-               label_file=None, collection_name=None, on_hold=False, validate_labels=False):
-        # TODO: Add `raise_for_status` flag to this method and remove the hard-coded `raise_for_status()` call.
-        # TODO: Purifying the methods by taking the specific retry policy out from this method.
+               label_file=None, collection_name=None, on_hold=False, validate_labels=False,
+               raise_for_status=False):
         """ Submits a workflow to Cromwell.
 
         Args:
@@ -156,6 +153,8 @@ def submit(cls, auth, wdl_file, inputs_files=None, options_file=None, dependenci
                 default None)
             on_hold: (Optional[bool]) Whether to submit the workflow in "On Hold" status. (default False)
             validate_labels (Optional[bool]) If True, validate cromwell labels. (default False)
+            raise_for_status (Optional[bool]): Whether to check and raise for status based on the response. (default
+                False)
 
         Raises:
             requests.exceptions.HTTPError: This will be raised when raise_for_status is True and Cromwell returns
@@ -180,7 +179,8 @@ def submit(cls, auth, wdl_file, inputs_files=None, options_file=None, dependenci
                                  auth=auth.auth,
                                  headers=auth.header)
 
-        response.raise_for_status()
+        if raise_for_status:
+            cls._check_and_raise_status(response)
         return response
 
     @classmethod
@@ -191,7 +191,7 @@ def wait(cls, workflow_ids, auth, timeout_minutes=120, poll_interval_seconds=30,
         one of the workflows fails or is aborted.
 
         Args:
-        workflow_ids (List): Workflow ids to wait for terminal status.
+        workflow_ids (List[str]): A list of workflow ids to wait for terminal status.
         timeout_minutes (int): Maximum number of minutes to wait. (default 120)
         auth (cromwell_tools._cromwell_auth.CromwellAuth): Authentication class holding headers
             or auth information to a Cromwell server.

diff --git a/cromwell_tools/tests/test_cromwell_api.py b/cromwell_tools/tests/test_cromwell_api.py
@@ -7,7 +7,6 @@
 import six
 import tempfile
 import unittest
-from tenacity import stop_after_attempt, stop_after_delay
 
 
 six.add_move(six.MovedModule('mock', 'mock', 'unittest.mock'))
@@ -82,24 +81,17 @@ def _request_callback(request, context):
             self.assertEqual(result.headers.get('test'), 'header')
 
     @requests_mock.mock()
-    def test_submit_workflow_retries_on_error(self, mock_request):
+    def test_submit_workflow_handlers_error_response(self, mock_request):
 
         def _request_callback(request, context):
             context.status_code = 500
             context.headers['test'] = 'header'
             return {'status': 'error', 'message': 'Internal Server Error'}
 
-        # Make the test complete faster by limiting the number of retries
-        CromwellAPI.submit.retry.stop = stop_after_attempt(2)
-
         # Check request actions
         for cromwell_auth in self.auth_options:
             with self.assertRaises(requests.HTTPError):
-                _ = self._submit_workflows(cromwell_auth, mock_request, _request_callback)
-                self.assertEqual(mock_request.call_count, 2)
-
-        # Reset default retry value
-        CromwellAPI.submit.retry.stop = stop_after_delay(20)
+                self._submit_workflows(cromwell_auth, mock_request, _request_callback).raise_for_status()
 
     @requests_mock.mock()
     def test_query_workflows_returns_200(self, mock_request):

diff --git a/cromwell_tools/utilities.py b/cromwell_tools/utilities.py
@@ -136,7 +136,7 @@ def read_local_file(path):
     Returns:
         contents (bytes or str): The loaded content. bytes in Python3 and str in Python2.
     """
-    with open(path, 'rb') as f:
+    with open(os.path.abspath(path), 'rb') as f:
         contents = f.read()
     return contents
 

diff --git a/requirements.txt b/requirements.txt
@@ -1,5 +1,4 @@
 requests>=2.20.0
 six>=1.11.0
 google-auth>=1.6.1,<2
-tenacity>=4.10.0
 setuptools_scm>=3.1.0,<4
diff --git a/setup.py b/setup.py
@@ -16,8 +16,7 @@
 install_requires = [
     'requests>=2.20.0',
     'six>=1.11.0',
-    'google-auth>=1.6.1,<2',
-    'tenacity>=4.10.0',
+    'google-auth>=1.6.1,<2'
     'setuptools_scm>=3.1.0,<4'
 ]