Analysis refactor allow users change dflt params (#2136)

josenavas · antgonza · commit 3051113f1667 · 2017-06-01T07:18:58.000-06:00
* fix #1505 * improving some GUI stuff * improving some GUI stuff - missing lines * addressing all comments * ready for review * fix #1987 * initial commit * requested changes * fix filter job list * Fixing server cert (#2051) * fix get_studies * flake8 * fix #503 * fix #2010 * fix #1913 * fix errors * addressing @josenavas comment * flake8 * fix #1010 * fix #1066 (#2058) * addressing @josenavas comments * fix #1961 * fix #1837 * Automatic jobs & new stats (#2057) * fix #814, fix #1636 * fixing error in test-env * fixing stats.html call * adding img * addressing @josenavas comments * rm for loops * addresssing @ElDeveloper comments * generalizing this functionality * fix #1816 * fix #1959 * addressing @josenavas comments * addressing @josenavas comments * fixing error * fixed? * addressing @josenavas comments * addressing @wasade comments * fix flake8 * generate biom and metadata release (#2066) * initial commit * adding portal * addressing @josenavas comments * pid -> qiita_artifact_id * addressing @josenavas comments * addressing @ElDeveloper comments * rm 50.sql * database changes to fix 969 * adding delete * addressing @josenavas comments * addressing @ElDeveloper comments * duh! * fix generate_biom_and_metadata_release (#2072) * fix generate_biom_and_metadata_release * addressing @ElDeveloper comment * Removing qiita ware code that will not be used anymore * Organizing the handlers and new analysis description page * fixing timestamp * rm formats * st -> pt * Connecting the analysis creation and making interface responsive * Addressing @antgonza's comments * Initial artifact GUI refactor * Removing unused code * moving to ISO 8601 - wow :'( * fix errors * addressing @wasade comments * Adding can_edit call to the analysis * Fixing artifact rest API since not all artifacts have study * Adding can_be_publicized call to analysis * Adding QiitaHTTPError to handle errors gracefully * Adding safe_execution contextmanager * Fixing typo * Adding qiita test checker * Adapting some artifact handlers * Abstracting the graph reloading and adding some documentation * Fixing typo * Fixing changing artifact visibility * Fixing delete * Fixing artifact deletion * Adding default parameters to the commands * Fixing processing page * Fixing variable name * fixing private/public studies * Changing bdiv metrics to single choice * sanbox-to-sandbox * flake8 * Fixing patch * fixing other issues * adding share documentation * psycopg2 <= 2.7 * psycopg2 < 2.7 * Various small fixes to be able to run tests on the plugins * Adding private module * Fixing processing job completion * Fixing patch 52 * Fixing call * Fixing complete * small fixes * Adding processing handlers * Fixing url and bug on processing job workflow * Adding the private script runner * Adding is_analysis column to the command * Adding retrieval of commands excluding analysis commands * Addressing bug on retrieving information from redis * Enabling the command register endpoint to provide if the command is analysis only * Addressing @antgonza's comments * Addressing @wasade's comments * Supporting multiple choice * Adding documentation * Modifying handler to pass allow_change_optionals * returning optional parameters * Addressing bug found by @antgonza * Enabling changing the default parameters * Adding correct class * Allowing user to change default parameters * Fixing bug with commands listing * Addressing @wasade's comments
diff --git a/qiita_db/handlers/plugin.py b/qiita_db/handlers/plugin.py
@@ -100,6 +100,11 @@ def post(self, name, version):
             cmd_desc = self.get_argument('description')
             req_params = loads(self.get_argument('required_parameters'))
             opt_params = loads(self.get_argument('optional_parameters'))
+
+            for p_name, (p_type, dflt) in opt_params.items():
+                if p_type.startswith('mchoice'):
+                    opt_params[p_name] = [p_type, loads(dflt)]
+
             outputs = self.get_argument('outputs', None)
             if outputs:
                 outputs = loads(outputs)
diff --git a/qiita_db/handlers/tests/test_plugin.py b/qiita_db/handlers/tests/test_plugin.py
@@ -74,9 +74,12 @@ def test_post(self):
             'description': 'Command added for testing',
             'required_parameters': dumps(
                 {'in_data': ['artifact:["FASTA"]', None]}),
-            'optional_parameters': dumps({'param1': ['string', ''],
-                                          'param2': ['float', '1.5'],
-                                          'param3': ['boolean', 'True']}),
+            'optional_parameters': dumps(
+                {'param1': ['string', ''],
+                 'param2': ['float', '1.5'],
+                 'param3': ['boolean', 'True'],
+                 'param4': ['mchoice:["opt1", "opt2", "opt3"]',
+                            dumps(['opt1', 'opt2'])]}),
             'outputs': dumps({'out1': 'BIOM'}),
             'default_parameter_sets': dumps(
                 {'dflt1': {'param1': 'test',
diff --git a/qiita_db/software.py b/qiita_db/software.py
@@ -55,6 +55,8 @@ def get_commands_by_input_type(cls, artifact_types, active_only=True,
         active_only : bool, optional
             If True, return only active commands, otherwise return all commands
             Default: True
+        exclude_analysis : bool, optional
+            If True, return commands that are not part of the analysis pipeline
 
         Returns
         -------
@@ -272,16 +274,25 @@ def create(cls, software, name, description, parameters, outputs=None,
             supported_types = ['string', 'integer', 'float', 'reference',
                                'boolean', 'prep_template']
             if ptype not in supported_types and not ptype.startswith(
-                    ('choice', 'artifact')):
-                supported_types.extend(['choice', 'artifact'])
+                    ('choice', 'mchoice', 'artifact')):
+                supported_types.extend(['choice', 'mchoice', 'artifact'])
                 raise qdb.exceptions.QiitaDBError(
                     "Unsupported parameters type '%s' for parameter %s. "
                     "Supported types are: %s"
                     % (ptype, pname, ', '.join(supported_types)))
 
-            if ptype.startswith('choice') and dflt is not None:
-                choices = loads(ptype.split(':')[1])
-                if dflt not in choices:
+            if ptype.startswith(('choice', 'mchoice')) and dflt is not None:
+                choices = set(loads(ptype.split(':')[1]))
+                dflt_val = dflt
+                if ptype.startswith('choice'):
+                    # In the choice case, the dflt value is a single string,
+                    # create a list with it the string on it to use the
+                    # issuperset call below
+                    dflt_val = [dflt_val]
+                else:
+                    # jsonize the list to store it in the DB
+                    dflt = dumps(dflt)
+                if not choices.issuperset(dflt_val):
                     raise qdb.exceptions.QiitaDBError(
                         "The default value '%s' for the parameter %s is not "
                         "listed in the available choices: %s"
@@ -452,7 +463,17 @@ def optional_parameters(self):
                      WHERE command_id = %s AND required = false"""
             qdb.sql_connection.TRN.add(sql, [self.id])
             res = qdb.sql_connection.TRN.execute_fetchindex()
-            return {pname: [ptype, dflt] for pname, ptype, dflt in res}
+
+            # Define a function to load the json storing the default parameters
+            # if ptype is multiple choice. When I added it to the for loop as
+            # a one liner if, made the code a bit hard to read
+            def dflt_fmt(dflt, ptype):
+                if ptype.startswith('mchoice'):
+                    return loads(dflt)
+                return dflt
+
+            return {pname: [ptype, dflt_fmt(dflt, ptype)]
+                    for pname, ptype, dflt in res}
 
     @property
     def default_parameter_sets(self):
diff --git a/qiita_db/test/test_software.py b/qiita_db/test/test_software.py
@@ -28,6 +28,8 @@ def setUp(self):
             'req_param': ['string', None],
             'opt_int_param': ['integer', '4'],
             'opt_choice_param': ['choice:["opt1", "opt2"]', 'opt1'],
+            'opt_mchoice_param': ['mchoice:["opt1", "opt2", "opt3"]',
+                                  ['opt1', 'opt2']],
             'opt_bool': ['boolean', 'False']}
         self.outputs = {'out1': 'BIOM'}
 
@@ -291,6 +293,8 @@ def test_create(self):
         exp_optional = {
             'opt_int_param': ['integer', '4'],
             'opt_choice_param': ['choice:["opt1", "opt2"]', 'opt1'],
+            'opt_mchoice_param': ['mchoice:["opt1", "opt2", "opt3"]',
+                                  ['opt1', 'opt2']],
             'opt_bool': ['boolean', 'False']}
         self.assertEqual(obs.optional_parameters, exp_optional)
         self.assertFalse(obs.analysis_only)
@@ -300,13 +304,7 @@ def test_create(self):
             self.parameters, analysis_only=True)
         self.assertEqual(obs.name, "Test Command 2")
         self.assertEqual(obs.description, "This is a command for testing")
-        exp_required = {'req_param': ('string', [None]),
-                        'req_art': ('artifact', ['BIOM'])}
         self.assertEqual(obs.required_parameters, exp_required)
-        exp_optional = {
-            'opt_int_param': ['integer', '4'],
-            'opt_choice_param': ['choice:["opt1", "opt2"]', 'opt1'],
-            'opt_bool': ['boolean', 'False']}
         self.assertEqual(obs.optional_parameters, exp_optional)
         self.assertTrue(obs.analysis_only)
 
diff --git a/qiita_pet/handlers/api_proxy/processing.py b/qiita_pet/handlers/api_proxy/processing.py
@@ -41,13 +41,15 @@ def process_artifact_handler_get_req(artifact_id):
             'study_id': artifact.study.id}
 
 
-def list_commands_handler_get_req(artifact_types):
+def list_commands_handler_get_req(artifact_types, exclude_analysis):
     """Retrieves the commands that can process the given artifact types
 
     Parameters
     ----------
     artifact_types : str
         Comma-separated list of artifact types
+    exclude_analysis : bool
+        If True, return commands that are not part of the analysis pipeline
 
     Returns
     -------
@@ -62,7 +64,8 @@ def list_commands_handler_get_req(artifact_types):
     artifact_types = artifact_types.split(',')
     cmd_info = [
         {'id': cmd.id, 'command': cmd.name, 'output': cmd.outputs}
-        for cmd in Command.get_commands_by_input_type(artifact_types)]
+        for cmd in Command.get_commands_by_input_type(
+            artifact_types, exclude_analysis=exclude_analysis)]
 
     return {'status': 'success',
             'message': '',
@@ -92,21 +95,22 @@ def list_options_handler_get_req(command_id):
     return {'status': 'success',
             'message': '',
             'options': options,
-            'req_options': command.required_parameters}
+            'req_options': command.required_parameters,
+            'opt_options': command.optional_parameters}
 
 
-def workflow_handler_post_req(user_id, dflt_params_id, req_params):
+def workflow_handler_post_req(user_id, command_id, params):
     """Creates a new workflow in the system
 
     Parameters
     ----------
     user_id : str
         The user creating the workflow
-    dflt_params_id : int
-        The default parameters to use for the first command of the workflow
-    req_params : str
-        JSON representations of the required parameters for the first
-        command of the workflow
+    command_id : int
+        The first command to execute in the workflow
+    params : str
+        JSON representations of the parameters for the first command of
+        the workflow
 
     Returns
     -------
@@ -116,9 +120,7 @@ def workflow_handler_post_req(user_id, dflt_params_id, req_params):
          'message': str,
          'workflow_id': int}
     """
-    dflt_params = DefaultParameters(dflt_params_id)
-    req_params = loads(req_params)
-    parameters = Parameters.from_default_params(dflt_params, req_params)
+    parameters = Parameters.load(Command(command_id), json_str=params)
     wf = ProcessingWorkflow.from_scratch(User(user_id), parameters)
     # this is safe as we are creating the workflow for the first time and there
     # is only one node. Remember networkx doesn't assure order of nodes
@@ -136,14 +138,14 @@ def workflow_handler_post_req(user_id, dflt_params_id, req_params):
 
 def workflow_handler_patch_req(req_op, req_path, req_value=None,
                                req_from=None):
-    """Patches an ontology
+    """Patches a workflow
 
     Parameters
     ----------
     req_op : str
-        The operation to perform on the ontology
+        The operation to perform on the workflow
     req_path : str
-        The ontology to patch
+        Path parameter with the workflow to patch
     req_value : str, optional
         The value that needs to be modified
     req_from : str, optional
diff --git a/qiita_pet/handlers/api_proxy/tests/test_processing.py b/qiita_pet/handlers/api_proxy/tests/test_processing.py
@@ -9,7 +9,6 @@
 from json import dumps
 
 from qiita_core.util import qiita_test_checker
-from qiita_db.util import get_count
 from qiita_db.processing_job import ProcessingWorkflow
 from qiita_db.software import Command, Parameters
 from qiita_db.user import User
@@ -38,20 +37,36 @@ def test_process_artifact_handler_get_req(self):
         self.assertEqual(obs, exp)
 
     def test_list_commands_handler_get_req(self):
-        obs = list_commands_handler_get_req('FASTQ')
+        obs = list_commands_handler_get_req('FASTQ', True)
         exp = {'status': 'success',
                'message': '',
                'commands': [{'id': 1, 'command': 'Split libraries FASTQ',
                              'output': [['demultiplexed', 'Demultiplexed']]}]}
         self.assertEqual(obs, exp)
 
-        obs = list_commands_handler_get_req('Demultiplexed')
+        obs = list_commands_handler_get_req('Demultiplexed', True)
         exp = {'status': 'success',
                'message': '',
                'commands': [{'id': 3, 'command': 'Pick closed-reference OTUs',
                              'output': [['OTU table', 'BIOM']]}]}
         self.assertEqual(obs, exp)
 
+        obs = list_commands_handler_get_req('BIOM', False)
+        exp = {'status': 'success',
+               'message': '',
+               'commands': [
+                    {'command': 'Summarize Taxa', 'id': 11,
+                     'output': [['taxa_summary', 'taxa_summary']]},
+                    {'command': 'Beta Diversity', 'id': 12,
+                     'output': [['distance_matrix', 'distance_matrix']]},
+                    {'command': 'Alpha Rarefaction', 'id': 13,
+                     'output': [['rarefaction_curves', 'rarefaction_curves']]},
+                    {'command': 'Single Rarefaction', 'id': 14,
+                     'output': [['rarefied_table', 'BIOM']]}]}
+        # since the order of the commands can change, test them separately
+        self.assertItemsEqual(obs.pop('commands'), exp.pop('commands'))
+        self.assertEqual(obs, exp)
+
     def test_list_options_handler_get_req(self):
         obs = list_options_handler_get_req(3)
         exp = {'status': 'success',
@@ -64,11 +79,20 @@ def test_list_options_handler_get_req(self):
                                        'sortmerna_e_value': 1,
                                        'sortmerna_max_pos': 10000,
                                        'threads': 1}}],
-               'req_options': {'input_data': ('artifact', ['Demultiplexed'])}}
+               'req_options': {'input_data': ('artifact', ['Demultiplexed'])},
+               'opt_options': {'reference': ['reference', '1'],
+                               'similarity': ['float', '0.97'],
+                               'sortmerna_coverage': ['float', '0.97'],
+                               'sortmerna_e_value': ['float', '1'],
+                               'sortmerna_max_pos': ['integer', '10000'],
+                               'threads': ['integer', '1']}}
+        # First check that the keys are the same
+        self.assertItemsEqual(obs, exp)
         self.assertEqual(obs['status'], exp['status'])
         self.assertEqual(obs['message'], exp['message'])
         self.assertEqual(obs['options'], exp['options'])
         self.assertEqual(obs['req_options'], exp['req_options'])
+        self.assertEqual(obs['opt_options'], exp['opt_options'])
 
     def test_job_ajax_get_req(self):
         obs = job_ajax_get_req("063e553b-327c-4818-ab4a-adfe58e49860")
@@ -94,15 +118,21 @@ def test_job_ajax_get_req(self):
 @qiita_test_checker()
 class TestProcessingAPI(TestCase):
     def test_workflow_handler_post_req(self):
-        next_id = get_count('qiita.processing_job_workflow_root') + 1
-        obs = workflow_handler_post_req("test@foo.bar", 1, '{"input_data": 1}')
-        wf = ProcessingWorkflow(next_id)
+        params = ('{"max_barcode_errors": 1.5, "barcode_type": "golay_12", '
+                  '"max_bad_run_length": 3, "phred_offset": "auto", '
+                  '"rev_comp": false, "phred_quality_threshold": 3, '
+                  '"input_data": 1, "rev_comp_barcode": false, '
+                  '"rev_comp_mapping_barcodes": false, '
+                  '"min_per_read_length_fraction": 0.75, "sequence_max_n": 0}')
+        obs = workflow_handler_post_req("test@foo.bar", 1, params)
+        wf_id = obs['workflow_id']
+        wf = ProcessingWorkflow(wf_id)
         nodes = wf.graph.nodes()
         self.assertEqual(len(nodes), 1)
         job = nodes[0]
         exp = {'status': 'success',
                'message': '',
-               'workflow_id': next_id,
+               'workflow_id': wf_id,
                'job': {'id': job.id,
                        'inputs': [1],
                        'label': "Split libraries FASTQ",
diff --git a/qiita_pet/handlers/artifact_handlers/process_handlers.py b/qiita_pet/handlers/artifact_handlers/process_handlers.py
@@ -36,7 +36,8 @@ def process_artifact_handler_get_req(artifact_id):
             'message': '',
             'name': artifact.name,
             'type': artifact.artifact_type,
-            'artifact_id': artifact.id}
+            'artifact_id': artifact.id,
+            'allow_change_optionals': artifact.analysis is not None}
 
 
 class ProcessArtifactHandler(BaseHandler):
diff --git a/qiita_pet/handlers/artifact_handlers/tests/test_process_handlers.py b/qiita_pet/handlers/artifact_handlers/tests/test_process_handlers.py
@@ -9,6 +9,7 @@
 from unittest import TestCase, main
 
 from qiita_core.util import qiita_test_checker
+from qiita_pet.test.tornado_test_base import TestHandlerBase
 from qiita_pet.handlers.artifact_handlers.process_handlers import (
     process_artifact_handler_get_req)
 
@@ -17,9 +18,36 @@
 class TestProcessHandlersUtils(TestCase):
     def test_process_artifact_handler_get_req(self):
         obs = process_artifact_handler_get_req(1)
-        exp = {}
+        exp = {'status': 'success',
+               'message': '',
+               'name': 'Raw data 1',
+               'type': 'FASTQ',
+               'artifact_id': 1,
+               'allow_change_optionals': False}
         self.assertEqual(obs, exp)
 
+        obs = process_artifact_handler_get_req(8)
+        exp = {'status': 'success',
+               'message': '',
+               'name': 'noname',
+               'type': 'BIOM',
+               'artifact_id': 8,
+               'allow_change_optionals': True}
+        self.assertEqual(obs, exp)
+
+
+class TestProcessHandlers(TestHandlerBase):
+    def test_get_process_artifact_handler(self):
+        response = self.get("/artifact/1/process/")
+        self.assertEqual(response.code, 200)
+        self.assertNotEqual(response.body, "")
+        self.assertIn('load_artifact_type(params.nodes, false);',
+                      response.body)
+
+        response = self.get("/artifact/8/process/")
+        self.assertEqual(response.code, 200)
+        self.assertNotEqual(response.body, "")
+        self.assertIn('load_artifact_type(params.nodes, true);', response.body)
 
 if __name__ == '__main__':
     main()
diff --git a/qiita_pet/handlers/study_handlers/processing.py b/qiita_pet/handlers/study_handlers/processing.py
@@ -26,7 +26,9 @@ def get(self):
         # Fun fact - if the argument is a list, JS adds '[]' to the
         # argument name
         artifact_types = self.get_argument("artifact_types[]")
-        self.write(list_commands_handler_get_req(artifact_types))
+        exclude_analysis = self.get_argument('include_analysis') == 'false'
+        self.write(
+            list_commands_handler_get_req(artifact_types, exclude_analysis))
 
 
 class ListOptionsHandler(BaseHandler):
@@ -46,10 +48,10 @@ def post(self):
 class WorkflowHandler(BaseHandler):
     @authenticated
     def post(self):
-        dflt_params_id = self.get_argument('dflt_params_id')
-        req_params = self.get_argument('req_params')
+        command_id = self.get_argument('command_id')
+        params = self.get_argument('params')
         self.write(workflow_handler_post_req(
-            self.current_user.id, dflt_params_id, req_params))
+            self.current_user.id, command_id, params))
 
     @authenticated
     def patch(self):
diff --git a/qiita_pet/templates/artifact_ajax/processing_artifact.html b/qiita_pet/templates/artifact_ajax/processing_artifact.html
