Skip to content

docs(zipper): recommend uncompressed BAM input for streaming pipelines - #362

Merged
nh13 merged 1 commit into
mainfrom
nh/zipper-bam-input-docs
May 21, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/zipper-bam-input-docs

Conversation

@nh13

@nh13 nh13 commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

fgumi zipper accepts uncompressed BAM on stdin via the existing raw-byte path, which is faster end-to-end than piping SAM text — it skips ASCII formatting in the aligner and SAM parsing in zipper. The prior recommendation in the CLI help, long_about, two warn! calls, and four guide pages predates aligners that can emit uncompressed BAM directly. bwa-mem3 supports --bam[=N] with N=0 (uncompressed BGZF, no DEFLATE) and builds bam1_t records directly from mem_aln_t without going through a SAM-text intermediate, so the recommended pipeline shape now is uncompressed BAM end-to-end.

This PR flips the recommendation:

  • Updates --input CLI help and the long_about example block to recommend bwa-mem3 mem --bam=0 for streaming pipelines, keeping a SAM example for aligners that can't emit BAM.
  • Replaces the stdin-BAM warn! with a neutral info!("Reading BAM from stdin") mirroring the existing stdin-SAM info log; drops the file-BAM warn! entirely (the SAM-file path doesn't log either, so this restores symmetry).
  • Applies the same recommendation flip to docs/src/guide/{getting-started,best-practices,performance-tuning,migration-from-fgbio}.md.
  • Removes a stale integration-test assertion that expected the dropped warn text.
  • Adds a ## [Unreleased] ### Documentation CHANGELOG entry.

No behavior change.

Why uncompressed BAM beats SAM text on a pipe

Both formats end up at roughly the same on-pipe byte count (BGZF level 0 adds ~26 bytes of framing per ~64 KiB block — header 18 + trailer 8). The win is on the CPU side at both ends:

  • Aligner side: writing BAM skips formatting integers, flags, and CIGAR ops to ASCII. bwa-mem3 builds bam1_t directly (mem_aln_to_bam, no SAM-text intermediate).
  • zipper side: the BAM-stdin path feeds raw bytes straight into TemplateIterator (MappedReader::StdinBam → RawBamReader). The SAM path goes through noodles RecordBuf decode + RecordBufEncoder re-encode (record_bufs_to_templates in src/lib/commands/zipper.rs), which is meaningful CPU per record.

Compressed BAM on a pipe (--bam=1+) is still discouraged in the new copy — it burns DEFLATE CPU on the writer and inflate on the reader for data the downstream sort step recompresses anyway.

Test plan

  • cargo ci-fmt
  • cargo ci-lint
  • cargo nextest run -E 'test(zipper)' — 75/75 zipper tests pass, including test_zipper_bam_mapped_input (file BAM input) and test_zipper_bam_stdin_input (stdin BAM auto-detect)
  • mdBook docs render — only prose changed; no syntax disturbed
  • CLI help renders cleanly: cargo run -- zipper --help

Notes for reviewers

  • I left the README.md Quick Start example and fgumi fastq's long_about examples on plain bwa mem — those are aligner-agnostic illustrations and inverting them feels out of scope for a zipper-docs PR. Happy to do a follow-up if the team wants consistency.
  • The new long_about block is slightly longer than sibling commands' help text (e.g. fastq, sort) — included a three-sentence rationale paragraph because zipper is the most pipeline-shape-sensitive command. Will trim if reviewers prefer a terser feel.

`zipper` accepts uncompressed BAM on stdin via the existing raw-byte path,
which is faster end-to-end than piping SAM text: it skips ASCII formatting
in the aligner and SAM parsing in zipper. Aligners like `bwa-mem3` support
emitting uncompressed BAM directly (`--bam=0`) without going through a
SAM-text intermediate.

Update the `--input` CLI help and `long_about` to recommend uncompressed
BAM for streaming pipelines while keeping a SAM example for aligners that
can't emit BAM. Replace the stdin-BAM `warn!` with a neutral `info!`
mirroring the existing stdin-SAM info log, and drop the file-BAM warn
entirely (the SAM-file path doesn't log either, so this restores
symmetry). Apply the same recommendation flip to the getting-started,
best-practices, performance-tuning, and migration-from-fgbio guides, and
remove the integration-test assertion on the dropped warn text.

No behavior change.
@nh13
nh13 temporarily deployed to github-actions May 20, 2026 23:57 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: df2afbbf-1e32-46ea-adce-bb7a2768cbea

📥 Commits

Reviewing files that changed from the base of the PR and between c781d53 and 7aa32f4.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • docs/src/guide/best-practices.md
  • docs/src/guide/getting-started.md
  • docs/src/guide/migration-from-fgbio.md
  • docs/src/guide/performance-tuning.md
  • src/lib/commands/zipper.rs
  • tests/integration/test_zipper_command.rs
💤 Files with no reviewable changes (1)
  • tests/integration/test_zipper_command.rs

📝 Walkthrough

Walkthrough

This PR updates fgumi zipper documentation and logging to recommend uncompressed BAM (bwa-mem3 mem --bam=0) as the preferred stdin input format instead of SAM, reflecting improved streaming performance. The changelog, getting-started, best-practices, performance-tuning, and migration guides are all updated with the new recommendation and performance rationale. The zipper command's help text and logging behavior are adjusted accordingly: the warning about BAM input is removed, a misleading stdin-BAM warning becomes informational, and tests are cleaned up to match.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: recommending uncompressed BAM input for zipper's streaming pipelines.
Description check ✅ Passed The description is directly related to the changeset, providing detailed context on why uncompressed BAM is preferred and what was updated.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/zipper-bam-input-docs

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.97%. Comparing base (c781d53) to head (7aa32f4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
- Coverage   90.97%   90.97%   -0.01%     
==========================================
  Files          78       78              
  Lines       51017    51016       -1     
==========================================
- Hits        46412    46410       -2     
- Misses       4605     4606       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nh13
nh13 merged commit c3b6dc6 into main May 21, 2026
10 checks passed
@nh13
nh13 deleted the nh/zipper-bam-input-docs branch May 21, 2026 21:45
@nh13 nh13 mentioned this pull request May 21, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 7aa32f43 Deployed May 20, 2026 by nh13 via coverage #1467
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.

1 participant