Skip to content
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

Tool Documentation and Testing Improvements #3457

Merged
merged 4 commits into from Jan 20, 2017

Conversation

Projects
None yet
5 participants
@jmchilton
Copy link
Member

commented Jan 20, 2017

  • Add/fix XSD documentation for shared_inputs on parallelism tag.
  • Add tool demonstrating and testing <conversion> tag on data inputs.
  • Add/fix XSD documentation for <conversion> tag on data inputs.

Fixes #3453.
Fixes #3303.

</xs:attribute>
<xs:attribute name="type" type="xs:string">
<xs:annotation>
<xs:documentation xml:lang="en">The short extension describing the datatype to convert to - Galaxy must be populate with a datatype converter from the parent input's type to this.</xs:documentation>

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jan 20, 2017

Member

s/must be populate with/must be populated with/

Or even a simpler must have.

<!-- Test implicit conversion. -->
<test>
<param name="input1" value="1.fasta" ftype="fasta" />
<param name="col" value="1" />

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jan 20, 2017

Member

This line is not needed.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Jan 20, 2017

Author Member

Indeed - rebased with this change and also remove the line in the file this was copied and pasted from.

@jmchilton jmchilton force-pushed the jmchilton:xsd_fixes_2 branch from beee41d to 095ff4f Jan 20, 2017

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

Thanks a bunch!

Fix tool XSD to document <conversion> tag for data parameters.
Rebased with English fixes and small tool cleanups thanks to @nsoranzo.

@jmchilton jmchilton force-pushed the jmchilton:xsd_fixes_2 branch from 095ff4f to 074e10a Jan 20, 2017

@@ -0,0 +1,23 @@
<tool id="explicit_conversion" name="explicit_conversion">
<command>
cut -f 1 '${input1_table}' > 'col_output'

This comment has been minimized.

Copy link
@bgruening

bgruening Jan 20, 2017

Member

Why not piping to '${output1}' and removing from_work_dir?

This comment has been minimized.

Copy link
@jmchilton

jmchilton Jan 20, 2017

Author Member

Is there a preference toward not using from_work_dir? I don't really know which I find better - and I think this tool was just copied from implicit_conversion.xml which was copied from column_param.xml.

This comment has been minimized.

Copy link
@bgruening

bgruening Jan 20, 2017

Member

I would argue that the from_work_dir needs an additional copy/move operation.
Not really needed for this PR, but I start to use this test more and more in workshops to show people examples ;)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Jan 20, 2017

Author Member

I made the change but I'm not sure I like it. I get the extra copy is sort of the "default" behavior. But I'm not sure it should be thought of that way - when using various object stores or Pulsar or Docker or "run as real user" or that workflow language you hate the way outputs are copied around are subtlety different. I'd imagine one day all outputs are copied the way from_work_dir is - it is just too insecure and error prone to have every worker node writing the global object store. I feel like conceptual simplicity should be deciding factor - and there I think that from_work_dir might be easier to think or more pure since it involves less templating. This isn't clear cut though.

This comment has been minimized.

Copy link
@bgruening

bgruening Jan 20, 2017

Member

I don't hate it, don't say this. I just don't agree with how things went, this is a difference.
I can not judge about the technical difficulties about writing directly to the object, but the way it currently is with $output, fits to how we treat $inputs. If we want to do the shift to from_work_dir as recommended way, we should discuss this elsewhere and then also consider to no remove the name attribute from outputs - otherwise this is really confusing.

@martenson martenson merged commit 3945f3b into galaxyproject:dev Jan 20, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 256 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 135 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.