Skip to content

Add multi-threading to samtools consensus. Needs htslib#1875#2174

Merged
whitwham merged 1 commit into
samtools:developfrom
jkbonfield:region-walk
Feb 13, 2025
Merged

Add multi-threading to samtools consensus. Needs htslib#1875#2174
whitwham merged 1 commit into
samtools:developfrom
jkbonfield:region-walk

Conversation

@jkbonfield

Copy link
Copy Markdown
Contributor

It's a major refactoring, but paves the way for similar work in mpileup. When an index is present, this breaks the queries down into continguous regions and performs consensus fasta/fastq/pileup generation in parallel. Each thread independently operates on its own file descriptor and iterator.

This gives considerably higher multi-threading capabilities than the old system which simply performed parallel file decompression. 16 threads gives 12-13x speed increase.

More work needs to be done on reference sequence handling, but this is probably a task for htslib. Ideally we need a dedicated thread-safe reference pool that permits ref-incr and ref-decr calls to share references between multiple threads and tracks those in active usage. (This exists in CRAM, but needs generalising.)

A few subtle changes occurred during the refactoring too.

  • Mpileup and fasta/fastq consensus are now consistent in for heterozygous gaps that fail to meet the consensus cutoff. We prefer to call the most likely base now, but will keep quality 0.

  • The combination of region queries and whether or not to pad out with preceeding Ns for zero coverage is now consistent. In some cases it used to always count from 1 (or region start) irrespective of where data coverage was, but did stop early if coverage didn't extend to the end. This sort of half-way -a (all) mode no longer exists.

    Sequence reported is always first observed base to last observed, unless -a is used in which case it is 1 to chr length.

@jkbonfield

jkbonfield commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

Draft while I do some final checking. Some benchmarks as it currently stands:

$ for t in 0 4 16;do time samtools consensus -f fastq -C0 ~/scratch/data/novaseq.10m.cram -@$t -r 1 -o /dev/null;done
real	2m53.528s
user	2m53.172s
sys	0m0.340s

real	0m45.231s
user	2m57.295s
sys	0m1.177s

real	0m13.398s
user	3m19.251s
sys	0m1.249s

Corresponding to 13x speed up with 16 threads and 3.8x with 4 threads.

@jkbonfield jkbonfield force-pushed the region-walk branch 2 times, most recently from b88ee03 to 6095b9d Compare January 20, 2025 16:48
@jkbonfield jkbonfield changed the title DRAFT: Add multi-threading to samtools consensus. DRAFT: Add multi-threading to samtools consensus. Needs htslib#1875 Jan 20, 2025
@jkbonfield jkbonfield force-pushed the region-walk branch 3 times, most recently from 5f9204f to 70ce4ed Compare January 21, 2025 10:36
@jkbonfield jkbonfield marked this pull request as ready for review January 21, 2025 11:19
@jkbonfield jkbonfield changed the title DRAFT: Add multi-threading to samtools consensus. Needs htslib#1875 Add multi-threading to samtools consensus. Needs htslib#1875 Jan 21, 2025
@jkbonfield

Copy link
Copy Markdown
Contributor Author

Also added a trivial one line change to fix #2167, as the code has moved and it'd just cause a conflict to make a new PR. We can delay it until later if desired though.

(Plus documented a strange quirk of --show-del yes impacting insertion visibility in pileup. That may be a bug - I'm unsure currently! It's debateable, depending on your point of view as to what a deletion means.

@jkbonfield jkbonfield force-pushed the region-walk branch 2 times, most recently from f65e3de to 0b0e2f2 Compare February 4, 2025 17:48
It's a major refactoring, but paves the way for similar work in
mpileup. When an index is present, this breaks the queries down into
continguous regions and performs consensus fasta/fastq/pileup
generation in parallel.  Each thread independently operates on its own
file descriptor and iterator.

This gives considerably higher multi-threading capabilities than the
old system which simply performed parallel file decompression.
16 threads gives 12-13x speed increase.

More work needs to be done on reference sequence handling, but this is
probably a task for htslib.  Ideally we need a dedicated thread-safe
reference pool that permits ref-incr and ref-decr calls to share
references between multiple threads and tracks those in active usage.
(This exists in CRAM, but needs generalising.)

A few subtle changes occurred during the refactoring too.

- Mpileup and fasta/fastq consensus are now consistent in for
  heterozygous gaps that fail to meet the consensus cutoff.  We prefer
  to call the most likely base now, but will keep quality 0.

  Previously this differed between pileup (which used N) and
  fasta/fastq (which used the most likely non-* base).

- The combination of region queries and whether or not to pad out with
  preceeding Ns for zero coverage is now consistent.  In some cases it
  used to always count from 1 (or region start) irrespective of where
  data coverage was, but did stop early if coverage didn't extend to
  the end.  This sort of half-way -a (all) mode no longer exists.

  Sequence reported is always first observed base to last observed,
  unless -a is used in which case it is 1 to chr length.
@whitwham whitwham merged commit bb96e23 into samtools:develop Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants