Skip to content

Implement detect_errors attribute on command of tool XML.#117

Merged
natefoo merged 1 commit intogalaxyproject:devfrom
jmchilton:detect_errors
Apr 15, 2015
Merged

Implement detect_errors attribute on command of tool XML.#117
natefoo merged 1 commit intogalaxyproject:devfrom
jmchilton:detect_errors

Conversation

@jmchilton
Copy link
Member

If present, it can be one of

  • default, no-op fallback to stdio tags and erroring on standard error output.
  • exit_code, error if tool exit code is not 0. (The @jmchilton recommendation).
  • aggressive, error if tool exit code is not 0 or either Exception: or Error: appears in standard error/output. (The @bgruening recommendation).

The stdio tag is a very heavy solution when many (most?) tools use either this exit_code or aggressive strategy leading to a lot of duplicated XML - and if there is one thing I like less than XML it is duplicated XML.

Refactoring and unit/functional tests to support and demonstrate this.

Run relevant functional tests with:

./run_tests.sh -framework -id detect_errors_aggressive

Run relevant unit tests:

nosetests test/unit/tools/test_parsing.py

@jmchilton jmchilton changed the title Implement detect_errors attribute on XML. Implement detect_errors attribute on tool XML. Apr 14, 2015
@nsoranzo
Copy link
Member

What about making it an attribute of <command> instead of <tool>, since there can be only 1 command?

@jmchilton
Copy link
Member Author

@nsoranzo Is there a reason you don't like <tool>? <tool>seems to be more consistent with <stdio> tag which is a child of <tool> and not <command>?

@hexylena
Copy link
Member

@jmchilton IMO how exits are handled aren't tool metadata, they're command metadata. my gut prefers @nsoranzo's suggestion.

@peterjc
Copy link
Contributor

peterjc commented Apr 14, 2015

Aggressive should probably allow case variants like ERROR as well as Error.

@natefoo
Copy link
Member

natefoo commented Apr 14, 2015

FWIW, there can only be one command tag now, but it'd be handy to be able to be able to define more in the future for things like map/reduce-type workflows in a single tool, or embarrassingly parallel steps in between serial steps.

@blankenberg
Copy link
Member

+1 to the idea of potentially allowing multiple command tags. It might be nice to be able to put different failure modes on each command execution though.

@jmchilton
Copy link
Member Author

@peterjc I consider aggressive to be @bgruening's baby so I will let him weigh in on that modification. Whatever regex you guys decide is fine - but then we have to stick with it I think (post-release). We can always add other approaches though sorta_aggressive, very_aggressive, check_java (looks for standard Java style error logs and stack traces), check_python (checks for python tracebacks), etc... .

@blankenberg, @natefoo so I am going to assume you guys are siding with @erasche and @nsoranzo that the detect_errors should be on command then. If no one is with me that it is inconsistent with stdio I will make this modification.

@bgruening
Copy link
Member

@jmchilton I'm fine with @peterjc suggestion to make it case insensitive.
For the other question I'm on your side this is inconsistent, but this this does not mean it's correct in first place. If we are really moving to multiple commands, a great thing to do, we should start now to put this as attribute to the command tag.

@jmchilton
Copy link
Member Author

Okay - I will make these two modifications then.

Thanks for the input all!

If present, it can be one of

 - "default", no-op fallback to stdio tags and erroring on standard error output.
 - "exit_code", error if tool exit code is not 0. (The @jmchilton recommendation).
 - "aggressive", error if tool exit code is not 0 or either Exception: or Error: appears in standard error/output. (The @bgruening recommendation).

Refactoring and unit/functional tests to support and demonstrate this.

Run functional test with:

    ./run_tests.sh -framework -id detect_errors_aggressive

Run relevant unit tests:

    nosetests test/unit/tools/test_parsing.py

Updated from original version to reflect comments on pull request galaxyproject#117 - in particular the ``detect_errors`` tag was moved from ``tool`` to ``command``.
@jmchilton
Copy link
Member Author

Okay - rebased and moved the tag and learned that these expressions have been case insensitive all along (though I made them lower case and added a comment to reflect this).

natefoo added a commit that referenced this pull request Apr 15, 2015
Implement ``detect_errors`` attribute on tool XML.
@natefoo natefoo merged commit a4e4e59 into galaxyproject:dev Apr 15, 2015
@nsoranzo
Copy link
Member

I think the title of this PR should be renamed s/tool/command/ for people looking at it in the future.

@jmchilton jmchilton changed the title Implement detect_errors attribute on tool XML. Implement detect_errors attribute on commamd of tool XML. Apr 17, 2015
@jmchilton
Copy link
Member Author

Done.

@peterjc
Copy link
Contributor

peterjc commented Apr 17, 2015

Other than the typo, commamd [sic] --> command :(

@jmchilton jmchilton changed the title Implement detect_errors attribute on commamd of tool XML. Implement detect_errors attribute on command of tool XML. Apr 17, 2015
@jmchilton jmchilton deleted the detect_errors branch November 25, 2015 15:06
mvdbeek pushed a commit to mvdbeek/galaxy that referenced this pull request Jan 24, 2017
mvdbeek pushed a commit to mvdbeek/galaxy that referenced this pull request Jan 24, 2017
mvdbeek pushed a commit to mvdbeek/galaxy that referenced this pull request Jan 24, 2017
mvdbeek pushed a commit to mvdbeek/galaxy that referenced this pull request Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants