Bring work packages A-F onto main (reviews docs/29, docs/30, docs/31) - #68
Merged
Conversation
Fixes from the 2026-09-07 code review, now tracked as docs/29 and indexed: - quant read the pooled-table guard column `source` as i32 while rescore writes u32, and treated the type error as an absent column, so the refusal never ran on engine output. It reads the declared type and errors on a present column of another type (#1). - The streamed library loader checked finiteness on physical Arrow buffers and turned NULL cells into NaN, "" and candidate 0. Every required fragment column now rejects NULLs before its values are read (mumdia_io::table::require_no_nulls), with a fixture per column (#2). - AtomicPath no longer removes the destination before the rename, so a failed publication leaves the previous artifact; temporary names carry a process-wide counter; the failure and replacement cases are tested (#4). - Met excision is judged on the excised form alone, so an N-terminal peptide of max_len + 1 residues yields its excised form (#9). - rescore.max_feature_matrix_gib is checked before allocation, from the footers and the selected feature count, on the flat f32 layout, with checked arithmetic (#11). - A candidate DeepLC or MS2PIP returned nothing for is dropped together with its pair (base peptide, charge, modification set) instead of receiving iRT 0.0 or the native intensities; the counts are in the library report; the sidecar clients reject ids that were not requested or appear twice (#17). - Numeric configuration domains are validated at load, documented zero meanings kept (#18). - gen_config_reference.py and check_workflows.py scan tracked files via git ls-files, with the filesystem glob as the fallback outside a checkout (#21). Verified: cargo test --workspace (193 + 41 + 10 + ...), clippy -D warnings, ci/smoke.sh on the release build with byte-identical fixture output, gen_config_reference --check, gen_cli_reference --check against the fresh binary, check_workflows, check_doc_refs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…no_nulls rustdoc under -D warnings rejects an intra-doc link to a private item (rustdoc::private_intra_doc_links), which failed the fmt + clippy job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- entrapment_worker: a training fold with a single class is an error that names the condition; the final model scores the decoys only. It used to skip the fold and score its held-out rows with a model trained on those rows, so in-sample scores entered the entrapment FDR (#3). - mbr_worker: the transfer q carries the engine's +1 pseudocount, so a pool no permuted residual undercuts no longer gets q = 0 (#7); the summary names the permuted-null draws inside the accepted window instead of a decoy fraction over a population that cannot contain a decoy (#6); the per-run apex is the peak rescore selected (selected_peak_rank), falling back to the highest prelim_score peak, when a competed table carries several peaks per candidate (#8). - augment_library: an added form of a sequence the imported library already has keeps that sequence's base_peptide_id; only new sequences get fresh ids (assign_base_ids, unit-tested without the engine) (#12). - fs_lib: the fold key hashes the base sequence with the DECOY_ prefix stripped, and every benchmark row records the code revision, fold rule, feature count, seed and training recipe (#20). docs/28 states the difference from the earlier studies. - Tests: single-class fold refusal; three-candidate pool not accepted at 1% and its q is 1/3; the transfer measured on the rescore-selected peak; base-id reuse. conftest gains optional int columns for the scored fixture and top-K columns for the psms fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… per request, publish one terminal state Code review C (docs/29 findings 5, 13, 14). Output ownership (#5). A repeated Start in the desktop application launched a second engine into the same results folder: the start flow had several awaits and no in-progress guard, and the backend launched every request. The frontend now refuses a Start while one is in progress or while the run it follows is still running. The backend reserves a run's results folder by canonical path before spawning the engine and releases it when the run's end is published, so a request for an active folder is refused with the owning run named. The CLI's --run-names check compares names without regard to case on every platform, because RunA and runa are one directory on Windows, macOS and most network shares. Converter preflight (#13). The desktop asked `doctor --json` without the request's configuration, so a converter named in convert.thermo_raw_parser or convert.msconvert was reported missing and the search refused; it also required ThermoRawFileParser for Thermo .raw even when msconvert, the engine's fallback for a parser left at auto, was present. The probe now carries the configuration and the verdict is the engine's rule: only msconvert present runs with a note, an explicitly configured parser that is missing blocks. Cancel race (#14). `cancel` and the process waiter both wrote the terminal status, whichever ran second won, and the cancellation flag was never read, so a stopped run could be shown as failed with the last log line as its error. The waiter is the only writer now: it reads the intent after reaping the engine and publishes cancelled, done (the engine had already finished) or failed; until then the snapshot carries cancel_requested and the interface shows Stopping. Also: Dependabot for /desktop and a cargo audit of desktop/Cargo.lock in CI (one documented ignore, RUSTSEC-2024-0429 via Tauri 2's gtk 0.18). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…precursor-q audit, transfer basis in TSVs, replayable experiment manifest Code review D (docs/29 findings 10, 15, 16, 19) plus stale guide statements. LOESS (#10). Outside the anchor range the smoother switched from the boundary local fit to the global least-squares line, and the two need not agree: on y = 200 + 10x^2 (span 0.3) the prediction jumped from 193.4 at x = 1e-6 to 38.3 at x = 0 and from 1173.5 to 1018.4 at the top, about 155 s discontinuities for gradient-edge peptides against a narrow extraction window. The map now continues the boundary local fit (value and slope) and is continuous at both ends; the global line remains the degenerate fallback only. Endpoint continuity and the local-slope extrapolation are tested on the review's reproduction. Experiment manifest (#15). Records config_json next to config_hash, the model identities that produced the artifacts (RT source, fragment predictor, the classifier that actually ran from the rescore report, feature schema id, MBR strategy), the configured and effective quant.q_filter, and hashes the inputs at the start of the run rather than after it. Audit (#16). The passed_precursor_fdr gate and FAILED_PRECURSOR_FDR reason read precursor_q, the unit the label names, falling back to the PSM q_value only on a table without it and recording q_unit in the metrics JSON. A pooled scored table is refused, since the audit keys on candidate_id. The coarse extraction bucket NO_PEAK_GROUP is DID_NOT_SURVIVE_EXTRACTION: extract does not write the table that would separate presence, matched-fraction and gate failures, so the old name claimed a cause the audit cannot see. Report (#19). peptides.tsv and proteins.tsv, single-run and experiment-wide, carry is_transferred and transfer_q so the acceptance basis of a match-between- runs row is visible next to its untouched grouped q; the MBR worker's augmented scored table gains transfer_q. The rule is unchanged and now documented: a tighter report threshold does not revoke a transfer, and a transfer is not protein-group confidence. Guide corrections: retain_top_peaks alternatives do reach features, compete and rescore (peak_rank, selected_peak_rank); the rescore matrix is flat f32; the desktop comments no longer say run-experiment writes no report. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n status (docs/29)
…he docs/29 merge check
…rtable run names, cancel lifetime, conversion ownership, numeric domains, rewrite coverage, audit acceptance, nested reservations, main-thread stack Code review E (docs/30_code_review_2026-09-08.md, findings R1 to R9) plus the cleanup notes on model identity and the MBR worker's empty outputs. R1. Enabling DeepLC fine-tuning with its own defaults was rejected at load, because finetune_batch = 0 is the documented automatic batch size and review A's validation demanded a positive batch. Only the epoch count is bounded now. R2. run-experiment --run-names accepted `a` and `a.`, one directory on Windows, and the second run overwrote the first with exit 0. Names ending in a dot or a space, containing <>:"|?* or a control character, or naming a Windows reserved device are rejected on every platform before anything is written. R3. A desktop stop could sweep temporary files that belonged to the next run in the same folder: the sweep ran after the reservation was released, and a stop on a finished run swept too. Cancellation is intent and kill only, inert once the run is terminal, and holds the pid lock across the kill; the waiter retires the pid under that lock, sweeps, then releases, so a stop still killing finishes before the folder changes hands. The progress thread writes only while active. R4. Two searches converting the same vendor file concurrently shared one temporary output and one could publish the other's bytes. Each conversion writes a unique partial file under a lock beside the destination; a concurrent converter waits and reuses the result; a lock whose holder stopped writing for fifteen minutes is broken. R5. Domain checks for the numeric settings review A left unchecked, with the boundaries tested as accepted. R6. The fine-tune and re-prediction worker zipped predictions with peptidoforms without checking the count and silently kept the imported iRT for anything it did not predict. A count mismatch is an error; rows keeping their imported value are counted in <lib_out>.summary.json and the engine warns when there are any. Rows are not dropped: candidate_id is the contiguous index the fragment table references, and renumbering both tables for a rare event is a worse risk. R7. The audit's reported flag repeated the precursor gate; it follows the rejection reason now, a decoy past both gates is REMOVED_DURING_REPORTING, and a present precursor_q column of the wrong type is an error rather than a fallback. R8. The desktop reservation compared exact folders only; ancestors and descendants of an active folder are refused, siblings are not. R9. The Windows debug binary overflowed its 1 MiB main-thread stack on --version. The CLI runs on a thread with a 256 MiB reservation, and tests/cli_version.rs runs the built binary. Model identities carry the installed DeepLC and MS2PIP versions. With nothing to transfer the MBR worker writes a full-schema empty transfer table and the requested augmented scored table unflagged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t, E. coli FASTA run) in docs/30
…ansfer flag, script resolution, NaN calibration query, rescorer standardisation, output-over-input, and four regressions from A/D/E Package F of docs/31_code_review_2026-09-08_full.md: the six confirmed correctness and security defects, plus the regressions packages A, D and E introduced. F1. prescan read the infinite-bounds sentinel that rt-im-train writes for "calibration unavailable, search the whole gradient" as "cannot be screened" and dropped the candidate. On the documented FASTA/MS2PIP path, where no confident seed exists and every window row is infinite, that discarded the entire library and exited 0 with a zero-row survivors table; the single-label bail could not see it, being gated on a survivor count. An unbounded window now screens over the whole gradient, a candidate with no run_windows row is treated the same, both are counted and warned about, and screening every candidate away is an error. F2. A present-but-wrong-typed is_transferred was swallowed as "no transfers", silently removing every match-between-runs identification from peptides.tsv and proteins.tsv while the parquet still carried them. mbr_worker.py writes that column through pandas, so a nullable dtype or int8 reaches the stage. Present columns are read in their declared type and a mismatch is an error; only an absent column falls back. This is the absent-versus-malformed rule quant and audit already follow. F3. sidecar::resolve_script tried the working directory before the directory beside the binary, the ordering python::resolve_script_dir was hardened against and documents at length. That resolver only claims a directory holding mbr_worker.py or deeplc_worker.py, so a directory with any of the other ten workers reached resolve_script still relative and was executed from the working directory. An absolute directory is taken as given, then the executable's directory, then <exe>/scripts, and the working directory last. F4. Loess::predict indexed before the start of its grid for a non-finite query: both boundary comparisons are false, partition_point returns 0, and grid_x[j - 1] underflows. One library row with a null predicted_irt is enough, because the parquet reader maps a null f32 to NaN. It returns NaN now, and rt-im-train treats a non-finite library iRT as "no calibrated RT" and counts those rows. F5. The rescorer's in-memory TSV backend standardised with median/IQR while the parquet and streaming backends used mean/std, so one pool scored differently depending on rescore.handoff and on the 4 GB streaming threshold, with nothing logged. All three use mean/std, which leaves the shipped parquet default and every published benchmark unchanged. The MUMDIA_NN_FOLD_KEYS companion is length-checked instead of sliced short, which used to leave the tail rows in no fold, unscored, emerging from the rank normalisation at a plausible mid-rank score that satisfies the completeness contract. The backend-size estimate counts feature columns by name rather than subtracting a hardcoded three. F6. refuse_output_over_input existed and was wired into two of eighteen stages, so compete --features f.parquet --out f.parquet replaced the widest artifact of the run with the competed subset at exit 0. It guards every output of search-seed, rt-im-train, extract, features, compete, rescore, quant and audit. F7 (regression, package D). The LOESS boundary extrapolation slope was the pointwise local slope at the sparsest, most one-sided point of the fit: unbounded, free to be negative and so to invert the iRT-to-RT map, multiplying a distance that is unbounded by construction. It is the secant of the fitted curve over its end decile, clamped non-negative and to at most four times the global least-squares slope. The test uses noisy anchors rather than a noiseless quadratic. F8 (regression, package E). A stop arriving between the reap and the end of publish_exit could pass a recycled process id to the tree kill. The waiter retires the id the instant wait returns, before it reads the output directory. F9 (regression, package E). The conversion lock spun without pause on an undeletable stale lock, mistook clock skew and a peer's partial file for evidence about its own holder, could be held by two processes at once, and left every interrupted conversion's partial mzML behind for ever. Take-overs are bounded and paced, waiting has a deadline, the holder is identified by a token it reads back and only removes a lock that is still its own, a future modification time counts as fresh, the partial-file probe matches this destination only, and abandoned partials are swept under the lock. F10 (regression, package A). Dropping an unpredicted candidate with everything sharing its pair key also removed positional isomers that predicted correctly, bounded only by the library being emptied. The direct misses and the collateral are counted separately and exceeding 2% of the library is an error naming the sidecar. The key stays position-free deliberately: a positional key would stop matching a reverse decoy to its target, trading a sensitivity defect for an FDR one, and a test pins that trade. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
review F: whole-repository review docs/31 (F1-F10)
Merged
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.
Brings work packages A to F onto
main. They are already reviewed and merged, but into each other rather than intomain: the stack was chained child-to-parent, #61 went tomain, and the remaining six merged intoperf/ms2pip-worker-throughput,review/a-data-integrity,review/b-workers,review/c-desktop,review/d-provenanceandreview/e-followuprespectively.maintherefore carries #61 alone.Because the branches were stacked,
review/e-followupalready contains every package. Verified before opening this: each of the seven package heads (ab7049e,7587265,fdcbf3b,3cd4363,d7874f2,21b8ac7,2853ff2) is an ancestor of this branch,main's tree is identical to the #61 head somainholds nothing this branch lacks, and the merge against the shared base is clean. 14 commits, 66 files.What lands
source, NULL rejection in the streamed library loader, failure-preserving artifact publication, the Met-excision length boundary, the rescore memory ceiling checked before allocation on the real f32 layout, predictor misses dropped with their pair, numeric configuration domains, tracked-file scanners+1pseudocount and a permuted-null summary, the rescore-selected peak joined,augment_library.pykeeping imported base ids, benchmark fold hashing after the decoy prefixcargo auditfor the desktop lockfileprecursor_qwith a single-source guard,is_transferredandtransfer_qin both TSV reportsThree review records land with it:
docs/29_code_review_2026-09-07.md,docs/30_code_review_2026-09-08.mdanddocs/31_code_review_2026-09-08_full.md, none of which are onmaintoday. Ten second-tier findings are listed in docs/31 as deliberately not fixed.Validation already on record
run-experimentwith the shipped library configuration: 80,803 experiment-wide stripped peptides at 1%, 1.00% decoy fraction, 1:54 wall, recorded in docs/30.After merging
The other five review branches contain nothing this one lacks and can be deleted. A
0.2.0version bump is the next step before tagging, since the unreleased section changes user-visible behaviour: the default MS2PIP model, out-of-range configurations now failing at load, two new TSV columns, the renamed audit rejection code, and predictor misses dropping candidates.🤖 Generated with Claude Code