perf(groups): decode a run's spectra once per phase instead of once per band - #112
Merged
Merged
Conversation
A grouped search cuts the library into m/z bands and searches each one over the whole run, so every band's search-seed and every band's extract called `load_ms2` (and extract `load_ms1`) on the same artifact. One 63-band production run decoded a ~1 GB MS2 artifact 126 times and its MS1 63 times, and held as many copies at once as there were bands in flight. That is the same resident set the banding exists to bound, and the same per-scan allocations the per-process mapping limit is what kills the run on (live grouped run: 556,573 mappings at 244 GB, limit 1,048,576). `SearchSeedParams::ms2_scans` and `ExtractParams::scans` (a `SharedScans` of MS2 plus MS1) now take an optional borrowed slice; the stage uses it when present and opens the path when not. `run_groups` decodes MS2 once before the seeding phase and MS1 once before the extraction phase, and lends both to every band. The standalone CLI, the ungrouped `run` and `run-experiment` pass `None` and are unchanged: sharing there would hold ~1 GB across the retention-time phase to save a decode that happens twice. Verified read-only before writing it, rather than assumed: both stages bind the scans immutably, `load_ms2`/`load_ms1` sort by retention time before returning, the mass recalibration is applied to the library m/z at probe time (`MassOffset`, `Prober::probe`) rather than by rewriting peaks, and the destructive peak-claim strategies rewrite the band's own `Hit` intensities. MS1 gets the same treatment as MS2 (extract takes it, the seed does not, so it was m decodes rather than 2m), but is decoded later so its buffer is not resident across the DeepLC sidecars. Numbers. Fixture, `window_groups: 3, parallel: 2`: 6 MS2 decodes and 3 MS1 decodes become 1 and 1. Resident copies at the extraction peak go from `parallel` x (MS2 + MS1) to one of each, so at `parallel: 8` the extraction phase holds 1 GB of MS2 instead of 8 and at `parallel: 48` (a 100-band run on a smaller library) 1 GB instead of 48; the one new cost does not scale with `parallel`, being the single MS2 buffer held across the retention-time phase, where before nothing held one. A decoded MS2 scan is two heap blocks (its `Vec<Peak>` and its id `String`), so one copy of a 301,127-scan run is about 602,000 blocks and eight concurrent copies about 4.8 million; how many become distinct mappings is allocator-dependent and not measured here. Risk to output equality: none measured. The same fixture searched with the pre-change and post-change release binaries, grouped (`window_groups: 3, parallel: 2, calibration: global`) and ungrouped, gives byte-identical artifacts: 67 of 67 files grouped and 32 of 32 ungrouped, with the five JSON sidecars that differ differing only in the recorded output directory, the binary path and `elapsed_ms`. Two new integration tests pin it from the outside, including the old path: extract and search-seed run once from the path and twice from one lent buffer write byte-identical artifacts, and the lent buffer still equals a freshly decoded one afterwards. The extract test also asserts its MS1 fixture reaches the output, so the shared-MS1 half cannot pass over a buffer nothing reads. Collateral: removed an identity `as f32` on `sum_near(...)` in extract, which clippy flags once `ms1_scans` is a slice rather than a `Vec`. `sum_near` returns f32, so the cast was a no-op. cargo fmt --check, clippy --workspace --all-targets -D warnings, test --workspace (393 tests) and ci/smoke.sh (SMOKE_OK, 144 assertions, output hashes unchanged) all pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r it An adversarial review of 026fc9b found that the change is right and the reasoning around it is not. Six findings accepted, two rejected. WHAT THE REVIEW FOUND, AND WHAT IS FIXED 1. The safety argument was stated backwards in three places. The comments and docs said the per-run mass recalibration "is applied to the library m/z at probe time rather than by rewriting observed peaks". It is the other way round: every call site reads `peak.mz / mass_off.factor_at(peak.mz)`, the OBSERVED peak is the side corrected, and under `groups.calibration = per_group` each band applies a different factor to the one shared buffer. The conclusion survived by accident, because that value goes into a local. The three comments and the docs section now say what actually makes sharing safe: the correction is computed into `q_mz` and never written back, the stages take a shared slice they cannot write through, the loaders sort before returning, and `select_peaks` returns indices rather than truncating. The note above the probe loop, which already records that hoisting `q_mz` out of the loop was tried and reverted, now also records that the memory-cheap version of that hoist -- writing it back into the scan -- has gone from harmless to forbidden. 2. Made enforceable rather than commented, at no release cost. `run_groups::scan_fingerprint` is an FNV-1a over every field the stages read, compiled only under `debug_assertions` (a constant otherwise), and asserted either side of both band loops. It does not duplicate the borrow checker: a stage writing through `&[Ms2Scan]` cannot compile. It covers what the borrow checker does not -- interior mutability, `unsafe`, and the second decode differing from the first, which this commit now depends on. 3. A caller could silently lose every MS1 feature. With `scans: Some(...)` the `ms1` path was never consulted, so lending `SharedScans { ms2, ms1: &[] }` while still passing `ms1: Some(path)` dropped every MS1 feature and every `ms1_mono` / `ms1_iso1` / `ms1_iso2` row, silently, both being written under `!ms1_scans.is_empty()`. An empty lent slice is now never believed over a named path: extract and search-seed decode the artifact and warn. It costs nothing when the run really has no such scans, because the decode is then empty too. New test `extract_does_not_believe_an_empty_lent_ms1_over_a_named_one`. 4. The buffers lived longer than the accounting admitted, and the accounting is corrected. MS2 was held across the per-band DeepLC loop (63 sequential sidecar processes on the 63-band run) and across pooling; MS1 across features, compete and pooling. Rather than only dropping each after its last reader, the phases are now scoped: MS2 is decoded for the seeding phase and dropped at the end of it, MS2 and MS1 are decoded again for the extraction phase and both dropped before `pool::run`. Neither is resident during the retention-time phase, which is where `run.rs` already declines to share for exactly this reason and where CLAUDE.md puts the whole-run peak. That is 2 MS2 + 1 MS1 decodes per run against the 2m + m this replaces, and it removes the one new cost the previous message disclosed instead of leaving it to be paid. The docs table now has separate MS2 and MS1 residency columns and says "decoded once per phase". 5. Nothing covered the grouped path, in this repository or in CI. `ci/smoke.sh` now runs a grouped arm: `window_groups: 3, parallel: 2, calibration: per_group`, asserting three planned bands with a competed table each, exactly 2 MS2 and 1 MS1 decode for those three bands, at least 100 peptides, and two grouped runs agreeing byte for byte. Chosen over a grouped fixture integration test, not instead of one: `run_groups` needs converted spectra with several isolation windows, a precursor table with row-group statistics, and an RT model fitted on confident seed anchors, all of which the smoke fixture has and the hand-crafted one in `tests/pipeline.rs` would have to fake. The decode counts are the assertion that regresses if a band is ever handed its own copy again. 6. The new tests exercised none of the mechanisms the argument named, so they now do. The MS2 fixture is written in DESCENDING retention time, which makes `load_ms2`'s sort load-bearing (the file order was already ascending, so a stage that re-sorted or reversed was indistinguishable from one that did not). The seed test lowers `top_n_peaks` to 3 against 8-peak scans so `select_peaks` actually takes its capping branch, and asserts against an uncapped arm that the cap binds. The extract test enables `PeakClaim::CoelutionWinner` over a library variant where the target and the decoy share a fragment, and asserts against a no-claim arm that the destructive path changes the output. And it alternates TWO mass calibrations over one shared buffer, each arm having to equal the arm that decoded for itself under the same calibration, which is the `per_group` hazard reduced to a unit test. While doing this: the seed test was comparing EMPTY tables. The default `search_seed.min_matched_peaks` is 4 and these candidates have three fragments each, so every arm wrote a zero-row seed and every equality held vacuously. The review did not catch this one. Both shared-buffer tests now assert their first arm produced rows before comparing anything. 7. `assert_scans_identical` is discharged by the borrow checker, as the review says, so its doc no longer claims to catch a stage mutating the slice. It now states what it does guard: reproducibility of a decode, which the two-phase decode depends on, and interior mutability or `unsafe`. WHAT IS REJECTED - "UNEXPLAINED JUSTIFICATION FOR THE CLIPPY COLLATERAL". The review argues that indexing a `Vec<Ms1Scan>` and a `&[Ms1Scan]` both yield `Ms1Scan`, so `unnecessary_cast` has no reason to change verdict, and calls the stated reason wrong. Measured instead of argued, on this tree with clippy 1.96.0: with the cast restored on the post-change code clippy errors on it; with the cast present on a816768, where `ms1_scans` is a `Vec`, clippy is silent (0 hits, forced rebuild). The commit's stated reason is therefore correct and the objection is rejected. Which of the lint's heuristics distinguishes `Vec` indexing from slice indexing was not established, and the code comment says so rather than inventing a mechanism. - The review's implied remedy of decoding MS2 twice is adopted, but its framing of the MS1/MS2 asymmetry as "unjustified" is only half right: the asymmetry was real and the fix was to make MS2 symmetric with MS1, not to abandon sharing. The review's own arithmetic was checked and holds: 301,127 x 2 = 602,254 blocks per copy, x8 = 4.8M, 63 x 2 = 126 decodes, and every file:line reference (extract.rs 806/1136-1139/1292-1300/1321/1550/1604/1676/1787/1858/2102-2112/ 2269/2785/3010/3019, run_groups.rs 155/367-399/453/488-491/709, config.rs 608/1055, search_seed.rs select_peaks, spectra.rs:155) resolves to what it claims. OUTPUT EQUALITY, RE-MEASURED RATHER THAN INHERITED The smoke fixture searched with the pre-change (a816768) and post-change release binaries, three arms, artifacts compared byte for byte: | arm | files | byte-identical | differing | |---|---|---|---| | grouped, `calibration: global` | 87 | 60 | 27 JSON sidecars | | grouped, `calibration: per_group` | 87 | 64 | 23 JSON sidecars | | ungrouped | 43 | 28 | 15 JSON sidecars | Every non-JSON artifact is identical in every arm: 38/38 grouped in both modes, 20/20 ungrouped. The JSON differences are the recorded output directory, the binary path, `git_sha`, `commit_date` and `elapsed_ms`; the two binaries are built from different commits, which is why more sidecars differ here than in the previous measurement. `per_group` is covered this time, as the review asked: it is the mode in which the bands hand different `MassOffset`s to the same peaks, and it produces materially different results from `global` on this fixture (58 of 87 files differ between the two), so it is a real second arm rather than a relabelled first. cargo fmt --check, clippy --workspace --all-targets -D warnings, test --workspace (394 tests, one more than before) and ci/smoke.sh (SMOKE_OK, 144 assertions plus the new grouped arm: 3 bands, 2 MS2 + 1 MS1 decodes, 146 peptides, reproducible) all pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reference records the environment reads whose name is not a literal by source line, and sharing the spectra moved three of them in extract.rs. Co-Authored-By: Claude Opus 5 (1M context) <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
1 participant
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.
A grouped run decoded its spectra once per band per stage. On the six-file HYE Astral benchmark banded into 100 groups, that was 3.84 GiB of MS2 scans decoded per band to search a 0.016 GiB slice of library, 200 decodes per file, and 48 concurrent copies at the extraction peak. It is why that arm spent 215 CPU-minutes seeding where the unbanded arm spent 1.8, and why it peaked at 182 GB against 11.8.
The run now decodes once per phase and lends the buffer to every band: for m bands, 2 MS2 and 1 MS1 decodes instead of 2m and m. Neither buffer is resident across the per-band retention-time sidecars or across pooling.
What makes it safe, stated correctly
The first version of this change justified itself backwards, and the review caught it. The mass recalibration is applied to the OBSERVED peak (
peak.mz / mass_off.factor_at(peak.mz)), not to the library m/z, and undercalibration: per_groupeach band applies a different factor to the one shared buffer. What actually makes sharing safe is that the correction lands in a local, the stages take a slice they cannot write through, the loaders sort before returning, and peak selection returns indices rather than a filtered copy. The comments, the docs and the reverted-hoist note now say that, and the note records that writing a corrected m/z back into the scan has gone from harmless to forbidden.A debug-only fingerprint over every field the stages read is asserted either side of both band loops. It does not re-do the borrow checker's job; it covers interior mutability,
unsafe, and a second decode differing from the first. No release cost.Coverage
ci/smoke.shgains a grouped arm, which nothing had:window_groups: 3, parallel: 2, calibration: per_group, asserting three bands, exactly two MS2 and one MS1 decode, at least 100 peptides, and two grouped runs byte-identical.per_groupon purpose, since that is the mode where bands apply different mass offsets to the shared peaks.The review also found that the tests were weaker than they looked: the fixture's scans were already retention-time ordered so the loader's sort never reordered anything, its peak counts never reached the seed's cap, and the destructive peak-claim strategies were off. The fixture is now written in descending retention time, the cap is made to bind with a guard arm proving it, a coelution-winner arm proves the claim changes the output, and two mass calibrations are alternated over one shared buffer. Separately, the seed's shared-buffer test had been comparing empty tables, because the default minimum matched peaks is 4 and the fixture's candidates carry three fragments; both shared-buffer tests now assert their first arm produced rows.
Equality, re-measured over three arms
globalper_groupEvery non-JSON artifact is identical in every arm. The JSON differences are the output directory, the binary path, the git SHA, the commit date and elapsed milliseconds.
per_groupis a genuine second arm, differing fromglobalin 58 of 87 files on this fixture.cargo fmt --check,clippy -D warnings, 394 workspace tests andci/smoke.shall pass.🤖 Generated with Claude Code