[WIP] Tool linting fixes and proper exit_code detection

[hexylena]

I'm linting all of the tools in the tool directory with planemo and identifying and fixing issues. I'm opening this PR very early into the process so I can track some discussion questions.

The primary purpose of the PR is to use #117 throughout so tools don't die on any stderr, as that's really poor behaviour. This is specifically in response to a UserWarning showing up during upload, a warning which causes the job to unnecessarily error.

• can we nix import.py or replace it with a cp command?
• removed echo.py
• there are at least a couple of tools with the same ID
  • ucsc_proxy for ucsc_archaea.xml and ucsc_proxy.xml
  • Grep1 for grep.xml and fileGrep.xml
• access_libraries.xml? Is this still a thing? It seems like an API call, but no idea if it's even functional.
• Making use of Implement detect_errors attribute on command of tool XML. #117
• liftOver_wrapper has disabled test cases. 77b167b
• Historical behaviour question on this, does format="input" imply format_source="first_input_dataset"?
• how do we feel about test-data again? Some tools are easy to add tests for (e.g. commWrapper.xml/pl)
• should joiner2.xml be removed? when joiner.xml exists?
• command line parameters in maf_* were re-arranged as part of the rewrite to de-duplicate CLI strings.
• is this intentional?
• could someone provide documentation for https://github.com/galaxyproject/galaxy/blob/dev/tools/validation/fix_errors.xml ?

Related TODOs

• linting misses detect_errors on commands. planemo#233
• Linting rules for tool_type="data_source" planemo#232
• Linting fails for hapmapmart.xml in tools/data_source/ planemo#231

[jmchilton]

I don't know if it would work but I feel like the data source tools should essentially all have citations. The data source tools should assign a tool id to the outputs and those ids will resolve to tools in the toolbox - so I feel like say building a citation list for a history should work for these outputs. At very least I wouldn't add empty citation tags to please planemo thereby making it more likely we would forget to add citations and make them work in the future.

[jmchilton]

I do like adding help text though - because this will probably come through in the API for instance.

[nsoranzo]

is this intentional?

I think so, the ' is opened at https://github.com/galaxyproject/galaxy/blob/dev/tools/meme/fimo.xml#L4 .

[hexylena]

@nsoranzo great, thanks. I couldn't seem to see where that was.

@jmchilton absolutely makes sense. Should they cite the "Blankenberg et al. in preparation" or whatever that citation was that got applied to so many tools?

[martenson]

this is great, especially proper exit code detection is a tremendous improvement

[jmchilton]

10.1093/bioinformatics/bts286

[hexylena]

@jmchilton amazing, doing my citation lookup work for me, much appreciated!!

[jmchilton]

No - we are both doing @blankenberg's work for him :) - what do we get out of this.

There are a ton though - so unless you want to track everyone down - I would just eliminate the empty citations blocks and run planemo with that check excluded. We can come back to this some other day. If you are in the mood to track them all down though - I won't complain :).

[hexylena]

Agreed, I'll remove the citation blocks so we can keep generating warnings until that date

(I had hoped that we'd be able to add planemo linting to the travis job, but without citations for everything, without test cases for everything, there's no sense in turning linting on)

[jmchilton]

I understand the desire to add those checks to travis - though I don't think we will be modifying these tools frequently so it is probably better to spend our Travis time on something like including some integration testing or API tests.

[hexylena]

Eh, fair point. I just want to test ALL the things :P

[jmchilton]

Historical behaviour question on this, does format="input" imply format_source="first_input_dataset"?

If I recall correctly, I actually think format="input" implies format_source="random_input_dataset" - since this stuff is set by iterating over dict and just keeping the variable set to the last element encountered. This is why planemo discourages it.

[hexylena]

Gotcha, that was the assumption I was working under. Thanks for the confirmation.

I replaced a lot of format and format_source with fixed datatypes, because most of the time that was used, the input was hardcoded to tabular.

[jmchilton]

I want to test all the things also - but IMO travis is only a place to test the most usefully and quickly tested things.

[hexylena]

Yep, fair enough. Wish travis had a better concept of "what's changed since last time", and it didn't require ugly hacks in Jenkins to do those sort of tests.

[jmchilton]

how do we feel about test-data again? Some tools are easy to add tests for (e.g. commWrapper.xml/pl)

Would love to see a tests added - just commit the test-data to https://github.com/galaxyproject/galaxy-test-data instead of here. It will be fetched dynamically as needed by the test framework.

[jmchilton]

I replaced a lot of format and format_source with fixed datatypes, because most of the time that was used, the input was hardcoded to tabular.

That is probably a mistake - many tabular tools when operating a bed file for instance should produce another bed file. The inputs are polymorphic - and so the output datatypes should be inherited instead of hardcoded.

[hexylena]

.... eww. That's...wow, okay. I didn't remember that bed datatypes subclassed tabular and could be accepted as inputs there. The rest of the format tree is so flat...

[hexylena]

Note to self from #524:

Note separately that if the first dataset is considered as a bed file, then the output file is also considered bed, and this appears to wrongly mark column c6 (description) as strand.

So whenever I go back to refactor the datatypes I mistakenly hardcoded to tabular, there are cases when format_source isn't appropriate.

[jmchilton]

New oldest PR, I'm thinking i going to try to unwind into smaller PRs and then convert what is left into an issue - is that okay @erasche or were you super eager to return to this mega effort?

[jmchilton]

xref #846

[hexylena]

@jmchilton you are absolutely welcome to. I may likewise try and find time to unwind a task or two out of this. Thanks for reviving this zombie :)

[jmchilton]

Okay - I have transitioned all my DOI comments into PRs. Maybe it would be best to just remove all the empty citations blocks in this PR.

[hexylena]

@jmchilton it may be best to just kill this PR and use the smaller ones you've been opening. This PR did a couple things very wrong (i.e. removing format_from statements)

[jmchilton]

@erasche - there is good stuff in there, but yeah I agree lets just kill it for now and try to pull out the good stuff in small doses going forward.