From 0dac0c207757cf160901df387214dcfc56016469 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 15 Feb 2016 21:29:50 +0000 Subject: [PATCH 1/2] Refactor workflow loading. Remove even more duplication between updating and creating workflows. A few new test cases to verify errors under certain conditions. --- lib/galaxy/managers/workflows.py | 134 +++++++++++++++---------------- lib/galaxy/workflow/modules.py | 2 + test/api/test_workflows.py | 47 ++++++++--- 3 files changed, 103 insertions(+), 80 deletions(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 8ac7bd2fcbf..e15de6c6103 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -191,50 +191,18 @@ class WorkflowContentsManager(UsesAnnotations): ): # Put parameters in workflow mode trans.workflow_building_mode = True - # Create new workflow from incoming dict - workflow = model.Workflow() # If there's a source, put it in the workflow name. if source: name = "%s (imported from %s)" % ( data['name'], source ) else: name = data['name'] - workflow.name = name + workflow, missing_tool_tups = self._workflow_from_dict( + trans, + data, + name=name, + ) if 'uuid' in data: workflow.uuid = data['uuid'] - # Assume no errors until we find a step that has some - workflow.has_errors = False - # Create each step - steps = [] - # The editor will provide ids for each step that we don't need to save, - # but do need to use to make connections - steps_by_external_id = {} - # Keep track of tools required by the workflow that are not available in - # the local Galaxy instance. Each tuple in the list of missing_tool_tups - # will be ( tool_id, tool_name, tool_version ). - missing_tool_tups = [] - for step_dict in self.__walk_step_dicts( data ): - module, step = self.__track_module_from_dict( trans, steps, steps_by_external_id, step_dict, secure=False ) - if module.type == 'tool' and module.tool is None: - # A required tool is not available in the local Galaxy instance. - if 'content_id' in step_dict: - tool_id = step_dict[ 'content_id' ] - else: - # Support legacy workflows... (created pre 16.01) - tool_id = step_dict[ 'tool_id' ] - missing_tool_tup = ( tool_id, step_dict[ 'name' ], step_dict[ 'tool_version' ]) - if missing_tool_tup not in missing_tool_tups: - missing_tool_tups.append( missing_tool_tup ) - # Save the entire step_dict in the unused config field, be parsed later - # when we do have the tool - step.config = json.dumps(step_dict) - if step.tool_errors: - workflow.has_errors = True - - # Second pass to deal with connections between steps - self.__connect_workflow_steps( steps, steps_by_external_id ) - - # Order the steps if possible - attach_ordered_steps( workflow, steps ) if create_stored_workflow: # Connect up @@ -276,41 +244,19 @@ class WorkflowContentsManager(UsesAnnotations): trans.workflow_building_mode = True # Convert incoming workflow data from json if coming from editor data = json.loads(workflow_data) if from_editor else workflow_data - # Create new workflow from incoming data - workflow = model.Workflow() - # Just keep the last name (user can rename later) - workflow.name = stored_workflow.name - # Assume no errors until we find a step that has some - workflow.has_errors = False - # Create each step - steps = [] - # The editor will provide ids for each step that we don't need to save, - # but do need to use to make connections - steps_by_external_id = {} - errors = [] - for key, step_dict in data['steps'].iteritems(): - is_tool = is_tool_module_type( step_dict[ 'type' ] ) - tool_id = step_dict.get('content_id', step_dict.get('tool_id', None)) - if is_tool and tool_id is None: - raise exceptions.RequestParameterInvalidException("No tool_id could be located for for step [%s]" % step_dict) - if is_tool and not trans.app.toolbox.has_tool( tool_id, exact=True ): - errors.append("Step %s requires tool '%s'." % (step_dict['id'], step_dict['tool_id'])) - if errors: + + workflow, missing_tool_tups = self._workflow_from_dict( + trans, + data, + name=stored_workflow.name, + ) + + if missing_tool_tups: + errors = [] + for missing_tool_tup in missing_tool_tups: + errors.append("Step %s requires tool '%s'." % (missing_tool_tup[3], missing_tool_tup[0])) raise MissingToolsException(workflow, errors) - # First pass to build step objects and populate basic values - for step_dict in self.__walk_step_dicts( data ): - module, step = self.__track_module_from_dict( trans, steps, steps_by_external_id, step_dict, secure=from_editor ) - - if step.tool_errors: - # DBTODO Check for conditional inputs here. - workflow.has_errors = True - - # Second pass to deal with connections between steps - self.__connect_workflow_steps( steps, steps_by_external_id ) - - # Order the steps if possible - attach_ordered_steps( workflow, steps ) # Connect up workflow.stored_workflow = stored_workflow stored_workflow.latest_workflow = workflow @@ -324,6 +270,52 @@ class WorkflowContentsManager(UsesAnnotations): errors.append( "This workflow contains cycles" ) return workflow, errors + def _workflow_from_dict(self, trans, data, name): + # Create new workflow from source data + workflow = model.Workflow() + + workflow.name = name + + # Assume no errors until we find a step that has some + workflow.has_errors = False + # Create each step + steps = [] + # The editor will provide ids for each step that we don't need to save, + # but do need to use to make connections + steps_by_external_id = {} + + # Keep track of tools required by the workflow that are not available in + # the local Galaxy instance. Each tuple in the list of missing_tool_tups + # will be ( tool_id, tool_name, tool_version ). + missing_tool_tups = [] + + for step_dict in self.__walk_step_dicts( data ): + module, step = self.__track_module_from_dict( trans, steps, steps_by_external_id, step_dict, secure=False ) + is_tool = is_tool_module_type( module.type ) + if is_tool and module.tool is None: + # A required tool is not available in the local Galaxy instance. + tool_id = step_dict.get('content_id', step_dict.get('tool_id', None)) + assert tool_id is not None # Threw an exception elsewhere if not + + missing_tool_tup = ( tool_id, step_dict[ 'name' ], step_dict[ 'tool_version' ], step_dict[ 'id'] ) + if missing_tool_tup not in missing_tool_tups: + missing_tool_tups.append( missing_tool_tup ) + + # Save the entire step_dict in the unused config field, be parsed later + # when we do have the tool + step.config = json.dumps(step_dict) + + if step.tool_errors: + workflow.has_errors = True + + # Second pass to deal with connections between steps + self.__connect_workflow_steps( steps, steps_by_external_id ) + + # Order the steps if possible + attach_ordered_steps( workflow, steps ) + + return workflow, missing_tool_tups + def workflow_to_dict( self, trans, stored, style="export" ): """ Export the workflow contents to a dictionary ready for JSON-ification and to be sent out via API for instance. There are three styles of export allowed 'export', 'instance', and @@ -871,7 +863,7 @@ class WorkflowContentsManager(UsesAnnotations): del step.temp_input_connections -class MissingToolsException(object): +class MissingToolsException(exceptions.MessageException): def __init__(self, workflow, errors): self.workflow = workflow diff --git a/lib/galaxy/workflow/modules.py b/lib/galaxy/workflow/modules.py index 2a276eb5333..34a8f99d83b 100644 --- a/lib/galaxy/workflow/modules.py +++ b/lib/galaxy/workflow/modules.py @@ -820,6 +820,8 @@ class ToolModule( WorkflowModule ): tool_id = d.get( 'content_id', None ) if tool_id is None: tool_id = d.get( 'tool_id', None ) # Older workflows will have exported this as tool_id. + if tool_id is None: + raise exceptions.RequestParameterInvalidException("No content id could be located for for step [%s]" % d) tool_version = str( d.get( 'tool_version', None ) ) module = Class( trans, tool_id, tool_version=tool_version ) module.state = galaxy.tools.DefaultToolState() diff --git a/test/api/test_workflows.py b/test/api/test_workflows.py index 30984fa67da..67726b6ed00 100644 --- a/test/api/test_workflows.py +++ b/test/api/test_workflows.py @@ -331,7 +331,7 @@ class WorkflowsApiTestCase( BaseWorkflowsApiTestCase ): def test_upload_deprecated( self ): self.__test_upload( use_deprecated_route=True ) - def __test_upload( self, use_deprecated_route=False, name="test_import", workflow=None ): + def __test_upload( self, use_deprecated_route=False, name="test_import", workflow=None, assert_ok=True ): if workflow is None: workflow = self.workflow_populator.load_workflow( name=name ) data = dict( @@ -342,8 +342,9 @@ class WorkflowsApiTestCase( BaseWorkflowsApiTestCase ): else: route = "workflows" upload_response = self._post( route, data=data ) - self._assert_status_code_is( upload_response, 200 ) - self._assert_user_has_workflow_with_name( "%s (imported from API)" % name ) + if assert_ok: + self._assert_status_code_is( upload_response, 200 ) + self._assert_user_has_workflow_with_name( "%s (imported from API)" % name ) return upload_response def test_update( self ): @@ -370,12 +371,7 @@ class WorkflowsApiTestCase( BaseWorkflowsApiTestCase ): workflow_id = upload_response.json()["id"] def update(workflow_object): - data = dict( - workflow=workflow_object - ) - raw_url = 'workflows/%s' % workflow_id - url = self._api_url( raw_url, use_key=True ) - put_response = put( url, data=dumps(data) ) + put_response = self._update_workflow(workflow_id, workflow_object) self._assert_status_code_is( put_response, 200 ) return put_response @@ -410,6 +406,23 @@ class WorkflowsApiTestCase( BaseWorkflowsApiTestCase ): # Make sure the positions have been updated. map(tweak_step, updated_workflow_content['steps'].iteritems()) + def test_update_no_tool_id( self ): + workflow_object = self.workflow_populator.load_workflow( name="test_import" ) + upload_response = self.__test_upload( workflow=workflow_object ) + workflow_id = upload_response.json()["id"] + del workflow_object["steps"]["2"]["tool_id"] + put_response = self._update_workflow(workflow_id, workflow_object) + self._assert_status_code_is( put_response, 400 ) + + def test_update_missing_tool( self ): + # Create allows missing tools, update doesn't currently... + workflow_object = self.workflow_populator.load_workflow( name="test_import" ) + upload_response = self.__test_upload( workflow=workflow_object ) + workflow_id = upload_response.json()["id"] + workflow_object["steps"]["2"]["tool_id"] = "cat-not-found" + put_response = self._update_workflow(workflow_id, workflow_object) + self._assert_status_code_is( put_response, 400 ) + def test_require_unique_step_uuids( self ): workflow_dup_uuids = self.workflow_populator.load_workflow( name="test_import" ) uuid0 = str(uuid4()) @@ -534,6 +547,13 @@ class WorkflowsApiTestCase( BaseWorkflowsApiTestCase ): missing_tool_steps = filter(lambda v: v['tool_id'] == 'cat_missing_tool', steps.values()) assert len(missing_tool_steps) == 1 + def test_import_no_tool_id( self ): + # Import works with missing tools, but not with absent content/tool id. + workflow = self.workflow_populator.load_workflow_from_resource( name="test_workflow_missing_tool" ) + del workflow["steps"]["2"]["tool_id"] + create_response = self.__test_upload(workflow=workflow, assert_ok=False) + self._assert_status_code_is( create_response, 400 ) + def test_import_export_with_runtime_inputs( self ): workflow = self.workflow_populator.load_workflow_from_resource( name="test_workflow_with_runtime_input" ) workflow_id = self.workflow_populator.create_workflow( workflow ) @@ -1427,6 +1447,15 @@ steps: self._assert_status_code_is( step_response, 200 ) self._assert_has_keys( step_response.json(), "id", "order_index" ) + def _update_workflow(self, workflow_id, workflow_object): + data = dict( + workflow=workflow_object + ) + raw_url = 'workflows/%s' % workflow_id + url = self._api_url( raw_url, use_key=True ) + put_response = put( url, data=dumps(data) ) + return put_response + def _invocation_step_details( self, workflow_id, invocation_id, step_id ): invocation_step_response = self._get( "workflows/%s/usage/%s/steps/%s" % ( workflow_id, invocation_id, step_id ) ) self._assert_status_code_is( invocation_step_response, 200 ) From 92d978dd9d9b15bab7b3852787c2ce094880f97d Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 15 Feb 2016 21:56:47 +0000 Subject: [PATCH 2/2] Eliminate from_editor param to simplify workflow manager... ... slightly. --- lib/galaxy/managers/workflows.py | 12 ++++++++---- lib/galaxy/webapps/galaxy/controllers/workflow.py | 1 - 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index e15de6c6103..cab16d79b75 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -1,5 +1,7 @@ from __future__ import absolute_import +from six import string_types + from collections import namedtuple import logging import json @@ -239,15 +241,13 @@ class WorkflowContentsManager(UsesAnnotations): missing_tools=missing_tool_tups ) - def update_workflow_from_dict(self, trans, stored_workflow, workflow_data, from_editor=False): + def update_workflow_from_dict(self, trans, stored_workflow, workflow_data): # Put parameters in workflow mode trans.workflow_building_mode = True - # Convert incoming workflow data from json if coming from editor - data = json.loads(workflow_data) if from_editor else workflow_data workflow, missing_tool_tups = self._workflow_from_dict( trans, - data, + workflow_data, name=stored_workflow.name, ) @@ -271,6 +271,10 @@ class WorkflowContentsManager(UsesAnnotations): return workflow, errors def _workflow_from_dict(self, trans, data, name): + if isinstance(data, string_types): + # If coming from the editor... + data = json.loads(data) + # Create new workflow from source data workflow = model.Workflow() diff --git a/lib/galaxy/webapps/galaxy/controllers/workflow.py b/lib/galaxy/webapps/galaxy/controllers/workflow.py index 380b558f2a8..a8c4a7238aa 100644 --- a/lib/galaxy/webapps/galaxy/controllers/workflow.py +++ b/lib/galaxy/webapps/galaxy/controllers/workflow.py @@ -707,7 +707,6 @@ class WorkflowController( BaseUIController, SharableMixin, UsesStoredWorkflowMix trans, stored, workflow_data, - from_editor=True, ) except workflows.MissingToolsException as e: return dict(