From 710177f3983d5e66b11c329f699079f67667329c Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 14 Dec 2017 17:10:02 +0100 Subject: [PATCH 1/8] Add API tests for history uploads from URL --- test/api/test_tools.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/api/test_tools.py b/test/api/test_tools.py index 8846c4dcbc2..d487d35d907 100644 --- a/test/api/test_tools.py +++ b/test/api/test_tools.py @@ -1366,6 +1366,15 @@ class ToolsTestCase(api.ApiTestCase): } self._check_combined_mapping_and_subcollection_mapping(history_id, inputs) + def test_upload_from_invalid_url(self): + history_id, dataset_id = self._upload_from_url('https://usegalaxy.org/bla123') + datset_details = self.dataset_populator.get_history_dataset_details(history_id, dataset_id=dataset_id, assert_ok=False) + assert datset_details['state'] == 'error', "expected dataset state to be 'error', but got '%s'" % datset_details['state'] + + def test_upload_from_valid_url(self): + history_id, dataset_id = self._upload_from_url('https://usegalaxy.org/api/version') + self.dataset_populator.get_history_dataset_details(history_id, dataset_id=dataset_id, assert_ok=True) + def _check_combined_mapping_and_subcollection_mapping(self, history_id, inputs): self.dataset_populator.wait_for_history(history_id, assert_ok=True) outputs = self._run_and_get_outputs("collection_mixed_param", history_id, inputs) @@ -1407,6 +1416,19 @@ class ToolsTestCase(api.ApiTestCase): else: return create_response + def _upload_from_url(self, url): + inputs = {"dbkey": "?", + "file_type": "auto", + "files_0|type": + "upload_dataset", + "files_0|space_to_tab": '', + "files_0|to_posix_lines": "Yes", + "files_0|url_paste": url} + history_id = self.dataset_populator.new_history() + new_dataset_id = self._run('upload1', history_id=history_id, inputs=inputs).json()['outputs'][0]['id'] + self.dataset_populator.wait_for_history(history_id, assert_ok=False) + return history_id, new_dataset_id + def _upload(self, content, **upload_kwds): history_id = self.dataset_populator.new_history() new_dataset = self.dataset_populator.new_dataset(history_id, content=content, **upload_kwds) From a1f399c451715424dcd8d1bc9d9ec804e44de24d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 14 Dec 2017 16:22:25 +0100 Subject: [PATCH 2/8] Read in stderr form galaxy.json before checking whether stderr and exit code combination should result in a failed job. This should fix https://github.com/galaxyproject/galaxy/issues/5214. --- lib/galaxy/jobs/__init__.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py index bfad1c028cf..3189be4a004 100644 --- a/lib/galaxy/jobs/__init__.py +++ b/lib/galaxy/jobs/__init__.py @@ -1178,13 +1178,22 @@ class JobWrapper(object, HasResourceParameters): # the tasks failed. So include the stderr, stdout, and exit code: return self.fail(job.info, stderr=stderr, stdout=stdout, exit_code=tool_exit_code) + # We collect the stderr from tools that write their stderr to galaxy.json + legacy_tool_provided_metadata = self.get_tool_provided_job_metadata() + extra_stderr = "\n".join([item.get('stderr') for item in legacy_tool_provided_metadata.tool_provided_job_metadata if item.get('stderr')]) + if extra_stderr: + if stderr: + stderr = "%s\n%s" % (stderr, extra_stderr) + else: + stderr = extra_stderr + # Check the tool's stdout, stderr, and exit code for errors, but only # if the job has not already been marked as having an error. # The job's stdout and stderr will be set accordingly. # We set final_job_state to use for dataset management, but *don't* set # job.state until after dataset collection to prevent history issues - if (self.check_tool_output(stdout, stderr, tool_exit_code, job)): + if self.check_tool_output(stdout, stderr, tool_exit_code, job): final_job_state = job.states.OK else: final_job_state = job.states.ERROR From 3d2adcd1f310b94d0d6f6e37e71d1ee74da2fb3d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 14 Dec 2017 17:45:57 +0100 Subject: [PATCH 3/8] Restrict reading tool provided metadata to tools that actually provide metadata --- lib/galaxy/jobs/__init__.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py index 3189be4a004..10fbbcf82df 100644 --- a/lib/galaxy/jobs/__init__.py +++ b/lib/galaxy/jobs/__init__.py @@ -27,6 +27,7 @@ from galaxy.exceptions import ObjectInvalid, ObjectNotFound from galaxy.jobs.actions.post import ActionBox from galaxy.jobs.mapper import JobRunnerMapper from galaxy.jobs.runners import BaseJobRunner, JobState +from galaxy.tools.parameters.output_collect import NullToolProvidedMetadata from galaxy.util import safe_makedirs, unicodify from galaxy.util.bunch import Bunch from galaxy.util.expressions import ExpressionContext @@ -1179,13 +1180,14 @@ class JobWrapper(object, HasResourceParameters): return self.fail(job.info, stderr=stderr, stdout=stdout, exit_code=tool_exit_code) # We collect the stderr from tools that write their stderr to galaxy.json - legacy_tool_provided_metadata = self.get_tool_provided_job_metadata() - extra_stderr = "\n".join([item.get('stderr') for item in legacy_tool_provided_metadata.tool_provided_job_metadata if item.get('stderr')]) - if extra_stderr: - if stderr: - stderr = "%s\n%s" % (stderr, extra_stderr) - else: - stderr = extra_stderr + tool_provided_metadata = self.get_tool_provided_job_metadata() + if not isinstance(tool_provided_metadata, NullToolProvidedMetadata): + extra_stderr = "\n".join([item.get('stderr') for item in tool_provided_metadata.tool_provided_job_metadata if item.get('stderr')]) + if extra_stderr: + if stderr: + stderr = "%s\n%s" % (stderr, extra_stderr) + else: + stderr = extra_stderr # Check the tool's stdout, stderr, and exit code for errors, but only # if the job has not already been marked as having an error. From 62a3ff3562c5b9c534794c2d0bb6320de7747135 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Thu, 14 Dec 2017 18:16:51 +0100 Subject: [PATCH 4/8] Do not assert dataset state in test_gzipped_html_content_blocked_by_default test --- test/integration/test_upload_configuration_options.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/test_upload_configuration_options.py b/test/integration/test_upload_configuration_options.py index 1e88759cefd..dc2d91a60ef 100644 --- a/test/integration/test_upload_configuration_options.py +++ b/test/integration/test_upload_configuration_options.py @@ -82,9 +82,9 @@ class DefaultBinaryContentFiltersTestCase(BaseCheckUploadContentConfigurationTes def test_gzipped_html_content_blocked_by_default(self): dataset = self.dataset_populator.new_dataset( - self.history_id, 'file://%s/bad.html.gz' % TEST_DATA_DIRECTORY, file_type="auto", wait=True + self.history_id, 'file://%s/bad.html.gz' % TEST_DATA_DIRECTORY, file_type="auto", wait=True, assert_ok=False ) - dataset = self.dataset_populator.get_history_dataset_details(self.history_id, dataset=dataset) + dataset = self.dataset_populator.get_history_dataset_details(self.history_id, dataset=dataset, assert_ok=False) assert dataset["file_size"] == 0 From 9ee2a7d4577a715137edcf33fd2b0a474a7a8b8d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 15 Dec 2017 09:44:00 +0100 Subject: [PATCH 5/8] Adjust stderr collection to (Legacy-)ToolProvidedMetadata --- lib/galaxy/jobs/__init__.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py index 10fbbcf82df..745aeb76848 100644 --- a/lib/galaxy/jobs/__init__.py +++ b/lib/galaxy/jobs/__init__.py @@ -27,7 +27,10 @@ from galaxy.exceptions import ObjectInvalid, ObjectNotFound from galaxy.jobs.actions.post import ActionBox from galaxy.jobs.mapper import JobRunnerMapper from galaxy.jobs.runners import BaseJobRunner, JobState -from galaxy.tools.parameters.output_collect import NullToolProvidedMetadata +from galaxy.tools.parameters.output_collect import ( + LegacyToolProvidedMetadata, + ToolProvidedMetadata +) from galaxy.util import safe_makedirs, unicodify from galaxy.util.bunch import Bunch from galaxy.util.expressions import ExpressionContext @@ -1181,13 +1184,17 @@ class JobWrapper(object, HasResourceParameters): # We collect the stderr from tools that write their stderr to galaxy.json tool_provided_metadata = self.get_tool_provided_job_metadata() - if not isinstance(tool_provided_metadata, NullToolProvidedMetadata): - extra_stderr = "\n".join([item.get('stderr') for item in tool_provided_metadata.tool_provided_job_metadata if item.get('stderr')]) - if extra_stderr: - if stderr: - stderr = "%s\n%s" % (stderr, extra_stderr) - else: - stderr = extra_stderr + extra_stderr = '' + if isinstance(tool_provided_metadata, LegacyToolProvidedMetadata): + extra_stderr = [item.get('stderr') for item in tool_provided_metadata.tool_provided_job_metadata if item.get('stderr')] + elif isinstance(tool_provided_metadata, ToolProvidedMetadata): + extra_stderr = [item.get('stderr') for item in tool_provided_metadata.tool_provided_job_metadata.values() if item.get('stderr')] + if extra_stderr: + extra_stderr = "\n".join(extra_stderr) + if stderr: + stderr = "%s\n%s" % (stderr, extra_stderr) + else: + stderr = extra_stderr # Check the tool's stdout, stderr, and exit code for errors, but only # if the job has not already been marked as having an error. From b39772011a717dc7c70f1e05dd581b427e33a521 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 15 Dec 2017 13:36:38 +0100 Subject: [PATCH 6/8] Optionally allow dataset populators to fail --- test/base/populators.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/base/populators.py b/test/base/populators.py index 169b9935cfb..455f2b892d6 100644 --- a/test/base/populators.py +++ b/test/base/populators.py @@ -114,15 +114,15 @@ class BaseDatasetPopulator(object): payload = self.upload_payload(history_id, content, **kwds) run_response = self.tools_post(payload) if wait: - self.wait_for_tool_run(history_id, run_response) + self.wait_for_tool_run(history_id, run_response, kwds.get('assert_ok', True)) return run_response - def wait_for_tool_run(self, history_id, run_response): + def wait_for_tool_run(self, history_id, run_response, assert_ok=True): run = run_response.json() assert run_response.status_code == 200, run job = run["jobs"][0] self.wait_for_job(job["id"]) - self.wait_for_history(history_id, assert_ok=True) + self.wait_for_history(history_id, assert_ok=assert_ok) return run_response def wait_for_history(self, history_id, assert_ok=False, timeout=DEFAULT_TIMEOUT): From 121285b40be5d7233af33b40045a9c2ffb5a9c46 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 15 Dec 2017 08:34:11 -0500 Subject: [PATCH 7/8] Let ToolProvidedMetadata interface more directly decide if it has failed outputs. I like this better for three reasons: - Since usually it is scripts producing this JSON - we have the most control at that point for determining the failure and we don't have to deal with an artificial dependency between the tool's stdio and the output. - At some point we could potentially allow some datasets to be ok now even though the job fails. - It is a cleaner interface at the Python level between job finish and output collection IMO (no need for isinstance checking). --- lib/galaxy/jobs/__init__.py | 17 +---------------- lib/galaxy/tools/parameters/output_collect.py | 19 +++++++++++++++++++ tools/data_source/upload.py | 3 ++- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/lib/galaxy/jobs/__init__.py b/lib/galaxy/jobs/__init__.py index 745aeb76848..420f29d4d52 100644 --- a/lib/galaxy/jobs/__init__.py +++ b/lib/galaxy/jobs/__init__.py @@ -27,10 +27,6 @@ from galaxy.exceptions import ObjectInvalid, ObjectNotFound from galaxy.jobs.actions.post import ActionBox from galaxy.jobs.mapper import JobRunnerMapper from galaxy.jobs.runners import BaseJobRunner, JobState -from galaxy.tools.parameters.output_collect import ( - LegacyToolProvidedMetadata, - ToolProvidedMetadata -) from galaxy.util import safe_makedirs, unicodify from galaxy.util.bunch import Bunch from galaxy.util.expressions import ExpressionContext @@ -1184,17 +1180,6 @@ class JobWrapper(object, HasResourceParameters): # We collect the stderr from tools that write their stderr to galaxy.json tool_provided_metadata = self.get_tool_provided_job_metadata() - extra_stderr = '' - if isinstance(tool_provided_metadata, LegacyToolProvidedMetadata): - extra_stderr = [item.get('stderr') for item in tool_provided_metadata.tool_provided_job_metadata if item.get('stderr')] - elif isinstance(tool_provided_metadata, ToolProvidedMetadata): - extra_stderr = [item.get('stderr') for item in tool_provided_metadata.tool_provided_job_metadata.values() if item.get('stderr')] - if extra_stderr: - extra_stderr = "\n".join(extra_stderr) - if stderr: - stderr = "%s\n%s" % (stderr, extra_stderr) - else: - stderr = extra_stderr # Check the tool's stdout, stderr, and exit code for errors, but only # if the job has not already been marked as having an error. @@ -1202,7 +1187,7 @@ class JobWrapper(object, HasResourceParameters): # We set final_job_state to use for dataset management, but *don't* set # job.state until after dataset collection to prevent history issues - if self.check_tool_output(stdout, stderr, tool_exit_code, job): + if self.check_tool_output(stdout, stderr, tool_exit_code, job) and not tool_provided_metadata.has_failed_outputs(): final_job_state = job.states.OK else: final_job_state = job.states.ERROR diff --git a/lib/galaxy/tools/parameters/output_collect.py b/lib/galaxy/tools/parameters/output_collect.py index cefa90132e9..98c628f67f0 100644 --- a/lib/galaxy/tools/parameters/output_collect.py +++ b/lib/galaxy/tools/parameters/output_collect.py @@ -32,6 +32,9 @@ class NullToolProvidedMetadata(object): def get_new_dataset_meta_by_basename(self, output_name, basename): return {} + def has_failed_outputs(self): + return False + class LegacyToolProvidedMetadata(object): @@ -74,6 +77,14 @@ class LegacyToolProvidedMetadata(object): log.warning("Called get_new_datasets with legacy tool metadata provider - that is unimplemented.") return [] + def has_failed_outputs(self): + found_failed = False + for meta in self.tool_provided_job_metadata: + if meta.get("failed", False): + found_failed = True + + return found_failed + class ToolProvidedMetadata(object): @@ -112,6 +123,14 @@ class ToolProvidedMetadata(object): extra_kwds.update(element) yield extra_kwds + def has_failed_outputs(self): + found_failed = False + for meta in self.tool_provided_job_metadata.values(): + if meta.get("failed", False): + found_failed = True + + return found_failed + def collect_dynamic_collections( tool, diff --git a/tools/data_source/upload.py b/tools/data_source/upload.py index 3011e5f9967..92dd6afd7c1 100644 --- a/tools/data_source/upload.py +++ b/tools/data_source/upload.py @@ -43,7 +43,8 @@ def file_err(msg, dataset, json_file): json_file.write(dumps(dict(type='dataset', ext='data', dataset_id=dataset.dataset_id, - stderr=msg)) + "\n") + stderr=msg, + failed=True)) + "\n") # never remove a server-side upload if dataset.type in ('server_dir', 'path_paste'): return From 3f6c45578e2c4e99fa725054c829e03a9c409a1b Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 15 Dec 2017 15:49:51 +0100 Subject: [PATCH 8/8] Fix dataset spelling (thx @nsoranzo) --- test/api/test_tools.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/api/test_tools.py b/test/api/test_tools.py index d487d35d907..4ef635860b6 100644 --- a/test/api/test_tools.py +++ b/test/api/test_tools.py @@ -1368,8 +1368,8 @@ class ToolsTestCase(api.ApiTestCase): def test_upload_from_invalid_url(self): history_id, dataset_id = self._upload_from_url('https://usegalaxy.org/bla123') - datset_details = self.dataset_populator.get_history_dataset_details(history_id, dataset_id=dataset_id, assert_ok=False) - assert datset_details['state'] == 'error', "expected dataset state to be 'error', but got '%s'" % datset_details['state'] + dataset_details = self.dataset_populator.get_history_dataset_details(history_id, dataset_id=dataset_id, assert_ok=False) + assert dataset_details['state'] == 'error', "expected dataset state to be 'error', but got '%s'" % dataset_details['state'] def test_upload_from_valid_url(self): history_id, dataset_id = self._upload_from_url('https://usegalaxy.org/api/version')