Add fastp tool to IUC #1763

mblue9 · 2018-03-07T21:31:03Z

fastp - a tool for fastq processing, trimming, QC etc (website: https://github.com/OpenGene/fastp)
has been wrapped by @HassanAmr (hooray! 🎉) bgruening/galaxytools#691
and this PR is to move it to IUC

bgruening · 2018-03-07T21:54:53Z

tools/fastp/.shed.yml

+  A tool designed to provide fast all-in-one preprocessing for FastQ files. This tool is developed in C++ with multithreading supported to afford high performance.
+remote_repository_url: https://github.com/bgruening/galaxytools/tree/master/tools/fastp
+name: fastp
+owner: rnateam


bgruening · 2018-03-08T07:10:53Z

@nsoranzo I increased the Galaxy version against which we are testing here. I don't know why this matters for the test but it worked in galaxytools since a few days and there we are testing against 18.01.

Can we merge it with this change, as 18.01 will be out "soon"?

nsoranzo · 2018-03-08T11:04:55Z

@bgruening I re-ran the last Travis build using 17.09 (i.e. the one for commit e8a18b1) and it passed this time, but I see no problem running tests on 18.01 at this point of the release cycle.

bgruening · 2018-03-08T11:13:13Z

@nsoranzo I'm also not sure, it passed for me as well locally, but travis failed multiple times. Please feel free to merge this. @mblue9 would be happy to use it :)

bgruening · 2018-03-08T11:13:22Z

Danke Nicola!

nsoranzo · 2018-03-08T13:40:53Z

tools/fastp/.shed.yml

+homepage_url: https://github.com/OpenGene/fastp
+long_description: |
+  A tool designed to provide fast all-in-one preprocessing for FastQ files. This tool is developed in C++ with multithreading supported to afford high performance.
+remote_repository_url: https://github.com/bgruening/galaxytools/tree/master/tools/fastp


https://github.com/galaxyproject/tools-iuc/tree/master/tools/fastp

nsoranzo · 2018-03-08T13:41:42Z

tools/fastp/.shed.yml

+name: fastp
+owner: iuc
+type: unrestricted
+


Remove empty line

nsoranzo · 2018-03-08T13:42:46Z

tools/fastp/fastp.xml

@@ -0,0 +1,357 @@
+<tool id="fastp" name="fastp" version="0.12.4">
+    <description>Fast all-in-one preprocessing for FastQ files</description>


s/FastQ/FASTQ/

nsoranzo · 2018-03-08T15:39:00Z

tools/fastp/fastp.xml

+    <requirements>
+        <requirement type="package" version="0.12.4">fastp</requirement>
+    </requirements>
+    <version_command>fastp --version | tail -1</version_command>


tail -n 1, -1 is not POSIX

nsoranzo · 2018-03-08T15:39:22Z

tools/fastp/fastp.xml

+    </requirements>
+    <version_command>fastp --version | tail -1</version_command>
+    <command><![CDATA[
+


Remove empty line

nsoranzo · 2018-03-08T15:56:41Z

tools/fastp/fastp.xml

+    </tests> 
+    <help>
+<![CDATA[
+A tool designed to provide fast all-in-one preprocessing for FastQ files. This tool is developed in C++ with multithreading supported to afford high performance.


s/FastQ/FASTQ/

nsoranzo · 2018-03-08T15:58:11Z

tools/fastp/fastp.xml

+
+10. support long reads (data from PacBio / Nanopore devices).
+
+This tool is being intensively developed, and new features can be implemented soon if they are considered useful. If you have any additional requirement for fastp, please file an issue:https://github.com/OpenGene/fastp/issues/new


I'd remove this line.

nsoranzo · 2018-03-08T16:01:40Z

tools/fastp/fastp.xml

+<![CDATA[
+A tool designed to provide fast all-in-one preprocessing for FastQ files. This tool is developed in C++ with multithreading supported to afford high performance.
+
+**features**


Capital F

nsoranzo · 2018-03-08T16:03:48Z

tools/fastp/.shed.yml

@@ -0,0 +1,11 @@
+categories:
+- Sequence Analysis
+description: Fast all-in-one preprocessing for FastQ files


s/FastQ/FASTQ/

nsoranzo · 2018-03-08T16:03:53Z

tools/fastp/.shed.yml

+description: Fast all-in-one preprocessing for FastQ files
+homepage_url: https://github.com/OpenGene/fastp
+long_description: |
+  A tool designed to provide fast all-in-one preprocessing for FastQ files. This tool is developed in C++ with multithreading supported to afford high performance.


s/FastQ/FASTQ/

mblue9 · 2018-03-08T21:20:01Z

Thanks for the review @nsoranzo ! I've made those changes.

mvdbeek · 2018-03-09T10:32:51Z

tools/fastp/fastp.xml

+                </section>
+            </when>
+            <when value="paired">
+                <param name="in1" argument="-i" type="data" format="fastq" label="Input 1" help="Input FASTQ file #1."/>


we should be able to add fastq.gz here and for the other inputs, right ?

Well I wondered that and so tested it with a fastq.gz file and it ran fine so it didn't seem necessary to add 😕. But what do you think? Should I instead add all possibilities- fastq, fastq.gz, fastqsanger, fastqsanger.gz?

fastqsanger, fastqillumina etc are all subclasses of fastq, so that's fine.
fastqsanger.gz, fastqillumina.gz etc, are subclasses of fastq.gz, and should normally require implicit conversion to their uncompressed equivalent if you want to use them with a fastq input.
Is that perhaps why it worked for you ? The disadvantage of not specifying fastq.gz is the additional, unnecessary uncompressed dataset that would be created.

mvdbeek · 2018-03-09T10:34:50Z

tools/fastp/fastp.xml

+    </inputs>
+
+    <outputs>
+        <data name="out1" format="fastq" label="${tool.name} on ${on_string}: Read 1 Output"/>


can we make this format_source="in1" (and below) ?

mvdbeek · 2018-03-09T12:29:46Z

tools/fastp/fastp.xml

+        <test>
+            <param name="in1" value="R1.fq"/>
+            <param name="single_paired_selector" value="single"/>
+            <output name="out1" file="out1.fq" ftype="fastq"/>


this would be fastqsanger now, unless you change <param name="in1" value="R1.fq"/> to <param name="in1" value="R1.fq" ftype="fastq"/>

nsoranzo · 2018-03-09T12:41:27Z

tools/fastp/fastp.xml

+
+#if $single_paired.adapter_trimming_options.disable_adapter_trimming:
+    $single_paired.adapter_trimming_options.disable_adapter_trimming
+#end if


The #if and #end if are not needed since you're defining truevalue and falsevalue in the <param>. Same for most other boolean params below.

mblue9 · 2018-03-09T13:24:10Z

Thanks @mvdbeek and @nsoranzo I've made those changes

nsoranzo · 2018-03-09T14:19:01Z

@mblue9 Thanks a lot for the changes! There are still 3 standing comments from my previous review at https://github.com/galaxyproject/tools-iuc/pull/1763/files#diff-12b0b82edec68d4ab4e953daf7087da6 , if you have time.

nsoranzo · 2018-03-09T19:00:34Z

@bgruening The merge of master didn't work well, the new tools in tools/charts/ appear now in the diff.

bgruening · 2018-03-09T19:04:29Z

@nsoranzo on it, one commit is also missing. Please review in 10min and merge :)
@mblue9 needs it when she wakes up :)

bgruening · 2018-03-09T19:10:54Z

So done.

nsoranzo · 2018-03-09T19:16:52Z

tools/fastp/fastp.xml

+
+fastp
+
+-i input1.${in1.ext}


Just a doubt: does fastp support all our weird ext, like fastqillumina.gz or fastqsolexa, which are all subtypes of fastq/fastq.gz?

Most likely not and if we want to support this we porbably need to add --phred64 as well.

nsoranzo · 2018-03-09T20:00:35Z

tools/fastp/fastp.xml

+    #set $ext = 'fastq'
+#else:
+    #set $ext = $in1.ext
+#end if


I think this should be:

#if $in1.is_of_type('fastq.gz') #set ext = 'fastq.gz' #else #set ext = 'fastq' #end if

nsoranzo · 2018-03-09T20:03:10Z

tools/fastp/fastp.xml

 #end if


 ## Run fastp

 fastp

-i input1.${in1.ext}
-o first.${in1.ext}
+#if $in1.ext in ['fastqillumina', 'fastqsolexa', 'fastqillumina.gz', 'fastqsolexa.gz']:


#if $in1.is_of_type('fastqillumina', 'fastqsolexa', 'fastqillumina.gz', 'fastqsolexa.gz')

mblue9 · 2018-03-09T23:35:23Z

Sorry @nsoranzo I missed those other changes! I've added your suggestions now and if you think I should make more changes please let me know.

mblue9 · 2018-03-10T00:07:06Z

:( I don't know why it's failing here on the 9th test with "No output has been received in the last 20m0s", it passes fine locally 😕

nsoranzo · 2018-03-10T01:02:42Z

It's green after a restart!

Thanks @mblue9 and @bgruening!

mblue9 · 2018-03-10T05:09:24Z

Thanks a lot @nsoranzo ! I'm trying it now and it's looking good! 👍

bgruening reviewed Mar 7, 2018

View reviewed changes

nsoranzo reviewed Mar 8, 2018

View reviewed changes

mvdbeek reviewed Mar 9, 2018

View reviewed changes

nsoranzo reviewed Mar 9, 2018

View reviewed changes

mblue9 and others added 11 commits March 9, 2018 20:02

Add fastp to IUC

f9a5446

Update .shed.yml

300dead

use latest version

4531193

Update test 2 output to see if Travis test will pass

281d4a1

what is going on here?

0459a42

Fixes from @nsoranzo review

c15cdde

Use format_source for outputs

8a584d8

fix tests

83718d8

Fixes from review

0165608

fix review comments include gz test, add macros

67f1187

add sim_size

53cd20a

bgruening added 2 commits March 9, 2018 20:07

cleaner cheetah syntax

42faa48

quote output files

ac4c0d9

bgruening force-pushed the fastp branch from 2563ae3 to ac4c0d9 Compare March 9, 2018 19:10

nsoranzo reviewed Mar 9, 2018

View reviewed changes

add phred64, enhance the ext handling a little bit

c6e14f4

nsoranzo reviewed Mar 9, 2018

View reviewed changes

More fixes from @nsoranzo review

a9b4c3f

nsoranzo merged commit 2e41f5d into galaxyproject:master Mar 10, 2018

This was referenced Mar 10, 2018

Module request: fastp MultiQC/MultiQC#669

Closed

Galaxy wrapper & commit to Tool Shed OpenGene/fastp#29

Closed

mblue9 deleted the fastp branch March 10, 2018 20:49

		@@ -0,0 +1,357 @@
		<tool id="fastp" name="fastp" version="0.12.4">
		<description>Fast all-in-one preprocessing for FastQ files</description>


		10. support long reads (data from PacBio / Nanopore devices).

		This tool is being intensively developed, and new features can be implemented soon if they are considered useful. If you have any additional requirement for fastp, please file an issue:https://github.com/OpenGene/fastp/issues/new

Add fastp tool to IUC #1763

Add fastp tool to IUC #1763

Conversation

mblue9 commented Mar 7, 2018

Choose a reason for hiding this comment

bgruening commented Mar 8, 2018

nsoranzo commented Mar 8, 2018

bgruening commented Mar 8, 2018

bgruening commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mblue9 commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mblue9 commented Mar 9, 2018

nsoranzo commented Mar 9, 2018

nsoranzo commented Mar 9, 2018

bgruening commented Mar 9, 2018

bgruening commented Mar 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mblue9 commented Mar 9, 2018

mblue9 commented Mar 10, 2018

nsoranzo commented Mar 10, 2018

mblue9 commented Mar 10, 2018