bowtie2: do not overcommit #5985

bernt-matthias · 2024-05-03T16:02:36Z

by using a pipe bowtie2 and samtools run in parallel. since already bowtie2 alone uses more CPU than the assigned ones (less than 1 core) we should not do this, but run them serially.
background had a user running 2 fastq files (each 2.6GB) against a file containing 500k contigs. Running bowtie there was extremely efficient (tested up to 40 cores), i.e. for GALAXY_SLOTS=X the CPU usage (according to top) was (X+1)*100% .. probably because X alignment + 1 control thread are used.

Alternatively we could do this, but I do not see a reason why we should.

also properly redirects stderr.

FOR CONTRIBUTOR:

I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
License permits unrestricted use (educational + commercial)
This PR adds a new tool or tool collection
This PR updates an existing tool or tool collection
This PR does something else (explain below)

fixes galaxyproject#5983 by using a pipe bowtie2 and samtools run in parallel. since already bowtie2 alone uses more CPU than the assigned ones (less that 1 core) we should not do this, but run them serially. also properly redirects stderr.

bwlang · 2024-05-05T12:19:30Z

One problem with the non piping is that you have to write to the disk one more time... that can be very expensive for some environments. Maybe there is a way to partition the thread usage so that samtools can use some of the available resources.

bwlang · 2024-05-05T18:48:35Z

tools/bowtie2/bowtie2_wrapper.xml

 #end if

 ## output file
 #if str( $sam_options.sam_options_selector ) == "no" or (str( $sam_options.sam_opt ) == "false" and str($sam_options.reorder) == ''):
-    | samtools sort --no-PG -@\${GALAXY_SLOTS:-2} -T "\${TMPDIR:-.}" -O bam -o '$output'
+    > alignment.sam
+    && samtools sort --no-PG -@\${GALAXY_SLOTS:-2} -T "\${TMPDIR:-.}" -O bam -o '$output' alignment.sam


why no-PG... i think it's good to have the headers that describe the tools that worked on a bam in place.

On the other hand we record everything in Galaxy. So one could say it's redundant.

Also it makes the tests more "unstable", i.e. more lines that may differ due to version changes.

I really don't care. But we should make this an IUC guideline My impression was that we added --no-PG to quite a few tools in the past and we should not go back and forth.

i see that complexity for testing, but i think that bams produced by galaxy do not only stay within galaxy.
E.g. they get submitted to SRA as data associated with scientific publications.

Yes, we should come to an IUC-wide decision with this, and I would go with @bwlang's arguments for recording everything in the header.

One case that comes to my mind immediately where --no-PG seems justified to me is with the reheader tool itself because you use it explicitly to shape the header like you think it's best - and even there we only make it optional to omit the additional @PG record

tools-iuc/tool_collections/samtools/samtools_reheader/samtools_reheader.xml

Lines 19 to 21 in 57fa4dc

<param argument="--no-PG" name="no_pg" type="boolean" truevalue="--no-PG" falsevalue=""

label="Keep the new header unmodified?"

help="By default, samtools reheader will add itself to the chain of tools used to manipulate the target dataset, which will cause one or more @PG records to be inserted into the new header. If you want the output dataset to end up with the header of the source dataset exactly, you should activate this option." />

.

Here's another case of what I'd consider a valid use of --no-PG:

tools-iuc/tool_collections/bamtools/bamtools_split/bamtools_split_ref.xml

Line 13 in 57fa4dc

samtools view -bH localbam.bam --no-PG -o header.bam &&

bamtools_split_ref first gets the header of the BAM it's operating on so that it can add it back to "empty" split results that don't have reads in them.

agreed - those seem reasonable to me. I can't think of a case where view is important to the analysis history of a file - it would be implied if we had some analysis tool that only takes sam for example.

right, so @bernt-matthias in your HISAT2 sorting solution:

tools-iuc/tools/hisat2/hisat2.xml

Line 351 in 57fa4dc

| samtools sort --no-PG -l 0 -T "\${TMPDIR:-.}" -O bam | samtools view --no-PG -O bam -@ \${GALAXY_SLOTS:-1} -o '${output_alignments}'

I'd say that the --no-PG with samtools view is fine (since it's an implementation detail of how you write the result of sorting), but the samtools sort should be added as a PG record.

I guess when you use samtools view for read filtering, that's when it deserves its own record?

wm75 · 2024-05-06T06:28:54Z

Alternatively we could do this, but I do not see a reason why we should.

Seems you have given the solution for HISAT2 quite some thought. What makes you think it's not the way to go here?

wm75 · 2024-05-06T06:39:59Z

the one extra thread needed by samtools sort or view could be subtracted from the ones used by bowtie2 with logic like that used in the samtools collection of tools:

tools-iuc/tool_collections/samtools/macros.xml

