Repository navigation
fix(canu): make canu failing to run fatal instead of continuing (#92) - #93
Merged
Merged
Conversation
Any non-zero exit from canu -correct was reported as "This probably means there is low depth, don't be too concerned", and plassembler went on with uncorrected reads. In #92 that hid a canu that could not run at all: in the Biocontainers image, canu's job scripts call `expr -- ...`, which BusyBox's expr rejects, so meryl-count failed every time. Going on with uncorrected reads left Unicycler assembling 110 spurious contigs. canu ends every failed run with the same "ABORT: Don't panic" banner, including the coverage stop a plasmid-free sample is expected to hit, so the banner cannot decide. The lines before it can: - "-- <stage> jobs failed, tried N times, giving up." is how canu reports any compute job that never produced its output. Jobs only start once the reads pass canu's coverage checks, so this is canu's environment failing. Fatal. - A "CRASH:" banner is canu's own internal error. Fatal. - "Read coverage (N) lower than allowed." is the thin-reads stop. Carries on as before, now saying why. - Anything else carries on too, but shows canu's stderr instead of reassurance. canu has close to 200 ways of stopping, and one this does not know must not turn a plasmid-free sample into a failed run. Checked against real canu 2.3 runs with plassembler's own arguments: a busybox-style expr reproduces #92 and is now fatal; too few reads, no reads, thin reads and non-overlapping reads either stop on coverage or exit 0, and none of them prints the job-failure line. The signatures read the same in canu 2.2. The fatal path keeps the canu directory, which holds each job's own log - for #92, the meryl-count log with "expr: syntax error" in it. The same handling covers canu assembly with --canu_flag. canu now runs directly rather than through ExternalTool.run_tool, so its two special cases there are removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Left unreleased: no version bump. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.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
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.
Refs #92. This is the plassembler side. The root cause in the image is BusyBox's
exprrejecting theexpr --that canu's job scripts use, and that is being fixed separately by addingcoreutilsto the bioconda recipe. So this PR doesn't close the issue.Problem
Any non-zero exit from
canu -correctwas reported as:and plassembler went on with uncorrected reads. In #92 canu could not run at all: meryl-count failed on every attempt. Going on with uncorrected reads left Unicycler assembling 110 spurious contigs, which the depth filter then had to discard. Once canu worked there were none, and the run took 2.5× less time.
Change
canu ends every failed run with the same
ABORT: Don't panic …banner, including the low-coverage stop that a plasmid-free sample is expected to hit. So the banner can't decide. The line before it can:-- <stage> jobs failed, tried N times, giving up.CRASH:bannerRead coverage (N) lower than allowed.This matches failure lines rather than whitelisting harmless stops. canu has ~200
caExitsites, and missing one harmless stop would turn plasmid-free samples into failed runs.On the fatal path the canu directory is kept, because the run exits before
remove_directory. The message includes the tail of canu's stderr, which names each job's own log. For #92 that iscorrection/0-mercounts/meryl-count.000001.out, which containsexpr: syntax error.canu now runs directly from
run_canu.py, the same wayqc.pyhandles chopper. The two canu special cases inExternalTool.run_toolare removed.Verification
I ran real canu 2.3 with plassembler's exact arguments on
tests/test_data/end_to_end/abaumanii_plasmid.fastq.gz:exprwrapper rejecting--like BusyBoxgenomeSizeNone of the thin-data cases prints the job-failure line. I also drove the fixed
run_canu_correctagainst real canu under the CLI's exiting ERROR sink. The BusyBox case exits 1, low coverage warns and carries on, and a normal run is silent. In canu's source the three lines are identical in 2.2 and 2.3.tests/test_run_canu.pyadds 12 tests that replay real canu 2.3 stderr (fixtures intests/test_data/canu/) through a stubcanu. 10 of them fail againstmain, and the other 2 are the success cases, which are meant to pass on both. The non-slow suite passes (102), andruff format --checkandruff checkare clean. I didn't run the slow tests locally, because this machine has no bioinformatics tool stack.Also bumps the version to 1.8.6, with release notes crediting the reporter.