New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[17.09] Fail job if tools that use galaxy.json write to stderr #5217

Merged
merged 9 commits into from Dec 15, 2017

Conversation

Projects
None yet
3 participants
@mvdbeek
Member

mvdbeek commented Dec 14, 2017

This should fix #5214.

mvdbeek added some commits Dec 14, 2017

Read in stderr form galaxy.json
before checking whether stderr and exit code combination should result in a
failed job. This should fix #5214.
@mvdbeek

This comment has been minimized.

Member

mvdbeek commented Dec 14, 2017

@@ -1407,6 +1416,19 @@ def _run(self, tool_id, history_id, inputs, assert_ok=False, tool_version=None):
else:
return create_response
def _upload_from_url(self, url):

This comment has been minimized.

@jmchilton

jmchilton Dec 14, 2017

Member

In this same directory there is a test_tools_upload.py test case filled with tons of upload tests - can you move these there?

This comment has been minimized.

@mvdbeek

mvdbeek Dec 15, 2017

Member

Hmm, that's only in dev. Should I create the file ?

This comment has been minimized.

@jmchilton

jmchilton Dec 15, 2017

Member

Doh - good point. Lets keep it as is - I'll try to remember to move things around after we merge this forward.

)
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)

This comment has been minimized.

@jmchilton

jmchilton Dec 14, 2017

Member

Yes - this is good. Thanks!

mvdbeek and others added some commits Dec 15, 2017

Let ToolProvidedMetadata interface more directly decide if it has fai…
…led 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).
@jmchilton

This comment has been minimized.

Member

jmchilton commented Dec 15, 2017

Any interest in rearranging this so the tool directly just declares the output in question is in error? (mvdbeek#6).

Merge pull request #6 from jmchilton/fail_if_galaxy_json_tools_write_…
…to_stderr

Let ToolProvidedMetadata interface more directly decide if it has failed outputs.
@@ -1366,6 +1366,15 @@ def test_combined_mapping_and_subcollection_mapping(self):
}
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)

This comment has been minimized.

@nsoranzo

nsoranzo Dec 15, 2017

Member

s/datset_details/dataset_details/

Also on the line below.

@jmchilton jmchilton merged commit 44635ea into galaxyproject:release_17.09 Dec 15, 2017

4 of 6 checks passed

api test Test started.
Details
framework test Test started.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
integration test Build finished. 46 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. No test results found.
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@nsoranzo nsoranzo added this to the 18.01 milestone Dec 15, 2017

@galaxyproject galaxyproject deleted a comment from galaxybot Dec 15, 2017

@jmchilton jmchilton referenced this pull request Dec 15, 2017

Merged

Merge 17.09. #5227

@mvdbeek mvdbeek deleted the mvdbeek:fail_if_galaxy_json_tools_write_to_stderr branch Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment