Repository navigation
Buffer --output-file writes - #307
Conversation
OutputWriter wrapped a bare File, so each small write!() was its own write syscall (~1 per byte). Writing a 1.49M-contig x 399-sample metabat matrix took ~66 min on Lustre, as long as reading the BAMs. Wrap the file in a BufWriter, and flush it explicitly on drop so a failed final write is logged rather than silently truncating the output. Fixes wwood#306 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex please review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 899d0518d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Err(e) = std::io::Write::flush(&mut *f) { | ||
| error!("Failed to flush output file: {e}"); |
There was a problem hiding this comment.
When --output-file produces less than the buffer capacity, or the filesystem fills while the trailing buffer is pending, every write! can return success and this drop path only logs the eventual flush error. The command then exits with status 0 despite a missing or truncated result (for example, a short result written to /dev/full), whereas the former unbuffered File made the caller's .unwrap() fail; finalization needs to return or raise this error so batch pipelines cannot accept corrupt output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 896cd71: CoveragePrinter::finalise_printing, which every output path ends in, now flushes the writer and exits 1 if the flush fails, so a failed final write no longer exits 0. The logging-only Drop impl is removed.
With --output-file buffered, the last writes reach the disk only when the buffer is flushed, and the Drop impl just logged a failure there, so a full disk could leave a truncated file behind exit status 0. Flush in CoveragePrinter::finalise_printing, which every output path ends in, and exit 1 on error. Drop the logging-only Drop impl. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks, much appreciated |
Fixes #306.
OutputWriterwrapped a barestd::fs::File, so each smallwrite!()became its ownwrite()syscall, about one per byte. Writing a 1.49M-contig × 399-samplemetabatmatrix (2.5 GB) with--output-filetook ~66 min on Lustre, about as long as reading the 436 GB of BAMs. Details are in #306.Changes, all in
src/lib.rs:std::io::BufWriter.DropforOutputWriterthat flushes explicitly and logs an error if the final write fails.BufWriter's own drop-time flush ignores errors, so without this a failed write would silently truncate the output.The output from the buffered path should be identical. Piping stdout (
| cat > file) as a workaround gave byte-identical results to--output-fileon a 100,967-contig × 17-sample run.Testing: I could not build locally (no Rust toolchain on hand), so this relies on CI's
cargo test. The change is confined toOutputWriter, and onlysrc/lib.rsconstructs it.🤖 Generated with Claude Code