Merge pull request #20152 from jmchilton/collection_cleanup_2

Fix certain cases of collection parameter handling during workflow execution.

lib/galaxy/model/dataset_collections/query.py

@@ -1,8 +1,30 @@
 import logging
+from typing import (
+    List,
+    Optional,
+)
+
+from typing_extensions import Protocol
 
 log = logging.getLogger(__name__)
 
 
+# protocols for things that look like Galaxy model HDCAs and collections for this module
+class CollectionLike(Protocol):
+    collection_type: str
+
+
+class HdcaLike(Protocol):
+    collection: CollectionLike
+
+
+class DataCollectionParameterLike(Protocol):
+
+    @property
+    def collection_types(self) -> Optional[List[str]]:
+        """Return a list of collection type strings the parameter accepts."""
+
+
 class HistoryQuery:
     """An object for describing the collections to pull out of a history,
     used by DataCollectionToolParameter.
@@ -17,7 +39,7 @@ class HistoryQuery:
         return HistoryQuery(**kwargs)
 
     @staticmethod
-    def from_collection_types(collection_types, collection_type_descriptions):
+    def from_collection_types(collection_types: Optional[List[str]], collection_type_descriptions):
         if collection_types:
             collection_type_descriptions = [
                 collection_type_descriptions.for_collection_type(t) for t in collection_types
@@ -33,12 +55,12 @@ class HistoryQuery:
         return HistoryQuery(**kwargs)
 
     @staticmethod
-    def from_parameter(param, collection_type_descriptions):
+    def from_parameter(param: DataCollectionParameterLike, collection_type_descriptions):
         """Take in a tool parameter element."""
         collection_types = param.collection_types
         return HistoryQuery.from_collection_types(collection_types, collection_type_descriptions)
 
-    def direct_match(self, hdca):
+    def direct_match(self, hdca: HdcaLike) -> bool:
         collection_type_descriptions = self.collection_type_descriptions
         if collection_type_descriptions is not None:
             for collection_type_description in collection_type_descriptions:
@@ -48,7 +70,7 @@ class HistoryQuery:
 
         return True
 
-    def can_map_over(self, hdca):
+    def can_map_over(self, hdca: HdcaLike):
         collection_type_descriptions = self.collection_type_descriptions
         if collection_type_descriptions is None:
             return False

lib/galaxy/tools/parameters/basic.py

@@ -41,7 +41,10 @@ from galaxy.model import (
     HistoryDatasetCollectionAssociation,
     LibraryDatasetDatasetAssociation,
 )
-from galaxy.model.dataset_collections import builder
+from galaxy.model.dataset_collections import (
+    builder,
+    query,
+)
 from galaxy.schema.fetch_data import FilesPayload
 from galaxy.tool_util.parameters.factory import get_color_value
 from galaxy.tool_util.parser import get_input_source as ensure_input_source
@@ -70,7 +73,6 @@ from galaxy.util.hash_util import HASH_NAMES
 from galaxy.util.rules_dsl import RuleSet
 from . import (
     dynamic_options,
-    history_query,
     validation,
 )
 from .dataset_matcher import get_dataset_matcher_factory
@@ -2433,7 +2435,7 @@ class DataToolParameter(BaseDataToolParameter):
         assert self.multiple
         dataset_collection_type_descriptions = trans.app.dataset_collection_manager.collection_type_descriptions
         # If multiple data parameter, treat like a list parameter.
-        return history_query.HistoryQuery.from_collection_type("list", dataset_collection_type_descriptions)
+        return query.HistoryQuery.from_collection_type("list", dataset_collection_type_descriptions)
 
 
 class DataCollectionToolParameter(BaseDataToolParameter):
@@ -2459,12 +2461,12 @@ class DataCollectionToolParameter(BaseDataToolParameter):
             )
 
     @property
-    def collection_types(self):
+    def collection_types(self) -> Optional[List[str]]:
         return self._collection_types
 
     def _history_query(self, trans):
         dataset_collection_type_descriptions = trans.app.dataset_collection_manager.collection_type_descriptions
-        return history_query.HistoryQuery.from_parameter(self, dataset_collection_type_descriptions)
+        return query.HistoryQuery.from_parameter(self, dataset_collection_type_descriptions)
 
     def match_collections(self, trans, history, dataset_collection_matcher):
         dataset_collections = trans.app.dataset_collection_manager.history_dataset_collections(

lib/galaxy/workflow/modules.py

@@ -42,6 +42,7 @@ from galaxy.model import (
 )
 from galaxy.model.base import ensure_object_added_to_session
 from galaxy.model.dataset_collections import matching
+from galaxy.model.dataset_collections.query import HistoryQuery
 from galaxy.schema.invocation import (
     CancelReason,
     FailureReason,
@@ -94,7 +95,6 @@ from galaxy.tools.parameters.grouping import (
     ConditionalWhen,
     Repeat,
 )
-from galaxy.tools.parameters.history_query import HistoryQuery
 from galaxy.tools.parameters.options import ParameterOption
 from galaxy.tools.parameters.populate_model import populate_model
 from galaxy.tools.parameters.workflow_utils import (
@@ -603,6 +603,9 @@ class WorkflowModule:
                     effective_input_collection_type,
                     dataset_collection_type_descriptions,
                 )
+                if not progress.subworkflow_structure and history_query.direct_match(data):
+                    continue
+
                 subcollection_type_description = history_query.can_map_over(data) or None
                 if subcollection_type_description:
                     subcollection_type_list = subcollection_type_description.collection_type.split(":")

lib/galaxy_test/api/test_workflows.py

@@ -4960,6 +4960,57 @@ test_data:
         hda_content = self.dataset_populator.get_history_dataset_content(history_id, content_id=hda_id)
         assert hda_content.strip() == "1"
 
+    @skip_without_tool("identifier_multiple")
+    def test_invocation_double_map_over(self, history_id):
+        summary = self._run_workflow(
+            """
+class: GalaxyWorkflow
+inputs:
+  input_collection:
+    collection_type: list:list
+    type: collection
+outputs:
+  main_out:
+    outputSource: subworkflow/sub_out
+steps:
+  subworkflow:
+    in:
+      data_input: input_collection
+    run:
+      class: GalaxyWorkflow
+      inputs:
+        data_input:
+          type: data
+      outputs:
+        sub_out:
+          outputSource: output_step/output1
+      steps:
+        intermediate_step:
+          tool_id: identifier_single
+          in:
+            input1: data_input
+        output_step:
+          tool_id: identifier_single
+          in:
+            input1: intermediate_step/output1
+test_data:
+  input_collection:
+    collection_type: list:list
+        """,
+            history_id=history_id,
+            assert_ok=True,
+            wait=True,
+        )
+        invocation = self.workflow_populator.get_invocation(summary.invocation_id)
+        # For consistency and conditional subworkflow steps this really needs to remain
+        # a collection and not get reduced.
+        assert "main_out" in invocation["output_collections"], invocation
+        hdca_details = self.dataset_populator.get_history_collection_details(history_id)
+        assert hdca_details["collection_type"] == "list:list"
+        elements = hdca_details["elements"]
+        assert len(elements) == 1
+        assert elements[0]["element_identifier"] == "test_level_1"
+
     @skip_without_tool("identifier_multiple")
     def test_invocation_map_over_inner_collection(self, history_id):
         summary = self._run_workflow(
@@ -5074,6 +5125,53 @@ test_data:
         assert elements[0]["element_identifier"] == "test_level_1"
         assert elements[0]["element_type"] == "hda"
 
+    @skip_without_tool("identifier_multiple")
+    def test_invocation_double_map_over_inner_collection_with_tool_collection_input(self, history_id):
+        summary = self._run_workflow(
+            """
+class: GalaxyWorkflow
+inputs:
+  input_collection:
+    collection_type: list:list:list
+    type: collection
+outputs:
+  main_out:
+    outputSource: subworkflow/sub_out
+steps:
+  subworkflow:
+    in:
+      list_input: input_collection
+    run:
+      class: GalaxyWorkflow
+      inputs:
+        list_input:
+          type: collection
+          collection_type: list
+      outputs:
+        sub_out:
+          outputSource: output_step/output1
+      steps:
+        output_step:
+          tool_id: identifier_all_collection_types
+          in:
+            input1: list_input
+test_data:
+  input_collection:
+    collection_type: list:list:list
+        """,
+            history_id=history_id,
+            assert_ok=True,
+            wait=True,
+        )
+        invocation = self.workflow_populator.get_invocation(summary.invocation_id)
+        assert "main_out" in invocation["output_collections"], invocation
+        input_hdca_details = self.dataset_populator.get_history_collection_details(
+            history_id, content_id=invocation["inputs"]["0"]["id"]
+        )
+        assert input_hdca_details["collection_type"] == "list:list:list"
+        assert len(input_hdca_details["elements"]) == 1
+        assert input_hdca_details["elements"][0]["element_identifier"] == "test_level_2"
+
     @skip_without_tool("cat")
     def test_pause_outputs_with_deleted_inputs(self):
         self._deleted_inputs_workflow(purge=False)

lib/galaxy_test/workflow/subcollection_rank_sorting.gxwf-tests.yml

@@ -0,0 +1,51 @@
+- doc: | 
+    Test to verify a tool that takes in a list or nested list is sent the most
+    specific list possible during workflow execution (the nested list). This should
+    not trigger subcollection mapping.
+  job:
+    input:
+      type: collection
+      collection_type: list:list
+  outputs:
+    out:
+      asserts:
+      - that: has_text
+        text: "Nested List"
+- doc: |
+    Once again with mapping, should reduce the result into a flat list.
+  job:
+    input:
+      type: collection
+      collection_type: list:list:list
+  outputs:
+    out:
+      class: Collection
+      collection_type: list
+      elements:
+        test_level_2:
+          asserts:
+            - that: has_line
+              line: "Nested List"
+            - that: has_line
+              line: "identifier is test_level_1"
+
+- doc: |
+    A test case with two levels of mapping. The inner two levels should be consumed and the outer
+    two levels should be mapped over.
+  job:
+    input:
+      type: collection
+      collection_type: list:list:list:list
+  outputs:
+    out:
+      class: Collection
+      collection_type: list:list
+      elements:
+        test_level_3:
+          elements:
+            test_level_2:
+              asserts:
+                - that: has_text
+                  text: "Nested List"
+                - that: has_line
+                  line: "identifier is test_level_1"

lib/galaxy_test/workflow/subcollection_rank_sorting.gxwf.yml

@@ -0,0 +1,13 @@
+class: GalaxyWorkflow
+inputs:
+  input:
+    type: collection
+    collection_type: list:list
+outputs:
+  out:
+    outputSource: list_handling/out1
+steps:
+  list_handling:
+    tool_id: collection_list_or_nested_list_input
+    in:
+      f1: input

lib/galaxy_test/workflow/subcollection_rank_sorting_paired.gxwf-tests.yml

@@ -0,0 +1,51 @@
+- doc: | 
+    Test to verify a tool that takes in a paired or list of paired is sent the most
+    specific list possible during workflow execution (the list of paired datasets). This should
+    not trigger subcollection mapping.
+  job:
+    input:
+      type: collection
+      collection_type: list:paired
+  outputs:
+    out:
+      asserts:
+      - that: has_text
+        text: "collection_type<paired>"
+
+- doc: |
+    Once again with mapping, should reduce the result into a flat list.
+  job:
+    input:
+      type: collection
+      collection_type: list:list:paired
+  outputs:
+    out:
+      class: Collection
+      collection_type: list
+      elements:
+        test_level_2:
+          asserts:
+            - that: has_text
+              text: "identifier is test_level_1"
+            - that: has_line
+              line: "collection_type<paired>"
+
+- doc: |
+    Once again with double mapping, should reduce the result into a nested list (list:list).
+  job:
+    input:
+      type: collection
+      collection_type: list:list:list:paired
+  outputs:
+    out:
+      class: Collection
+      collection_type: list:list
+      elements:
+        test_level_3:
+          elements:
+            test_level_2:
+              asserts:
+                - that: has_text
+                  text: "identifier is test_level_1"
+                - that: has_line
+                  line: "collection_type<paired>"

lib/galaxy_test/workflow/subcollection_rank_sorting_paired.gxwf.yml

@@ -0,0 +1,13 @@
+class: GalaxyWorkflow
+inputs:
+  input:
+    type: collection
+    collection_type: list:paired
+outputs:
+  out:
+    outputSource: list_handling/out1
+steps:
+  list_handling:
+    tool_id: collection_paired_or_list_paired_input
+    in:
+      f1: input

lib/galaxy_test/workflow/triply_nested_list_mapping.gxwf-tests.yml

@@ -0,0 +1,32 @@
+- doc: | 
+    Test that a basic list:list:list parameter is consumed directly by the tool
+    and not mapped over.
+  job:
+    input:
+      type: collection
+      collection_type: list:list:list
+  outputs:
+    out:
+      asserts:
+      - that: has_text
+        text: "identifier is test_level_2"
+      - that: has_text
+        text: "collection_type<list:list>"
+
+- doc: | 
+    Test that mapping over a list:list:list input parameter.
+  job:
+    input:
+      type: collection
+      collection_type: list:list:list:list
+  outputs:
+    out:
+      class: Collection
+      collection_type: list
+      elements:
+        test_level_3:
+          asserts:
+          - that: has_text
+            text: "identifier is test_level_2"
+          - that: has_text
+            text: "collection_type<list:list>"

lib/galaxy_test/workflow/triply_nested_list_mapping.gxwf.yml

@@ -0,0 +1,13 @@
+class: GalaxyWorkflow
+inputs:
+  input:
+    type: collection
+    collection_type: list:list:list
+outputs:
+  out:
+    outputSource: list_handling/out1
+steps:
+  list_handling:
+    tool_id: collection_list_2x_or_3x_nested
+    in:
+      f1: input

run_tests.sh

@@ -409,7 +409,7 @@ do
           framework_test=1
           shift 1
           ;;
-      -w|--framework-workflows)
+      -w|-framework-workflows|--framework-workflows)
           marker="workflow"
           report_file="run_framework_workflows_tests.html"
           framework_workflows_test=1

test/functional/tools/collection_list_2x_or_3x_nested.xml

@@ -0,0 +1,17 @@
+<tool id="collection_list_2x_or_3x_nested" name="collection_list_2x_or_3x_nested" version="0.1.0">
+  <command><![CDATA[
+    #for $key in $f1.keys()#
+      echo "identifier is $key" >> "$out1";
+      #set $element = $f1[$key]
+      echo 'collection_type<${element.collection.collection_type}>' >> "$out1";
+    #end for#
+  ]]></command>
+  <inputs>
+    <param name="f1" type="data_collection" collection_type="list:list,list:list:list" label="Input nested list" />
+  </inputs>
+  <outputs>
+    <data format="txt" name="out1" />
+  </outputs>
+  <tests>
+  </tests>
+</tool>

test/functional/tools/collection_list_or_nested_list_input.xml

@@ -0,0 +1,22 @@
+<tool id="collection_list_or_nested_list_input" name="collection_list_or_nested_list_input" version="0.1.0">
+  <command><![CDATA[
+    #for $key in $f1.keys()#
+      echo "identifier is $key" >> "$out1";
+      #set $element = $f1[$key]
+
+    #if $element.is_collection:
+        echo "Nested List" >> "$out1";
+    #else
+        echo "Simple Dataset" >> "$out1";
+    #end if
+    #end for#
+  ]]></command>
+  <inputs>
+    <param name="f1" type="data_collection" collection_type="list,list:list" label="Input list" />
+  </inputs>
+  <outputs>
+    <data format="txt" name="out1" />
+  </outputs>
+  <tests>
+  </tests>
+</tool>

test/functional/tools/collection_paired_or_list_paired_input.xml

@@ -0,0 +1,21 @@
+<tool id="collection_paired_or_list_paired_input" name="collection_paired_or_list_paired_input" version="0.1.0">
+  <command><![CDATA[
+    #for $key in $f1.keys()#
+      echo "identifier is $key" >> "$out1";
+      #set $element = $f1[$key]
+    #if $element.is_collection:
+      echo 'collection_type<${element.collection.collection_type}>' >> "$out1";
+    #else:
+      echo 'Simple dataset' >> "$out1";
+    #end if#
+    #end for#
+  ]]></command>
+  <inputs>
+    <param name="f1" type="data_collection" collection_type="paired,list:paired" label="Input collection" />
+  </inputs>
+  <outputs>
+    <data format="txt" name="out1" />
+  </outputs>
+  <tests>
+  </tests>
+</tool>

test/functional/tools/sample_tool_conf.xml

@@ -210,6 +210,9 @@
   <tool file="collection_type_source_map_over.xml" />
   <tool file="collection_creates_list_fail.xml" />
   <tool file="collection_creates_dynamic_nested_fail.xml" />
+  <tool file="collection_list_or_nested_list_input.xml" />
+  <tool file="collection_list_2x_or_3x_nested.xml" />
+  <tool file="collection_paired_or_list_paired_input.xml" />
   <tool file="collection_cat_group_tag.xml" />
   <tool file="collection_cat_group_tag_multiple.xml" />
   <tool file="discover_sort_by.xml" />

test/unit/data/dataset_collections/test_matching.py

@@ -1,5 +1,6 @@
 from galaxy.model.dataset_collections import (
     matching,
+    query,
     registry,
     type_description,
 )
@@ -17,14 +18,7 @@ def test_lists_of_same_cardinality_match():
 
 
 def test_nested_lists_match():
-    nested_list = list_instance(
-        elements=[
-            pair_element("data1"),
-            pair_element("data2"),
-            pair_element("data3"),
-        ],
-        collection_type="list:paired",
-    )
+    nested_list = nested_list = example_list_of_paired_datasets()
     assert_can_match(nested_list, nested_list)
 
 
@@ -42,19 +36,60 @@ def test_lists_of_different_cardinality_do_not_match():
 
 def test_valid_collection_subcollection_matching():
     flat_list = list_instance(ids=["data1", "data2", "data3"])
-    nested_list = list_instance(
-        elements=[
-            pair_element("data11"),
-            pair_element("data21"),
-            pair_element("data31"),
-        ],
-        collection_type="list:paired",
-    )
+    nested_list = example_list_of_paired_datasets()
     assert_cannot_match(flat_list, nested_list)
     assert_cannot_match(nested_list, flat_list)
     assert_can_match((nested_list, "paired"), flat_list)
 
 
+def test_query_can_match_list_to_list():
+    flat_list = list_instance(ids=["data1", "data2", "data3"])
+    q = query.HistoryQuery.from_collection_types(["list"], TYPE_DESCRIPTION_FACTORY)
+    assert q.can_map_over(flat_list) is False
+    assert q.direct_match(flat_list) is True
+
+
+def test_query_can_match_list_of_paireds_to_paired():
+    list_of_paired_datasets = example_list_of_paired_datasets()
+    q = query.HistoryQuery.from_collection_types(["paired"], TYPE_DESCRIPTION_FACTORY)
+    assert q.can_map_over(list_of_paired_datasets).collection_type == "paired"
+
+
+def test_query_can_match_list_of_lists_to_paired():
+    list_of_lists = example_list_of_lists()
+    q = query.HistoryQuery.from_collection_types(["paired"], TYPE_DESCRIPTION_FACTORY)
+    assert not q.can_map_over(list_of_lists)
+    assert not q.direct_match(list_of_lists)
+
+
+def test_query_can_match_list_of_lists_to_list():
+    list_of_lists = example_list_of_lists()
+    q = query.HistoryQuery.from_collection_types(["list"], TYPE_DESCRIPTION_FACTORY)
+    assert q.can_map_over(list_of_lists).collection_type == "list"
+    assert not q.direct_match(list_of_lists)
+
+
+def test_query_can_match_list_of_paireds_to_list_or_paired():
+    list_of_paired_datasets = example_list_of_paired_datasets()
+    q = query.HistoryQuery.from_collection_types(["list", "paired"], TYPE_DESCRIPTION_FACTORY)
+    assert q.can_map_over(list_of_paired_datasets).collection_type == "paired"
+    assert q.direct_match(list_of_paired_datasets) is False
+
+
+def test_query_can_match_list_of_lists_to_list_or_paired():
+    list_of_lists = example_list_of_lists()
+    q = query.HistoryQuery.from_collection_types(["list", "paired"], TYPE_DESCRIPTION_FACTORY)
+    assert q.can_map_over(list_of_lists).collection_type == "list"
+    assert q.direct_match(list_of_lists) is False
+
+
+def test_query_always_direct_match_if_no_collection_type_on_input_specified():
+    list_of_lists = example_list_of_lists()
+    q = query.HistoryQuery.from_collection_types([], TYPE_DESCRIPTION_FACTORY)
+    assert q.can_map_over(list_of_lists) is False
+    assert q.direct_match(list_of_lists) is True
+
+
 def assert_can_match(*items):
     to_match = build_collections_to_match(*items)
     matching.MatchingCollections.for_collections(to_match, TYPE_DESCRIPTION_FACTORY)
@@ -82,6 +117,27 @@ def build_collections_to_match(*items):
     return to_match
 
 
+def example_list_of_paired_datasets():
+    return list_instance(
+        elements=[
+            pair_element("data1"),
+            pair_element("data2"),
+            pair_element("data3"),
+        ],
+        collection_type="list:paired",
+    )
+
+
+def example_list_of_lists():
+    return list_instance(
+        elements=[
+            list_instance(),
+            list_instance(),
+        ],
+        collection_type="list:list",
+    )
+
+
 def pair_element(element_identifier):
     return collection_element(element_identifier, pair_instance().collection)