Merge pull request #105 from galaxyproject/check_remote_regex_multipl…

…e_matches

Fix issue with how default inheritance is applied for multiple files

docs/topics/tpv_by_example.rst

@@ -294,7 +294,7 @@ Metascheduling
 --------------
 
 Custom rank functions can be used to implement metascheduling capabilities. A rank function is used to select
-the best matching destination from a list of matching destination. If no rank function is provided, the default
+the best matching destination from a list of matching destinations. If no rank function is provided, the default
 rank function simply chooses the most preferred destination out of the available destinations.
 
 When more sophisticated control over scheduling is required, a rank function can be implemented through custom

tests/fixtures/mapping-merge-multiple-local.yml

@@ -46,6 +46,12 @@ tools:
         scheduling:
           require:
             - pulsar
+  toolshed.g2.bx.psu.edu/repos/iuc/disco/disco/.*:
+    env:
+      DISCO_MORE_PARAMS: "just another param"
+    scheduling:
+      require:
+        - pulsar
 
 users:
   [email protected]:

tests/fixtures/mapping-merge-multiple-remote.yml

@@ -42,6 +42,11 @@ tools:
         scheduling:
           require:
             - highmem
+  toolshed.g2.bx.psu.edu/repos/iuc/disco/.*:
+    cores: 8
+    mem: 24
+    env:
+      DISCO_MAX_MEMORY: '{mem}'
 
 destinations:
   another_k8s_environment:

tests/fixtures/mapping-user.yml

@@ -77,6 +77,8 @@ users:
     scheduling:
       require:
         - special_resources
+      reject:
+        - pulsar
 
 destinations:
   local:

tests/test_mapper_merge_multiple.py

@@ -38,7 +38,7 @@ def test_merge_remote_and_local(self):
         datasets = [mock_galaxy.DatasetAssociation("test", mock_galaxy.Dataset("test.txt", file_size=7*1024**3))]
         destination = self._map_to_destination(tool, user, datasets, tpv_config_paths=[config_first, config_second])
         self.assertEqual(destination.id, "k8s_environment")
-        self.assertEqual([env['value'] for env in destination.env if env['name'] == 'TEST_JOB_SLOTS'], ['2'])
+        self.assertEqual([env['value'] for env in destination.env if env['name'] == 'TEST_JOB_SLOTS'], ['4'])
         self.assertEqual(destination.params['native_spec'], '--mem 8 --cores 2')
         self.assertEqual(destination.params['custom_context_remote'], 'remote var')
         self.assertEqual(destination.params['custom_context_local'], 'local var')
@@ -65,7 +65,7 @@ def test_merge_local_with_local(self):
         datasets = [mock_galaxy.DatasetAssociation("test", mock_galaxy.Dataset("test.txt", file_size=7*1024**3))]
         destination = self._map_to_destination(tool, user, datasets, tpv_config_paths=[config_first, config_second])
         self.assertEqual(destination.id, "k8s_environment")
-        self.assertEqual([env['value'] for env in destination.env if env['name'] == 'TEST_JOB_SLOTS'], ['2'])
+        self.assertEqual([env['value'] for env in destination.env if env['name'] == 'TEST_JOB_SLOTS'], ['4'])
         self.assertEqual(destination.params['native_spec'], '--mem 8 --cores 2')
         self.assertEqual(destination.params['custom_context_remote'], 'remote var')
         self.assertEqual(destination.params['custom_context_local'], 'local var')
@@ -104,3 +104,20 @@ def test_merge_rules_with_multiple_matches(self):
         # this var is not overridden by the last defined defaults, and therefore, the remote value of cores*2 applies
         self.assertEqual([env['value'] for env in destination.env if env['name'] == 'MORE_JOB_SLOTS'], ['12'])
         self.assertEqual(destination.params['native_spec'], '--mem 18 --cores 6')
+
+    def test_merge_rules_local_defaults_do_not_override_remote_tool(self):
+        tool = mock_galaxy.Tool('toolshed.g2.bx.psu.edu/repos/iuc/disco/disco/v1.0')
+        user = mock_galaxy.User('ford', '[email protected]')
+
+        config_first = os.path.join(os.path.dirname(__file__), 'fixtures/mapping-merge-multiple-remote.yml')
+        config_second = os.path.join(os.path.dirname(__file__), 'fixtures/mapping-merge-multiple-local.yml')
+
+        # the disco rules should take effect, with local override winning
+        datasets = [mock_galaxy.DatasetAssociation("test", mock_galaxy.Dataset("test.txt", file_size=42*1024**3))]
+        destination = self._map_to_destination(tool, user, datasets, tpv_config_paths=[config_first, config_second])
+        self.assertEqual(destination.id, "k8s_environment")
+        # since the last defined hisat2 contains overridden defaults, those defaults will apply
+        self.assertEqual([env['value'] for env in destination.env if env['name'] == 'DISCO_MAX_MEMORY'], ['24'])
+        self.assertEqual([env['value'] for env in destination.env if env['name'] == 'DISCO_MORE_PARAMS'], ['just another param'])
+        # this var is not overridden by the last defined defaults, and therefore, the remote value applies
+        self.assertEqual(destination.params['native_spec'], '--mem 24 --cores 8')

tpv/core/loader.py

@@ -61,13 +61,9 @@ def process_inheritance(self, entity_list: dict[str, Entity], entity: Entity):
                 raise InvalidParentException(f"The specified parent: {entity.inherits} for"
                                              f" entity: {entity} does not exist")
             return entity.inherit(self.process_inheritance(entity_list, parent_entity))
-        else:
-            default_inherits = self.global_settings.get('default_inherits')
-            if default_inherits and not entity.id == default_inherits:
-                default_parent = entity_list.get(default_inherits)
-                return entity.inherit(default_parent)
-            else:
-                return entity
+        # do not process default inheritance here, only at runtime, as multiple can cause default inheritance
+        # to override later matches.
+        return entity
 
     def recompute_inheritance(self, entities: dict[str, Entity]):
         for key, entity in entities.items():

tpv/core/mapper.py

@@ -35,6 +35,10 @@ def __compile_tool_regex(self, key):
 
     def _find_entities_matching_id(self, entity_list, entity_name):
         matches = []
+        if self.default_inherits:
+            default_match = entity_list.get(self.default_inherits)
+            if default_match:
+                matches.append(default_match)
         for key in entity_list.keys():
             if self.lookup_tool_regex(key).match(entity_name):
                 match = entity_list[key]
@@ -43,10 +47,6 @@ def _find_entities_matching_id(self, entity_list, entity_name):
                     raise JobMappingException(f"This entity is abstract and cannot be mapped : {match}")
                 else:
                     matches.append(match)
-        if not matches and self.default_inherits:
-            default_match = entity_list.get(self.default_inherits)
-            if default_match:
-                matches.append(default_match)
         return matches
 
     def __inherit_matching_entities(self, entity_type, entity_name):