From ecbce1369df5eb314d407f5470f8e1bde121d44d Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 27 May 2021 13:29:15 +0200 Subject: [PATCH 1/8] fix pysam.view call otherwise samtools output is the return value of `pysam.view` and the output stays empty. see also https://github.com/pysam-developers/pysam/issues/677 --- lib/galaxy/tool_util/verify/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/verify/__init__.py b/lib/galaxy/tool_util/verify/__init__.py index b3b1abda323..e59be9540db 100644 --- a/lib/galaxy/tool_util/verify/__init__.py +++ b/lib/galaxy/tool_util/verify/__init__.py @@ -184,12 +184,12 @@ def _bam_to_sam(local_name, temp_name): temp_local = tempfile.NamedTemporaryFile(suffix='.sam', prefix='local_bam_converted_to_sam_') with tempfile.NamedTemporaryFile(suffix='.sam', prefix='history_bam_converted_to_sam_', delete=False) as temp: try: - pysam.view('-h', '-o%s' % temp_local.name, local_name) + pysam.view('-h', '-o', temp_local.name, local_name, catch_stdout=False) except Exception as e: msg = "Converting local (test-data) BAM to SAM failed: %s" % unicodify(e) raise Exception(msg) try: - pysam.view('-h', '-o%s' % temp.name, temp_name) + pysam.view('-h', '-o', temp.name, temp_name, catch_stdout=False) except Exception as e: msg = "Converting history BAM to SAM failed: %s" % unicodify(e) raise Exception(msg) From 1cf34d74d063094343328d18adb8e4328e06a69e Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 27 May 2021 14:58:26 +0200 Subject: [PATCH 2/8] psyam should not add a `@PG` headerline i.e. leave the sam content of the bam file untouched --- lib/galaxy/tool_util/verify/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/galaxy/tool_util/verify/__init__.py b/lib/galaxy/tool_util/verify/__init__.py index e59be9540db..5414d31ac75 100644 --- a/lib/galaxy/tool_util/verify/__init__.py +++ b/lib/galaxy/tool_util/verify/__init__.py @@ -184,12 +184,12 @@ def _bam_to_sam(local_name, temp_name): temp_local = tempfile.NamedTemporaryFile(suffix='.sam', prefix='local_bam_converted_to_sam_') with tempfile.NamedTemporaryFile(suffix='.sam', prefix='history_bam_converted_to_sam_', delete=False) as temp: try: - pysam.view('-h', '-o', temp_local.name, local_name, catch_stdout=False) + pysam.view('-h', '--no-PG', '-o', temp_local.name, local_name, catch_stdout=False) except Exception as e: msg = "Converting local (test-data) BAM to SAM failed: %s" % unicodify(e) raise Exception(msg) try: - pysam.view('-h', '-o', temp.name, temp_name, catch_stdout=False) + pysam.view('-h', '--no-PG', '-o', temp.name, temp_name, catch_stdout=False) except Exception as e: msg = "Converting history BAM to SAM failed: %s" % unicodify(e) raise Exception(msg) From 01eba8ee7a509ca52d8d9846a9e8d44609851d86 Mon Sep 17 00:00:00 2001 From: Matthias Bernt Date: Thu, 27 May 2021 15:10:05 +0200 Subject: [PATCH 3/8] add ftype for sam_to_bam test to also convert this to sam in tests --- test/functional/tools/sam_to_bam.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/tools/sam_to_bam.xml b/test/functional/tools/sam_to_bam.xml index b01152a7670..3420ed73ec9 100644 --- a/test/functional/tools/sam_to_bam.xml +++ b/test/functional/tools/sam_to_bam.xml @@ -11,7 +11,7 @@ cat '$input1' > '$out_file1' - + From bd6fe02aa84033967d6ae20b832d66ce9a54cd76 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Tue, 1 Jun 2021 17:45:12 +0200 Subject: [PATCH 4/8] Fix set metadata for primary discovered outputs We were previously setting metadata on the path that `filename_override` points to, which (should be an output in the job working directory, or an output in the object store. That does not work for `unnnamed_outputs` that are inferred via galaxy.json and that are discovered using collect_dynamic_outputs. In most cases that doesn't lead to an exception, but it does fail for bam files (and a small subset of other datatypes I would guess) if you specify the extension explicitly (if the extension is not set, sniffing on the empty dataset results in `data`, which will prevent the exception). For datatypes that don't fail in `set_metadata` on empty input files metadata would then be set again when collecting dynamic outputs, this time using the correct path. The fix here is to check whether the file we're setting metadata for is actually an unnamed output, and override the path with the path to the file in the working directory. To prevent setting metadata twice we also skip setting metadata on files that already have a database id (which should only be these primary discovered outputs). --- lib/galaxy/metadata/set_metadata.py | 41 ++++++++++++++++++----------- lib/galaxy/model/store/discover.py | 3 ++- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/lib/galaxy/metadata/set_metadata.py b/lib/galaxy/metadata/set_metadata.py index f17bd01a3ce..48010bcd561 100644 --- a/lib/galaxy/metadata/set_metadata.py +++ b/lib/galaxy/metadata/set_metadata.py @@ -139,6 +139,7 @@ def set_metadata_portable(): version_string = "" export_store = None + final_job_state = 'ok' if extended_metadata_collection: tool_dict = metadata_params["tool"] stdio_exit_code_dicts, stdio_regex_dicts = tool_dict["stdio_exit_codes"], tool_dict["stdio_regexes"] @@ -185,7 +186,7 @@ def set_metadata_portable(): if os.path.exists(COMMAND_VERSION_FILENAME): version_string = open(COMMAND_VERSION_FILENAME).read() - job_context = ExpressionContext(dict(stdout=tool_stdout, stderr=tool_stderr)) + expression_context = ExpressionContext(dict(stdout=tool_stdout, stderr=tool_stderr)) # Load outputs. export_store = store.DirectoryModelExportStore('metadata/outputs_populated', serialize_dataset_objects=True, for_edit=True, strip_metadata_files=False, serialize_jobs=False) @@ -195,6 +196,27 @@ def set_metadata_portable(): # Remove in 21.09, this should only happen for jobs that started on <= 20.09 and finish now import_model_store = None + job_context = SessionlessJobContext( + metadata_params, + tool_provided_metadata, + object_store, + export_store, + import_model_store, + os.path.join(tool_job_working_directory, "working"), + final_job_state=final_job_state, + ) + + unnamed_id_to_path = {} + for unnamed_output_dict in job_context.tool_provided_metadata.get_unnamed_outputs(): + destination = unnamed_output_dict["destination"] + elements = unnamed_output_dict["elements"] + destination_type = destination["type"] + if destination_type == 'hdas': + for element in elements: + filename = element.get('filename') + if filename: + unnamed_id_to_path[element['object_id']] = os.path.join(job_context.job_working_directory, filename) + for output_name, output_dict in outputs.items(): dataset_instance_id = output_dict["id"] klass = getattr(galaxy.model, output_dict.get('model_class', 'HistoryDatasetAssociation')) @@ -219,7 +241,7 @@ def set_metadata_portable(): # Same block as below... set_meta_kwds = stringify_dictionary_keys(json.load(open(filename_kwds))) # load kwds; need to ensure our keywords are not unicode try: - dataset.dataset.external_filename = dataset_filename_override + dataset.dataset.external_filename = unnamed_id_to_path.get(dataset_instance_id, dataset_filename_override) store_by = output_dict.get("object_store_store_by", legacy_object_store_store_by) extra_files_dir_name = "dataset_%s_files" % getattr(dataset.dataset, store_by) files_path = os.path.abspath(os.path.join(tool_job_working_directory, "working", extra_files_dir_name)) @@ -240,9 +262,9 @@ def set_metadata_portable(): if extended_metadata_collection: meta = tool_provided_metadata.get_dataset_meta(output_name, dataset.dataset.id, dataset.dataset.uuid) if meta: - context = ExpressionContext(meta, job_context) + context = ExpressionContext(meta, expression_context) else: - context = job_context + context = expression_context # Lazy and unattached # if getattr(dataset, "hidden_beneath_collection_instance", None): @@ -301,17 +323,6 @@ def set_metadata_portable(): if extended_metadata_collection: # discover extra outputs... - - job_context = SessionlessJobContext( - metadata_params, - tool_provided_metadata, - object_store, - export_store, - import_model_store, - os.path.join(tool_job_working_directory, "working"), - final_job_state=final_job_state, - ) - output_collections = {} for name, output_collection in metadata_params["output_collections"].items(): output_collections[name] = import_model_store.sa_session.query(HistoryDatasetCollectionAssociation).find(output_collection["id"]) diff --git a/lib/galaxy/model/store/discover.py b/lib/galaxy/model/store/discover.py index 07ea899ce20..5771c200a20 100644 --- a/lib/galaxy/model/store/discover.py +++ b/lib/galaxy/model/store/discover.py @@ -171,7 +171,8 @@ class ModelPersistenceContext(metaclass=abc.ABCMeta): if info is not None: primary_data.info = info - if filename: + if filename and not primary_data.id: + # If primary data has an id it should already have gone through the metadata process self.set_datasets_metadata(datasets=[primary_data], datasets_attributes=[dataset_attributes]) return primary_data From 82d66bdd2980e96327a5f3643a505e34a00890d0 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 2 Jun 2021 11:47:31 +0200 Subject: [PATCH 5/8] That's needed for things set via galaxy.json But it does mean that we might be running set_meta twice. Need to figure out a better way of doing this. --- lib/galaxy/model/store/discover.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/galaxy/model/store/discover.py b/lib/galaxy/model/store/discover.py index 5771c200a20..07ea899ce20 100644 --- a/lib/galaxy/model/store/discover.py +++ b/lib/galaxy/model/store/discover.py @@ -171,8 +171,7 @@ class ModelPersistenceContext(metaclass=abc.ABCMeta): if info is not None: primary_data.info = info - if filename and not primary_data.id: - # If primary data has an id it should already have gone through the metadata process + if filename: self.set_datasets_metadata(datasets=[primary_data], datasets_attributes=[dataset_attributes]) return primary_data From 5b5ebf90d5be2cb88ceb1da5c783d1a095db82c1 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 2 Jun 2021 18:34:54 +0200 Subject: [PATCH 6/8] Add API test for fetch_data with bam fails This fails without the preceding commit. --- lib/galaxy_test/api/test_tools_upload.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lib/galaxy_test/api/test_tools_upload.py b/lib/galaxy_test/api/test_tools_upload.py index 824b381361d..4acaac47690 100644 --- a/lib/galaxy_test/api/test_tools_upload.py +++ b/lib/galaxy_test/api/test_tools_upload.py @@ -322,6 +322,28 @@ class ToolsUploadTestCase(ApiTestCase): assert output0["state"] == "ok" assert output1["state"] == "error" + @uses_test_history(require_new=False) + def test_fetch_bam_file_from_url_with_extension_set(self, history_id): + destination = {"type": "hdas"} + targets = [{ + "destination": destination, + "items": [ + { + "src": "url", + "url": "https://raw.githubusercontent.com/galaxyproject/galaxy/dev/test-data/1.bam", + "ext": "bam" + }, + ] + }] + payload = { + "history_id": history_id, + "targets": json.dumps(targets), + } + fetch_response = self.dataset_populator.fetch(payload) + self._assert_status_code_is(fetch_response, 200) + outputs = fetch_response.json()["outputs"] + self.dataset_populator.get_history_dataset_details(history_id, dataset=outputs[0], assert_ok=True) + @skip_without_datatype("velvet") def test_composite_datatype(self): with self.dataset_populator.test_history() as history_id: From c16446b9a313f921092d71c7570f08c524584243 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 2 Jun 2021 18:36:23 +0200 Subject: [PATCH 7/8] Use Job.states.OK --- lib/galaxy/metadata/set_metadata.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/metadata/set_metadata.py b/lib/galaxy/metadata/set_metadata.py index 48010bcd561..dea9b931275 100644 --- a/lib/galaxy/metadata/set_metadata.py +++ b/lib/galaxy/metadata/set_metadata.py @@ -139,7 +139,7 @@ def set_metadata_portable(): version_string = "" export_store = None - final_job_state = 'ok' + final_job_state = Job.states.OK if extended_metadata_collection: tool_dict = metadata_params["tool"] stdio_exit_code_dicts, stdio_regex_dicts = tool_dict["stdio_exit_codes"], tool_dict["stdio_regexes"] From bd5596f3828a419a1c4b44fdc7ea0ba41b813985 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 2 Jun 2021 16:30:58 +0200 Subject: [PATCH 8/8] Maybe skip setting metadata twice --- lib/galaxy/metadata/set_metadata.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/metadata/set_metadata.py b/lib/galaxy/metadata/set_metadata.py index dea9b931275..1d25c32de58 100644 --- a/lib/galaxy/metadata/set_metadata.py +++ b/lib/galaxy/metadata/set_metadata.py @@ -257,7 +257,10 @@ def set_metadata_portable(): setattr(dataset.metadata, metadata_name, metadata_file_override) if output_dict.get("validate", False): set_validated_state(dataset) - set_meta(dataset, file_dict) + if dataset_instance_id not in unnamed_id_to_path: + # We're going to run through set_metadata in collect_dynamic_outputs with more contextual metadata, + # so skip set_meta here. + set_meta(dataset, file_dict) if extended_metadata_collection: meta = tool_provided_metadata.get_dataset_meta(output_name, dataset.dataset.id, dataset.dataset.uuid)