From dfccd902b00426c79670e77ef11696f381f5e10d Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 13:34:02 -0400 Subject: [PATCH 1/9] Fix setting metadata in galaxy.json for working directory isolation change. This has been broken since 16.04 I guess? Maybe retry internally means it hasn't been an issue? --- lib/galaxy_ext/metadata/set_metadata.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/galaxy_ext/metadata/set_metadata.py b/lib/galaxy_ext/metadata/set_metadata.py index a540ff36df5..6f70a9d0f63 100644 --- a/lib/galaxy_ext/metadata/set_metadata.py +++ b/lib/galaxy_ext/metadata/set_metadata.py @@ -141,10 +141,11 @@ def set_metadata(): json.dump( ( False, str( e ) ), open( filename_results_code, 'wb+' ) ) # setting metadata has failed somehow for i, ( filename, file_dict ) in enumerate( new_job_metadata_dict.iteritems(), start=1 ): - new_dataset = galaxy.model.Dataset( id=-i, external_filename=os.path.join( tool_job_working_directory, file_dict[ 'filename' ] ) ) + new_dataset_filename = os.path.join( tool_job_working_directory, "working", file_dict[ 'filename' ] ) + new_dataset = galaxy.model.Dataset( id=-i, external_filename=new_dataset_filename ) extra_files = file_dict.get( 'extra_files', None ) if extra_files is not None: - new_dataset._extra_files_path = os.path.join( tool_job_working_directory, extra_files ) + new_dataset._extra_files_path = os.path.join( tool_job_working_directory, "working", extra_files ) new_dataset.state = new_dataset.states.OK new_dataset_instance = galaxy.model.HistoryDatasetAssociation( id=-i, dataset=new_dataset, extension=file_dict.get( 'ext', 'data' ) ) set_meta_with_tool_provided( new_dataset_instance, file_dict, set_meta_kwds, datatypes_registry ) From af6bd40a57608464e3e70226c46490681d213446 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 13:33:41 -0400 Subject: [PATCH 2/9] Fix setting dbkey in galaxy.json for discovered primary datasets. Based on the change - I guess this must have been broken a long time ago - so maybe it isn't used? A subsequent change will add a test though - so hopefully there will be no regressions of this again. --- lib/galaxy/tools/parameters/output_collect.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/galaxy/tools/parameters/output_collect.py b/lib/galaxy/tools/parameters/output_collect.py index 1fb52296c77..6dff4027022 100644 --- a/lib/galaxy/tools/parameters/output_collect.py +++ b/lib/galaxy/tools/parameters/output_collect.py @@ -314,6 +314,8 @@ def collect_primary_datasets( tool, output, job_working_directory, input_ext, in ) metadata_dict = new_primary_datasets_attributes.get( 'metadata', None ) if metadata_dict: + if "dbkey" in new_primary_datasets_attributes: + metadata_dict["dbkey"] = new_primary_datasets_attributes["dbkey"] primary_data.metadata.from_JSON_dict( json_dict=metadata_dict ) else: primary_data.set_meta() From e72201d2041a80db0042af9493b1f2f1512d68f7 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 11:32:18 -0400 Subject: [PATCH 3/9] Improvements for metadata specification w/galaxy.json of explicit outputs. I call explicit outputs galaxy.json entries of type="dataset" (as opposed to type="new_primary_dataset" - which I call discovered datasets). Previously galaxy.json could set the name and extension of these datasets. I've added a test case to verify this and a small tweak to the tool test framework to allow easy testing of such metadata setting. In order to bring explicit output metadata setting closer to parity with discovered dataset metadata setting, I've added the ability to define 'dbkey' and 'info' entries in galaxy.json for these outputs. --- lib/galaxy/jobs/__init__.py | 11 +++---- test/base/interactor.py | 16 ++++++++-- test/functional/tools/samples_tool_conf.xml | 1 + .../tools/tool_provided_metadata_1.xml | 29 +++++++++++++++++++ 4 files changed, 49 insertions(+), 8 deletions(-) create mode 100644 test/functional/tools/tool_provided_metadata_1.xml diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py index 9fca963bc96..648c4fd1a3d 100644 --- a/lib/galaxy/jobs/__init__.py +++ b/lib/galaxy/jobs/__init__.py @@ -1298,11 +1298,12 @@ class JobWrapper( object ): dataset.set_peek( is_multi_byte=True ) else: dataset.set_peek() - try: - # set the name if provided by the tool - dataset.name = context['name'] - except: - pass + for context_key in ['name', 'info', 'dbkey']: + try: + context_value = context[context_key] + setattr(dataset, context_key, context_value) + except Exception: + pass else: dataset.blurb = "empty" if dataset.ext == 'auto': diff --git a/test/base/interactor.py b/test/base/interactor.py index 69e32f1b1a0..13755a585e1 100644 --- a/test/base/interactor.py +++ b/test/base/interactor.py @@ -91,11 +91,21 @@ class GalaxyInteractorApi( object ): self._verify_metadata( history_id, hda_id, attributes ) def _verify_metadata( self, history_id, hid, attributes ): + """Check dataset metadata. + + ftype on output maps to `file_ext` on the hda's API description, `name`, `info`, + and `dbkey` all map to the API description directly. Other metadata attributes + are assumed to be datatype-specific and mapped with a prefix of `metadata_`. + """ metadata = attributes.get( 'metadata', {} ).copy() for key, value in metadata.copy().items(): - new_key = "metadata_%s" % key - metadata[ new_key ] = metadata[ key ] - del metadata[ key ] + if key not in ['name', 'info']: + new_key = "metadata_%s" % key + metadata[ new_key ] = metadata[ key ] + del metadata[ key ] + elif key == "info": + metadata[ "misc_info" ] = metadata[ "info" ] + del metadata[ "info" ] expected_file_type = attributes.get( 'ftype', None ) if expected_file_type: metadata[ "file_ext" ] = expected_file_type diff --git a/test/functional/tools/samples_tool_conf.xml b/test/functional/tools/samples_tool_conf.xml index 67ee93e5112..3df077ec1c9 100644 --- a/test/functional/tools/samples_tool_conf.xml +++ b/test/functional/tools/samples_tool_conf.xml @@ -15,6 +15,7 @@ + diff --git a/test/functional/tools/tool_provided_metadata_1.xml b/test/functional/tools/tool_provided_metadata_1.xml new file mode 100644 index 00000000000..bfbc515e577 --- /dev/null +++ b/test/functional/tools/tool_provided_metadata_1.xml @@ -0,0 +1,29 @@ + + + echo "This is a line of text." > $out1; + cp $c1 galaxy.json; + + + {"type": "dataset", "dataset_id": ${str($out1.id)}, "name": "my dynamic name", "ext": "txt", "info": "my dynamic info", "dbkey": "cust1"} + + + + + + + + + + + + + + + + + + + + + From 1fab4dfaff3ebbb0f1c80c7b51f44125272b6cb9 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Thu, 4 Aug 2016 13:13:16 -0400 Subject: [PATCH 4/9] Add hack to test sleeping when verifying metadata. Seems like a race condition but I can't determine why there would be one here - this check should help determine if it is a race condition or something else. --- test/base/interactor.py | 10 +++++++++- test/functional/tools/tool_provided_metadata_1.xml | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/test/base/interactor.py b/test/base/interactor.py index 13755a585e1..2e78457017d 100644 --- a/test/base/interactor.py +++ b/test/base/interactor.py @@ -87,7 +87,13 @@ class GalaxyInteractorApi( object ): def verify_output_dataset( self, history_id, hda_id, outfile, attributes, shed_tool_id ): fetcher = self.__dataset_fetcher( history_id ) - self.twill_test_case.verify_hid( outfile, hda_id=hda_id, attributes=attributes, dataset_fetcher=fetcher, shed_tool_id=shed_tool_id ) + self.twill_test_case.verify_hid( + outfile, + hda_id=hda_id, + attributes=attributes, + dataset_fetcher=fetcher, + shed_tool_id=shed_tool_id + ) self._verify_metadata( history_id, hda_id, attributes ) def _verify_metadata( self, history_id, hid, attributes ): @@ -111,6 +117,8 @@ class GalaxyInteractorApi( object ): metadata[ "file_ext" ] = expected_file_type if metadata: + import time + time.sleep(5) dataset = self._get( "histories/%s/contents/%s" % ( history_id, hid ) ).json() for key, value in metadata.items(): try: diff --git a/test/functional/tools/tool_provided_metadata_1.xml b/test/functional/tools/tool_provided_metadata_1.xml index bfbc515e577..02d971a85f4 100644 --- a/test/functional/tools/tool_provided_metadata_1.xml +++ b/test/functional/tools/tool_provided_metadata_1.xml @@ -4,7 +4,7 @@ cp $c1 galaxy.json; - {"type": "dataset", "dataset_id": ${str($out1.id)}, "name": "my dynamic name", "ext": "txt", "info": "my dynamic info", "dbkey": "cust1"} + {"type": "dataset", "dataset_id": $out1.dataset.dataset.id, "name": "my dynamic name", "ext": "txt", "info": "my dynamic info", "dbkey": "cust1"} From 76e84eb5e00e78e9e49667d39b2f3d91352f3d5a Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 13:37:24 -0400 Subject: [PATCH 5/9] Add tool test for discovered dataset metadata from galaxy.json. --- test/functional/tools/samples_tool_conf.xml | 1 + .../tools/tool_provided_metadata_2.xml | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+) create mode 100644 test/functional/tools/tool_provided_metadata_2.xml diff --git a/test/functional/tools/samples_tool_conf.xml b/test/functional/tools/samples_tool_conf.xml index 3df077ec1c9..5b12a2bae9f 100644 --- a/test/functional/tools/samples_tool_conf.xml +++ b/test/functional/tools/samples_tool_conf.xml @@ -16,6 +16,7 @@ + diff --git a/test/functional/tools/tool_provided_metadata_2.xml b/test/functional/tools/tool_provided_metadata_2.xml new file mode 100644 index 00000000000..804cc4e5883 --- /dev/null +++ b/test/functional/tools/tool_provided_metadata_2.xml @@ -0,0 +1,40 @@ + + + echo "Log" > $sample; + echo "1" > sample1.report.tsv; + echo "2" > sample2.report.tsv; + cp $c1 galaxy.json; + + + {"type": "new_primary_dataset", "filename": "sample1.report.tsv", "name": "cool name 1", "ext": "txt", "info": "cool 1 info", "dbkey": "hg19"} +{"type": "new_primary_dataset", "filename": "sample2.report.tsv", "name": "cool name 2", "ext": "txt", "info": "cool 2 info", "dbkey": "hg19"} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 41701aa6fe696e2ef1b26dec261f416686d02e20 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 13:37:55 -0400 Subject: [PATCH 6/9] Improved error message when testing metadata in tool tests. --- test/base/interactor.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/base/interactor.py b/test/base/interactor.py index 2e78457017d..99dfac10450 100644 --- a/test/base/interactor.py +++ b/test/base/interactor.py @@ -124,8 +124,8 @@ class GalaxyInteractorApi( object ): try: dataset_value = dataset.get( key, None ) if dataset_value != value: - msg = "Dataset metadata verification for [%s] failed, expected [%s] but found [%s]." - msg_params = ( key, value, dataset_value ) + msg = "Dataset metadata verification for [%s] failed, expected [%s] but found [%s]. Dataset API value was [%s]." + msg_params = ( key, value, dataset_value, dataset ) msg = msg % msg_params raise Exception( msg ) except KeyError: From 56bb42ad2536732c195c218c1b89cf174cd02b78 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 15:10:23 -0400 Subject: [PATCH 7/9] Fix allow tool to just test discovered datasets. Previously some sort of test on the base file the primary datasets were keyed on was required. --- lib/galaxy/tools/parser/xml.py | 25 +++++++++++-------- .../tools/tool_provided_metadata_2.xml | 2 -- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/galaxy/tools/parser/xml.py b/lib/galaxy/tools/parser/xml.py index 178cf6a3f46..47b990901c1 100644 --- a/lib/galaxy/tools/parser/xml.py +++ b/lib/galaxy/tools/parser/xml.py @@ -391,15 +391,7 @@ def __parse_output_elem( output_elem ): if name is None: raise Exception( "Test output does not have a 'name'" ) - file, attributes = __parse_test_attributes( output_elem, attrib ) - primary_datasets = {} - for primary_elem in ( output_elem.findall( "discovered_dataset" ) or [] ): - primary_attrib = dict( primary_elem.attrib ) - designation = primary_attrib.pop( 'designation', None ) - if designation is None: - raise Exception( "Test primary dataset does not have a 'designation'" ) - primary_datasets[ designation ] = __parse_test_attributes( primary_elem, primary_attrib ) - attributes[ "primary_datasets" ] = primary_datasets + file, attributes = __parse_test_attributes( output_elem, attrib, parse_discovered_datasets=True ) return name, file, attributes @@ -436,7 +428,7 @@ def __parse_element_tests( parent_element ): return element_tests -def __parse_test_attributes( output_elem, attrib, parse_elements=False ): +def __parse_test_attributes( output_elem, attrib, parse_elements=False, parse_discovered_datasets=False ): assert_list = __parse_assert_list( output_elem ) # Allow either file or value to specify a target file to compare result with @@ -466,8 +458,18 @@ def __parse_test_attributes( output_elem, attrib, parse_elements=False ): if parse_elements: element_tests = __parse_element_tests( output_elem ) + primary_datasets = {} + if parse_discovered_datasets: + for primary_elem in ( output_elem.findall( "discovered_dataset" ) or [] ): + primary_attrib = dict( primary_elem.attrib ) + designation = primary_attrib.pop( 'designation', None ) + if designation is None: + raise Exception( "Test primary dataset does not have a 'designation'" ) + primary_datasets[ designation ] = __parse_test_attributes( primary_elem, primary_attrib ) + has_checksum = md5sum or checksum - if not (assert_list or file or extra_files or metadata or has_checksum or element_tests): + has_nested_tests = extra_files or element_tests or primary_datasets + if not (assert_list or file or metadata or has_checksum or has_nested_tests): raise Exception( "Test output defines nothing to check (e.g. must have a 'file' check against, assertions to check, metadata or checksum tests, etc...)") attributes['assert_list'] = assert_list attributes['extra_files'] = extra_files @@ -475,6 +477,7 @@ def __parse_test_attributes( output_elem, attrib, parse_elements=False ): attributes['md5'] = md5sum attributes['checksum'] = checksum attributes['elements'] = element_tests + attributes['primary_datasets'] = primary_datasets return file, attributes diff --git a/test/functional/tools/tool_provided_metadata_2.xml b/test/functional/tools/tool_provided_metadata_2.xml index 804cc4e5883..8879d0f62af 100644 --- a/test/functional/tools/tool_provided_metadata_2.xml +++ b/test/functional/tools/tool_provided_metadata_2.xml @@ -1,6 +1,5 @@ - echo "Log" > $sample; echo "1" > sample1.report.tsv; echo "2" > sample2.report.tsv; cp $c1 galaxy.json; @@ -22,7 +21,6 @@ - From 9ae333987b2cd713526ae68800ade5e0a73b1e08 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Mon, 1 Aug 2016 15:31:49 -0400 Subject: [PATCH 8/9] Test tool demo-ing discovered dataset datatype metadata w/galaxy.json. Also update the metadata test comparison to support non-textual values (e.g. floating point and integer metadata fields). --- test/base/interactor.py | 3 +- test/functional/tools/samples_tool_conf.xml | 1 + .../tools/tool_provided_metadata_3.xml | 43 +++++++++++++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 test/functional/tools/tool_provided_metadata_3.xml diff --git a/test/base/interactor.py b/test/base/interactor.py index 99dfac10450..04a115a5d59 100644 --- a/test/base/interactor.py +++ b/test/base/interactor.py @@ -7,6 +7,7 @@ from logging import getLogger from requests import get, post, delete, patch from six import StringIO +from six import text_type from galaxy import util from galaxy.tools.parser.interface import TestCollectionDef @@ -123,7 +124,7 @@ class GalaxyInteractorApi( object ): for key, value in metadata.items(): try: dataset_value = dataset.get( key, None ) - if dataset_value != value: + if text_type(dataset_value) != text_type(value): msg = "Dataset metadata verification for [%s] failed, expected [%s] but found [%s]. Dataset API value was [%s]." msg_params = ( key, value, dataset_value, dataset ) msg = msg % msg_params diff --git a/test/functional/tools/samples_tool_conf.xml b/test/functional/tools/samples_tool_conf.xml index 5b12a2bae9f..ef44e49a456 100644 --- a/test/functional/tools/samples_tool_conf.xml +++ b/test/functional/tools/samples_tool_conf.xml @@ -17,6 +17,7 @@ + diff --git a/test/functional/tools/tool_provided_metadata_3.xml b/test/functional/tools/tool_provided_metadata_3.xml new file mode 100644 index 00000000000..a9a64f6ee82 --- /dev/null +++ b/test/functional/tools/tool_provided_metadata_3.xml @@ -0,0 +1,43 @@ + + + echo "1" > sample1.report.tsv; + echo "2" > sample2.report.tsv; + cp $c1 galaxy.json; + + + {"type": "new_primary_dataset", "filename": "sample1.report.tsv", "name": "cool name 1", "ext": "txt", "info": "cool 1 info", "dbkey": "hg19", "metadata": {"data_lines": 10, "foo": "bar"}} +{"type": "new_primary_dataset", "filename": "sample2.report.tsv", "name": "cool name 2", "ext": "txt", "info": "cool 2 info", "dbkey": "hg19", "metadata": {"data_lines": 20, "foo": "bar"}} + + + + + + + + + + + + + + + + + + + + + + + + + + + + + From 1e8dd73b6def69aa8533f087e22d99d3423f94b3 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 16 Aug 2016 14:00:01 -0400 Subject: [PATCH 9/9] Rework small for loop per @mvdbeek's suggestion... ... see https://github.com/galaxyproject/galaxy/pull/2697#discussion_r74939815 for context. --- lib/galaxy/jobs/__init__.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py index 648c4fd1a3d..73e3b4b6dc0 100644 --- a/lib/galaxy/jobs/__init__.py +++ b/lib/galaxy/jobs/__init__.py @@ -1299,11 +1299,9 @@ class JobWrapper( object ): else: dataset.set_peek() for context_key in ['name', 'info', 'dbkey']: - try: + if context_key in context: context_value = context[context_key] setattr(dataset, context_key, context_value) - except Exception: - pass else: dataset.blurb = "empty" if dataset.ext == 'auto':