diff --git a/CHANGELOG.md b/CHANGELOG.md index 5539559..be097b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,12 @@ than a number. Both are recorded in every run's `manifest.json`. ### Changed +- Model identities carry the installed predictor versions: `deeplc-4.1.1-base`, + `deeplc-4.1.1-finetuned`, `ms2pip-4.2.0-HCDch2` in the library report and the manifests, + in place of the family labels `deeplc-4.0-mt` and `ms2pip-` (docs/30). +- With nothing to transfer, the MBR worker writes a transfer table with its ten columns and + zero rows, and the requested augmented scored table with every row unflagged, instead of + a one-column placeholder and no scored table (docs/30). - The candidate-audit rejection code `NO_PEAK_GROUP` is `DID_NOT_SURVIVE_EXTRACTION` (`RejectionReason::DidNotSurviveExtraction`). The audit assigns it to every candidate with no extracted row, and `extract` does not write the per-candidate table that would @@ -171,6 +177,41 @@ than a number. Both are recorded in every run's `manifest.json`. cancellation flag was written and never read. The waiter is now the only writer: it reads the intent after reaping the engine and publishes `cancelled`, `done` when the engine had already finished, or `failed`; until then the run shows "Stopping" (#14). +- Code review E, follow-up (`docs/30_code_review_2026-09-08.md`, R1 to R9): + - Enabling DeepLC fine-tuning with its own defaults was rejected at load, because the + documented automatic batch size is `finetune_batch = 0` and the new validation demanded + a positive batch. Only the epoch count has a lower bound now (R1; a regression from + review A). + - `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 (R2). + - A desktop stop could sweep temporary files that belonged to the next run in the same + folder: cancellation swept after the reservation had been released, and a stop on a + finished run swept as well. Cancellation is now intent and kill only and inert once the + run is terminal; the sweep happens in the waiter, after the reap and before the release, + and a stop still killing finishes before the folder changes hands (R3). + - 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 (R4). + - Domain checks for the numeric settings review A left unchecked: `mbr.q_anchor`, + `min_anchor_runs`, `extract.min_matched_fraction`, `features.bound_peak_fraction`, + `quant.reliable_q` and the remaining fractions, correlations, tolerances and counts (R5). + - The DeepLC fine-tune and re-prediction worker zipped predictions with peptidoforms + without checking the count and silently kept the imported iRT for anything missing. A + count mismatch is an error; rows that keep their imported value are counted in + `.summary.json` and the engine warns when there are any (R6). + - The audit's `reported` flag repeated the precursor gate, so it could read `true` next to + `FAILED_PEPTIDE_FDR`, and a decoy could be `REPORTED`; the flag now follows the reason, + a decoy past both gates is `REMOVED_DURING_REPORTING`, and a present `precursor_q` of the + wrong type is an error rather than a fallback (R7). + - The desktop results-folder reservation compared exact folders only, so a search into a + child of an active experiment's folder was allowed; ancestors and descendants are + refused, siblings are not (R8). + - 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 an integration test runs the built + binary (R9). - Code review D, calibration, provenance, reporting (`docs/29`, findings 10, 15, 16, 19): - LOESS retention-time calibration switched to the global least-squares line the moment a query left the anchor range, while the grid just inside used the local fit, diff --git a/desktop/src-tauri/src/run.rs b/desktop/src-tauri/src/run.rs index b785d5d..67f9129 100644 --- a/desktop/src-tauri/src/run.rs +++ b/desktop/src-tauri/src/run.rs @@ -79,11 +79,25 @@ fn ownership_key(dir: &Path) -> String { pub fn reserve_out_dir(dir: &Path, id: &str) -> Result { let key = ownership_key(dir); let mut active = ACTIVE_OUT_DIRS.lock().unwrap_or_else(|e| e.into_inner()); - if let Some(owner) = active.get(&key) { + // Equal keys, and also one folder inside the other (docs/30 R8): an experiment writes + // into its per-run subfolders and cleanup walks its whole folder, so a search into a + // child of an active experiment, or an experiment over the parent of an active search, + // is an overlapping writer. Component-wise, so `out` and `out2` stay independent. + if let Some((held, owner)) = active + .iter() + .find(|(held, _)| **held == key || paths_nest(held, &key)) + { + let relation = if *held == key { + "is in use".to_string() + } else if Path::new(&key).starts_with(Path::new(held)) { + format!("is inside the results folder {held}, which is in use") + } else { + format!("contains the results folder {held}, which is in use") + }; return Err(format!( - "the results folder {} is in use by a search that is still running ({owner}). \ + "the results folder {} {relation} by a search that is still running ({owner}). \ Wait for it to finish or stop it, or choose another folder: two searches \ - writing one folder overwrite each other's results.", + writing one folder tree overwrite each other's results.", dir.display() )); } @@ -91,6 +105,12 @@ pub fn reserve_out_dir(dir: &Path, id: &str) -> Result { Ok(key) } +/// True when one path is an ancestor of the other, by path components. +fn paths_nest(a: &str, b: &str) -> bool { + let (pa, pb) = (Path::new(a), Path::new(b)); + pa.starts_with(pb) || pb.starts_with(pa) +} + /// Give a reserved results folder back. pub fn release_out_dir(key: &str) { let mut active = ACTIVE_OUT_DIRS.lock().unwrap_or_else(|e| e.into_inner()); @@ -231,16 +251,39 @@ impl Run { /// publish `failed`, with the last log line as the "error", and this method then /// declined to replace a terminal status (docs/29 #14). pub fn cancel(&self) { + // Inert once terminal: there is no process to kill, and the folder may already + // belong to a later run (docs/30 R3). Cleanup is not done here at all any more: + // it belongs to `publish_exit`, which runs after the reap and before the + // reservation is released, so it can only ever touch this run's own files. + if !self.is_active() { + return; + } self.cancelled.store(true, Ordering::SeqCst); self.set(|s| s.cancel_requested = true); - let pid = self.pid.lock().ok().and_then(|p| *p); - if let Some(pid) = pid { + // The pid lock is held across the kill, and `publish_exit` retires the pid under + // the same lock before it sweeps and releases. A stop still in flight when the + // engine is reaped therefore finishes before the folder changes hands, and a stop + // that arrives after the reap finds no pid. + let guard = self.pid.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(pid) = *guard { kill_tree(pid); } - // A first sweep once the kill has returned. `publish_exit` sweeps again after - // the process is reaped, the only moment nothing can still be writing. - let out_dir = self.snapshot().out_dir; - sweep_temp_files(Path::new(&out_dir)); + drop(guard); + } + + fn is_active(&self) -> bool { + matches!(self.snapshot().status.as_str(), "running" | "starting") + } + + /// Apply `f` only while the run is still active; returns whether it was. + fn set_if_active(&self, f: F) -> bool { + if let Ok(mut s) = self.snapshot.lock() { + if matches!(s.status.as_str(), "running" | "starting") { + f(&mut s); + return true; + } + } + false } /// Publish the terminal state of the run from how its process ended. @@ -255,8 +298,16 @@ impl Run { /// engine finished before the kill landed and its outputs are complete, and /// calling them cancelled would hide a finished result. fn publish_exit(&self, outcome: std::io::Result, out_dir: &Path) { + // Retire the pid first. This waits for a stop that is still killing (it holds the + // same lock), so nothing below overlaps a kill, and a later stop finds nothing to + // signal (docs/30 R3). + if let Ok(mut p) = self.pid.lock() { + *p = None; + } let cancelled = self.cancelled.load(Ordering::SeqCst); if cancelled { + // The only sweep: after the reap, before the release, inside this run's + // ownership of the folder. sweep_temp_files(out_dir); } let stages = scan_stages(out_dir); @@ -671,15 +722,15 @@ pub fn start(id: String, req: Request) -> Result, String> { let started = Instant::now(); loop { let stages = scan_stages(&out_dir); - let running = { - let s = run.snapshot(); - s.status == "running" || s.status == "starting" - }; - run.set(|s| { + // Written only while the run is still active, under the snapshot lock: + // once `publish_exit` has published, a scan that was in flight must not + // replace the finished snapshot's stages with whatever the folder holds + // now, which may already be a later run's contents (docs/30). + let still_active = run.set_if_active(|s| { s.stages = stages; s.elapsed_ms = started.elapsed().as_millis() as u64; }); - if !running { + if !still_active { // The final scan belongs to the waiter, not here: it has to happen // BEFORE the status becomes terminal, or a caller that polls until // the run is finished can read a snapshot whose stages and results @@ -903,7 +954,8 @@ mod tests { let s = run.snapshot(); assert_eq!(s.status, "failed"); assert_eq!(s.error.as_deref(), Some("Error: no such file")); - assert!(s.cancel_requested); + // A stop that arrives after the end is inert and records nothing (docs/30 R3). + assert!(!s.cancel_requested); let _ = std::fs::remove_dir_all(&dir); } @@ -948,6 +1000,84 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn a_late_stop_after_the_run_ended_leaves_the_folder_alone() { + // docs/30 R3: A finished and released its folder; B took it and is writing. A + // stop delivered to A must neither sweep B's temporary file nor change A's state. + let (run, dir) = running("late_stop"); + let key = reserve_out_dir(&dir, "run-A").unwrap(); + *run.reservation.lock().unwrap() = Some(key); + run.publish_exit(Ok(exit_status(1)), &dir); + assert_eq!(run.snapshot().status, "failed"); + let key_b = reserve_out_dir(&dir, "run-B").expect("A released its folder"); + let b_file = dir.join("new.parquet.tmp-999-1"); + std::fs::write(&b_file, b"B's partial write").unwrap(); + run.cancel(); + assert!( + b_file.is_file(), + "a late stop must not sweep another run's files" + ); + let s = run.snapshot(); + assert_eq!(s.status, "failed"); + assert!(!s.cancel_requested, "a terminal run records no stop"); + release_out_dir(&key_b); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_stop_still_in_flight_finishes_before_the_folder_is_released() { + // docs/30 R3, the concurrent route: the engine is reaped while the stop thread is + // still inside the kill. Publication must wait for the kill to finish, so the + // reservation cannot be released, and taken by a new run, while the stop is + // still active in that folder. The kill is simulated by holding the pid lock. + let (run, dir) = running("inflight_stop"); + let key = reserve_out_dir(&dir, "run-A").unwrap(); + *run.reservation.lock().unwrap() = Some(key); + run.cancelled.store(true, Ordering::SeqCst); + let killing = run.pid.lock().unwrap(); + let (r2, d2) = (Arc::clone(&run), dir.clone()); + let waiter = std::thread::spawn(move || r2.publish_exit(Ok(exit_status(1)), &d2)); + std::thread::sleep(Duration::from_millis(300)); + assert!( + reserve_out_dir(&dir, "run-B").is_err(), + "the folder must stay reserved while the stop is in flight" + ); + assert_eq!( + run.snapshot().status, + "running", + "nothing is published mid-kill" + ); + drop(killing); + waiter.join().unwrap(); + assert_eq!(run.snapshot().status, "cancelled"); + let k = reserve_out_dir(&dir, "run-B").expect("released once the stop completed"); + release_out_dir(&k); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn overlapping_result_folders_are_refused_in_both_orders_but_siblings_are_not() { + // docs/30 R8: an experiment owns its per-run subfolders and its cleanup walks the + // whole tree, so a parent and a child are one writer. + let parent = scratch("nest"); + let child = parent.join("run1"); + let sibling = std::env::temp_dir().join(format!("mumdia_run_{}_nest2", std::process::id())); + std::fs::create_dir_all(&child).unwrap(); + std::fs::create_dir_all(&sibling).unwrap(); + let k = reserve_out_dir(&parent, "run-1").unwrap(); + let e = reserve_out_dir(&child, "run-2").unwrap_err(); + assert!(e.contains("is inside") && e.contains("run-1"), "{e}"); + let ks = reserve_out_dir(&sibling, "run-3").expect("a sibling is independent"); + release_out_dir(&k); + release_out_dir(&ks); + let kc = reserve_out_dir(&child, "run-2").unwrap(); + let e = reserve_out_dir(&parent, "run-1").unwrap_err(); + assert!(e.contains("contains") && e.contains("run-2"), "{e}"); + release_out_dir(&kc); + let _ = std::fs::remove_dir_all(&parent); + let _ = std::fs::remove_dir_all(&sibling); + } + #[test] fn a_results_folder_owned_by_an_active_run_is_refused_to_a_second() { let dir = scratch("owned"); diff --git a/docs/04_convert.md b/docs/04_convert.md index 9617003..1bb2504 100644 --- a/docs/04_convert.md +++ b/docs/04_convert.md @@ -540,7 +540,13 @@ than its input is either from a different acquisition of the same name or from before the input was re-acquired, and searching it would search the wrong data. An unreadable timestamp counts as not reusable. -The converter writes to `.partial.mzML` and the engine renames it to +The converter writes to `.partial--.mzML`, a name unique to this +conversion, under a `.mzML.converting` lock beside the destination; a second +process converting the same input waits for the lock and reuses the finished mzML +instead of converting into the same destination (docs/30 R4: two concurrent +conversions used to share one partial file, and one of them published the other's +bytes). A lock whose holder stopped writing for fifteen minutes is broken. The engine +then renames the partial file to `.mzML` only after a zero exit and a file at that path, so a killed run or a converter crash leaves nothing the reuse rule can mistake for a finished conversion; a stale `.partial.mzML` is removed at the next attempt. The marker sits in the stem diff --git a/docs/06_predict_frag_index_matchers.md b/docs/06_predict_frag_index_matchers.md index a8df45b..3701a1f 100644 --- a/docs/06_predict_frag_index_matchers.md +++ b/docs/06_predict_frag_index_matchers.md @@ -156,9 +156,10 @@ paired decoy leave together; the counts land in the library report as used to be anchored at `irt = 0.0` with a warning, which collapsed the RT window onto the gradient origin for those candidates (docs/29 #17). `run_deeplc` also rejects a returned id that was not requested or that appears twice. The DeepLC branch requires `deeplc_python` -and errors otherwise (`predict_frag.rs:318-321`); its returned model id is the -hardcoded string `"deeplc-4.0-mt"` (`predict_frag.rs:353`), not a trait -`identity()` (the sidecar path has no `RtPredictor` impl to query). +and errors otherwise (`predict_frag.rs:318-321`); its returned model id is +`deeplc--base`, the version read from the interpreter, not a trait +`identity()` (the sidecar path has no `RtPredictor` impl to query). It was the literal +`"deeplc-4.0-mt"` until docs/30, which named a family, not the release that predicted. **intensity assignment** (`assign_intensities`, `predict_frag.rs:359`). Native path calls `NativeFrag::predict_intensities`. MS2PIP path runs the sidecar over @@ -422,8 +423,8 @@ m/z (`Library::local_frag_index`, `index.rs:325`). | `PredictFragParams` | `predict_frag.rs:24` | Stage C entry args: in/out paths, `cfg`, `work_dir`, `config_hash` | | `predict_frag::run` | `predict_frag.rs:50` | Stage C entry: parse, fragment, assign intensity/iRT, top-N, sort, write; returns `(n_prec, n_frag)` | | `Raw` | `predict_frag.rs:34` | one candidate pre-assignment; caches the `ParsedPeptidoform` so RT/intensity reuse the parse | -| `assign_rt` | `predict_frag.rs:308` | native or DeepLC iRT; emits the DeepLC-miss warning; DeepLC id `"deeplc-4.0-mt"` | -| `assign_intensities` | `predict_frag.rs:359` | native or MS2PIP intensity with per-charge-group normalization + native charge-2 fallback; MS2PIP id `"ms2pip-{model}"` | +| `assign_rt` | `predict_frag.rs:308` | native or DeepLC iRT; emits the DeepLC-miss warning; DeepLC id `deeplc--base` | +| `assign_intensities` | `predict_frag.rs:359` | native or MS2PIP intensity with per-charge-group normalization + native charge-2 fallback; MS2PIP id `ms2pip--{model}` | | `fragment_cardinality` | `predict_frag.rs:457` | distinct precursors per 0.01 Da fragment-m/z bin, per fragment row; diagnostic column, no consumer yet | | `RtPredictor` / `FragmentPredictor` | `predict.rs:13` / `predict.rs:19` | predictor traits (predict + `identity`); implemented only by the native structs | | `NativeRt` | `predict.rs:25` | additive retention-coefficient model + `sqrt(len)` + `0.01*mod` term, `identity` `native-rt-v1` | diff --git a/docs/12_quant_lfq_align_mbr_report_audit.md b/docs/12_quant_lfq_align_mbr_report_audit.md index 9a2a05a..796452d 100644 --- a/docs/12_quant_lfq_align_mbr_report_audit.md +++ b/docs/12_quant_lfq_align_mbr_report_audit.md @@ -110,8 +110,10 @@ and one `psms.parquet` per run in `source` order. Produces `.parquet`, one per accepted transfer (`mbr_worker.py:254`): `candidate_id`, `source`, `peptidoform`, `charge`, `protein_group`, `label`, `expected_rt`, `observed_rt`, `rt_delta`, `transfer_q` (10 columns). When there are no transfer candidates at all the worker -short-circuits and writes a placeholder table with a single empty `candidate_id` -column (`pa_write_empty`, `mbr_worker.py:289`), so `.parquet` always exists. +short-circuits and writes the same ten columns with zero rows (`write_empty_transfers`), +and, when `--out-scored` was asked for, the scored table unchanged with `is_transferred` +false and `transfer_q` NaN on every row (`write_unflagged_scored`), so a downstream stage +never meets a missing file or a one-column placeholder (docs/30). Optionally writes an augmented scored table (`--out-scored`) that lowers each accepted transfer's PSM q columns to `min(q, transfer_q)` on the matching `(candidate_id, source)` row and adds an `is_transferred` bool plus a `transfer_q` column (the accepted @@ -535,7 +537,7 @@ FDR/reporting). | `run_mbr` | sidecar.rs:162 | build argv and spawn `mbr_worker.py` | | `binned_map` | mbr_worker.py:31 | monotone binned-median RT calibration | | `expected_rt` | mbr_worker.py:116 | cross-run predicted RT for a candidate in a run | -| `pa_write_empty` | mbr_worker.py:289 | placeholder output when there are no transfer candidates | +| `write_empty_transfers` / `write_unflagged_scored` | mbr_worker.py | full-schema outputs when there are no transfer candidates | | `ReportParams` / `report::run` | report.rs:13 / 49 | TSV writer | | `strip` | report.rs:24 | stripped sequence from a peptidoform | | `qcell` | report.rs:39 | quantity cell formatting (1 decimal; empty on NaN) | @@ -648,10 +650,14 @@ but do not affect the wired `mumdia mbr` path. `traces_extracted` are all set from the same `traces` flag (`audit.rs:170`) because the artifacts only record presence in `psms`; the (not-yet-written) in-extract sidecar would be the only way to split "no traces" from "traces but no accepted peak". -- **audit `reported` vs `REPORTED`.** The `reported` bool column is set from - `passed_prec` alone (`audit.rs:176`), while the `REPORTED` rejection reason additionally - requires the peptide gate. A candidate can therefore have `reported=true` yet - `rejection_reason=FAILED_PEPTIDE_FDR`. Treat `rejection_reason` as authoritative. +- **audit `reported` and `REPORTED` agree.** The `reported` bool is `rejection_reason == + REPORTED`: a target that passed the precursor and the peptide gate. A decoy that passes + both is `REMOVED_DURING_REPORTING`, because the report never writes a decoy. The two + gate columns (`passed_precursor_fdr`, `passed_peptide_fdr`) remain the diagnostics. + Until docs/30 R7 the flag repeated the precursor gate alone, so a row could read + `reported=true` next to `FAILED_PEPTIDE_FDR`, and a decoy could be `REPORTED`. A present + `precursor_q` column of the wrong type is an error; only an absent column falls back to + the PSM `q_value` (recorded as `q_unit`). - **audit reason coverage.** In the current chain `audit.rs` can only ever emit `DID_NOT_SURVIVE_EXTRACTION`, `OUTCOMPETED_BY_TARGET`/`OUTCOMPETED_BY_DECOY`, `FAILED_PRECURSOR_FDR`, `FAILED_PEPTIDE_FDR`, and `REPORTED`. The five refined extract codes diff --git a/docs/13_sidecars.md b/docs/13_sidecars.md index 3b390ae..2a5365d 100644 --- a/docs/13_sidecars.md +++ b/docs/13_sidecars.md @@ -157,7 +157,8 @@ code. observed_rt, rt_delta, transfer_q` (`mbr_worker.py:254-265`). Optional `--out-scored` writes the scored table with accepted transfers' PSM q columns lowered to `transfer_q`, an `is_transferred` flag and a `transfer_q` column (NaN - on non-transferred rows) added. Optional + on non-transferred rows) added; with no transfer candidates the transfer table has + its ten columns and zero rows and the augmented table is the input, unflagged. Optional `--emit-transfer-targets` writes per-run `run_windows`-format tables (`candidate_id, rt_pred_cal, rt_lo, rt_hi, im_*`) plus a permuted-RT decoy file for the re-extraction tier (`mbr_worker.py:142-151`). diff --git a/docs/24_config_reference.md b/docs/24_config_reference.md index 496f22b..10899bf 100644 --- a/docs/24_config_reference.md +++ b/docs/24_config_reference.md @@ -789,8 +789,8 @@ one exception noted in its own help text: it sets `MUMDIA_NN_THREADS` and | `NUMEXPR_NUM_THREADS` | sidecar | `"1"` | `scripts/deeplc_finetune.py:32` | | `OMP_NUM_THREADS` | both | `"1"` in deeplc_finetune.py; `n.to_string()` in main.rs | `rust/mumdia/crates/mumdia/src/main.rs:94`, `scripts/deeplc_finetune.py:29` | | `OPENBLAS_NUM_THREADS` | sidecar | `"1"` | `scripts/deeplc_finetune.py:30` | -| `PYTHONIOENCODING` | engine | `"utf-8"` | `rust/mumdia/crates/mumdia/src/sidecar.rs:371` | -| `PYTHONUTF8` | engine | `"1"` | `rust/mumdia/crates/mumdia/src/sidecar.rs:371`, `rust/mumdia/crates/mumdia/src/stages/rescore.rs:1103`, `rust/mumdia/crates/mumdia/src/stages/rescore.rs:1341` | +| `PYTHONIOENCODING` | engine | `"utf-8"` | `rust/mumdia/crates/mumdia/src/sidecar.rs:409` | +| `PYTHONUTF8` | engine | `"1"` | `rust/mumdia/crates/mumdia/src/sidecar.rs:409`, `rust/mumdia/crates/mumdia/src/stages/rescore.rs:1103`, `rust/mumdia/crates/mumdia/src/stages/rescore.rs:1341` | ## Unresolved by the generator diff --git a/docs/30_code_review_2026-09-08.md b/docs/30_code_review_2026-09-08.md new file mode 100644 index 0000000..6ce3a1a --- /dev/null +++ b/docs/30_code_review_2026-09-08.md @@ -0,0 +1,265 @@ +# MuMDIA follow-up code review — 2026-09-08 + +Baseline: `d7874f2099e9bfb9ced3069e2f53ea8e942a2584`. Compared with the original review baseline `ab7049e`, including the four implemented work packages and their surrounding callers. This review changes no implementation. + +**The earlier corrections are substantially in place, but the tree is not ready for an “everything fixed” sign-off.** Nine remaining findings are listed below. The most urgent are a new fine-tuning configuration regression and three output-integrity problems. The ordinary native release pipeline passes its smoke tests. + +Scope: first-party engine/core/IO/numerical code, Python workers and library helpers, desktop supervision and UI, tests, benchmark utilities, and delivery configuration. The tracked implementation inventory is 81 Rust files (44,839 lines), 42 Python files (12,332 lines), and three JavaScript/HTML/CSS files (2,456 lines). Verification concentrated on the 21 previous findings, changed code, and contracts between components. External comparison checkouts, acquisition datasets, and historical result directories are outside the source review. This is a code and test review, not proof that every scientific workflow is correct. + +P1 means fix before relying on the affected workflow; P2 means a correctness or reliability follow-up. “Reproduced” below identifies an executed probe; “source-confirmed” identifies a code path whose consequence was established without a full biological-data run. + +| Priority | Finding | Trigger | +|---|---|---| +| P1 | R1. Validation rejects the documented automatic fine-tune batch | Enable DeepLC fine-tuning without overriding its default batch | +| P1 | R2. Windows run-name aliases still overwrite runs | Experiment run names such as `a` and `a.` | +| P1 | R3. Finished-run cancellation can delete the next run's files | Late or still-running cancellation after output-directory release | +| P1 | R4. Concurrent vendor conversions share a temporary output | Two searches convert the same input concurrently | +| P2 | R5. Several active numeric settings still bypass domain checks | Invalid extraction fractions, anchor thresholds, or reliability thresholds | +| P2 | R6. The library RT rewrite still accepts incomplete predictions | Short prediction result in the fine-tune/re-prediction worker | +| P2 | R7. Audit reporting flags, reasons, and counts disagree | Precursor/peptide q disagreement, decoys, or malformed precursor q | +| P2 | R8. Output reservations allow overlapping directory trees | Concurrent parent experiment and child-directory search through the backend | +| P2 | R9. The Windows debug CLI overflows its initial stack | Normal debug binary invoked even with `--version` | + +**R1. Preserve `finetune_batch = 0` as automatic sizing. Reproduced; regression in package A.** + +`rust/mumdia/crates/mumdia-core/src/config.rs:2110` rejects `finetune_batch == 0` whenever `finetune_deeplc` is enabled. However, the field documentation at line 592 and the default at line 677 explicitly define zero as automatic sizing. `scripts/deeplc_finetune.py` implements that automatic mode. + +This minimal configuration now fails `Config::from_json`: + +```json +{"rt_im_train":{"finetune_deeplc":true}} +``` + +The returned error is `rt_im_train.finetune_epochs and finetune_batch must be >= 1 when finetune_deeplc is on`. Thus simply enabling a supported feature makes its defaults invalid, before any model is launched. Changing the batch to an arbitrary positive number also changes the intended training recipe. + +Keep the positive-epoch check, retain zero's documented batch semantics, and test enabling fine-tuning with an omitted batch, an explicit zero, and a positive override. + +**R2. Reject trailing-dot/space run-name aliases on Windows. Reproduced end to end; remaining gap in original finding 5.** + +`rust/mumdia/crates/mumdia/src/stages/run_experiment.rs:331` now compares names case-insensitively, but the directory-name check at line 419 only rejects empty names, separators, `.` and `..`. It accepts `a` and `a.`, which address the same directory in this Windows execution path. + +I ran the release binary over the generated smoke fixtures with `--run-names a --run-names a.`. The command exited **0**, created only one physical `a` directory, and both logical split paths contained `source = 1`. Source 0 had been overwritten. This is a successful experiment with broken run identity, not merely a naming error that stops execution. + +Validate portable directory names before computation. In particular, reject trailing dots/spaces and Windows-invalid names, in addition to case-insensitive duplicates. Test the actual per-run paths and their source columns after a valid experiment, and reject the alias pair before writing artifacts. Filesystem probing is unnecessary for these syntactic aliases. + +**R3. Keep cancellation cleanup inside the run's ownership lifetime. Reproduced; remaining gap in original finding 14.** + +`desktop/src-tauri/src/run.rs:233` performs cancellation and a recursive temporary-file sweep even if the run is already terminal. `publish_exit` releases the directory reservation at line 267 without coordinating with an outstanding `cancel()` call. The stored PID is never cleared. The command at `desktop/src-tauri/src/main.rs:403` also accepts cancellation of retained, finished run handles. + +A probe using the production `Run` methods performed this sequence: + +1. Finish run A and release its reservation. +2. Reserve the same directory for run B and create `new.parquet.tmp-999-1`. +3. Deliver a late cancellation to run A. + +The new file was deleted while A remained `failed`. No process was killed in this reproduction: A deliberately had no PID. The deletion follows from `sweep_temp_files` at line 401 matching every `.tmp-` filename, regardless of owner. + +There is also an ordinary concurrent route: A's waiter can reap the engine and release the folder while A's cancellation thread is still finishing `kill_tree`, after which that thread sweeps the reused folder. Unix's TERM/KILL delay makes the separation especially visible. + +Make terminal cancellation inert, retire the PID when the child is reaped, and synchronize cancellation completion with final publication and reservation release. Cleanup should affect only artifacts owned by the ending run. Test late cancellation and an in-flight cancellation overlapping a subsequent start; the existing status-only race tests miss file ownership. + +**R4. Give concurrent vendor conversions independent output ownership. Reproduced with converter stubs.** + +`rust/mumdia/crates/mumdia/src/raw.rs:637` always uses `.partial.mzML`, removes any existing file at that path, and passes it to the converter. Publication at line 774 then renames that shared file. Different search output directories do not isolate this path: conversion normally writes beside the acquisition input. + +A deterministic probe ran the actual `raw::ensure_mzml` concurrently with two small converter executables. Converter A wrote its bytes; converter B replaced the shared temporary file; A then completed. **A returned success with converter B's bytes.** B subsequently failed because its temporary output had already been renamed away. This demonstrates the ownership failure without depending on proprietary acquisition data or converter timing. + +Use unique temporary paths and coordinate ownership of the converted destination. Unique temporary names alone do not fully solve the problem if two conversion recipes can replace the same final mzML while another search is about to read it. An input/recipe-specific conversion cache or an appropriately scoped conversion reservation can make the consumed artifact stable. Preserve the existing `.mzML` extension requirement. + +**R5. Finish numeric validation for the active configuration surface. Reproduced; original finding 18 remains partial.** + +The domain checks added at `rust/mumdia/crates/mumdia-core/src/config.rs:1988` validate several important settings, but leave other active fractions and thresholds unchecked. All of these still pass `Config::from_json`: + +```json +{"mbr":{"q_anchor":-0.1}} +{"mbr":{"q_anchor":2.0,"min_anchor_runs":0}} +{"extract":{"min_matched_fraction":2.0}} +{"features":{"bound_peak_fraction":-1.0}} +{"quant":{"reliable_q":2.0}} +``` + +These fields are used. For example, `extract.rs:2132` compares the matched fraction with `min_matched_fraction`, so a value above one rejects the candidates. `quant.rs:769` uses `reliable_q` to select the supposedly confident population for consensus bounds. The MBR worker directly receives `q_anchor` from `run_experiment.rs:678`. + +Audit the remaining active numeric fields by their documented domains, including anchor support and correlation thresholds. Apply checks at the relevant entry points; standalone command-line thresholds should not bypass the same contract. Preserve intentional zero sentinels, as R1 illustrates. Test invalid values alongside supported boundary values rather than adding only rejection cases. + +**R6. Apply prediction coverage checks to the library rewrite too. Reproduced with a predictor stub; adjacent gap to original finding 17.** + +The new `predict-frag` handling correctly drops uncovered candidates with their pairs. The separate full-library path in `scripts/deeplc_finetune.py:274` still zips `batch` with predictions without checking length, then uses `preds.get(base_pf(pf), orig[i])` at line 287. Both fine-tuning and `run_deeplc_repredict` use this worker. + +I executed the current rewrite control flow with three standard peptidoforms, original iRT values `[10, 20, 30]`, and a predictor returning just `[100]`. It exited normally and wrote `[100, 20, 30]`, while announcing a library re-predicted with the DeepLC base model. That mixes RT scales and hides missing predictions. The probe replaced the ML implementation, not the loop and fallback responsible for the result. + +Check result shape and coverage before rewriting the library. Preserve the agreed policy of dropping unsupported candidates with a counted warning rather than aborting a whole-proteome build for a few misses. This rewrite must coordinate precursor/fragment references and target-decoy pairing when dropping rows. Treat a structurally malformed predictor response distinctly from a documented unsupported peptidoform. If retained imported values remain an intentional mode, make their count and RT-source mixture explicit. + +**R7. Make audit's `reported` field agree with its reasons and actual report eligibility. Reproduced; adjacent gaps in original finding 16.** + +`rust/mumdia/crates/mumdia/src/stages/audit.rs:188` assigns `FAILED_PEPTIDE_FDR` when precursor q passes but peptide q fails. Line 208 nevertheless writes `reported = passed_prec`. The metrics count `REPORTED` reasons at line 237, producing a different reported count. Decoys can also receive `REPORTED`, although the report stage excludes them. + +A three-row probe produced: + +```text +precursor_id: [1, 2, 3] +reported: [true, true, true] +rejection_reason: [FAILED_PEPTIDE_FDR, REPORTED, REPORTED] +metrics.reported: 2 +``` + +Row 3 was a decoy. Thus neither the row flags nor the reason/count pair consistently represents the final target report. + +There is a related error-handling gap at line 109: any failure to read `precursor_q`, including a wrong declared type, triggers the legacy PSM-q fallback. An explicitly present Int32 precursor-q column was accepted, and metrics recorded `q_unit = q_value` instead. This repeats the absent-versus-malformed mistake fixed in quant. + +Use one explicit acceptance definition for the audit's reporting flag, rejection reason, and summary. Keep precursor-gate diagnostics separate from final-report membership, which must follow the report's existing target/q/transfer rules. Restrict legacy fallback to genuinely absent columns. Test opposite-side precursor/peptide q values, decoy rows, and a malformed present precursor-q column. This does not require the deferred export redesign. + +**R8. Reserve overlapping output trees, not just equal directory strings. Reproduced at the backend API.** + +`desktop/src-tauri/src/run.rs:79` only checks `active.get(&key)`. Reserving a directory for an experiment and then reserving its `run1` child for another search both succeed. An experiment writes inside its per-run subdirectories, and cancellation cleanup walks the entire reserved parent recursively, so these are overlapping writers even though the keys differ. + +The frontend's one-active-run check limits the ordinary single-window trigger; this finding concerns the backend reservation contract and callers of `run::start`. It is separate from duplicate-click protection, which is implemented. + +Check ancestor/descendant overlap using canonical path components while holding the reservation lock. Avoid plain string-prefix comparison, which would incorrectly equate siblings such as `out` and `out2`. Test both reservation orders and a non-overlapping sibling. + +**R9. Make the normal Windows debug CLI runnable. Reproduced.** + +After `cargo build --locked`, `C:/Users/robbi/mumdia_build/debug/mumdia.exe --version` aborts with `thread 'main' has overflowed its stack`. This happens directly from PowerShell as well as from the smoke harness, before a search begins. The CLI entry point is `rust/mumdia/crates/mumdia/src/main.rs:1036`. + +The PE header reserves 1 MiB of stack. Changing only that reservation to 16 MiB in a temporary copy makes the identical debug binary print its version successfully. This establishes a stack-size problem; the exact allocation responsible was not isolated. The release binary works and passed the full smoke test. + +Reduce the debug entry/dispatch stack requirement, or establish and document an appropriate Windows stack reservation. Add a subprocess test of the normal debug executable's help/version path. Library unit tests and release-only CLI smoke coverage currently miss this developer-build failure. + +**Status of the original 21 findings** + +| Original findings | Current assessment | +|---|---| +| 1, 2, 4, 9 | Fixed for the reported triggers; the previous executable probes now reject pooled U32 quant and NULL fragments, preserve an existing artifact after publication failure, use distinct temp paths, and retain the Met-excised boundary peptide. | +| 3 | Fixed: class-deficient training folds error and non-decoy held-out gaps are not filled in sample. | +| 5 | Duplicate-start and case-only fixes implemented; remaining output-ownership gaps are R2 and R8. | +| 6, 7, 8 | Implemented: named permuted-null diagnostics, `+1` numerator, and selected-peak join. MBR/top-K tests pass. | +| 10 | Endpoint-continuity fix implemented and tested. Both HYE B01 comparison pairs are documented in the previous review's implementation-status section; those biological-data runs were not repeated here. | +| 11 | Fixed: matrix sizing uses the flat f32 layout and checks before allocation. | +| 12 | Implemented: augmentation reuses the canonical stripped sequence's base ID. | +| 13 | Implemented: converter probe carries request configuration and mirrors the auto-parser/msconvert fallback. | +| 14 | Single terminal-state writer implemented, but cancellation lifetime remains unsafe: R3. | +| 15 | Resolved config, effective quant filter, start-time input hashes, and model-identity fields added. More precise model provenance is still useful; see below. | +| 16 | Correct precursor-q read, pooled-table rejection, and coarse extraction label added; R7 remains. | +| 17 | Original `predict-frag` gap corrected; the adjacent library RT rewrite remains exposed: R6. | +| 18 | Partial, with a new valid-config regression: R1 and R5. | +| 19 | Accepted minimal scope implemented: TSV transfer flag/q columns and documented acceptance rule. The separate-threshold/protein-confidence redesign remains deferred. | +| 20 | Accepted minimal scope implemented: strip `DECOY_` before benchmark hashing and record recipe metadata. Production/benchmark fold identity is still explicitly different where expected. | +| 21 | Tracked-file scanners implemented; no `-covr2` copies found in the checked source/test/workflow trees. No scratch files were moved or deleted during this review. | + +**Other focused cleanup opportunities** + +- Model identities remain mostly family or recipe labels. `predict_frag.rs:378` returns the literal `deeplc-4.0-mt`; experiment provenance reconstructs RT/fragment names from configuration at `run_experiment.rs:822`. Preserve installed worker versions and checkpoint identifiers/hashes where available, alongside the intended recipe. Do not interpret the current strings as a complete software/model identity. The rescorer now records the actual executed fallback correctly. +- `scripts/mbr_worker.py:232` exits successfully without writing a requested `--out-scored` when there are no candidates, and its empty transfer file has only `candidate_id`. The experiment orchestrator handles absence, but the standalone worker has an unstable output contract. Its current test explicitly pins this behavior. A full-schema empty transfer table and an unchanged augmented scored output would make downstream use simpler and safer. +- Several fixed-contract tests still construct lookalike fixtures instead of connecting the producer and consumer. Add the originally requested rescore-emitted-table-to-quant check, a worker-to-report transfer-q check, and lifecycle tests that assert artifact ownership. The fine-tune regression also calls for testing valid non-default feature combinations. +- The desktop progress thread can still write stage snapshots after the waiter publishes completion (`run.rs:677` onward). Stop updates once terminal and coordinate the final scan with publication, so a stale scan or a new run's directory contents cannot replace the old run's finished snapshot. This fits the R3 lifecycle work. + +The earlier deferrals remain deferrals: unified orchestration, resume/recovery, relocatable provenance, separate peptide/precursor exports, shared benchmark utilities, MBR redesign, and rewriting the repository's inline incident-history style. They are not prerequisites for this correctness pass. + +**Verification performed** + +| Check | Result | +|---|---| +| Engine `cargo test --workspace --locked --quiet` | 258 tests passed across workspace unit, integration, and documentation test groups. | +| Engine formatting, Clippy (`--workspace --all-targets`, warnings denied), Rustdoc (warnings denied) | Passed. | +| Engine debug and release builds | Built successfully; debug runtime failure is R9. | +| Release native smoke workflow | Passed all 144 assertions, including repeatability, malformed-RT handling, the single-run and experiment orchestrators, splitting, quantification, LFQ, manifests, and reports. | +| Desktop library tests | 78 passed using a separate build directory outside OneDrive. | +| Desktop integration tests with the generated fixture and current release engine | Search completion and cancellation tests passed. The installation test returned early because `MUMDIA_TEST_INSTALL=1` was not set; it was not exercised. | +| Base Python suite | 67 passed, 12 skipped for optional dependencies. | +| Mokapot/NN subset in existing `py312_mumdia` environment | 11 passed, including previously skipped rescorer tests. This overlaps some base-suite tests; counts are not additive. | +| Predictor suite in existing `deeplc_mt` environment | 11 passed, 3 skipped. That environment has an older DeepLC than the required version and an MS2PIP/Pydantic import incompatibility. Real current DeepLC/MS2PIP prediction remains unverified here. | +| Workflow structure, desktop UI references, documentation references, generated CLI/configuration references and configuration schema | Passed. | +| Current engine dependency audit under repository policy | Passed; one allowed unmaintained-crate warning. | +| Current desktop dependency audit under repository policy | Passed with the repository's explicit `RUSTSEC-2024-0429` ignore and 16 allowed unmaintained-crate warnings. This is not an assertion of zero advisories. | +| Targeted regression probes | Confirmed R1–R9 at the levels specified above, and rechecked original integrity failures. | + +Probe sources, fixtures, logs, and temporary executable copies are under `C:/Users/robbi/AppData/Local/Temp/mumdia_review_20260908/`. Production source and user results were not modified. The smoke script was copied there with its working/output paths redirected and its initial recursive deletion removed; the pipeline commands and assertions were retained. Dependency environments were used as found, without installing or upgrading packages. + +No fresh Linux/macOS execution, packaged GUI interaction, container/release packaging test, real vendor conversion, full biological-data experiment, or model retraining benchmark was completed in this pass. The converter and incomplete-prediction reproductions used controlled stubs as stated. These limits do not weaken the demonstrated ownership/configuration failures, but they do limit broader scientific and platform sign-off. + +--- + +## Implementation status (2026-09-08) + +All nine findings are addressed in one further stacked pull request, E (`review/e-followup`, +on D), with a test per finding. Two of the accepted changes were narrowed on purpose: + +- **R6.** The library rewrite does not drop rows. A row's `candidate_id` is the contiguous + index the fragment table references, so dropping rows there would mean renumbering both + tables for a rare event. Instead a prediction count that differs from the batch is a hard + error, a peptidoform without a finite prediction keeps its imported value, and the counts + (`repredicted`, `retained_imported`, split into non-standard residues and no finite + prediction) are written to `.summary.json` and warned about by the engine. +- **R4.** No recipe-keyed conversion cache. The temporary name is unique per conversion + (`.partial--.mzML`) and a `.converting` lock makes a + concurrent converter of the same input wait and reuse the result. Two different + conversion recipes aimed at one destination remain a documented limitation of writing + beside the input. + +| Finding | Change | Test | +|---|---|---| +| R1 | `finetune_batch = 0` is the automatic batch again; only `finetune_epochs >= 1` is required | `enabling_the_fine_tune_keeps_the_automatic_batch` | +| R2 | `portable_dir_name_problem`: trailing dots and spaces, `<>:"|?*` and control characters, and the Windows reserved device names are rejected on every platform, before anything is written | `run_names_that_alias_on_windows_are_rejected_everywhere` | +| R3 | `cancel` is intent and kill only, inert once terminal, and holds the pid lock across the kill; `publish_exit` retires the pid under that lock, sweeps, then releases; the progress thread writes only while active | `a_late_stop_after_the_run_ended_leaves_the_folder_alone`, `a_stop_still_in_flight_finishes_before_the_folder_is_released` | +| R4 | unique partial names and the conversion lock | `a_conversion_lock_is_exclusive_and_released_on_drop`, `a_stale_lock_is_broken_and_a_fresh_one_is_honoured` | +| R5 | domain checks for the remaining numeric fields, boundaries accepted | `numeric_domains_are_validated_at_load` | +| R6 | structural mismatch is an error, retained imported values are counted and reported | `rewrite_irt` summary; engine warning | +| R7 | `reported` is `rejection_reason == REPORTED`; decoys past both gates are `REMOVED_DURING_REPORTING`; a malformed present `precursor_q` is an error | `the_reported_flag_follows_the_reason_and_the_report_rules`, `a_present_but_malformed_precursor_q_column_is_an_error_not_a_fallback` | +| R8 | reservations refuse an ancestor or descendant of an active folder, by path components | `overlapping_result_folders_are_refused_in_both_orders_but_siblings_are_not` | +| R9 | the CLI runs on a thread with a 256 MiB stack reservation | `tests/cli_version.rs` runs the built binary's `--version` and `--help` | + +Of the cleanup notes: model identities carry the installed DeepLC and MS2PIP versions +(`deeplc-4.1.1-base`, `ms2pip-4.2.0-HCDch2`, `deeplc-4.1.1-finetuned`); the MBR worker +writes a full-schema empty transfer table and an unflagged augmented scored table when +there is nothing to transfer; the desktop progress thread no longer writes after +publication. Checkpoint hashes and the producer-to-consumer unit tests beyond the smoke +harness are not in E. + +## Release check on real data (2026-09-08) + +Run on doxy with a binary built from the D head (`d7874f2`), which is the code of PRs #61 +to #65; E changes no stage behaviour on these paths. Two runs, both from the shipped example +configurations with the doxy interpreters (`configs/examples/diann-library.json`, +`configs/examples/fasta-sidecars.json`), `mumdia doctor --config` passing on both. + +**Full default HYE experiment**: `run-experiment` over the six ProteoBench HYE AIF files +(A_01 to A_03, B_01 to B_03), imported DIA-NN library with `rt_im_train.library_irt = auto` +(DeepLC 4.1.1 base-model re-prediction, once, 28 min), strict `nn_torch`, per-run quant on +the pooled `q_value`, cross-run MaxLFQ, 64 threads. Exit 0, 1:54:16 wall, 39.1 GB peak RSS, +109 GB of output. + +| run | PSMs at `run_psm_q` 1% | stripped peptides | decoy fraction | `w_rt` | anchors | +|---|---|---|---|---|---| +| A_01 | 78,101 | 66,108 | 0.0100 | 375 s | 18,456 | +| A_02 | 81,129 | 67,606 | 0.0100 | 349 s | 18,650 | +| A_03 | 72,865 | 62,168 | 0.0100 | 374 s | 18,493 | +| B_01 | 68,779 | 61,745 | 0.0100 | 397 s | 18,619 | +| B_02 | 71,958 | 62,810 | 0.0100 | 367 s | 18,660 | +| B_03 | 64,347 | 57,750 | 0.0100 | 399 s | 18,682 | + +Pooled rescore: 11,897,712 PSMs in 32.7 min, 437,208 target PSMs at 1% at a 1.00% decoy +fraction, **80,803 experiment-wide stripped peptides at `peptide_q_value` 1%**, 100,885 +precursors, 10,659 protein groups. The experiment-wide `peptides.tsv` has 80,803 rows with +six `quantity_` columns and the two transfer columns, 38,059 of them identified in all +six runs on `run_psm_q`; `proteins.tsv` has 10,659 rows with six `lfq_` columns; +`lfq_maxlfq.parquet` has 86,856 rows. `experiment_manifest.json` carries `config_json` +(4,163 characters), `model_identities` (`deeplc-base-model`, `imported-library`, +`nn-torch-semisup-sidecar-v1`, feature schema id, `None` for MBR), `inputs_hashed_at: +start`, eight inputs, twenty artifacts and `quant_q_filter` configured `RunPsmQ` / effective +`PsmQ`. Reference points: the 2026-08-26 pooled run of the same six files with the raw +imported iRT reported 72,044 stripped peptides; DIA-NN 2.2.0 library-free with `--reanalyse` +reports 65.6k to 72.5k stripped peptides per run at run and global 1% in 61 minutes on 32 +threads. The per-run MuMDIA counts above are 88 to 93 percent of DIA-NN's; the comparison is +not controlled for the changes since August and is a readiness check, not a sensitivity claim. + +**E. coli FASTA single run** (A_01, the 4,401 `_ECOLI` entries of the HYE FASTA, DeepLC 4.1.1 ++ MS2PIP 4.2.0 `HCDch2`, 12 fragments, strict `nn_torch`, 32 threads): exit 0, 12:22, 17.9 GB. +1,924,656 candidates predicted in 10:51 with `candidates_dropped_unpredicted = 0` and +`pairs_dropped_unpredicted = 0`, so review A's coverage checks ran against the real workers +and found complete predictions; 101 confident seeds, 97 anchors, `w_rt` 196 s; 527 stripped +peptides, 558 precursors, 155 protein groups at 1%. The count is small because condition A +carries 5 percent E. coli and the search space is E. coli only; the run exists to exercise +the FASTA path end to end, which it did. + +Not exercised by either run: `mumdia mbr` on real data, the desktop application against the +merged code, vendor conversion, entrapment. diff --git a/docs/README.md b/docs/README.md index 82cd0f5..3ef92df 100644 --- a/docs/README.md +++ b/docs/README.md @@ -100,3 +100,4 @@ measurement locally, so the same finding cannot drift into two versions. | [27_memory_footprint_audit.md](27_memory_footprint_audit.md) | Memory footprint audit of the default single-run workflow: per-stage resident-set model, ranked hotspots, reduction plan (streaming IO, incremental writes, f32 storage with f64 arithmetic), rewrite candidates, acceptance gates and the `bench/mem_profile.py` protocol. | | [28_feature_selection_analysis.md](28_feature_selection_analysis.md) | Feature-selection analysis for `nn_torch` rescoring: 43 of 387 Extended features are dead by construction, ~120 chosen multivariately reproduce the full set on two HYE runs (real worker confirmed), training time is flat in the feature count, and the parquet handoff is the larger memory lever. | | [29_code_review_2026-09-07.md](29_code_review_2026-09-07.md) | Repository-wide code review at `ab7049e`: 21 findings with reproductions (pooled `source` type, streaming NULLs, entrapment in-sample scores, artifact publication, output ownership, MBR confidence, Met excision, LOESS endpoints, provenance, validation), the agreed four work packages A-D and the deferrals. The decision record the fixes cite by finding number. | +| [30_code_review_2026-09-08.md](30_code_review_2026-09-08.md) | Follow-up review at `d7874f2` after work packages A-D: nine remaining findings (fine-tune batch regression, Windows run-name aliases, cancellation cleanup lifetime, concurrent conversions, remaining numeric domains, library rewrite coverage, audit flag consistency, overlapping reservations, debug-binary stack), the status of the original 21, and the implementation status of package E. | diff --git a/rust/mumdia/crates/mumdia-core/src/config.rs b/rust/mumdia/crates/mumdia-core/src/config.rs index 7c15f84..8243e7f 100644 --- a/rust/mumdia/crates/mumdia-core/src/config.rs +++ b/rust/mumdia/crates/mumdia-core/src/config.rs @@ -2107,15 +2107,167 @@ impl Config { .into(), )); } - if self.rt_im_train.finetune_deeplc - && (self.rt_im_train.finetune_epochs == 0 || self.rt_im_train.finetune_batch == 0) - { + // `finetune_batch = 0` is the documented automatic batch size (see the field), so + // only the epoch count has a lower bound. Rejecting the zero batch made simply + // enabling the fine-tune invalid with its own defaults (docs/30 R1). + if self.rt_im_train.finetune_deeplc && self.rt_im_train.finetune_epochs == 0 { return Err(Invalid( - "rt_im_train.finetune_epochs and finetune_batch must be >= 1 when \ - finetune_deeplc is on" - .into(), + "rt_im_train.finetune_epochs must be >= 1 when finetune_deeplc is on".into(), )); } + + // The remaining active numeric fields, by their documented domains (docs/30 R5): + // fractions and q-values inside their unit interval, correlations and percentiles + // inside theirs, tolerances and widths positive, counts at least one where zero + // has no documented meaning. Every default sits inside its domain, which the + // `Config::default().validate()` assertion in the tests keeps true. + for (name, value) in [ + ( + "extract.min_matched_fraction", + self.extract.min_matched_fraction, + ), + ( + "extract.alt_peak_min_area_frac", + self.extract.alt_peak_min_area_frac, + ), + ( + "extract.gate_coelution_min", + self.extract.gate_coelution_min, + ), + ("quant.baseline_quantile", self.quant.baseline_quantile), + ("mbr.consensus_corr_min", self.mbr.consensus_corr_min), + ] { + if !value.is_finite() || !(0.0..=1.0).contains(&value) { + return Err(Invalid(format!( + "{name} must be finite and in [0, 1] (got {value})" + ))); + } + } + for (name, value) in [ + ("mbr.q_anchor", self.mbr.q_anchor), + ("quant.reliable_q", self.quant.reliable_q), + ("quant.peak_fraction", self.quant.peak_fraction), + ( + "features.bound_peak_fraction", + self.features.bound_peak_fraction, + ), + ("rescore.train_fdr", self.rescore.train_fdr), + ] { + if !value.is_finite() || value <= 0.0 || value > 1.0 { + return Err(Invalid(format!( + "{name} must be finite and in (0, 1] (got {value})" + ))); + } + } + if !self.features.coelution_corr_threshold.is_finite() + || !(-1.0..=1.0).contains(&self.features.coelution_corr_threshold) + { + return Err(Invalid(format!( + "features.coelution_corr_threshold is a correlation and must be in [-1, 1] \ + (got {})", + self.features.coelution_corr_threshold + ))); + } + if !self.features.bound_confident_pct.is_finite() + || !(0.0..=100.0).contains(&self.features.bound_confident_pct) + { + return Err(Invalid(format!( + "features.bound_confident_pct is a percentile and must be in [0, 100] (got {})", + self.features.bound_confident_pct + ))); + } + for (name, value) in [ + ( + "search_seed.fragment_tol_ppm", + self.search_seed.fragment_tol_ppm, + ), + ("extract.frag_tol_ppm", self.extract.frag_tol_ppm), + ("extract.prec_tol_ppm", self.extract.prec_tol_ppm), + ("features.prec_tol_ppm", self.features.prec_tol_ppm), + ("prescan.tol_da", self.prescan.tol_da), + ("prescan.rt_bin_s", self.prescan.rt_bin_s), + ( + "extract.claim_cues.mz_close_sigma_ppm", + self.extract.claim_cues.mz_close_sigma_ppm, + ), + ( + "extract.claim_cues.rt_prior_tau_s", + self.extract.claim_cues.rt_prior_tau_s, + ), + ("extract.peak_claim_margin", self.extract.peak_claim_margin), + ("mbr.rt_window_s", self.mbr.rt_window_s), + ("rescore.entrapment_ratio", self.rescore.entrapment_ratio), + ] { + if !value.is_finite() || value <= 0.0 { + return Err(Invalid(format!( + "{name} must be finite and > 0 (got {value})" + ))); + } + } + for (name, value) in [ + ("prescan.rt_slack_s", self.prescan.rt_slack_s), + ("extract.demix_lambda", self.extract.demix_lambda), + ( + "extract.alt_peak_min_separation_s", + self.extract.alt_peak_min_separation_s, + ), + ( + "compete.apex_rt_tolerance_s", + self.compete.apex_rt_tolerance_s, + ), + ("compete.margin", self.compete.margin), + ("quant.fixed_window_s", self.quant.fixed_window_s), + ] { + if !value.is_finite() || value < 0.0 { + return Err(Invalid(format!( + "{name} must be finite and >= 0 (got {value}); 0 keeps its documented \ + meaning" + ))); + } + } + for (name, value) in [ + ("mbr.min_anchor_runs", self.mbr.min_anchor_runs), + ("rescore.num_iter", self.rescore.num_iter), + ("quant.top_n_fragments", self.quant.top_n_fragments), + ("quant.top_n_peptides", self.quant.top_n_peptides), + ( + "quant.baseline_flank_scans", + self.quant.baseline_flank_scans, + ), + ( + "extract.demix_max_candidates", + self.extract.demix_max_candidates, + ), + ("extract.demix_scan_stride", self.extract.demix_scan_stride), + ("extract.retain_top_peaks", self.extract.retain_top_peaks), + ("extract.promote_top_peaks", self.extract.promote_top_peaks), + ( + "rt_im_train.adaptive_rt_bins", + self.rt_im_train.adaptive_rt_bins, + ), + ] { + if value == 0 { + return Err(Invalid(format!("{name} must be >= 1"))); + } + } + if self.rescore.folds < 2 { + return Err(Invalid(format!( + "rescore.folds must be >= 2 for cross-validated scores (got {})", + self.rescore.folds + ))); + } + if self.extract.promote_top_peaks > self.extract.retain_top_peaks { + return Err(Invalid(format!( + "extract.promote_top_peaks ({}) must be <= extract.retain_top_peaks ({})", + self.extract.promote_top_peaks, self.extract.retain_top_peaks + ))); + } + if !self.extract.bucket_size.is_power_of_two() { + return Err(Invalid(format!( + "extract.bucket_size must be a power of two (got {})", + self.extract.bucket_size + ))); + } if self.rt_im_train.adaptive_rt_window && self.rt_im_train.adaptive_rt_bins == 0 { return Err(Invalid( "rt_im_train.adaptive_rt_bins must be >= 1 when adaptive_rt_window is on".into(), @@ -2372,11 +2524,27 @@ mod tests { r#"{"experiment":{"parallel_runs":0}}"#, r#"{"rescore":{"train_neg_ratio":-1.0}}"#, r#"{"predict_frag":{"top_n_fragments":0}}"#, + // docs/30 R5: the five values the follow-up review found still accepted. + r#"{"mbr":{"q_anchor":-0.1}}"#, + r#"{"mbr":{"q_anchor":2.0,"min_anchor_runs":0}}"#, + r#"{"mbr":{"min_anchor_runs":0}}"#, + r#"{"extract":{"min_matched_fraction":2.0}}"#, + r#"{"features":{"bound_peak_fraction":-1.0}}"#, + r#"{"quant":{"reliable_q":2.0}}"#, + r#"{"features":{"coelution_corr_threshold":1.5}}"#, + r#"{"features":{"bound_confident_pct":101}}"#, + r#"{"extract":{"frag_tol_ppm":0.0}}"#, + r#"{"extract":{"bucket_size":1000}}"#, + r#"{"extract":{"retain_top_peaks":1,"promote_top_peaks":3}}"#, + r#"{"rescore":{"folds":1}}"#, + r#"{"quant":{"top_n_peptides":0}}"#, + r#"{"rescore":{"train_fdr":0.0}}"#, ] { assert!(Config::from_json(bad).is_err(), "{bad} must be rejected"); } // Documented zero semantics survive: no negative cap, in-sample window sizing, - // an uncapped subsample, a row-cap subsample above one. + // an uncapped subsample, a row-cap subsample above one. And the boundaries of + // every new domain are accepted, not only their violations rejected. for ok in [ r#"{"rescore":{"train_neg_ratio":0}}"#, r#"{"rt_im_train":{"window_holdout_frac":0.0}}"#, @@ -2384,12 +2552,46 @@ mod tests { r#"{"rescore":{"train_subsample":2000}}"#, r#"{"rescore":{"max_feature_matrix_gib":0}}"#, r#"{"extract":{"apex_rt_prior_s":0.0}}"#, + r#"{"mbr":{"q_anchor":1.0}}"#, + r#"{"mbr":{"q_anchor":0.001,"min_anchor_runs":1}}"#, + r#"{"extract":{"min_matched_fraction":0.0}}"#, + r#"{"extract":{"min_matched_fraction":1.0}}"#, + r#"{"features":{"bound_peak_fraction":1.0}}"#, + r#"{"quant":{"reliable_q":0.001}}"#, + r#"{"features":{"coelution_corr_threshold":-1.0}}"#, + r#"{"features":{"bound_confident_pct":100}}"#, + r#"{"mbr":{"consensus_corr_min":0.0}}"#, + r#"{"compete":{"margin":0.0}}"#, + r#"{"quant":{"fixed_window_s":0.0}}"#, + r#"{"extract":{"bucket_size":4096}}"#, + r#"{"extract":{"retain_top_peaks":3,"promote_top_peaks":3}}"#, + r#"{"rescore":{"folds":2}}"#, ] { assert!(Config::from_json(ok).is_ok(), "{ok} must be accepted"); } assert!(Config::default().validate().is_ok()); } + #[test] + fn enabling_the_fine_tune_keeps_the_automatic_batch() { + // docs/30 R1: turning the fine-tune on with its own defaults was rejected because + // the documented automatic batch size is 0. Omitted, explicit zero and a positive + // override are all valid; a zero epoch count is not. + for ok in [ + r#"{"rt_im_train":{"finetune_deeplc":true}}"#, + r#"{"rt_im_train":{"finetune_deeplc":true,"finetune_batch":0}}"#, + r#"{"rt_im_train":{"finetune_deeplc":true,"finetune_batch":256}}"#, + ] { + let c = Config::from_json(ok).unwrap_or_else(|e| panic!("{ok} must be accepted: {e}")); + assert!(c.rt_im_train.finetune_deeplc); + } + let e = + Config::from_json(r#"{"rt_im_train":{"finetune_deeplc":true,"finetune_epochs":0}}"#) + .unwrap_err() + .to_string(); + assert!(e.contains("finetune_epochs"), "{e}"); + } + #[test] fn explicit_uncapped_seed_and_invalid_gate_are_distinguished() { let c = Config::from_json(r#"{"search_seed":{"top_n_peaks":0}}"#).unwrap(); diff --git a/rust/mumdia/crates/mumdia/src/main.rs b/rust/mumdia/crates/mumdia/src/main.rs index 9a06d39..475d382 100644 --- a/rust/mumdia/crates/mumdia/src/main.rs +++ b/rust/mumdia/crates/mumdia/src/main.rs @@ -1034,6 +1034,26 @@ fn load_config(path: &Option) -> Result { } fn main() -> Result<()> { + // The dispatch in `real_main` is one large function whose arms keep their locals in + // a single frame, and without optimisation that frame exceeds the 1 MiB main-thread + // stack Windows reserves: the debug binary overflowed on `--version` before printing + // anything (docs/30 R9). The CLI therefore runs on a thread with a generous + // reservation; pages are committed only as they are touched, so release builds pay + // nothing for it. `tests/cli_version.rs` runs the built binary to keep this true. + const MAIN_STACK_BYTES: usize = 256 << 20; + let handle = std::thread::Builder::new() + .name("mumdia-main".into()) + .stack_size(MAIN_STACK_BYTES) + .spawn(real_main) + .context("spawning the main thread")?; + match handle.join() { + Ok(result) => result, + // The panic hook has already printed the message and location. + Err(_) => anyhow::bail!("mumdia stopped on an internal error (see the panic above)"), + } +} + +fn real_main() -> Result<()> { // Held for the whole process: dropping the guard is what writes dhat-heap.json into // the working directory, so it must outlive the stage that is being profiled. #[cfg(feature = "dhat-heap")] diff --git a/rust/mumdia/crates/mumdia/src/raw.rs b/rust/mumdia/crates/mumdia/src/raw.rs index d7282d3..9a186dc 100644 --- a/rust/mumdia/crates/mumdia/src/raw.rs +++ b/rust/mumdia/crates/mumdia/src/raw.rs @@ -511,9 +511,126 @@ fn sciex_scan_hint(src: &Path) -> Option { /// 6:48 conversion of a 3.7 GB Astral run was reported as "exited successfully but wrote no /// file" and discarded (doxy, 2026-09-06). msconvert's `--outfile` has the same habit. With /// `x.partial.mzML` there is nothing for either to fix up. -fn partial_name(out_name: &str) -> String { +fn partial_name(out_name: &str, tag: &str) -> String { let stem = out_name.strip_suffix(".mzML").unwrap_or(out_name); - format!("{stem}.partial.mzML") + format!("{stem}.partial-{tag}.mzML") +} + +/// A tag no other conversion in any process shares: process id plus a per-process +/// counter. Two searches converting one acquisition at the same time used to share +/// `.partial.mzML` (docs/30 R4). +fn unique_tag() -> String { + static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + format!( + "{}-{}", + std::process::id(), + COUNTER.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + ) +} + +/// A claim on a conversion destination, held from before the temporary file is written +/// until the result has been renamed into place or the attempt has failed. +/// +/// Two searches converting the same input concurrently wrote one temporary file: +/// converter B replaced A's partial output, A renamed B's bytes into place and reported +/// success, and B failed because its output had been renamed away (docs/30 R4). The +/// temporary name is unique now, and this lock beside the destination makes a second +/// converter wait for the first and reuse what it produced rather than convert again +/// into the same destination. Dropping the guard releases the lock. +struct ConvertLock { + path: PathBuf, +} + +impl ConvertLock { + fn path_for(out: &Path) -> PathBuf { + let mut name = out + .file_name() + .map(|n| n.to_os_string()) + .unwrap_or_default(); + name.push(".converting"); + out.with_file_name(name) + } + + /// Take the lock for `out`, waiting for a holder to finish first. `Ok(None)` means the + /// holder finished and left a usable conversion at `out`, which the caller reuses. + fn acquire(out: &Path, src: &Path, reuse: bool) -> Result> { + let path = Self::path_for(out); + let mut announced = false; + loop { + match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&path) + { + Ok(mut f) => { + use std::io::Write; + let _ = writeln!(f, "{}", std::process::id()); + return Ok(Some(ConvertLock { path })); + } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + if lock_is_stale(&path) { + warn!( + lock = %path.display(), + "convert: removing a stale conversion lock; its holder stopped \ + writing" + ); + let _ = std::fs::remove_file(&path); + continue; + } + if !announced { + info!( + mzml = %out.display(), + "convert: another process is converting this input; waiting for \ + it rather than converting into the same destination" + ); + announced = true; + } + std::thread::sleep(std::time::Duration::from_secs(2)); + if !path.exists() && reuse && out.is_file() && is_newer_than(out, src) { + return Ok(None); + } + } + Err(e) => { + return Err(e).with_context(|| { + format!("creating the conversion lock {}", path.display()) + }) + } + } + } + } +} + +impl Drop for ConvertLock { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.path); + } +} + +/// A lock whose holder has stopped: neither the lock nor any partial conversion file +/// beside it has been written for `STALE_AFTER`. Converters write their output +/// continuously, so a live conversion keeps a `.partial-` file fresh; a crashed or killed +/// holder leaves both untouched. +fn lock_is_stale(lock: &Path) -> bool { + const STALE_AFTER: std::time::Duration = std::time::Duration::from_secs(15 * 60); + let fresh = |p: &Path| { + std::fs::metadata(p) + .and_then(|m| m.modified()) + .ok() + .and_then(|t| t.elapsed().ok()) + .is_some_and(|age| age < STALE_AFTER) + }; + if fresh(lock) { + return false; + } + let dir = lock.parent().unwrap_or(Path::new(".")); + if let Ok(rd) = std::fs::read_dir(dir) { + for e in rd.flatten() { + if e.file_name().to_string_lossy().contains(".partial-") && fresh(&e.path()) { + return false; + } + } + } + true } pub fn ensure_mzml( @@ -627,6 +744,17 @@ msconvert was not usable either: {e}" format!("{stem}.{}.mzML", path_discriminator(src)) }; let out = out_dir.join(&out_name); + // One converter per destination at a time; a concurrent one waits and reuses. + let _lock = match ConvertLock::acquire(&out, src, cfg.reuse_converted)? { + Some(lock) => lock, + None => { + info!( + mzml = %out.display(), + "convert: reusing the mzML a concurrent conversion of this input just wrote" + ); + return Ok(out.to_string_lossy().into_owned()); + } + }; // Convert to a temporary name and rename only on success. // // Writing straight to `out` meant a killed run, a power loss or a converter crash @@ -634,7 +762,8 @@ msconvert was not usable either: {e}" // `reuse_converted` accepted it on every later run: a partial acquisition searched // silently, for ever, presenting as unexplained low identification counts with // nothing in the interface to reveal it. Neither failure path removed it. - let tmp = out_dir.join(partial_name(&out_name)); + let tmp_name = partial_name(&out_name, &unique_tag()); + let tmp = out_dir.join(&tmp_name); let _ = std::fs::remove_file(&tmp); let args: Vec = if is_thermo { @@ -662,7 +791,7 @@ msconvert was not usable either: {e}" a.push("-o".into()); a.push(out_dir.to_string_lossy().into_owned()); a.push("--outfile".into()); - a.push(partial_name(&out_name)); + a.push(tmp_name.clone()); a }; @@ -822,13 +951,66 @@ mod tests { fn the_partial_name_keeps_the_mzml_extension() { // The regression this guards: `x.mzML.partial` made ThermoRawFileParser write // `x.mzML.partial.mzML`, and the conversion was thrown away as "wrote no file". - assert_eq!(partial_name("run.mzML"), "run.partial.mzML"); + assert_eq!(partial_name("run.mzML", "7-0"), "run.partial-7-0.mzML"); assert_eq!( - partial_name("run.1a2b3c4d.mzML"), - "run.1a2b3c4d.partial.mzML" + partial_name("run.1a2b3c4d.mzML", "7-1"), + "run.1a2b3c4d.partial-7-1.mzML" + ); + assert!(partial_name("odd", "1-1").ends_with(".mzML")); + assert_ne!(partial_name("run.mzML", "1-1"), "run.mzML"); + // Two conversions in one process never share a temporary file (docs/30 R4). + assert_ne!(unique_tag(), unique_tag()); + } + + #[test] + fn a_conversion_lock_is_exclusive_and_released_on_drop() { + let d = tmp("lock"); + let out = d.join("run.mzML"); + let src = d.join("run.raw"); + std::fs::write(&src, b"raw").unwrap(); + let first = ConvertLock::acquire(&out, &src, true) + .unwrap() + .expect("first holder"); + assert!(ConvertLock::path_for(&out).is_file()); + // A second claim cannot be taken while the first is held; the probe below asks + // the primitive directly rather than waiting through `acquire`. + let taken = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(ConvertLock::path_for(&out)); + assert!(taken.is_err(), "the lock must be exclusive"); + drop(first); + assert!( + !ConvertLock::path_for(&out).exists(), + "dropping releases the lock" ); - assert!(partial_name("odd").ends_with(".mzML")); - assert_ne!(partial_name("run.mzML"), "run.mzML"); + let second = ConvertLock::acquire(&out, &src, true).unwrap(); + assert!(second.is_some(), "free again once released"); + let _ = std::fs::remove_dir_all(&d); + } + + #[test] + fn a_stale_lock_is_broken_and_a_fresh_one_is_honoured() { + let d = tmp("stale"); + let lock = d.join("run.mzML.converting"); + std::fs::write(&lock, b"1").unwrap(); + assert!(!lock_is_stale(&lock), "a lock written a moment ago is live"); + // Age the lock past the staleness window; nothing partial is being written. + let old = std::time::SystemTime::now() - std::time::Duration::from_secs(20 * 60); + std::fs::OpenOptions::new() + .write(true) + .open(&lock) + .unwrap() + .set_modified(old) + .unwrap(); + assert!( + lock_is_stale(&lock), + "an old lock with no live partial file is stale" + ); + // A partial file still being written keeps even an old lock alive. + std::fs::write(d.join("run.partial-99-0.mzML"), b"...").unwrap(); + assert!(!lock_is_stale(&lock)); + let _ = std::fs::remove_dir_all(&d); } #[test] diff --git a/rust/mumdia/crates/mumdia/src/sidecar.rs b/rust/mumdia/crates/mumdia/src/sidecar.rs index 31d25c5..168abaf 100644 --- a/rust/mumdia/crates/mumdia/src/sidecar.rs +++ b/rust/mumdia/crates/mumdia/src/sidecar.rs @@ -8,7 +8,7 @@ use std::process::Command; use anyhow::{bail, Context, Result}; use mumdia_io::table::{write_table, Col, TableFile}; -use tracing::info; +use tracing::{info, warn}; /// Per candidate row: `(ion byte, ordinal, fragment charge)` -> linear predicted intensity. /// The charge is 1 for every series a single-charge MS2PIP model emits and 2 for the @@ -149,6 +149,40 @@ pub fn require_deeplc_version(python: &str) -> Result { } } +/// `deeplc--` when the interpreter answers, `deeplc-` when there +/// is none to ask: the manifest's RT identity should say which DeepLC release produced the +/// library, not only the recipe (docs/30, model identity). +pub fn deeplc_identity(python: Option<&str>, suffix: &str) -> String { + match python.and_then(|py| module_version(py, "deeplc")) { + Some(v) => format!("deeplc-{v}-{suffix}"), + None => format!("deeplc-{suffix}"), + } +} + +/// Read the `.summary.json` the fine-tune worker writes beside a rewritten +/// library and warn when rows kept their imported iRT, so a mixed RT source is visible +/// in the log rather than only in the file (docs/30 R6). +fn warn_on_retained_imported(lib_out: &str) { + let path = format!("{lib_out}.summary.json"); + let Ok(v) = mumdia_io::json::read_json::(&path) else { + return; + }; + let n = |k: &str| v.get(k).and_then(|x| x.as_u64()).unwrap_or(0); + let retained = n("retained_imported"); + if retained > 0 { + warn!( + rows = n("rows"), + repredicted = n("repredicted"), + retained_imported = retained, + retained_non_standard = n("retained_non_standard"), + retained_no_prediction = n("retained_no_prediction"), + summary = %path, + "sidecar: the rewritten library keeps the imported iRT on some rows, so its RT \ + source is mixed; see the summary for the counts" + ); + } +} + /// DeepLC: predict retention time per peptidoform. Returns `id -> predicted_rt`. pub fn run_deeplc( python: &str, @@ -261,7 +295,9 @@ pub fn run_deeplc_finetune( ], true, ) - .context("DeepLC fine-tune failed") + .context("DeepLC fine-tune failed")?; + warn_on_retained_imported(lib_out); + Ok(()) } /// DeepLC base-model re-prediction of an imported library's `predicted_irt`: the @@ -298,7 +334,9 @@ pub fn run_deeplc_repredict( ], true, ) - .context("DeepLC library re-prediction failed") + .context("DeepLC library re-prediction failed")?; + warn_on_retained_imported(lib_out); + Ok(()) } /// MBR transfer (Stage D3): match-between-runs identification transfer over the diff --git a/rust/mumdia/crates/mumdia/src/stages/audit.rs b/rust/mumdia/crates/mumdia/src/stages/audit.rs index 2ab512f..8b90198 100644 --- a/rust/mumdia/crates/mumdia/src/stages/audit.rs +++ b/rust/mumdia/crates/mumdia/src/stages/audit.rs @@ -106,16 +106,21 @@ pub fn run(p: AuditParams) -> Result { // the PSM `q_value` (docs/29 #16), a different unit: a PSM can pass at 1% while // its precursor group does not, and the other way round. Older scored tables have // no `precursor_q`; there the PSM q is used and the metrics say so. - let (scored_q, q_unit) = match scored_t.f64("precursor_q") { - Ok(v) => (v, "precursor_q"), - Err(_) => { - tracing::warn!( - scored = p.scored, - "audit: no `precursor_q` column; the precursor gate falls back to the PSM \ - q_value, which is not the same unit" - ); - (scored_t.f64("q_value")?, "q_value") - } + let (scored_q, q_unit) = if scored_t.has_column("precursor_q") { + // Present means present: a column of the wrong type is an error, not a reason to + // read another unit in its place (docs/30 R7, the absent-versus-malformed rule + // quant applies to `source`). + let v = scored_t + .f64("precursor_q") + .with_context(|| format!("audit: reading precursor_q from {}", p.scored))?; + (v, "precursor_q") + } else { + tracing::warn!( + scored = p.scored, + "audit: no `precursor_q` column; the precursor gate falls back to the PSM \ + q_value, which is not the same unit" + ); + (scored_t.f64("q_value")?, "q_value") }; // peptide-level q is optional (only present in some scored schemas). let scored_pep_q = scored_t.f64("peptide_q_value").ok(); @@ -186,6 +191,9 @@ pub fn run(p: AuditParams) -> Result { RejectionReason::FailedPrecursorFdr } else if !passed_pep { RejectionReason::FailedPeptideFdr + } else if is_decoy { + // Passed every gate, and the report never writes a decoy (docs/30 R7). + RejectionReason::RemovedDuringReporting } else { RejectionReason::Reported }; @@ -205,7 +213,11 @@ pub fn run(p: AuditParams) -> Result { f_td_winner.push(in_scored); f_prec_fdr.push(passed_prec); f_pep_fdr.push(passed_pep && passed_prec); - f_reported.push(passed_prec); + // One definition of "reported": the rejection reason. The flag used to repeat + // the precursor gate alone, so a row could read `reported = true` next to + // `FAILED_PEPTIDE_FDR`, and a decoy could be reported (docs/30 R7). The gate + // diagnostics keep their own columns above. + f_reported.push(reason == RejectionReason::Reported); reason_c.push(reason.code().to_string()); } @@ -419,6 +431,91 @@ mod tests { assert_eq!(m["q_unit"], "precursor_q"); } + #[test] + fn the_reported_flag_follows_the_reason_and_the_report_rules() { + // docs/30 R7: a target passing the precursor gate but not the peptide gate, a + // target passing both, and a decoy passing both. Only the second is reported, + // the flag says so, and the metrics count the same row. + let lib = tmp("lib_rep.parquet"); + let psms = tmp("psms_rep.parquet"); + let comp = tmp("comp_rep.parquet"); + let scored = tmp("scored_rep.parquet"); + let out = tmp("audit_rep.parquet"); + write_lib(&lib, &[1, 2, 3], &["target", "target", "decoy"]); + write_cid_only(&psms, &[1, 2, 3]); + write_cid_only(&comp, &[1, 2, 3]); + write_table( + &scored, + vec![ + Col::U32("candidate_id".into(), vec![1, 2, 3]), + Col::F64("q_value".into(), vec![0.001, 0.001, 0.001]), + Col::F64("precursor_q".into(), vec![0.001, 0.001, 0.001]), + Col::F64("peptide_q_value".into(), vec![0.5, 0.001, 0.001]), + ], + ) + .unwrap(); + run(AuditParams { + library_precursors: &lib, + psms: &psms, + competed: &comp, + scored: &scored, + out: &out, + q_threshold: 0.01, + run_id: "t", + entrapment_substr: "", + }) + .unwrap(); + let a = TableFile::open(&out).unwrap(); + let cid = a.u32("precursor_id").unwrap(); + let reason = a.str("rejection_reason").unwrap(); + let reported = a.bool("reported").unwrap(); + let by: std::collections::HashMap = cid + .iter() + .cloned() + .zip(reason.into_iter().zip(reported)) + .collect(); + assert_eq!(by[&1], ("FAILED_PEPTIDE_FDR".to_string(), false)); + assert_eq!(by[&2], ("REPORTED".to_string(), true)); + assert_eq!(by[&3], ("REMOVED_DURING_REPORTING".to_string(), false)); + let m: serde_json::Value = + mumdia_io::json::read_json(&format!("{out}.metrics.json")).unwrap(); + assert_eq!(m["reported"], 1); + } + + #[test] + fn a_present_but_malformed_precursor_q_column_is_an_error_not_a_fallback() { + let lib = tmp("lib_bad.parquet"); + let psms = tmp("psms_bad.parquet"); + let comp = tmp("comp_bad.parquet"); + let scored = tmp("scored_bad.parquet"); + let out = tmp("audit_bad.parquet"); + write_lib(&lib, &[1], &["target"]); + write_cid_only(&psms, &[1]); + write_cid_only(&comp, &[1]); + write_table( + &scored, + vec![ + Col::U32("candidate_id".into(), vec![1]), + Col::F64("q_value".into(), vec![0.001]), + Col::I32("precursor_q".into(), vec![0]), + ], + ) + .unwrap(); + let e = run(AuditParams { + library_precursors: &lib, + psms: &psms, + competed: &comp, + scored: &scored, + out: &out, + q_threshold: 0.01, + run_id: "t", + entrapment_substr: "", + }) + .unwrap_err() + .to_string(); + assert!(e.contains("precursor_q"), "{e}"); + } + #[test] fn a_pooled_scored_table_is_refused() { // Keyed by candidate_id alone, a two-source table would let the second run's q diff --git a/rust/mumdia/crates/mumdia/src/stages/predict_frag.rs b/rust/mumdia/crates/mumdia/src/stages/predict_frag.rs index e395809..8e885f1 100644 --- a/rust/mumdia/crates/mumdia/src/stages/predict_frag.rs +++ b/rust/mumdia/crates/mumdia/src/stages/predict_frag.rs @@ -375,7 +375,10 @@ fn assign_rt(p: &PredictFragParams, raws: &mut [Raw]) -> Result<(String, Vec missing.push(i), } } - Ok(("deeplc-4.0-mt".to_string(), missing)) + // The installed DeepLC version, not a family label: two libraries predicted by + // different DeepLC releases are different libraries (docs/30, model identity). + let version = sidecar::require_deeplc_version(python)?; + Ok((format!("deeplc-{version}-base"), missing)) } } } @@ -462,7 +465,9 @@ fn assign_intensities(p: &PredictFragParams, raws: &mut [Raw]) -> Result<(String .filter(|(_, &c)| !c) .map(|(i, _)| i) .collect(); - Ok((format!("ms2pip-{}", p.cfg.ms2pip_model), missing)) + let version = + sidecar::module_version(python, "ms2pip").unwrap_or_else(|| "unknown".into()); + Ok((format!("ms2pip-{version}-{}", p.cfg.ms2pip_model), missing)) } } } diff --git a/rust/mumdia/crates/mumdia/src/stages/run.rs b/rust/mumdia/crates/mumdia/src/stages/run.rs index 160fec0..46571d0 100644 --- a/rust/mumdia/crates/mumdia/src/stages/run.rs +++ b/rust/mumdia/crates/mumdia/src/stages/run.rs @@ -615,13 +615,14 @@ pub fn run(p: RunParams) -> Result<()> { // Model identities reflect the path that produced the downstream artifacts, // including imported libraries and per-run RT fine-tuning. let library_input = p.lib_precursors.is_some(); + let deeplc_py = cfg.predict_frag.deeplc_python.as_deref(); let rt_identity = if cfg.rt_im_train.finetune_deeplc { - "deeplc-finetuned".to_string() + crate::sidecar::deeplc_identity(deeplc_py, "finetuned") } else if cfg .rt_im_train - .repredicts_library_irt(library_input, cfg.predict_frag.deeplc_python.is_some()) + .repredicts_library_irt(library_input, deeplc_py.is_some()) { - "deeplc-base-model".to_string() + crate::sidecar::deeplc_identity(deeplc_py, "base") } else if library_input { "imported-library".to_string() } else { diff --git a/rust/mumdia/crates/mumdia/src/stages/run_experiment.rs b/rust/mumdia/crates/mumdia/src/stages/run_experiment.rs index ca78064..aa65942 100644 --- a/rust/mumdia/crates/mumdia/src/stages/run_experiment.rs +++ b/rust/mumdia/crates/mumdia/src/stages/run_experiment.rs @@ -349,6 +349,43 @@ fn check_run_names_distinct(ns: &[String]) -> Result<()> { Ok(()) } +/// Why `name` cannot be a per-run directory name on every platform, or `None`. +/// +/// Syntactic, not probed: Windows's rules are applied everywhere, because an experiment's +/// output may be written to any filesystem and a name that is one directory on NTFS must +/// not be two on ext4. `a` and `a.` passed the old check (empty, separators, `.`, `..`) and +/// were one directory on Windows: the second run overwrote the first and the experiment +/// exited 0 with both split tables holding `source = 1` (docs/30 R2). +fn portable_dir_name_problem(name: &str) -> Option<&'static str> { + const RESERVED: [&str; 22] = [ + "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", + "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9", + ]; + if name.is_empty() { + return Some("it is empty"); + } + if name == "." || name == ".." { + return Some("`.` and `..` are not names"); + } + if name.contains('/') || name.contains('\\') { + return Some("it contains a path separator"); + } + if name + .chars() + .any(|c| matches!(c, '<' | '>' | ':' | '"' | '|' | '?' | '*') || (c as u32) < 0x20) + { + return Some("it contains a character Windows forbids in a file name (<>:\"|?* or a control character)"); + } + if name.ends_with('.') || name.ends_with(' ') { + return Some("it ends with a dot or a space, which Windows strips, so it names the same directory as the trimmed form"); + } + let stem = name.split('.').next().unwrap_or(name).to_ascii_uppercase(); + if RESERVED.contains(&stem.as_str()) { + return Some("it is a Windows reserved device name (CON, PRN, AUX, NUL, COM1-9, LPT1-9), with or without an extension"); + } + None +} + pub fn run(p: RunExperimentParams) -> Result<()> { let t0 = Instant::now(); // Same contract as the single-run orchestrator, and it matters more here: an @@ -416,12 +453,13 @@ pub fn run(p: RunExperimentParams) -> Result<()> { ); } check_run_names_distinct(ns)?; - if let Some(bad) = ns.iter().find(|n| { - n.is_empty() || n.contains('/') || n.contains('\\') || *n == "." || *n == ".." - }) { + if let Some((bad, why)) = ns + .iter() + .find_map(|n| portable_dir_name_problem(n).map(|why| (n, why))) + { anyhow::bail!( - "--run-names entry {bad:?} is not usable as a directory name; each \ - becomes a subdirectory of --out-dir" + "--run-names entry {bad:?} is not usable as a directory name: {why}; each \ + name becomes a subdirectory of --out-dir" ); } ns.to_vec() @@ -819,17 +857,18 @@ pub fn run(p: RunExperimentParams) -> Result<()> { // in the single-run manifest (docs/29 #15): which RT source the library carried, // which fragment predictor, and the classifier that actually ran. let library_input = p.lib_precursors.is_some(); + let deeplc_py = cfg.predict_frag.deeplc_python.as_deref(); let rt_identity = if cfg.rt_im_train.finetune_deeplc { if matches!(cfg.experiment.finetune_scope, FinetuneScope::FirstRunOnly) { - "deeplc-finetuned-first-run".to_string() + crate::sidecar::deeplc_identity(deeplc_py, "finetuned-first-run") } else { - "deeplc-finetuned-per-run".to_string() + crate::sidecar::deeplc_identity(deeplc_py, "finetuned-per-run") } } else if cfg .rt_im_train - .repredicts_library_irt(library_input, cfg.predict_frag.deeplc_python.is_some()) + .repredicts_library_irt(library_input, deeplc_py.is_some()) { - "deeplc-base-model".to_string() + crate::sidecar::deeplc_identity(deeplc_py, "base") } else if library_input { "imported-library".to_string() } else { @@ -954,6 +993,35 @@ mod tests { assert!(e.contains("RunA") && e.contains("runa"), "{e}"); } + #[test] + fn run_names_that_alias_on_windows_are_rejected_everywhere() { + // docs/30 R2: `a` and `a.` are one directory on Windows and the experiment ran + // to completion with one run overwriting the other. The syntactic rules apply on + // every platform so the output is portable. + for (bad, why) in [ + ("a.", "dot or a space"), + ("a ", "dot or a space"), + ("NUL", "reserved"), + ("com1.log", "reserved"), + ("run:1", "forbids"), + ("run?", "forbids"), + ("a/b", "separator"), + ("", "empty"), + ("..", "not names"), + ] { + let why_got = portable_dir_name_problem(bad) + .unwrap_or_else(|| panic!("{bad:?} must be rejected")); + assert!(why_got.contains(why), "{bad:?}: {why_got}"); + } + for ok in ["r0", "run.1", "A-b_c", "B_01", "sample 3", "com10", "conx"] { + assert_eq!( + portable_dir_name_problem(ok), + None, + "{ok:?} must be accepted" + ); + } + } + #[test] fn repeated_run_names_are_rejected_and_distinct_ones_pass() { let e = check_run_names_distinct(&names(&["a", "b", "a"])) diff --git a/rust/mumdia/crates/mumdia/tests/cli_version.rs b/rust/mumdia/crates/mumdia/tests/cli_version.rs new file mode 100644 index 0000000..cb02406 --- /dev/null +++ b/rust/mumdia/crates/mumdia/tests/cli_version.rs @@ -0,0 +1,43 @@ +//! The built binary, run as a subprocess. +//! +//! The debug build overflowed its 1 MiB Windows main-thread stack on `--version`, before +//! printing anything (docs/30 R9). No library test could see that: it is a property of the +//! binary's entry point under the developer profile, so it is checked here on whichever +//! profile `cargo test` builds. + +use std::process::Command; + +fn mumdia() -> Command { + Command::new(env!("CARGO_BIN_EXE_mumdia")) +} + +#[test] +fn the_built_binary_prints_its_version() { + let out = mumdia() + .arg("--version") + .output() + .expect("run mumdia --version"); + assert!( + out.status.success(), + "status {:?}\nstderr:\n{}", + out.status, + String::from_utf8_lossy(&out.stderr) + ); + let text = String::from_utf8_lossy(&out.stdout); + assert!( + text.contains(env!("CARGO_PKG_VERSION")), + "expected the crate version in {text:?}" + ); +} + +#[test] +fn the_built_binary_prints_its_help() { + let out = mumdia().arg("--help").output().expect("run mumdia --help"); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + let text = String::from_utf8_lossy(&out.stdout); + assert!(text.contains("run-experiment"), "{text}"); +} diff --git a/scripts/deeplc_finetune.py b/scripts/deeplc_finetune.py index 5f26f5d..8edaebb 100644 --- a/scripts/deeplc_finetune.py +++ b/scripts/deeplc_finetune.py @@ -32,6 +32,8 @@ os.environ["NUMEXPR_NUM_THREADS"] = "1" import argparse +import json +import math import re import time import deeplc # import before numpy (OpenMP load order) @@ -271,6 +273,13 @@ def main(): t0 = time.time() batch = uniq[s:s + chunk] p = agg(deeplc.predict(batch) if ft_model is None else deeplc.predict(batch, model=ft_model)) + # A structurally short or long answer is a broken predictor, not a set of + # unsupported peptidoforms: zipping it silently paired predictions with the wrong + # peptidoforms and left the tail on its imported value (docs/30 R6). + if len(p) != len(batch): + raise SystemExit( + f"DeepLC returned {len(p)} predictions for {len(batch)} peptidoforms in one " + f"batch; refusing to rewrite the library from a malformed response") for pf, v in zip(batch, p): preds[pf] = float(v) done = min(s + chunk, len(uniq)) @@ -281,14 +290,61 @@ def main(): f"({rate:.0f} peptidoforms/s, ETA {eta / 60:.1f} min)", flush=True) print(f"prediction phase: {time.time() - t_pred0:.1f}s total", flush=True) - # `base_pf` is recomputed here rather than cached from the pass above on purpose: - # caching it would retain one extra string per library row (hundreds of MB at - # library scale) to avoid a `startswith` and a slice. - new = np.array([preds.get(base_pf(pf), orig[i]) for i, pf in enumerate(pform)], dtype=np.float32) + new, summary = rewrite_irt(pform, orig, preds) idx = lib.schema.get_field_index("predicted_irt") lib = lib.set_column(idx, "predicted_irt", pa.array(new, pa.float32())) pq.write_table(lib, args.lib_out) + summary["model"] = which + summary["lib_in"] = args.lib_in + summary["lib_out"] = args.lib_out + with open(args.lib_out + ".summary.json", "w", encoding="utf-8") as fh: + json.dump(summary, fh, indent=2) print(f"wrote library with re-predicted iRT ({which}): {args.lib_out}") + print(f" rows={summary['rows']} repredicted={summary['repredicted']} " + f"retained_imported={summary['retained_imported']} " + f"(non-standard residues {summary['retained_non_standard']}, " + f"no finite prediction {summary['retained_no_prediction']})") + if summary["retained_imported"]: + print(f"WARNING: {summary['retained_imported']} of {summary['rows']} rows " + f"({100.0 * summary['retained_imported'] / max(1, summary['rows']):.2f}%) keep " + f"their imported iRT, which is on the imported model's scale, not {which}'s; " + f"the counts are in {args.lib_out}.summary.json", flush=True) + + +def rewrite_irt(pform, orig, preds): + """The new `predicted_irt` column and a count of where each value came from. + + A peptidoform with a finite prediction for its DECOY_-stripped sequence takes it. A + peptidoform without one keeps its imported value: rows with non-standard residues are + never sent to DeepLC (`is_std`), and a prediction that came back non-finite is an + unsupported input rather than a number. Both are counted so the mixture of RT sources + in the written library is explicit instead of silent (docs/30 R6). `base_pf` is + recomputed here rather than cached from the pass above on purpose: caching it would + retain one extra string per library row (hundreds of MB at library scale). + """ + n = len(pform) + new = np.empty(n, dtype=np.float32) + repredicted = 0 + no_prediction = 0 + non_standard = 0 + for i, pf in enumerate(pform): + v = preds.get(base_pf(pf)) + if v is None: + new[i] = orig[i] + non_standard += 1 + elif not math.isfinite(v): + new[i] = orig[i] + no_prediction += 1 + else: + new[i] = v + repredicted += 1 + return new, { + "rows": n, + "repredicted": repredicted, + "retained_imported": non_standard + no_prediction, + "retained_non_standard": non_standard, + "retained_no_prediction": no_prediction, + } if __name__ == "__main__": diff --git a/scripts/mbr_worker.py b/scripts/mbr_worker.py index 53796ab..bfc5d84 100644 --- a/scripts/mbr_worker.py +++ b/scripts/mbr_worker.py @@ -229,7 +229,14 @@ def expected_rt(c, i): target_delta = np.array(rows["rt_delta"]) decoy_delta = np.array(decoy_delta) if len(target_delta) == 0: - print("MBR: no transfer candidates"); pa_write_empty(a.out); return + # The same output contract as a run with transfers: a full-schema empty transfer + # table, and the requested augmented scored table with every row unflagged, so a + # downstream stage never sees a missing file or a one-column placeholder (docs/30). + print("MBR: no transfer candidates") + write_empty_transfers(a.out) + if a.out_scored: + write_unflagged_scored(a.scored, a.out_scored) + return # transfer q via target/decoy competition on rt_delta (smaller = better). At a # threshold delta, FDR = (#null <= delta + 1) / (#target <= delta), the same +1 @@ -345,8 +352,30 @@ def cos(pa_, pb): print(f"wrote {a.out_scored} (augmented scored; {int(is_tr.sum())} rows flagged transferred)") -def pa_write_empty(path): - write_engine_table(pa.table({"candidate_id": pa.array([], pa.uint32())}), path) +def write_empty_transfers(path): + """A zero-row transfer table with the same ten columns a run with transfers writes.""" + write_engine_table(pa.table({ + "candidate_id": pa.array([], pa.uint32()), + "source": pa.array([], pa.uint32()), + "peptidoform": pa.array([], pa.string()), + "charge": pa.array([], pa.int32()), + "protein_group": pa.array([], pa.string()), + "label": pa.array([], pa.string()), + "expected_rt": pa.array([], pa.float64()), + "observed_rt": pa.array([], pa.float64()), + "rt_delta": pa.array([], pa.float64()), + "transfer_q": pa.array([], pa.float64()), + }), path) + + +def write_unflagged_scored(scored_in, scored_out): + """The scored table unchanged, with `is_transferred` false and `transfer_q` NaN on + every row: the augmented schema with no transfer in it.""" + full = pq.read_table(scored_in).to_pandas() + full["is_transferred"] = np.zeros(len(full), dtype=bool) + full["transfer_q"] = np.full(len(full), np.nan) + write_engine_parquet(full, scored_out) + print(f"wrote {scored_out} (augmented scored; 0 rows flagged transferred)") if __name__ == "__main__": diff --git a/tests/python/test_mbr_worker.py b/tests/python/test_mbr_worker.py index a8d6c5d..3b3ea1c 100644 --- a/tests/python/test_mbr_worker.py +++ b/tests/python/test_mbr_worker.py @@ -525,13 +525,15 @@ def test_binned_map_removes_a_systematic_inter_run_rt_offset( ) -def test_no_transfer_candidates_writes_an_empty_table_and_no_scored_table(tmp_path): - """With nothing to transfer the worker exits 0, and `--out-scored` is skipped. - - `mbr_worker.py:187-188` returns before the M5 block, so a caller that - passed `--out-scored` gets no file. A downstream quant pointed at that path - fails on a missing input rather than on a nonzero MBR exit, so the true - cause is not in the MBR log; pinning the behaviour keeps that documented. +def test_no_transfer_candidates_writes_the_full_schema_and_an_unflagged_scored_table(tmp_path): + """With nothing to transfer the worker exits 0 and keeps its output contract. + + It used to return before the M5 block: a caller that passed `--out-scored` got no + file, and the transfer table had a single `candidate_id` column. Downstream stages + then failed on a missing input or an unexpected schema instead of reading an empty + result (docs/30). Now the transfer table carries all ten columns with zero rows, and + the augmented scored table is the input with `is_transferred` false and `transfer_q` + NaN on every row. """ ids = list(range(20)) cols = {k: [] for k in ("candidate_id", "source", "label", "q_value", @@ -558,8 +560,19 @@ def test_no_transfer_candidates_writes_an_empty_table_and_no_scored_table(tmp_pa "--out-scored", scored_out, ) assert "no transfer candidates" in stdout - assert pq.read_table(str(out)).num_rows == 0 - assert not scored_out.exists() + transfers = pq.read_table(str(out)) + assert transfers.num_rows == 0 + assert transfers.column_names == [ + "candidate_id", "source", "peptidoform", "charge", "protein_group", "label", + "expected_rt", "observed_rt", "rt_delta", "transfer_q", + ] + assert scored_out.exists(), "--out-scored is honoured even with nothing to transfer" + after = read_columns(scored_out) + assert len(after["candidate_id"]) == len(cols["candidate_id"]) + assert not np.asarray(after["is_transferred"], dtype=bool).any() + assert np.isnan(np.asarray(after["transfer_q"], dtype=float)).all() + for col in ("q_value", "peptidoform"): + assert list(after[col]) == list(cols[col]), "{} must be unchanged".format(col) def test_missing_psms_path_fails_loudly(mbr_dataset, tmp_path):