From 2185c814cc4475631effae7316ce14d69f6f8d49 Mon Sep 17 00:00:00 2001 From: nuwang <2070605+nuwang@users.noreply.github.com> Date: Mon, 26 Jun 2023 00:07:03 +0530 Subject: [PATCH 1/2] Add another test for remote regex multiple matches --- docs/topics/tpv_by_example.rst | 2 +- tests/fixtures/mapping-merge-multiple-local.yml | 7 +++++++ .../fixtures/mapping-merge-multiple-remote.yml | 5 +++++ tests/test_mapper_merge_multiple.py | 17 +++++++++++++++++ 4 files changed, 30 insertions(+), 1 deletion(-) diff --git a/docs/topics/tpv_by_example.rst b/docs/topics/tpv_by_example.rst index dbef5f2..bf90546 100644 --- a/docs/topics/tpv_by_example.rst +++ b/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 diff --git a/tests/fixtures/mapping-merge-multiple-local.yml b/tests/fixtures/mapping-merge-multiple-local.yml index cdaa4bd..5839870 100644 --- a/tests/fixtures/mapping-merge-multiple-local.yml +++ b/tests/fixtures/mapping-merge-multiple-local.yml @@ -46,6 +46,13 @@ tools: scheduling: require: - pulsar + toolshed.g2.bx.psu.edu/repos/iuc/disco/disco/.*: + mem: 40 + env: + DISCO_MORE_PARAMS: "just another param" + scheduling: + require: + - pulsar users: fairycake@vortex.org: diff --git a/tests/fixtures/mapping-merge-multiple-remote.yml b/tests/fixtures/mapping-merge-multiple-remote.yml index 0f245f2..2959bdc 100644 --- a/tests/fixtures/mapping-merge-multiple-remote.yml +++ b/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: diff --git a/tests/test_mapper_merge_multiple.py b/tests/test_mapper_merge_multiple.py index 2825b19..0b373e1 100644 --- a/tests/test_mapper_merge_multiple.py +++ b/tests/test_mapper_merge_multiple.py @@ -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_with_multiple_matches_regex(self): + tool = mock_galaxy.Tool('toolshed.g2.bx.psu.edu/repos/iuc/disco/disco/v1.0') + user = mock_galaxy.User('ford', 'prefect@vortex.org') + + 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'], ['40']) + 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 40 --cores 2') From 377de2450bf3d3cc9f96a9d876ded3f62b0423c8 Mon Sep 17 00:00:00 2001 From: nuwang <2070605+nuwang@users.noreply.github.com> Date: Mon, 26 Jun 2023 21:25:46 +0530 Subject: [PATCH 2/2] Apply default inheritance at runtime, so that remote overrides work --- tests/fixtures/mapping-merge-multiple-local.yml | 1 - tests/fixtures/mapping-user.yml | 2 ++ tests/test_mapper_merge_multiple.py | 10 +++++----- tpv/core/loader.py | 10 +++------- tpv/core/mapper.py | 8 ++++---- 5 files changed, 14 insertions(+), 17 deletions(-) diff --git a/tests/fixtures/mapping-merge-multiple-local.yml b/tests/fixtures/mapping-merge-multiple-local.yml index 5839870..5b7218b 100644 --- a/tests/fixtures/mapping-merge-multiple-local.yml +++ b/tests/fixtures/mapping-merge-multiple-local.yml @@ -47,7 +47,6 @@ tools: require: - pulsar toolshed.g2.bx.psu.edu/repos/iuc/disco/disco/.*: - mem: 40 env: DISCO_MORE_PARAMS: "just another param" scheduling: diff --git a/tests/fixtures/mapping-user.yml b/tests/fixtures/mapping-user.yml index 9a122c3..3a49bc4 100644 --- a/tests/fixtures/mapping-user.yml +++ b/tests/fixtures/mapping-user.yml @@ -77,6 +77,8 @@ users: scheduling: require: - special_resources + reject: + - pulsar destinations: local: diff --git a/tests/test_mapper_merge_multiple.py b/tests/test_mapper_merge_multiple.py index 0b373e1..4fa1c10 100644 --- a/tests/test_mapper_merge_multiple.py +++ b/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') @@ -105,7 +105,7 @@ def test_merge_rules_with_multiple_matches(self): 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_with_multiple_matches_regex(self): + 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', 'prefect@vortex.org') @@ -117,7 +117,7 @@ def test_merge_rules_with_multiple_matches_regex(self): 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'], ['40']) + 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 40 --cores 2') + self.assertEqual(destination.params['native_spec'], '--mem 24 --cores 8') diff --git a/tpv/core/loader.py b/tpv/core/loader.py index 79020cc..950d54e 100644 --- a/tpv/core/loader.py +++ b/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(): diff --git a/tpv/core/mapper.py b/tpv/core/mapper.py index 8b07467..3ca2d51 100644 --- a/tpv/core/mapper.py +++ b/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):