From c27374cf433a62b7d55e3c655619ea5c36b57a6a Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 6 Oct 2020 22:30:40 +0200 Subject: [PATCH 1/6] add test for column_param test testing non tabular (bed) inputs --- test/functional/tools/column_param.xml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/functional/tools/column_param.xml b/test/functional/tools/column_param.xml index b5933606e48..8da1374745e 100644 --- a/test/functional/tools/column_param.xml +++ b/test/functional/tools/column_param.xml @@ -30,5 +30,21 @@ echo "col_names $col_names" >> '$output2' + + + + + + + + + + + + + + + From 9716bcf4a8bcaf65e2a413753370b259a64eacb8 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 6 Oct 2020 13:39:35 +0200 Subject: [PATCH 2/6] convert data sets only if necessary fixes: https://github.com/galaxyproject/galaxy/issues/9002 Co-authored-by: Marius van den Beek --- lib/galaxy/datatypes/registry.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/datatypes/registry.py b/lib/galaxy/datatypes/registry.py index 3a06b5ee2ff..ff4fbb35b60 100644 --- a/lib/galaxy/datatypes/registry.py +++ b/lib/galaxy/datatypes/registry.py @@ -883,7 +883,10 @@ class Registry: return None def find_conversion_destination_for_dataset_by_extensions(self, dataset_or_ext, accepted_formats, converter_safe=True): - """Returns ( target_ext, existing converted dataset )""" + """ + Returns (target_ext, existing converted dataset) + where target_ext becomes None if conversion is not necessary or possible + """ if hasattr(dataset_or_ext, "ext"): ext = dataset_or_ext.ext dataset = dataset_or_ext @@ -891,6 +894,9 @@ class Registry: ext = dataset_or_ext dataset = None + if self.get_datatype_by_extension(ext) is not None and self.get_datatype_by_extension(ext).matches_any(accepted_formats): + return None, None + for convert_ext in self.get_converters_by_datatype(ext): convert_ext_datatype = self.get_datatype_by_extension(convert_ext) if convert_ext_datatype is None: @@ -904,7 +910,7 @@ class Registry: else: ret_data = None return (convert_ext, ret_data) - return (None, None) + return None, None def get_composite_extensions(self): return [ext for (ext, d_type) in self.datatypes_by_extension.items() if d_type.composite_type is not None] From 6ff8859057d1bf0788fa8e9135eed53284e5b02e Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 8 Oct 2020 13:10:47 +0200 Subject: [PATCH 3/6] differentiate direct match from un-convertable in find_conversion_destination_for_dataset_by_extensions --- lib/galaxy/datatypes/data.py | 2 +- .../datatypes/display_applications/parameters.py | 12 +++++++++--- lib/galaxy/datatypes/registry.py | 11 ++++++----- lib/galaxy/tools/actions/__init__.py | 4 ++-- lib/galaxy/tools/evaluation.py | 4 ++-- lib/galaxy/tools/parameters/basic.py | 4 ++-- lib/galaxy/tools/parameters/dataset_matcher.py | 11 +++++------ lib/galaxy/tools/wrappers.py | 4 ++-- test/unit/tools/test_data_parameters.py | 10 +++++----- test/unit/tools/test_dataset_matcher.py | 4 ++-- 10 files changed, 36 insertions(+), 30 deletions(-) diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py index 320a457f591..b43f22c2b08 100644 --- a/lib/galaxy/datatypes/data.py +++ b/lib/galaxy/datatypes/data.py @@ -639,7 +639,7 @@ class Data(metaclass=DataMeta): return datatypes_registry.get_converters_by_datatype(original_dataset.ext) def find_conversion_destination(self, dataset, accepted_formats, datatypes_registry, **kwd): - """Returns ( target_ext, existing converted dataset )""" + """Returns ( direct_match, converted_ext, existing converted dataset )""" return datatypes_registry.find_conversion_destination_for_dataset_by_extensions(dataset, accepted_formats, **kwd) def convert_dataset(self, trans, original_dataset, target_type, return_output=False, visible=True, deps=None, target_context=None, history=None): diff --git a/lib/galaxy/datatypes/display_applications/parameters.py b/lib/galaxy/datatypes/display_applications/parameters.py index 374a6ca1500..2af2b866304 100644 --- a/lib/galaxy/datatypes/display_applications/parameters.py +++ b/lib/galaxy/datatypes/display_applications/parameters.py @@ -90,7 +90,9 @@ class DisplayApplicationDataParameter(DisplayApplicationParameter): rval = data.get_converted_files_by_type(ext) if rval: return rval - assert data.find_conversion_destination(self.formats)[0] is not None, "No conversion path found for data param: %s" % self.name + + direct_match, target_ext, converted_dataset = data.find_conversion_destination(self.formats) + assert direct_match or target_ext is not None, "No conversion path found for data param: %s" % self.name return None return data @@ -108,8 +110,12 @@ class DisplayApplicationDataParameter(DisplayApplicationParameter): # start conversion # FIXME: Much of this is copied (more than once...); should be some abstract method elsewhere called from here # find target ext - target_ext, converted_dataset = data.find_conversion_destination(self.formats, converter_safe=True) - if target_ext and not converted_dataset: + direct_match, target_ext, converted_dataset = data.find_conversion_destination(self.formats, converter_safe=True) + # TODO: Q: I guess we could skip the first if branch but code might + # be more readable + if direct_match: + pass + elif target_ext and not converted_dataset: if isinstance(data, DisplayDataValueWrapper): data = data.value new_data = next(iter(data.datatype.convert_dataset(trans, data, target_ext, return_output=True, visible=False).values())) diff --git a/lib/galaxy/datatypes/registry.py b/lib/galaxy/datatypes/registry.py index ff4fbb35b60..b205e7b910e 100644 --- a/lib/galaxy/datatypes/registry.py +++ b/lib/galaxy/datatypes/registry.py @@ -884,8 +884,9 @@ class Registry: def find_conversion_destination_for_dataset_by_extensions(self, dataset_or_ext, accepted_formats, converter_safe=True): """ - Returns (target_ext, existing converted dataset) - where target_ext becomes None if conversion is not necessary or possible + returns (direct_match, converted_ext, converted_dataset) + - direct match is True iff no the data set already has an accepted format + - target_ext becomes None if conversion is not possible (or necesary) """ if hasattr(dataset_or_ext, "ext"): ext = dataset_or_ext.ext @@ -895,7 +896,7 @@ class Registry: dataset = None if self.get_datatype_by_extension(ext) is not None and self.get_datatype_by_extension(ext).matches_any(accepted_formats): - return None, None + return True, None, None for convert_ext in self.get_converters_by_datatype(ext): convert_ext_datatype = self.get_datatype_by_extension(convert_ext) @@ -909,8 +910,8 @@ class Registry: continue else: ret_data = None - return (convert_ext, ret_data) - return None, None + return False, convert_ext, ret_data + return False, None, None def get_composite_extensions(self): return [ext for (ext, d_type) in self.datatypes_by_extension.items() if d_type.composite_type is not None] diff --git a/lib/galaxy/tools/actions/__init__.py b/lib/galaxy/tools/actions/__init__.py index 23428f1d3ea..0b75003e192 100644 --- a/lib/galaxy/tools/actions/__init__.py +++ b/lib/galaxy/tools/actions/__init__.py @@ -88,8 +88,8 @@ class DefaultToolAction: if not data.datatype.matches_any(formats): # Need to refresh in case this conversion just took place, i.e. input above in tool performed the same conversion trans.sa_session.refresh(data) - target_ext, converted_dataset = data.find_conversion_destination(formats) - if target_ext: + direct_match, target_ext, converted_dataset = data.find_conversion_destination(formats) + if not direct_match and target_ext: if converted_dataset: data = converted_dataset else: diff --git a/lib/galaxy/tools/evaluation.py b/lib/galaxy/tools/evaluation.py index 068ef56b9c7..67a54927fc6 100644 --- a/lib/galaxy/tools/evaluation.py +++ b/lib/galaxy/tools/evaluation.py @@ -195,10 +195,10 @@ class ToolEvaluator: for conversion_name, conversion_extensions, conversion_datatypes in input.conversions: # If we are at building cmdline step, then converters # have already executed - conv_ext, converted_dataset = input_values[input.name].find_conversion_destination(conversion_datatypes) + direct_match, conv_ext, converted_dataset = input_values[input.name].find_conversion_destination(conversion_datatypes) # When dealing with optional inputs, we'll provide a # valid extension to be used for None converted dataset - if not conv_ext: + if not direct_match and not conv_ext: conv_ext = conversion_extensions[0] # input_values[ input.name ] is None when optional # dataset, 'conversion' of optional dataset should diff --git a/lib/galaxy/tools/parameters/basic.py b/lib/galaxy/tools/parameters/basic.py index 1dc2a1a23df..84d19f5859a 100644 --- a/lib/galaxy/tools/parameters/basic.py +++ b/lib/galaxy/tools/parameters/basic.py @@ -1262,8 +1262,8 @@ class ColumnListParameter(SelectToolParameter): if isinstance(dataset, trans.app.model.HistoryDatasetCollectionAssociation): dataset = dataset.to_hda_representative() if isinstance(dataset, trans.app.model.HistoryDatasetAssociation) and self.ref_input and self.ref_input.formats: - target_ext, converted_dataset = dataset.find_conversion_destination(self.ref_input.formats) - if target_ext: + direct_match, target_ext, converted_dataset = dataset.find_conversion_destination(self.ref_input.formats) + if not direct_match and target_ext: if not converted_dataset: raise ImplicitConversionRequired else: diff --git a/lib/galaxy/tools/parameters/dataset_matcher.py b/lib/galaxy/tools/parameters/dataset_matcher.py index 5addf8600a3..9d259550da1 100644 --- a/lib/galaxy/tools/parameters/dataset_matcher.py +++ b/lib/galaxy/tools/parameters/dataset_matcher.py @@ -121,8 +121,8 @@ class DatasetMatcher: else: if not check_implicit_conversions: return False - target_ext, converted_dataset = hda.find_conversion_destination(formats) - if target_ext: + direct_match, target_ext, converted_dataset = hda.find_conversion_destination(formats) + if not direct_match and target_ext: original_hda = hda if converted_dataset: hda = converted_dataset @@ -219,11 +219,10 @@ class SummaryDatasetCollectionMatcher: formats = self.dataset_matcher.param.formats uses_implicit_conversion = False for extension in extensions: - if self.dataset_matcher_factory.matches_any_format(extension, formats): - continue - datatypes_registry = self._trans.app.datatypes_registry - converted_ext, _ = datatypes_registry.find_conversion_destination_for_dataset_by_extensions(extension, formats) + direct_match, converted_ext, _ = datatypes_registry.find_conversion_destination_for_dataset_by_extensions(extension, formats) + if direct_match: + continue if not converted_ext: return False else: diff --git a/lib/galaxy/tools/wrappers.py b/lib/galaxy/tools/wrappers.py index bcb4e316a8f..b1cf46d4a59 100644 --- a/lib/galaxy/tools/wrappers.py +++ b/lib/galaxy/tools/wrappers.py @@ -264,8 +264,8 @@ class DatasetFilenameWrapper(ToolParameterValueWrapper): # so we will wrap it and keep the original around for file paths # Should we name this .value to maintain consistency with most other ToolParameterValueWrapper? if formats: - target_ext, converted_dataset = dataset.find_conversion_destination(formats) - if target_ext and converted_dataset: + direct_match, target_ext, converted_dataset = dataset.find_conversion_destination(formats) + if not direct_match and target_ext and converted_dataset: dataset = converted_dataset self.unsanitized = dataset self.dataset = wrap_with_safe_string(dataset, no_wrap_classes=ToolParameterValueWrapper) diff --git a/test/unit/tools/test_data_parameters.py b/test/unit/tools/test_data_parameters.py index a89425c03b6..3dafc434a78 100644 --- a/test/unit/tools/test_data_parameters.py +++ b/test/unit/tools/test_data_parameters.py @@ -66,7 +66,7 @@ class DataToolParameterTestCase(BaseParameterTestCase): def test_field_implicit_conversion_new(self): hda1 = MockHistoryDatasetAssociation(name="hda1", id=1) hda1.extension = 'data' - hda1.conversion_destination = ("tabular", None) + hda1.conversion_destination = (False, "tabular", None) self.stub_active_datasets(hda1) field = self._simple_field() assert len(field['options']['hda']) == 1 @@ -76,7 +76,7 @@ class DataToolParameterTestCase(BaseParameterTestCase): def test_field_implicit_conversion_existing(self): hda1 = MockHistoryDatasetAssociation(name="hda1", id=1) hda1.extension = 'data' - hda1.conversion_destination = ("tabular", MockHistoryDatasetAssociation(name="hda1converted", id=2)) + hda1.conversion_destination = (False, "tabular", MockHistoryDatasetAssociation(name="hda1converted", id=2)) self.stub_active_datasets(hda1) field = self._simple_field() assert len(field['options']['hda']) == 1 @@ -125,14 +125,14 @@ class DataToolParameterTestCase(BaseParameterTestCase): hda1 = MockHistoryDatasetAssociation(name="hda1", id=1) hda1.extension = 'data' converted = MockHistoryDatasetAssociation(name="hda1converted", id=2) - hda1.conversion_destination = ("tabular", converted) + hda1.conversion_destination = (False, "tabular", converted) self.stub_active_datasets(hda1) assert converted == self.param.get_initial_value(self.trans, {}) def test_get_initial_with_to_be_converted_data(self): hda1 = MockHistoryDatasetAssociation(name="hda1", id=1) hda1.extension = 'data' - hda1.conversion_destination = ("tabular", None) + hda1.conversion_destination = (False, "tabular", None) self.stub_active_datasets(hda1) assert hda1 == self.param.get_initial_value(self.trans, {}), hda1 @@ -190,7 +190,7 @@ class MockHistoryDatasetAssociation(object): self.deleted = False self.dataset = test_dataset self.visible = True - self.conversion_destination = (None, None) + self.conversion_destination = (False, None, None) self.extension = "txt" self.dbkey = "hg19" self.implicitly_converted_parent_datasets = False diff --git a/test/unit/tools/test_dataset_matcher.py b/test/unit/tools/test_dataset_matcher.py index 4c594956824..db5e0bf289f 100644 --- a/test/unit/tools/test_dataset_matcher.py +++ b/test/unit/tools/test_dataset_matcher.py @@ -42,7 +42,7 @@ class DatasetMatcherTestCase(TestCase, UsesApp): # dataset. self.mock_hda.extension = 'data' converted_hda = model.HistoryDatasetAssociation() - self.mock_hda.conversion_destination = ("tabular", converted_hda) + self.mock_hda.conversion_destination = (False, "tabular", converted_hda) hda_match = self.test_context.hda_match(self.mock_hda) assert hda_match @@ -54,7 +54,7 @@ class DatasetMatcherTestCase(TestCase, UsesApp): # Find conversion returns a target extension to convert to, but not # a previously implicitly converted dataset. self.mock_hda.extension = 'data' - self.mock_hda.conversion_destination = ("tabular", None) + self.mock_hda.conversion_destination = (False, "tabular", None) hda_match = self.test_context.hda_match(self.mock_hda) assert hda_match From 0317d28fcb9a4235206164edb604335005a67904 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Fri, 9 Oct 2020 15:09:37 +0200 Subject: [PATCH 4/6] apply suggestions from code review Co-authored-by: Marius van den Beek --- .../display_applications/parameters.py | 29 +++++++++---------- lib/galaxy/tools/actions/__init__.py | 18 ++++++------ .../tools/parameters/dataset_matcher.py | 6 ++-- 3 files changed, 25 insertions(+), 28 deletions(-) diff --git a/lib/galaxy/datatypes/display_applications/parameters.py b/lib/galaxy/datatypes/display_applications/parameters.py index 2af2b866304..d6b831396ec 100644 --- a/lib/galaxy/datatypes/display_applications/parameters.py +++ b/lib/galaxy/datatypes/display_applications/parameters.py @@ -111,22 +111,19 @@ class DisplayApplicationDataParameter(DisplayApplicationParameter): # FIXME: Much of this is copied (more than once...); should be some abstract method elsewhere called from here # find target ext direct_match, target_ext, converted_dataset = data.find_conversion_destination(self.formats, converter_safe=True) - # TODO: Q: I guess we could skip the first if branch but code might - # be more readable - if direct_match: - pass - elif target_ext and not converted_dataset: - if isinstance(data, DisplayDataValueWrapper): - data = data.value - new_data = next(iter(data.datatype.convert_dataset(trans, data, target_ext, return_output=True, visible=False).values())) - new_data.hid = data.hid - new_data.name = data.name - trans.sa_session.add(new_data) - assoc = trans.app.model.ImplicitlyConvertedDatasetAssociation(parent=data, file_type=target_ext, dataset=new_data, metadata_safe=False) - trans.sa_session.add(assoc) - trans.sa_session.flush() - elif converted_dataset and converted_dataset.state == converted_dataset.states.ERROR: - raise Exception("Dataset conversion failed for data parameter: %s" % self.name) + if not direct_match: + if target_ext and not converted_dataset: + if isinstance(data, DisplayDataValueWrapper): + data = data.value + new_data = next(iter(data.datatype.convert_dataset(trans, data, target_ext, return_output=True, visible=False).values())) + new_data.hid = data.hid + new_data.name = data.name + trans.sa_session.add(new_data) + assoc = trans.app.model.ImplicitlyConvertedDatasetAssociation(parent=data, file_type=target_ext, dataset=new_data, metadata_safe=False) + trans.sa_session.add(assoc) + trans.sa_session.flush() + elif converted_dataset and converted_dataset.state == converted_dataset.states.ERROR: + raise Exception("Dataset conversion failed for data parameter: %s" % self.name) return self.get_value(other_values, dataset_hash, user_hash, trans) def is_preparing(self, other_values): diff --git a/lib/galaxy/tools/actions/__init__.py b/lib/galaxy/tools/actions/__init__.py index 0b75003e192..636ff926d2e 100644 --- a/lib/galaxy/tools/actions/__init__.py +++ b/lib/galaxy/tools/actions/__init__.py @@ -85,15 +85,15 @@ class DefaultToolAction: return None if formats is None: formats = input.formats - if not data.datatype.matches_any(formats): - # Need to refresh in case this conversion just took place, i.e. input above in tool performed the same conversion - trans.sa_session.refresh(data) - direct_match, target_ext, converted_dataset = data.find_conversion_destination(formats) - if not direct_match and target_ext: - if converted_dataset: - data = converted_dataset - else: - data = data.get_converted_dataset(trans, target_ext, target_context=parent, history=history) + + # Need to refresh in case this conversion just took place, i.e. input above in tool performed the same conversion + trans.sa_session.refresh(data) + direct_match, target_ext, converted_dataset = data.find_conversion_destination(formats) + if not direct_match and target_ext: + if converted_dataset: + data = converted_dataset + else: + data = data.get_converted_dataset(trans, target_ext, target_context=parent, history=history) input_name = prefix + input.name # Checked security of whole collection all at once if mapping over this input, else diff --git a/lib/galaxy/tools/parameters/dataset_matcher.py b/lib/galaxy/tools/parameters/dataset_matcher.py index 9d259550da1..9a704d25733 100644 --- a/lib/galaxy/tools/parameters/dataset_matcher.py +++ b/lib/galaxy/tools/parameters/dataset_matcher.py @@ -116,13 +116,13 @@ class DatasetMatcher: """ rval = False formats = self.param.formats - if self.dataset_matcher_factory.matches_any_format(hda.extension, formats): + direct_match, target_ext, converted_dataset = hda.find_conversion_destination(formats) + if direct_match: rval = HdaDirectMatch(hda) else: if not check_implicit_conversions: return False - direct_match, target_ext, converted_dataset = hda.find_conversion_destination(formats) - if not direct_match and target_ext: + if target_ext: original_hda = hda if converted_dataset: hda = converted_dataset From 908250b7325ff620d2038ee86f0876482e8ced3b Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Tue, 13 Oct 2020 12:48:40 +0200 Subject: [PATCH 5/6] one more mock hda conversion dest change --- test/unit/tools/test_dataset_matcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tools/test_dataset_matcher.py b/test/unit/tools/test_dataset_matcher.py index db5e0bf289f..0c6ed0fa89a 100644 --- a/test/unit/tools/test_dataset_matcher.py +++ b/test/unit/tools/test_dataset_matcher.py @@ -64,7 +64,7 @@ class DatasetMatcherTestCase(TestCase, UsesApp): def test_hda_match_properly_skips_conversion(self): self.mock_hda.extension = 'data' - self.mock_hda.conversion_destination = ("tabular", bunch.Bunch()) + self.mock_hda.conversion_destination = (False, "tabular", bunch.Bunch()) hda_match = self.test_context.hda_match(self.mock_hda, check_implicit_conversions=False) assert not hda_match From 3104a2ecfd02ac221aa396cd7ac7c4d15ef1f09d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 15 Oct 2020 11:30:44 +0200 Subject: [PATCH 6/6] Default MockHDA to direct match And change conversion_destination to `(False, None, None)` if there shouldn't be a match. --- test/unit/tools/test_data_parameters.py | 3 ++- test/unit/tools/test_dataset_matcher.py | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/tools/test_data_parameters.py b/test/unit/tools/test_data_parameters.py index 3dafc434a78..cd993ed5a0c 100644 --- a/test/unit/tools/test_data_parameters.py +++ b/test/unit/tools/test_data_parameters.py @@ -39,6 +39,7 @@ class DataToolParameterTestCase(BaseParameterTestCase): assert field['options']['hda'][1]['name'] == "hda1" hda2.extension = 'data' + hda2.conversion_destination = (False, None, None) field = self._simple_field() assert len(field['options']['hda']) == 1, field assert field['options']['hda'][0]['name'] == "hda1" @@ -190,7 +191,7 @@ class MockHistoryDatasetAssociation(object): self.deleted = False self.dataset = test_dataset self.visible = True - self.conversion_destination = (False, None, None) + self.conversion_destination = (True, None, None) self.extension = "txt" self.dbkey = "hg19" self.implicitly_converted_parent_datasets = False diff --git a/test/unit/tools/test_dataset_matcher.py b/test/unit/tools/test_dataset_matcher.py index 0c6ed0fa89a..71730245337 100644 --- a/test/unit/tools/test_dataset_matcher.py +++ b/test/unit/tools/test_dataset_matcher.py @@ -23,6 +23,7 @@ class DatasetMatcherTestCase(TestCase, UsesApp): # Datasets that don't match datatype are not valid. self.mock_hda.visible = True self.mock_hda.extension = 'data' + self.mock_hda.conversion_destination = (False, None, None) assert not self.test_context.hda_match(self.mock_hda) def test_valid_hda_direct_match(self):