Conversation
Each worker owns one input list, and SingleProducerSingleConsumerList only lets a consumer take an item once another item has been produced behind it (or the producer has finished). Reader backpressure used a fixed PACK_IN_MEM_LIMIT (32) that is smaller than the worker count on machines with more than 32 cores. With 33+ workers the readers stop after 33 packs, before any worker list holds two items, so no pack is ever consumed and all reader and worker threads wait on the backpressure condition variable forever. The writer-backlog check has the same shape: the writer drains worker lists round-robin and waits on a list until its worker produces a second output, so a fixed limit below the worker count can also block the readers permanently. Allow at least two in-flight packs per worker in both the reader/processor and reader/writer backpressure checks (max(PACK_IN_MEM_LIMIT, 2 * threads)), for PE, interleaved PE and SE readers. The lock-free list semantics are unchanged, so this does not reintroduce OpenGene#695.
The reader/writer backlog check still gated on the fixed PACK_IN_MEM_LIMIT * PACK_SIZE period instead of the new mPackInMemLimit, so it fired far more often than the (now larger) buffer actually needs at high thread counts. Not a correctness bug — the comparisons inside already used mPackInMemLimit — just an inconsistency worth cleaning up alongside it. Also removed a stray extra blank line left in common.h.
- ci.yml: run the built-in unit tests and check_correctness.py, which requires byte-identical output at 1..48 workers in every output mode (catches OpenGene#721-style hangs; an LD_PRELOAD shim stops fastp capping -w to the runner's cores) and checks adapter auto-detection. - bench.yml: on PRs, build base and head on the same runner and time them interleaved (wall, adapter detection, processing, CPU, peak RSS) on synthetic reads and a public ATAC-seq subset; on master, chart history with github-action-benchmark. - Dependency builds move to a composite action shared by both workflows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Linux build is fully static, so the LD_PRELOAD nprocs shim had no effect and every -w above the runner's core count was capped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- bench.py marks each cell as a regression (worse than base by more than --threshold, default 10%, with no overlap between base and head runs), an improvement (better by more than 5%, no overlap), or noise. Wall time, CPU and peak RSS are gated; stage timings are informational. - The PR fails the "regression gate" step on any regression unless it has the perf-regression-ok label; adding or removing that label re-runs the benchmark. The threshold can be set with the BENCH_THRESHOLD_PCT variable. - The report is posted as one PR comment that is updated in place. Fork PRs get a read-only token, so bench-comment.yml posts it from a workflow_run in the base repo, using the artifact only as comment text and finding the PR by head SHA. - bench-compare.json (per-cell base, head, delta, verdict) and the regressions/improvements step outputs give machine-readable results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fastp benchmark✅ No regressions over 10%.
stage timings
|
- github-action-benchmark runs git in the workspace root, so check the PR out there (base goes to _base/) and keep results in $RUNNER_TEMP, out of reach of its gh-pages switch. - Alternate base/head order each rep. With base always first, every PR showed processing 1-4% slower than base, including ones that don't touch that path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Subsets overstate changes to fixed-cost stages: pre-processing, adapter detection (capped at 256K reads / 39.6M bases per mate) and the report cost the same on a subset as on a full run. Each run is now also projected to a full-size run (--project-reads, default 50M reads or pairs): fixed stages as measured plus processing scaled by read count. The report and the gate use projected wall and CPU; measured subset numbers move to a collapsed section. Checked against 30 full-size public runs (6 datasets, 2 builds, -w 8/16/48): projected wall within 8% of measured (median); base-vs-head deltas within 3 percentage points, against 10 points for raw subset deltas. The synthetic set grows to 300K pairs so detection reaches its cap, as it does on a full file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Projecting one subset run needed stage timestamps and still overpredicted some datasets. Each build is now run on the first 0.3M and 1.2M pairs of the same input (--reads_to_process, no recompression; both above the 256K-read detection cap), and a line through the two points gives a fixed cost (intercept) and a cost per pair (slope). Projected wall, CPU per pair and peak RSS are gated; the fixed cost and the measured subset numbers are shown in collapsed sections. On 3 complete public runs at 4 and 16 cores, a line through two subset sizes predicted full-run CPU within 1-2% and wall within 2-4% (median error), against 3% and 11% for scaling a single subset. - Only -w 4 is run: CPU per pair doesn't depend on the thread count, and -w 1 was the slowest cell. - gen_reads.py draws qualities from a pool built once instead of 150 Gaussians per read: 4x faster, so a 1.2M-pair file takes about 2 minutes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Two of three concurrent jobs failed in `prepare` when ENA closed the stream mid-transfer (EOFError: compressed file ended before the end-of-stream marker). Retry up to five times with backoff, and treat a short stream as a failed attempt instead of writing a truncated file. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Restarting from the top never finished for the 1.2M-record subset: ENA closes the stream partway through every time. Reconnect with a Range request at the byte where the stream stopped and keep feeding the same gzip decompressor, including across concatenated gzip members. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…complete sets The ENA stream for the public input can be left half-populated when two jobs start pulling the same file at once: the first connection returns a few KB and every ranged request then gets 416 Range Not Satisfiable. A PR's CI shouldn't fail on that. - On 416, restart the download from byte 0 instead of retrying the same offset. - If an input still can't be fetched, skip that dataset with a warning, say so in the report, and run the rest. - Cache the datasets only when the set is complete (separate restore and save steps), so a skipped input is retried on the next run. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
Fork-internal PR to exercise the new workflows before proposing them upstream. Includes the OpenGene#723 fix commits underneath (the correctness check hangs on master at -w 33/48 without them).
🤖 Generated with Claude Code