perf(concat): block-copy concatenation instead of SeqRecord round-trip (89x faster, -2.6 GB) - #5
Open
sanjaynagi-eit wants to merge 1 commit into
Open
perf(concat): block-copy concatenation instead of SeqRecord round-trip (89x faster, -2.6 GB)#5sanjaynagi-eit wants to merge 1 commit into
sanjaynagi-eit wants to merge 1 commit into
Conversation
…ords concatenate_single_fastq parsed both inputs into a list of BioPython SeqRecords and rewrote them. SeqRecord overhead is roughly 5-10x the file size, so concatenating short reads in a hybrid run could transiently need tens of GB. concatenate_single_fasta did the same for contigs, and Plass.get_depth_long / Assembly.combine_input_fastas had their own copies of the list(SeqIO.parse(...)) habit. Concatenating records requires no parsing. Copy in 1 MiB blocks instead, decompressing gzipped inputs on the way through, and guarantee a newline between files so a source with no trailing newline cannot run its last line into the next file's header. Measured on 573 MiB of ONT fastq: seconds 28.44 -> 0.32 (89x) peak RSS 2672 MB -> 88 MB (-2.6 GB) with byte-identical output and identical (id, sequence, qualities) records. Parsing every record used to catch a wrong-format input, and a test relies on that, so _append_file checks the first byte instead - O(1) rather than O(file). Empty inputs are still fine; they are normal when there are no unmapped reads. Assembly.combine_input_fastas genuinely has to parse, because it renames contigs, but it now streams records rather than materialising the whole assembly as a list first. Its odd 'records[0].description = ""' inside the per-record loop - which only ever cleared the first plasmid's description, while later ones kept theirs - is preserved with a comment, because get_contig_circularity() reads that description and changing it would change results.
sanjaynagi-eit
force-pushed
the
perf/concat-and-fasta-io
branch
from
August 13, 2026 21:46
4ec43d1 to
1147f80
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
concatenate_single_fastqparses both input FASTQs into a python list of BioPythonSeqRecords and then writes them out again:SeqRecordoverhead is roughly 5-10x the file size, so concatenating the short reads of a hybrid run can transiently need tens of GB.concatenate_single_fastadoes the same for contigs, andPlass.get_depth_long/Assembly.combine_input_fastaseach carry their own copy of thelist(SeqIO.parse(...))habit.Concatenating records requires no parsing at all.
Change
Copy the inputs in 1 MiB blocks, decompressing gzipped sources on the way through. Two details worth calling out:
_append_fileappends a newline when the copied data does not end with one.test_concat_single_fastq_baddepends on that. The first byte is now checked against the format's record marker (@/>) — same guarantee, O(1) instead of O(file).Plass.get_depth_longnow callsconcatenate_single_fastainstead of building a list of records.Assembly.combine_input_fastasgenuinely has to parse (it renames contigs) but streams them rather than materialising the assembly as a list.Measurements
573 MiB of ONT FASTQ, each implementation in a fresh process:
Output is byte-identical, and the parsed
(id, sequence, qualities)records match exactly.One thing deliberately left alone
Assembly.combine_input_fastascontains:That clears only the first plasmid's description while every later one keeps its own — almost certainly a copy-paste slip. It matters, because
get_contig_circularity()decides circularity by looking for "circular" in the description. Fixing it would change results, so the behaviour is preserved exactly and flagged with a comment for a separate PR.Tests
New
tests/test_concat.py: record preservation for FASTQ and FASTA, gzipped input, empty input, the missing-trailing-newline case, wrong-format rejection for both directions, and that FASTA descriptions survive (circularity depends on them). All existing tests pass (76 non-slow).