#88 plus review fixes - #89
Open
gbouras13 wants to merge 8 commits into
Open
Conversation
The chopper QC chain is gunzip -c | chopper --threads N | gzip. Only the compressor is parallel-unaware, and it dominated: on a 300 MB ONT fastq the whole chain took 47.7s, of which gunzip+chopper was 5.4s and gzip was 42s. Use bgzip -@ threads instead. bgzip ships with htslib/samtools, already a hard plassembler dependency, so this adds nothing to the environment; BGZF is a valid gzip stream, so flye, minimap2 and chopper read the result unchanged. It is also slightly smaller here (277 vs 293 MiB). Falls back to gzip if bgzip is somehow missing. gunzip | chopper (no compression) 5.4s ... | gzip (before) 47.7s 293.4 MiB ... | bgzip -@ 8 (after) 5.9s 276.7 MiB Verified content-identical: with chopper --threads 1 (chopper is not order-deterministic above 1 thread) the decompressed output of the two chains is byte-for-byte equal. Also fixes the process handling. Only gzip_proc was waited on, so a failing gunzip or chopper was silently swallowed - the bare 'except: logger.error' never saw it - and left zombies behind. Every stage is now waited on and a non-zero exit is reported with the stage name and log path. The parent also closes its copy of each upstream pipe read end, and plain (uncompressed) input is handed to chopper directly instead of through a pointless 'cat' process. gzip_file, used by --skip_qc, went through python's gzip module, which is slower still; it now uses the same compressor with a python fallback.
tests/test_data/end_to_end/input_half.fastq.gz is 778 lines - 194.5 FASTQ records. It ends mid-record, with a header and a sequence line but no + or quality line, so chopper rejects it (IncompleteRecord) and exits 101 after emitting 193 complete reads. That has been true since the fixture was committed; test_plassembler_case_depth_filter_some passed only because the chopper failure was swallowed. With this PR's process handling it is detected, and the test fails. Drop the two orphan lines. chopper then exits 0 and emits the same 193 records, so nothing the test exercises changes - and the test asserts only that the command exits 0, so no expectation depends on the fixture's exact contents. The full suite (including slow) is 169 passed / 1 failed before this commit and 170 passed after.
chopper v0.11.0 introduced --trim-approach and made --headcrop/--tailcrop apply only under `fixed-crop`. They were silently ignored otherwise, with no warning until v0.13.0 added one, so plassembler's intended 75bp head and tail crop has been a no-op for anyone on chopper >=0.11.0 since September 2025. Pass --trim-approach fixed-crop and raise the minimum chopper version to v0.11.0 in pyproject.toml, build/environment.yaml, README.md and docs/install.md. Older chopper rejects the flag outright, and plassembler does not check chopper's exit code, so allowing <0.11.0 would yield an empty read file. Note chopper >=0.11.0 also re-checks --minlength against the post-crop segment, which older versions did not, so reads within 150bp of --min_length are now dropped rather than kept and cropped. Verified end to end: plasmid sequences are byte-identical, with small shifts in long read depths and one copy number moving by 0.01. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tests/test_data/validation/concat.fastq was truncated to 0 bytes in the bgzip commit, unrelated to everything else in that change. It is not a fixture clobbered by a suite run. test_concat_single_fastq_bad (tests/test_plassembler.py:218) does name it as an output path, but concatenate_single_fastq consumes both inputs via records.extend(SeqIO.parse(handle, "fastq")) before it opens the output, so parsing test.fasta as fastq raises ValueError first and the file is never opened for writing. Restoring it and running the full non-slow suite leaves it at 104636 bytes. Nothing reads it as an input, so the deletion broke nothing, but it drops 100 KB of real ONT reads for no reason. Restored from main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The OSError handler kills each already-started stage but never waits on it, leaving exactly the zombies the rest of this change exists to avoid. Also records why the `return` below it is load-bearing rather than dead code: logger.error terminates the process under the ERROR sink that begin_plassembler installs, but plassembler.utils.qc is importable without that sink, and falling through would wait on the processes just killed above and report them a second time as failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only the last process in the gunzip -> chopper -> compressor chain was checked, so a chopper that exited non-zero went unnoticed: the compressor still wrote a valid but empty chopper_long_reads.fastq.gz, "Finished running chopper" was still logged, and Flye/Raven ran on zero reads. The real diagnosis reached only <logdir>/chopper.err, which users rarely open. Every stage is now checked, and any failure - a dead stage, a chopper that could not be spawned at all, or output with no reads in it - reports chopper's own stderr alongside the path to the logfile and exits, matching how external_tools.py handles a CalledProcessError. check_dependencies() also warns when the installed chopper predates v0.11.0. The pin was raised to >=0.11.0 for --trim-approach, but a pin only binds when an environment is first created, so an existing environment can still hold a chopper that rejects the flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 1.8.4 entry covered the cropping fix and the fatal-failure handling but said nothing about the change they were built on top of. Adds the bgzip compression speedup, including the caveat that bgzip only reaches us transitively via samtools -> htslib and does not resolve on osx-arm64, so Apple Silicon users get the gzip fallback and no speedup. Also credits the per-stage process handling on the existing fatal-failure bullet, and notes that it is what exposed the long-standing mid-record truncation in the input_half.fastq.gz fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
thanks @sanjaynagi-eit