Lines 157 to 160 in 57fa4dc

    
               <token name="@ADDTHREADS@"><![CDATA[ 
        
                   ##compute the number of ADDITIONAL threads to be used by samtools (-@) 
        
                   addthreads=\${GALAXY_SLOTS:-1} && (( addthreads-- )) && 
        
               ]]></token>

bernt-matthias · 2024-05-06T11:31:21Z

Seems you have given the solution for HISAT2 quite some thought. What makes you think it's not the way to go here?

It just seemed to be to complicated. Give me some time I will change it. Probably I would go for GALAXY_SLOTS - 1 threads for bowtie2 (here I will need to make sure that the result will be at least 1, since bowtie seems not to allow 0).

This reverts commit 6e1b4f2.

tools/bowtie2/bowtie2_wrapper.xml

wm75

Looks like a good solution to me!

Co-authored-by: Wolfgang Maier <[email protected]>

bernt-matthias added 2 commits May 3, 2024 17:54

bowtie2: do not use pipes

0b25490

fixes galaxyproject#5983 by using a pipe bowtie2 and samtools run in parallel. since already bowtie2 alone uses more CPU than the assigned ones (less that 1 core) we should not do this, but run them serially. also properly redirects stderr.

fix linting

6e1b4f2

bgruening assigned wm75 May 3, 2024

bwlang reviewed May 5, 2024

View reviewed changes

bernt-matthias added 2 commits May 7, 2024 11:22

remove --no-PG

a3c11d3

restore piping

45812b7

bernt-matthias force-pushed the bowtie-decouple branch from 4f26923 to 45812b7 Compare May 7, 2024 09:48

reduce number of used threads by one

1c04353

bernt-matthias force-pushed the bowtie-decouple branch from bf22085 to 1c04353 Compare May 7, 2024 10:15

bernt-matthias marked this pull request as draft May 7, 2024 10:40

bernt-matthias added 2 commits May 7, 2024 14:55

Revert "fix linting"

b4b6951

This reverts commit 6e1b4f2.

bump

10122de

bernt-matthias changed the title ~~bowtie2: do not use pipes~~ bowtie2: do not overcommit May 7, 2024

bernt-matthias marked this pull request as ready for review May 7, 2024 15:40

wm75 reviewed May 8, 2024

View reviewed changes

tools/bowtie2/bowtie2_wrapper.xml Outdated Show resolved Hide resolved

wm75 approved these changes May 8, 2024

View reviewed changes

Improve comment wording

99a6bf7

Co-authored-by: Wolfgang Maier <[email protected]>

bernt-matthias merged commit 7c23f32 into galaxyproject:main May 13, 2024
9 of 11 checks passed

bernt-matthias deleted the bowtie-decouple branch May 13, 2024 13:33

bernt-matthias mentioned this pull request May 21, 2024

Updating tools/bwameth from version 0.2.6 to 0.2.7 #5262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bowtie2: do not overcommit #5985

bowtie2: do not overcommit #5985

bernt-matthias commented May 3, 2024

bwlang commented May 5, 2024

bwlang May 5, 2024

bernt-matthias May 6, 2024

bwlang May 6, 2024

wm75 May 6, 2024 •

edited

Loading

wm75 May 6, 2024

bwlang May 6, 2024

wm75 May 6, 2024

wm75 May 6, 2024

wm75 commented May 6, 2024

wm75 commented May 6, 2024

bernt-matthias commented May 6, 2024

wm75 left a comment

	<param argument="--no-PG" name="no_pg" type="boolean" truevalue="--no-PG" falsevalue=""
	label="Keep the new header unmodified?"
	help="By default, samtools reheader will add itself to the chain of tools used to manipulate the target dataset, which will cause one or more @PG records to be inserted into the new header. If you want the output dataset to end up with the header of the source dataset exactly, you should activate this option." />

bowtie2: do not overcommit #5985

bowtie2: do not overcommit #5985

Conversation

bernt-matthias commented May 3, 2024

bwlang commented May 5, 2024

bwlang May 5, 2024

Choose a reason for hiding this comment

bernt-matthias May 6, 2024

Choose a reason for hiding this comment

bwlang May 6, 2024

Choose a reason for hiding this comment

wm75 May 6, 2024 • edited Loading

Choose a reason for hiding this comment

wm75 May 6, 2024

Choose a reason for hiding this comment

bwlang May 6, 2024

Choose a reason for hiding this comment

wm75 May 6, 2024

Choose a reason for hiding this comment

wm75 May 6, 2024

Choose a reason for hiding this comment

wm75 commented May 6, 2024

wm75 commented May 6, 2024

bernt-matthias commented May 6, 2024

wm75 left a comment

Choose a reason for hiding this comment

wm75 May 6, 2024 •

edited

Loading