From 735621971dbd27c0e7fc52ef3d37fcf6a8de19ec Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 15 Oct 2020 15:23:16 -0400 Subject: [PATCH 1/6] Rework new collection order testing in 20.09. This was added in 20.09 with https://github.com/galaxyproject/galaxy/pull/9684/files. There were some things I didn't love that I think are corrected here. - Preserve legacy behavior of not enforcing sort order but make it now contigent on profile being 20.09 or newer. - Run all the element tests and then do order checking for a more specific error message: ``galaxy.tool_util.verify.interactor.JobOutputsError: Collection identifier '1' found out of order, expected order (['2', '1', '10']) of the tool generated collection elements ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10']`` - Add test case for older default based on new test case. - Fix the new attribute that is used to check the order so that it isn't required (this broke Planemo workflow testing) and so the name doesn't clash with an existing element attribute on collection elements in the API/model. --- lib/galaxy/tool_util/parser/xml.py | 29 +++++----- lib/galaxy/tool_util/verify/interactor.py | 54 +++++++++++-------- test/functional/tools/discover_sort_by.xml | 2 +- .../tools/discover_sort_by_legacy_test.xml | 42 +++++++++++++++ 4 files changed, 90 insertions(+), 37 deletions(-) create mode 100644 test/functional/tools/discover_sort_by_legacy_test.xml diff --git a/lib/galaxy/tool_util/parser/xml.py b/lib/galaxy/tool_util/parser/xml.py index 297ce801c15..88206c1ec09 100644 --- a/lib/galaxy/tool_util/parser/xml.py +++ b/lib/galaxy/tool_util/parser/xml.py @@ -513,7 +513,8 @@ class XmlToolSource(ToolSource): if tests_elem is not None: for i, test_elem in enumerate(tests_elem.findall("test")): - tests.append(_test_elem_to_dict(test_elem, i)) + profile = self.parse_profile() + tests.append(_test_elem_to_dict(test_elem, i, profile)) return rval @@ -533,10 +534,10 @@ class XmlToolSource(ToolSource): return python_template_version -def _test_elem_to_dict(test_elem, i): +def _test_elem_to_dict(test_elem, i, profile=None): rval = dict( outputs=__parse_output_elems(test_elem), - output_collections=__parse_output_collection_elems(test_elem), + output_collections=__parse_output_collection_elems(test_elem, profile=profile), inputs=__parse_input_elems(test_elem, i), expect_num_outputs=test_elem.get("expect_num_outputs"), command=__parse_assert_list_from_elem(test_elem.find("assert_command")), @@ -579,36 +580,38 @@ def __parse_command_elem(test_elem): return __parse_assert_list_from_elem(assert_elem) -def __parse_output_collection_elems(test_elem): +def __parse_output_collection_elems(test_elem, profile=None): output_collections = [] for output_collection_elem in test_elem.findall("output_collection"): - output_collection_def = __parse_output_collection_elem(output_collection_elem) + output_collection_def = __parse_output_collection_elem(output_collection_elem, profile=profile) output_collections.append(output_collection_def) return output_collections -def __parse_output_collection_elem(output_collection_elem): +def __parse_output_collection_elem(output_collection_elem, profile=None): attrib = dict(output_collection_elem.attrib) name = attrib.pop('name', None) if name is None: raise Exception("Test output collection does not have a 'name'") - element_tests = __parse_element_tests(output_collection_elem) + element_tests = __parse_element_tests(output_collection_elem, profile=profile) return TestCollectionOutputDef(name, attrib, element_tests).to_dict() -def __parse_element_tests(parent_element): - element_tests = {} +def __parse_element_tests(parent_element, profile=None): + element_tests = OrderedDict() for idx, element in enumerate(parent_element.findall("element")): element_attrib = dict(element.attrib) identifier = element_attrib.pop('name', None) if identifier is None: raise Exception("Test primary dataset does not have a 'identifier'") - element_tests[identifier] = __parse_test_attributes(element, element_attrib, parse_elements=True) - element_tests[identifier][1]["element_index"] = idx + element_tests[identifier] = __parse_test_attributes(element, element_attrib, parse_elements=True, profile=profile) + if profile and profile >= "20.09": + element_tests[identifier][1]["expected_sort_order"] = idx + return element_tests -def __parse_test_attributes(output_elem, attrib, parse_elements=False, parse_discovered_datasets=False): +def __parse_test_attributes(output_elem, attrib, parse_elements=False, parse_discovered_datasets=False, profile=None): assert_list = __parse_assert_list(output_elem) # Allow either file or value to specify a target file to compare result with @@ -638,7 +641,7 @@ def __parse_test_attributes(output_elem, attrib, parse_elements=False, parse_dis checksum = attrib.get("checksum", None) element_tests = {} if parse_elements: - element_tests = __parse_element_tests(output_elem) + element_tests = __parse_element_tests(output_elem, profile=profile) primary_datasets = {} if parse_discovered_datasets: diff --git a/lib/galaxy/tool_util/verify/interactor.py b/lib/galaxy/tool_util/verify/interactor.py index 62eed869d7c..6801b41d4ae 100644 --- a/lib/galaxy/tool_util/verify/interactor.py +++ b/lib/galaxy/tool_util/verify/interactor.py @@ -724,36 +724,29 @@ def verify_collection(output_collection_def, data_collection, verify_dataset): message = template % (name, expected_element_count, actual_element_count) raise AssertionError(message) + def get_element(elements, id): + for element in elements: + if element["element_identifier"] == id: + return element + return False + def verify_elements(element_objects, element_tests): - sorted_test_ids = [None] * len(element_tests) + # sorted_test_ids = [None] * len(element_tests) + expected_sort_order = [] + + eo_ids = [_["element_identifier"] for _ in element_objects] for element_identifier, element_test in element_tests.items(): if isinstance(element_test, dict): element_outfile, element_attrib = None, element_test else: element_outfile, element_attrib = element_test - sorted_test_ids[element_attrib["element_index"]] = element_identifier + if 'expected_sort_order' in element_attrib: + expected_sort_order.append(element_identifier) - i = 0 - for element_identifier in sorted_test_ids: - element_test = element_tests[element_identifier] - if isinstance(element_test, dict): - element_outfile, element_attrib = None, element_test - else: - element_outfile, element_attrib = element_test - - element = None - while i < len(element_objects): - if element_objects[i]["element_identifier"] == element_identifier: - element = element_objects[i] - i += 1 - break - i += 1 - - if element is None: - template = "Failed to find identifier '%s' of test collection %s in the tool generated collection elements %s (at the correct position)" - eo_ids = [_["element_identifier"] for _ in element_objects] - message = template % (element_identifier, sorted_test_ids, - eo_ids) + element = get_element(element_objects, element_identifier) + if not element: + template = "Failed to find identifier '%s' in the tool generated collection elements %s" + message = template % (element_identifier, eo_ids) raise AssertionError(message) element_type = element["element_type"] @@ -763,6 +756,21 @@ def verify_collection(output_collection_def, data_collection, verify_dataset): elements = element["object"]["elements"] verify_elements(elements, element_attrib.get("elements", {})) + if len(expected_sort_order) > 0: + i = 0 + for element_identifier in expected_sort_order: + element = None + while i < len(element_objects): + if element_objects[i]["element_identifier"] == element_identifier: + element = element_objects[i] + i += 1 + break + i += 1 + if element is None: + template = "Collection identifier '%s' found out of order, expected order of %s for the tool generated collection elements %s" + message = template % (element_identifier, expected_sort_order, eo_ids) + raise AssertionError(message) + verify_elements(data_collection["elements"], output_collection_def.element_tests) diff --git a/test/functional/tools/discover_sort_by.xml b/test/functional/tools/discover_sort_by.xml index 5e46342253a..cb5c1949673 100644 --- a/test/functional/tools/discover_sort_by.xml +++ b/test/functional/tools/discover_sort_by.xml @@ -1,4 +1,4 @@ - + + + \$i.txt; +done +]]> + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 4c3932df60f687502e7b6c6d38aa0e44d46d6f98 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 16 Oct 2020 07:56:06 -0400 Subject: [PATCH 2/6] Improvements to #10434 based on comments from @bernt-matthias --- lib/galaxy/tool_util/parser/xml.py | 2 +- lib/galaxy/tool_util/xsd/galaxy.xsd | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/parser/xml.py b/lib/galaxy/tool_util/parser/xml.py index 88206c1ec09..88eace9a56a 100644 --- a/lib/galaxy/tool_util/parser/xml.py +++ b/lib/galaxy/tool_util/parser/xml.py @@ -598,7 +598,7 @@ def __parse_output_collection_elem(output_collection_elem, profile=None): def __parse_element_tests(parent_element, profile=None): - element_tests = OrderedDict() + element_tests = {} for idx, element in enumerate(parent_element.findall("element")): element_attrib = dict(element.attrib) identifier = element_attrib.pop('name', None) diff --git a/lib/galaxy/tool_util/xsd/galaxy.xsd b/lib/galaxy/tool_util/xsd/galaxy.xsd index 3da3646fa3e..3393572d00d 100644 --- a/lib/galaxy/tool_util/xsd/galaxy.xsd +++ b/lib/galaxy/tool_util/xsd/galaxy.xsd @@ -1507,7 +1507,7 @@ Note that this tool uses ``assign_primary_output="true"`` for `` Date: Wed, 21 Oct 2020 18:28:58 +0200 Subject: [PATCH 3/6] Fix maximum_workflow_jobs_per_scheduling_iteration and set to 1000 as default value --- doc/source/admin/galaxy_options.rst | 4 ++-- lib/galaxy/config/sample/galaxy.yml.sample | 4 ++-- lib/galaxy/tools/execute.py | 4 +++- lib/galaxy/webapps/galaxy/config_schema.yml | 4 ++-- test/integration/test_workflow_scheduling_options.py | 8 ++++---- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index 4921c3bff8c..f547452d8d8 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -3181,8 +3181,8 @@ associated with scheduling workflows at the expense of increased total DB traffic because model objects are expunged from the SQL alchemy session between workflow invocation scheduling iterations. - Set to -1 to disable any such maximum (the default). -:Default: ``-1`` + Set to -1 to disable any such maximum. +:Default: ``1000`` :Type: int diff --git a/lib/galaxy/config/sample/galaxy.yml.sample b/lib/galaxy/config/sample/galaxy.yml.sample index b0fe163e6d8..2ebfb89c599 100644 --- a/lib/galaxy/config/sample/galaxy.yml.sample +++ b/lib/galaxy/config/sample/galaxy.yml.sample @@ -1577,8 +1577,8 @@ galaxy: # scheduling workflows at the expense of increased total DB traffic # because model objects are expunged from the SQL alchemy session # between workflow invocation scheduling iterations. Set to -1 to - # disable any such maximum (the default). - #maximum_workflow_jobs_per_scheduling_iteration: -1 + # disable any such maximum. + #maximum_workflow_jobs_per_scheduling_iteration: 1000 # Force serial scheduling of workflows within the context of a # particular history diff --git a/lib/galaxy/tools/execute.py b/lib/galaxy/tools/execute.py index 52bddbf3cd5..0e125e16345 100644 --- a/lib/galaxy/tools/execute.py +++ b/lib/galaxy/tools/execute.py @@ -101,11 +101,13 @@ def execute(trans, tool, mapping_params, history, rerun_remap_job_id=None, colle break else: execute_single_job(execution_slice, completed_jobs[i]) + history = execution_slice.history or history + jobs_executed += 1 if execution_slice.datasets_to_persist: datasets_to_persist.extend(execution_slice.datasets_to_persist) if datasets_to_persist: - execution_slice.history.add_datasets(trans.sa_session, datasets_to_persist, set_hid=True, quota=False, flush=False) + history.add_datasets(trans.sa_session, datasets_to_persist, set_hid=True, quota=False, flush=False) # a side effect of history.add_datasets is a commit within db_next_hid (even with flush=False). else: # Make sure collections, implicit jobs etc are flushed even if there are no precreated output datasets diff --git a/lib/galaxy/webapps/galaxy/config_schema.yml b/lib/galaxy/webapps/galaxy/config_schema.yml index eeebaa25a9e..f91d15e924e 100644 --- a/lib/galaxy/webapps/galaxy/config_schema.yml +++ b/lib/galaxy/webapps/galaxy/config_schema.yml @@ -2368,7 +2368,7 @@ mapping: maximum_workflow_jobs_per_scheduling_iteration: type: int - default: -1 + default: 1000 required: false desc: | Specify a maximum number of jobs that any given workflow scheduling iteration can create. @@ -2376,7 +2376,7 @@ mapping: preventing other jobs from executing. This may also mitigate memory issues associated with scheduling workflows at the expense of increased total DB traffic because model objects are expunged from the SQL alchemy session between workflow invocation scheduling iterations. - Set to -1 to disable any such maximum (the default). + Set to -1 to disable any such maximum. history_local_serial_workflow_scheduling: type: bool diff --git a/test/integration/test_workflow_scheduling_options.py b/test/integration/test_workflow_scheduling_options.py index b0d93f4dd40..6c12e191727 100644 --- a/test/integration/test_workflow_scheduling_options.py +++ b/test/integration/test_workflow_scheduling_options.py @@ -24,7 +24,7 @@ class MaximumWorkflowInvocationDurationTestCase(integration_util.IntegrationTest def handle_galaxy_config_kwds(cls, config): config["maximum_workflow_invocation_duration"] = 20 - def do_test(self): + def test(self): workflow = self.workflow_populator.load_workflow_from_resource("test_workflow_pause") workflow_id = self.workflow_populator.create_workflow(workflow) history_id = self.dataset_populator.new_history() @@ -61,7 +61,7 @@ class MaximumWorkflowJobsPerSchedulingIterationTestCase(integration_util.Integra def handle_galaxy_config_kwds(cls, config): config["maximum_workflow_jobs_per_scheduling_iteration"] = 1 - def do_test(self): + def test(self): workflow_id = self.workflow_populator.upload_yaml_workflow(""" class: GalaxyWorkflow steps: @@ -73,11 +73,11 @@ steps: - tool_id: collection_paired_test state: f1: - $link: 1#paired_output + $link: 1/paired_output - tool_id: cat_list state: input1: - $link: 2#out1 + $link: 2/out1 """) with self.dataset_populator.test_history() as history_id: hdca1 = self.dataset_collection_populator.create_list_in_history(history_id, contents=["a\nb\nc\nd\n", "e\nf\ng\nh\n"]).json() From d0ad39a9098552fd81ca28384a8990e5c4e8d97b Mon Sep 17 00:00:00 2001 From: Dannon Baker Date: Thu, 22 Oct 2020 09:42:01 -0400 Subject: [PATCH 4/6] Rebuild config for 20.09, including missing option in galaxy_options doc --- doc/source/admin/galaxy_options.rst | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/doc/source/admin/galaxy_options.rst b/doc/source/admin/galaxy_options.rst index f547452d8d8..a85040cc812 100644 --- a/doc/source/admin/galaxy_options.rst +++ b/doc/source/admin/galaxy_options.rst @@ -2636,6 +2636,19 @@ :Type: float +~~~~~~~~~~~~~~~~~ +``tool_id_boost`` +~~~~~~~~~~~~~~~~~ + +:Description: + Boosts are used to customize this instance's toolbox search. The + higher the boost, the more importance the scoring algorithm gives + to the given field. Section refers to the tool group in the tool + panel. Rest of the fields are tool's attributes. +:Default: ``9.0`` +:Type: float + + ~~~~~~~~~~~~~~~~~~~~~~ ``tool_section_boost`` ~~~~~~~~~~~~~~~~~~~~~~ From 4c2f8124f02d2f3a0c1533f91e2fe2547025905a Mon Sep 17 00:00:00 2001 From: Nuwan Goonasekera <2070605+nuwang@users.noreply.github.com> Date: Wed, 21 Oct 2020 00:24:08 +0530 Subject: [PATCH 5/6] Move more non-dns compliant labels to annotations --- lib/galaxy/jobs/runners/kubernetes.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/jobs/runners/kubernetes.py b/lib/galaxy/jobs/runners/kubernetes.py index 9db6ea7a9b3..5ca18fff9a5 100644 --- a/lib/galaxy/jobs/runners/kubernetes.py +++ b/lib/galaxy/jobs/runners/kubernetes.py @@ -42,6 +42,8 @@ class KubernetesJobRunner(AsynchronousJobRunner): """ runner_name = "KubernetesRunner" + LABEL_REGEX = re.compile("[^-A-Za-z0-9_.]") + def __init__(self, app, nworkers, **kwargs): # Check if pykube was importable, fail if not ensure_pykube() @@ -205,19 +207,19 @@ class KubernetesJobRunner(AsynchronousJobRunner): k8s_spec_template = { "metadata": { "labels": { - "app.kubernetes.io/name": ajs.job_wrapper.tool.old_id, + "app.kubernetes.io/name": self.LABEL_REGEX.sub("_", ajs.job_wrapper.tool.old_id), "app.kubernetes.io/instance": self.__produce_k8s_job_prefix(), - "app.kubernetes.io/version": ajs.job_wrapper.tool.version, + "app.kubernetes.io/version": self.LABEL_REGEX.sub("_", str(ajs.job_wrapper.tool.version)), "app.kubernetes.io/component": "tool", "app.kubernetes.io/part-of": "galaxy", "app.kubernetes.io/managed-by": "galaxy", - "app.galaxyproject.org/job_id": ajs.job_wrapper.get_id_tag(), - "app.galaxyproject.org/instance": self._galaxy_instance_id or "", - "app.galaxyproject.org/handler": self.app.config.server_name, - "app.galaxyproject.org/destination": ajs.job_wrapper.job_destination.id, + "app.galaxyproject.org/job_id": self.LABEL_REGEX.sub("_", ajs.job_wrapper.get_id_tag()), + "app.galaxyproject.org/handler": self.LABEL_REGEX.sub("_", self.app.config.server_name), + "app.galaxyproject.org/destination": self.LABEL_REGEX.sub( + "_", str(ajs.job_wrapper.job_destination.id)) }, "annotations": { - "app.galaxyproject.org/tool_id": ajs.job_wrapper.tool.id, + "app.galaxyproject.org/tool_id": ajs.job_wrapper.tool.id } }, "spec": { From f0a55a399cf96cd8c11e3ab8a0d283e9df852088 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 19 Oct 2020 17:34:35 +0200 Subject: [PATCH 6/6] Cache container resolution during job execution That should be very useful when the Cached* container resolvers can't be used (e.g. on k8s). --- lib/galaxy/tool_util/deps/containers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/galaxy/tool_util/deps/containers.py b/lib/galaxy/tool_util/deps/containers.py index 278b7938d6a..84e0d627cab 100644 --- a/lib/galaxy/tool_util/deps/containers.py +++ b/lib/galaxy/tool_util/deps/containers.py @@ -244,6 +244,7 @@ class ContainerRegistry: return None if resolved_container_description is None else resolved_container_description.container_description def resolve(self, enabled_container_types, tool_info, index=None, resolver_type=None, install=True, resolution_cache=None): + resolution_cache = resolution_cache or self.mulled_resolution_cache for i, container_resolver in enumerate(self.container_resolvers): if index is not None and i != index: continue