diff --git a/pwiz_tools/Osprey/Documentation/Help/en/CommandLine.html b/pwiz_tools/Osprey/Documentation/Help/en/CommandLine.html index d53c9dcf58..8508825c4c 100644 --- a/pwiz_tools/Osprey/Documentation/Help/en/CommandLine.html +++ b/pwiz_tools/Osprey/Documentation/Help/en/CommandLine.html @@ -68,7 +68,6 @@

Osprey command-line usage

-
ArgumentDescription
--task <SpectraCache | PerFileScoring | FirstPassFDR | PerFileRescoring | SecondPassFDR | ModelDiagnostics>HPC: run exactly one pipeline task (one node = one task). Omit for the full pipeline. SpectraCache stages the .spectra.bin caches; ModelDiagnostics regenerates only the --model-diagnostics report for a COMPLETED run, writing no other artifact.
--input-scores <paths|dir>HPC: one or more .scores.parquet files, or a single directory (non-recursive). Mutex with --input.
Logging
@@ -87,22 +86,22 @@

Osprey command-line usage

-h, --helpShow this help message ([ascii|unicode|sections|html|<Section>])
-v, --versionShow version
-

EXAMPLES:

osprey -i sample.mzML -l library.tsv -o results.blib

osprey -i *.mzML -l library.tsv -o results.blib --resolution hram

HPC SPLIT (one node = one --task): see --task / --input-scores above.

Distributed execution (HPC)
+

EXAMPLES:

osprey -i sample.mzML -l library.tsv -o results.blib

osprey -i *.mzML -l library.tsv -o results.blib --resolution hram

HPC SPLIT (one node = one --task): see --task above.

Distributed execution (HPC)

Run with no --task for the whole pipeline in one process. For distributed (HPC / workflow-engine) execution the pipeline splits at its join / fan-out boundaries into four single-task workers — one node = one --task: PerFileScoring (split, per file) → FirstPassFDR (join, all files) → PerFileRescoring (split, per file) → SecondPassFDR (join, all files). Pass the same --library and search options to every task; the parquet integrity check rejects inputs whose search/library hash does not match.

 # split 1 - one process per mzML (writes <stem>.scores.parquet, <stem>.calibration.json beside each input)
 Osprey --task PerFileScoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
 
-# join 1 - one process over ALL parquets (pass a directory so the order is deterministic)
-Osprey --task FirstPassFDR --input-scores ./scores_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
+# join 1 - one process over ALL runs (pass a sorted list so the order is deterministic)
+Osprey --task FirstPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
 #   writes beside each parquet: <stem>.1st-pass.fdr_scores.bin, <stem>.reconciliation.json
 
 # split 2 - one process per file (parquet + its two sidecars co-located)
-Osprey --task PerFileRescoring --input-scores s1.scores.parquet -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
+Osprey --task PerFileRescoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
 #   writes: <stem>.scores-reconciled.parquet
 
-# join 2 - one process over ALL reconciled parquets (writes out.blib)
-Osprey --task SecondPassFDR --input-scores ./reconciled_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
+# join 2 - one process over ALL runs, reading their reconciled parquets (writes out.blib)
+Osprey --task SecondPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01
-

--input-scores takes a directory (globbed and sorted internally) or an explicit file list (used in the order given). FirstPassFDR reconciliation is order-sensitive, so for FirstPassFDR and SecondPassFDR pass a directory or a deterministically sorted list. The rehydration sidecars must travel with their parquet into each worker's working directory. Let the scheduler do the fan-out (one file per split process) rather than --parallel-files, which is the single-node multi-file mode.

+

EVERY task takes -i, naming the DATA files - the same names the first split was given. A join task derives each run's parquet and sidecars from the input stem, so the data file itself need not still exist: what has to be in the worker's working directory (or under --output-dir) is that run's artifacts. FirstPassFDR reconciliation is order-sensitive, so pass a deterministically sorted list - --input-list takes one path per line and is what a cohort past a few hundred runs needs, since -i spends the command line at O(files). Let the scheduler do the fan-out (one file per split process) rather than --parallel-files, which is the single-node multi-file mode.

\ No newline at end of file diff --git a/pwiz_tools/Osprey/Osprey-workflow.html b/pwiz_tools/Osprey/Osprey-workflow.html index 750abd1b3a..88613e60ff 100644 --- a/pwiz_tools/Osprey/Osprey-workflow.html +++ b/pwiz_tools/Osprey/Osprey-workflow.html @@ -395,7 +395,7 @@

Osprey DIA pipeline workflow

▸ --task FirstPassFDR join · 1 node · holds O(distinct), never O(runs x entries) - in  <stem>.scores.parquet, <stem>.calibration.json (every run, via --input-scores) + in  <stem>.scores.parquet, <stem>.calibration.json (every run, derived from --input) out <stem>.1st-pass.fdr_scores.bin, .1st-pass.stratum.json, <stem>.reconciliation.json · validity <out>.FirstPassFDR.osprey.task out <blib-stem>.1st-pass.fdr_experiment.bin, .1st-pass.retained_base_ids.bin (compaction key), <stem>.1st-pass.model.json relay: every experiment-wide file to EVERY downstream node @@ -462,7 +462,7 @@

Osprey DIA pipeline workflow

▸ --task PerFileRescoring per-run fan-out · reads its own runs + the experiment baseline only - in  <stem>.scores.parquet (via --input-scores) + .1st-pass.fdr_scores.bin, .reconciliation.json, .calibration.json, .spectra.bin + in  <stem>.scores.parquet (derived from --input) + .1st-pass.fdr_scores.bin, .reconciliation.json, .calibration.json, .spectra.bin in  <blib-stem>.1st-pass.fdr_experiment.bin, .1st-pass.retained_base_ids.bin, <stem>.1st-pass.model.json · resident baseline out <stem>.scores-reconciled.parquet, .2nd-pass.fdr_decoys.bin, .2nd-pass.fdr_scores.bin (pass-2 worker) · validity <out>.PerFileRescoring.osprey.task relay: the run's own set + every experiment-wide file @@ -506,7 +506,7 @@

Osprey DIA pipeline workflow

▸ --task SecondPassFDR join · 1 node · final aggregation - in  <stem>.scores-reconciled.parquet (via --input-scores; falls back to <stem>.scores.parquet), .2nd-pass.fdr_scores.bin (worker) + in  <stem>.scores-reconciled.parquet (every run - one parquet per run, no .scores.parquet fallback), .2nd-pass.fdr_scores.bin (worker) in  <blib-stem>.1st-pass.fdr_experiment.bin, <stem>.1st-pass.model.json, .1st-pass.stratum.json, <stem>.reconciliation.json, .calibration.json out <output>.blib, <blib-stem>.2nd-pass.fdr_experiment.bin, <stem>.2nd-pass.fdr_scores.bin (where no worker ran) · validity <out>.SecondPassFDR.osprey.task holds O(distinct) · folds the runs one at a time, rebuilding each from its own artifacts and dropping it diff --git a/pwiz_tools/Osprey/Osprey.Core/OspreyConfig.cs b/pwiz_tools/Osprey/Osprey.Core/OspreyConfig.cs index b5a995b5f4..2704e12f3f 100644 --- a/pwiz_tools/Osprey/Osprey.Core/OspreyConfig.cs +++ b/pwiz_tools/Osprey/Osprey.Core/OspreyConfig.cs @@ -328,22 +328,15 @@ public class OspreyConfig /// Pipeline-membership flag (read by each task's IsIncluded): /// include only the per-file fan-out, not the joining tasks. Set by both /// --task PerFileScoring and --task PerFileRescoring; the - /// concrete behavior depends on the input type. With -i mzML it - /// is the Stage 1-4 worker — each input produces a - /// {stem}.scores.parquet next to it, no FDR, no blib. With - /// it is the Stage 6 rescore worker. The two - /// are told apart by input type (see ). + /// concrete behavior depends on which of the two selected it. + /// PerFileScoring is the Stage 1-4 worker - each input produces a + /// {stem}.scores.parquet next to it, no FDR, no blib; + /// PerFileRescoring is the Stage 6 rescore worker. The two are told apart + /// by , which is the only thing that ever decided it - + /// they used to be told apart by input KIND as well, and that second seam is gone. /// public bool NoJoin { get; set; } - /// - /// HPC scoring split: when set (non-null, non-empty), skip Stages 1-4 - /// entirely and load these per-file scoring caches as the starting - /// point for Stage 5+. Set by --input-scores. When set, - /// is ignored. - /// - public List InputScores { get; set; } - /// /// HPC: when true, exit after Stage 5 + reconciliation planning, /// having written the boundary files @@ -371,10 +364,10 @@ public class OspreyConfig /// membership flags above (, /// , ) /// are derived from this and drive each task's IsIncluded; this - /// property additionally lets argument validation enforce the - /// task↔input-type contract (e.g. PerFileScoring takes mzML, - /// PerFileRescore takes ) and name the task the - /// user actually typed in error messages. + /// property additionally lets argument validation name the task the user actually + /// typed in error messages. It no longer has an input-KIND contract to enforce: + /// every task takes the same data files, and the second seam that said "you handed + /// me parquets, so Stage 1-4 is done" has retired into these flags. /// public HpcTask? SelectedTask { get; set; } diff --git a/pwiz_tools/Osprey/Osprey.Core/ResidentPaths.cs b/pwiz_tools/Osprey/Osprey.Core/ResidentPaths.cs index 67d32da953..494204fdc0 100644 --- a/pwiz_tools/Osprey/Osprey.Core/ResidentPaths.cs +++ b/pwiz_tools/Osprey/Osprey.Core/ResidentPaths.cs @@ -132,11 +132,15 @@ public static class ResidentPaths /// disclosure table had to be invented to see it at all. /// /// It admits ONLY the chosen case. A run that takes the resident join because no - /// streamed one was admissible - a straight-through run, where - /// ExpectReconciledInput is false - is not refused by this token, because there - /// is nothing for the operator to choose. That case goes when - /// CanStreamStage7Join's admission stops being expressed as a CLI flag; then this - /// token is the only way to be resident, and it can be deleted with the switch. + /// streamed one was admissible is not refused by this token, because there is nothing + /// for the operator to choose. That set has SHRUNK to one: the straight-through run + /// used to be in it - CanStreamStage7Join's admission was the CLI flag + /// ExpectReconciledInput, so the ordinary run could not stream by construction - + /// and it is now derived from the reconciled parquets on disk, which every route + /// satisfies. What is left is a pass-2 mode whose per-file half has no worker + /// (OSPREY_PASS2_QVALUE=transfer), and when that half moves to + /// Pass2PerFileWorker this exemption has no subject, the guard can refuse + /// unconditionally, and this token becomes the only way to be resident. /// public static readonly string STAGE7_STREAM_OFF = @"stage7-stream-off"; diff --git a/pwiz_tools/Osprey/Osprey.IO/ParquetScoreCache.cs b/pwiz_tools/Osprey/Osprey.IO/ParquetScoreCache.cs index 5140547189..0107ec7c11 100644 --- a/pwiz_tools/Osprey/Osprey.IO/ParquetScoreCache.cs +++ b/pwiz_tools/Osprey/Osprey.IO/ParquetScoreCache.cs @@ -246,13 +246,82 @@ private static ParquetSchema BuildWriteSchema(DataField[] featureFields, /// public static bool IsSubsetWithoutScoreIndex(string path) { - if (!File.Exists(path)) + return ProbeReconciledSurvivorShape(path, out bool hasScoreIndex) && !hasScoreIndex; + } + + /// + /// True when is a reconciled parquet THIS build can read in the + /// survivor-subset shape: it exists, carries the current + /// marker, and carries the score_index column + /// that ties each survivor row back to its Stage 4 ordinal. + /// + /// The positive form of plus the marker + /// test, in one open, because two callers ask the same question about the same file and + /// asking it twice is what let them drift. One is the Stage 7 refusal that names the + /// stale files; the other is the admission a per-run fold consults BEFORE it commits to + /// rebuilding each run from these parquets - and that admission has to be the SAME + /// question the refusal asks, or a run is admitted to a fold it then aborts. + /// + /// Says nothing about whether Stage 6 did any rescore WORK on the file - that is + /// the osprey.rescored footer key and a different question, deciding whether a + /// second Percolator pass is owed. This one asks only whether the rows are readable in + /// the shape a survivor rebuild needs. + /// + public static bool IsCurrentReconciledSurvivorSubset(string path) + { + return ProbeReconciledSurvivorShape(path, out bool hasScoreIndex) && hasScoreIndex; + } + + /// + /// The one open behind and + /// : does carry the + /// current marker, and does it have the + /// score_index column? Returns false for anything this build cannot read as a + /// reconciled survivor parquet, including a file that is absent, empty, half-written + /// or foreign. + /// + /// Answers, never throws. Both callers are boolean predicates whose + /// documented false covers "not readable in that shape", and five call sites branch on + /// them - so ONE zero-length or partially-written parquet turning a predicate into an + /// unhandled stack trace pre-empts SecondPassFdrTask's named, file-listing + /// refusal, which is the message the operator is supposed to get. + /// already wrapped the identical call, so the + /// convention existed before this did. + /// + /// One open, not two. The footer and the schema come off the same reader. + /// Read separately they were two opens per file per call, uncached across five call + /// sites - order 4,460 parquet opens on a 446-run cohort before any work begins, and + /// typically on a network artifact directory. + /// + private static bool ProbeReconciledSurvivorShape(string path, out bool hasScoreIndex) + { + hasScoreIndex = false; + if (string.IsNullOrEmpty(path) || !File.Exists(path)) return false; - var footer = LoadFooterMetadata(path); - footer.TryGetValue(@"osprey.reconciled", out string marker); - if (!string.Equals(marker, RECONCILED_SURVIVORS, StringComparison.Ordinal)) + try + { + using (var stream = new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read)) + using (var reader = RunSync(ParquetReader.CreateAsync(stream))) + { + reader.CustomMetadata.TryGetValue(@"osprey.reconciled", out string marker); + if (!string.Equals(marker, RECONCILED_SURVIVORS, StringComparison.Ordinal)) + return false; + foreach (var f in reader.Schema.GetDataFields()) + { + if (!string.Equals(f.Name, FIELD_SCORE_INDEX.Name, StringComparison.Ordinal)) + continue; + hasScoreIndex = true; + break; + } + return true; + } + } + catch (Exception ex) when (!(ex is OutOfMemoryException)) + { + // Unreadable IS "not a current reconciled survivor parquet". The caller that + // cares which file it was names it; see SecondPassFdrTask.UnusableReconciledParquets. return false; - return !HasColumn(path, FIELD_SCORE_INDEX.Name); + } } /// Whether a parquet's schema carries a column by this name. @@ -1845,24 +1914,6 @@ public static string ReconciledPathFromScoresPath(string scoresPath) return scoresPath; } - /// - /// The path a post-Stage-6 reader (Stage 7 feature reload, resume / - /// --task SecondPassFDR) should consume for a given original - /// .scores.parquet path: the reconciled sibling when it exists - /// on disk, otherwise the original. This per-file selection is the - /// read-side contract that makes the separate-reconciled-file design - /// byte-equivalent to the former in-place overwrite: files that had - /// reconciliation work read the reconciled bytes (which used to be - /// written over the original), while files with no Stage 6 work -- which - /// PerFileRescoreTask deliberately skips, leaving no reconciled - /// file -- read the untouched original (which used to be left in place). - /// - public static string EffectiveScoresPathFromScoresPath(string scoresPath) - { - string reconciled = ReconciledPathFromScoresPath(scoresPath); - return File.Exists(reconciled) ? reconciled : scoresPath; - } - /// /// Check if an existing Parquet file's custom metadata matches the expected values. /// Returns true if all expected keys exist with matching values. diff --git a/pwiz_tools/Osprey/Osprey.Tasks/BlibOutputWriter.cs b/pwiz_tools/Osprey/Osprey.Tasks/BlibOutputWriter.cs index c5f3636907..9a978c503a 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/BlibOutputWriter.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/BlibOutputWriter.cs @@ -117,9 +117,11 @@ internal static void Write( // Pre-create source file IDs once. SpectrumSourceFiles.fileName carries // the ABSOLUTE path of each spectrum source file, matching BiblioSpec's // BlibBuild (BuildParser.insertSpectrumFilename resolves every name it - // is given to a full path). On a from-scores run the acquisition itself - // is not among the inputs, so the path is synthesized beside the parquet - // - the same rule the rescore hydrate uses. The golden and cross-impl + // is given to a full path). Every route names the acquisition on -i, so the + // path comes from the input itself - it does not have to EXIST for that, and + // on a join node it usually does not. A from-scores run used to arrive with + // the acquisition absent from the inputs entirely, and the path was + // synthesized beside the parquet. The golden and cross-impl // comparators key these strings by BASENAME, which is what keeps the // committed goldens machine-independent. SpectrumSourceFiles.idFileName // carries the library filename (Skyline expects this - Rust @@ -130,9 +132,10 @@ private static Dictionary CreateSourceFiles( IReadOnlyList fileNames, double fdrThreshold) { string libraryIdName = Path.GetFileName(config.LibrarySource.Path); - var inputs = config.InputScores != null && config.InputScores.Count > 0 - ? config.InputScores.ConvertAll(RescoreHydration.SyntheticInputFromParquet) - : config.InputFiles; + // The data files, on every route. A Stage 7 node used to be handed parquets and + // had to convert them back into data-file names right here, to write the + // SpectrumSourceFiles rows Skyline reads; it is handed the names themselves now. + var inputs = config.InputFiles; var sourcePathByName = new Dictionary(); if (inputs != null) { diff --git a/pwiz_tools/Osprey/Osprey.Tasks/FirstPassFdrTask.cs b/pwiz_tools/Osprey/Osprey.Tasks/FirstPassFdrTask.cs index 26a17e0ae5..b562d6b000 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/FirstPassFdrTask.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/FirstPassFdrTask.cs @@ -94,18 +94,14 @@ internal sealed class FirstPassFdrTask : OspreyTask /// internal static bool IsIncludedFor(OspreyConfig c) { - bool inputs = c.InputScores != null && c.InputScores.Count > 0; - // The (inputs && StopAfterStage5) clause leans on a CLI-enforced - // invariant: StopAfterStage5 is set by --task FirstPassFDR, which - // requires --input-scores, so StopAfterStage5 implies inputs at - // parse time -- a --task FirstPassFDR run can never reach here without - // InputScores. - // ProgramTests.TestValidateFirstPassFdrRequiresInputScores pins that - // rejection, since the membership truth table (PipelineMembershipTest) - // does not encode the cross-flag dependency on its own. - return (!inputs && !c.NoJoin) - || (inputs && c.StopAfterStage5) - || (inputs && !c.NoJoin && !c.ExpectReconciledInput); + // Three clauses over two seams collapsed to one over the task flags. The + // retired term was `inputs` - were parquets supplied - which the truth table + // above shows was never doing independent work: it tracked exactly the tasks + // whose flags already say so. Excluded for the two per-file workers (NoJoin) + // and for the Stage 7 node (ExpectReconciledInput); included for the full + // pipeline, for --task FirstPassFDR itself, and for --task ModelDiagnostics, + // which needs first-pass state to render. + return !c.NoJoin && !c.ExpectReconciledInput; } // Stage 5/6 planning byproducts this task publishes. The same four types diff --git a/pwiz_tools/Osprey/Osprey.Tasks/Pass2FdrSidecar.cs b/pwiz_tools/Osprey/Osprey.Tasks/Pass2FdrSidecar.cs index cf16d9b15f..2c27c279d4 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/Pass2FdrSidecar.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/Pass2FdrSidecar.cs @@ -1486,9 +1486,13 @@ internal static void WritePass2ExperimentSidecar( var byEntryId = new Dictionary(); try { - string parquetPath = - ParquetScoreCache.EffectiveScoresPathFromScoresPath( - perFileParquetPaths[fileName]); + // The RECONCILED parquet, derived rather than probed. This runs both + // in-process - where the published map holds Stage 4 paths, because + // Stage 1-4 ran here - and on a --task SecondPassFDR node, where it + // already holds reconciled ones; the derivation is idempotent, so one + // expression states the same intent on both routes. + string parquetPath = ParquetScoreCache.ReconciledPathFromScoresPath( + perFileParquetPaths[fileName]); ParquetScoreCache.ReadFdrStubScalars(parquetPath, (entryId, charge, isDecoy, coelutionSum, modseq) => { @@ -2026,7 +2030,7 @@ void BeginFile(string fileKey) // The parquet lookup is established by the validation loop above (every file // has a parquet path or this method already returned false), and resolved // HERE so a key miss cannot be reported as a parquet failure by the reader. - string effectiveParquetPath = ParquetScoreCache.EffectiveScoresPathFromScoresPath( + string effectiveParquetPath = ParquetScoreCache.ReconciledPathFromScoresPath( perFileParquetPaths[fileKey]); // currentKey/currentEntries are staged by BeginFile now, on every path. // Read from the path the validation loop above checked with IsCurrentFormat, @@ -2385,13 +2389,12 @@ private static void ComputePass2Resident( kvp.Key, kvp.Value.Count)); continue; } - // Read the RECONCILED parquet (Stage 6's rescored - // features) when it exists; fall back to the original - // Stage 4 parquet for files that had no reconciliation - // work (no reconciled sibling was written). The - // perFileParquetPaths map holds original paths. + // Read the RECONCILED parquet - Stage 6's rescored features - which it + // writes for every run, so this is a derivation and not a preference. + // The published map holds Stage 4 paths in-process and reconciled ones on + // a --task SecondPassFDR node; the derivation is idempotent over both. string effectiveParquetPath = - ParquetScoreCache.EffectiveScoresPathFromScoresPath(parquetPath); + ParquetScoreCache.ReconciledPathFromScoresPath(parquetPath); Dictionary featByScoreIndex; try { @@ -3582,8 +3585,7 @@ private bool WriteCore(string fileName, Action write) { TaskValiditySidecar.Write(pass2Path, _taskName, OspreyVersion.Current, _taskValidityKey, - new[] { ParquetScoreCache.EffectiveScoresPathFromScoresPath( - ParquetScoreCache.GetScoresPath(inputFile)) }); + new[] { ParquetScoreCache.GetReconciledScoresPath(inputFile) }); } catch (Exception ex) when (!(ex is OutOfMemoryException)) { diff --git a/pwiz_tools/Osprey/Osprey.Tasks/PerFileRescoreTask.cs b/pwiz_tools/Osprey/Osprey.Tasks/PerFileRescoreTask.cs index 2999ba8ef5..fbda99a74f 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/PerFileRescoreTask.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/PerFileRescoreTask.cs @@ -151,26 +151,32 @@ private readonly Dictionary> _resetEntryIdsByFile public override string Name => TASK_NAME; /// - /// Computes the Stage 6 rescore in straight-through, the rescore worker - /// (--task PerFileRescoring), and the --input-scores - /// full-pipeline. Excluded in --task PerFileScoring, --task FirstPassFDR (stops at Stage 5), - /// and the --task SecondPassFDR run (where it rehydrates rather than - /// re-scoring, SecondPassFDR having no mzMLs). + /// Computes the Stage 6 rescore in the straight-through run and in the rescore + /// worker (--task PerFileRescoring). Excluded in --task PerFileScoring, + /// --task FirstPassFDR (stops at Stage 5), --task ModelDiagnostics (a render, which + /// also stops there) and --task SecondPassFDR, where it rehydrates rather than + /// re-scoring - that node has no data files to score from. /// public override bool IsIncluded(PipelineContext ctx) { var c = ctx.Config; - bool inputs = c.InputScores != null && c.InputScores.Count > 0; - // StopAfterStage5 is checked on BOTH input routes. It used to appear only in the - // --input-scores clause, which was enough while --task FirstPassFDR was the only - // thing that set it - that task rejects -i. --task ModelDiagnostics also stops - // after Stage 5 and takes -i, so this task ran anyway, demanded CompactedEntries - // that a diagnostics-only fold never publishes, and failed the run AFTER the report - // it was asked for had been written. A flag named for a stage boundary has to mean - // that boundary whatever the inputs look like. - return (!inputs && !c.NoJoin && !c.StopAfterStage5) - || (inputs && c.NoJoin) - || (inputs && !c.NoJoin && !c.StopAfterStage5 && !c.ExpectReconciledInput); + // The rescore worker is the ONE task NoJoin does not distinguish - it is set by + // --task PerFileScoring too - and the input KIND is what used to tell them + // apart: mzML in meant Stage 1-4, parquets in meant Stage 6. Both are named by + // their data files now, so the task says which worker this is, which is the only + // thing that ever actually decided it. + // + // StopAfterStage5 means that boundary whatever the inputs look like, and it is + // checked on every route rather than one - it used to be checked on one only, + // which failed a run AFTER writing the report it was asked for. + // + // --task FirstPassFDR is its ONLY setter (Program.cs has the single assignment). + // The paragraph here said ModelDiagnostics sets it too; it does not, and the same + // claim had been copied into docs/15-hpc-scoring-split.md's truth table and a unit + // test helper that built the config to match. ModelDiagnostics sets none of the + // three flags, so it is a member of every task and suppresses artifact WRITES. + return c.SelectedTask == HpcTask.PerFileRescore + || (!c.NoJoin && !c.StopAfterStage5 && !c.ExpectReconciledInput); } // The final milestone of the shared mutable entry buffer: this task @@ -318,22 +324,19 @@ public override bool Run(PipelineContext ctx) // it dropped, so streaming there would destroy the only copy of the survivors on // its way past. Null leaves StreamFiles walking the resident buffer, which is what // the OSPREY_STAGE6_STREAM_SURVIVORS=0 A/B oracle needs it to do. - // NO per-file source on this path, and the reason is a property of the materializer - // rather than a preference. MaterializeRescoredFile is ONE-SHOT: it overlays the - // reconciled parquet and appends gap-fill rows, so calling it twice for one run - // duplicates them - which is the same "run-once, and a failed build stays failed" - // rule RescoredEntries' own remarks state for the whole-run build. A Stage 7 fold - // re-enumerates, so it needs a source that can rebuild a run from disk repeatedly - // and identically; this one cannot, and handing it over produced a straight-through - // Stellar run that exited 1 on AssertSidecarDescribesPool. - // - // The leg that has such a source is the reconciled-input merge, where - // BuildStage7PerRunSource supplies it (Rehydrate, below). Straight-through Stage 7 - // therefore keeps its resident pool for now. That is the honest state and not a - // hidden one: making this materializer idempotent - or having it rebuild from the - // reconciled parquet alone, which already holds the merged gap-fill rows - is what - // extends the fold to this leg, and it is separate work. - var rescored = new RescoredEntries(_perFileEntries, () => BuildRescoredPool(ctx)); + // WITH a per-file source when this run can supply a re-enumerable one, which is + // what makes the bounded join the DEFAULT rather than a property of + // --task SecondPassFDR. The paragraph that stood here said this leg could not have + // one because MaterializeRescoredFile is ONE-SHOT - it overlays the reconciled + // parquet and appends gap-fill rows, so a second call for one run duplicates them, + // which once exited a straight-through Stellar run 1 on AssertSidecarDescribesPool. + // True, and true of a NON-EMPTY list only: given an empty one it takes its + // rebuild-from-the-reconciled-parquet branch, where the gap-fill rows are already + // merged and no overlay runs, and that branch repeats identically. So the condition + // is not "make the materializer idempotent" but "hand it a list it can rebuild", + // which is what BuildRunPerRunSource establishes before offering the source at all. + var rescored = new RescoredEntries(_perFileEntries, () => BuildRescoredPool(ctx), + BuildRunPerRunSource(ctx, survivorLoader)); ctx.Publish(rescored); // Self-gate: rescore + reconciliation only run when there is @@ -654,6 +657,30 @@ public override bool Rehydrate(PipelineContext ctx) { _perFileEntries = ctx.Get().Value; + // The SAME conversion the reconciled-input arm below already had, on the arm + // that carries every ordinary run. Both arms end with a milestone over the same + // shared buffer; what the source changes is that Stage 7 rebuilds one run, folds + // it and drops it, instead of the two whole-run loops beneath this block + // bringing all 446 to their post-rescore state and holding them - 91.1 GB + // private, measured on a 446-run resume, which is what admitting only + // --task SecondPassFDR to the fold left standing. + // + // Null when this run has no way to rebuild a dropped run, and then the resident + // loops below run exactly as they did. LAZY, unlike those loops: with a source + // in hand there is no reason to do the work before the consumer that folds asks + // for it, and a consumer that reads .Value instead still gets the whole pool + // through the same source (MaterializeAllFromSource), reported as the expense + // it is. + var resumeSource = BuildResumePerRunSource(ctx); + if (resumeSource != null) + { + var resumeBuffer = _perFileEntries; + ctx.Publish(new RescoredEntries(resumeBuffer, + () => MaterializeAllFromSource(resumeBuffer, resumeSource, ctx), + resumeSource)); + return true; + } + // PR-E: a fresh ExecuteRescore would overlay each file's reconciled // boundaries/area/features onto its CompactedEntries rows + append // gap-fill. On resume the driver skipped Run because the reconciled @@ -662,19 +689,22 @@ public override bool Rehydrate(PipelineContext ctx) // buffer stays at 1st-pass RTs and SecondPassFDR (which reads ApexRt/ // StartRt/EndRt/BoundsArea straight off these entries) writes 1st-pass // RTs into the final blib instead of the Stage 6 reconciled values. - // Files with no reconciled sibling on disk are no-work files; a fresh - // run leaves their entries at 1st-pass too, so they are left unchanged. + // A run with no current reconciled parquet is a FAILURE here, not a no-work + // file. Stage 6 writes one for every run - WriteUnchangedReconciled covers the + // run it did no work on - so absence means the write never landed (P13: a + // phase with nothing to say writes the file anyway, precisely so absence stays + // unambiguous). The Stage 4 parquet is not a substitute: it holds pre- + // reconciliation boundaries and none of the gap-fill rows. // - // Refill first when Stage 5 released the survivors (issue #4526). A resume - // that re-runs Stage 5 lands here with an EMPTY buffer, and overlaying onto - // empty lists produces an almost-empty blib instead of failing. // Eager here, unlike Run: Rehydrate itself only ever executes because a // consumer pulled this milestone, so the work is already lazy. - var resumeLoader = StreamedSurvivorLoader(ctx); - if (resumeLoader != null) - MaterializeAllSurvivors(_perFileEntries, resumeLoader, ctx); - OverlayReconciledIntoFiles(_perFileEntries, CurrentReconciledPaths(ctx), - ctx.Get().Value); + MaterializeAllResumedFiles(_perFileEntries, StreamedSurvivorLoader(ctx), + CurrentReconciledPaths(ctx), + // Lazy for the reason BuildResumePerRunSource states: with a survivor + // loader every run takes the load branch and the gap-fill map - order + // 10 GB of envelope JSON at 446 runs - is never read. + new Lazy>>( + () => ctx.Get().Value), ctx); ctx.Publish(new RescoredEntries(_perFileEntries)); return true; @@ -1072,8 +1102,10 @@ void WriteAnswer(string fileName, IReadOnlyList records, // PerFileRescoring. Stamping SecondPassFDR's key here would leave a file that // outlives the inputs it was computed from, which is the one thing a resume // cannot detect by looking. - var stampInputs = new[] { ParquetScoreCache.EffectiveScoresPathFromScoresPath( - ParquetScoreCache.GetScoresPath(inputFile)) }; + // The RECONCILED parquet, named: these artifacts were computed from the file + // this same task wrote moments ago, so that is what they must be stamped + // against. Its Stage 4 sibling is a different population. + var stampInputs = new[] { ParquetScoreCache.GetReconciledScoresPath(inputFile) }; TaskValiditySidecar.Write(decoysPath, taskName, OspreyVersion.Current, taskValidityKey, stampInputs); TaskValiditySidecar.Write(pass2Path, taskName, OspreyVersion.Current, @@ -1507,7 +1539,10 @@ private sealed class RescorePassInputs inputs.Pass2Worker.CompeteStampAndWrite( fileName, FdrScoresSidecar.Pass1Path(inputFile), - ParquetScoreCache.EffectiveScoresPathFromScoresPath(parquetPath), + // The reconciled parquet the refusal above is worded for. Derived rather + // than probed: this worker wrote it earlier in this same task, so its + // absence is a failure to report, not a reason to read Stage 4's. + ParquetScoreCache.ReconciledPathFromScoresPath(parquetPath), fdrEntries); } @@ -2000,12 +2035,10 @@ private static Func BuildPerRunHydrate( // and "the gate is green so the new path must have run" is precisely the inference // that let an earlier resume fix report success while testing the old path (defect // (b2), TODO-20260901_osprey_firstpassfdr_resume). A test can assert this line. - // Count the runs from the published parquet paths, NOT from config.InputScores - - // that list is null on a straight-through run, where the inputs are mzMLs. It read - // InputScores while the predicate above still required it; dropping that term left - // this line behind, and it threw a NullReferenceException on the first - // straight-through run. The paths are the right source on both shapes anyway: they - // are what the loop iterates. + // Count the runs from the published parquet paths. It used to read InputScores, + // which was null on a straight-through run, and it threw a NullReferenceException + // on the first one after the predicate above stopped requiring that list. The + // paths are the right source regardless: they are what the loop iterates. ctx.LogInfo(string.Format( @"Per-run rescore: hydrating each of {0} run(s) from its own artifacts " + @"(no all-runs pre-load; {1} retained base_id(s) read once).", @@ -2082,6 +2115,70 @@ private static void MaterializeAllFromSource( } } + /// + /// The per-run source Stage 7 folds through on the STRAIGHT-THROUGH resume: bring ONE + /// run's list to the post-rescore state the whole-run loop in + /// would have brought every run to, so the join holds one run at a time. + /// + /// Its body is - that loop's own per-file + /// half, called rather than reimplemented. Nothing in it reads another run's entries, + /// so run-at-a-time is the same work in the same order as all-runs - which is why the + /// streamed and resident arms write byte-identical output, and why the fix is a + /// call-shape change rather than a second implementation. + /// + /// RE-ENUMERABLE, which is the property + /// needs: a fold pass drops each run's list, so a second pass finds it empty, and the + /// half here rebuilds an empty list from disk identically. Unconditionally so, unlike + /// : a resume runs no rescore, so no run's list ever + /// holds state that is not already on disk, and there is nothing a drop can lose. + /// + /// Null when the run cannot stream. Two ways: the admission itself + /// (, which is where + /// the reconciled parquets are required to be readable - asked in FULL here, unlike + /// , because a resume enters after Stage 6 has + /// written them), or no published survivor loader - and the second is not a preference. + /// A run whose survivors were never released has nothing on disk to rebuild them from, + /// so folding would DROP the only copy on its way past. That is the same condition + /// states for its own deferred build; the difference is only that + /// this arm asks for the loader without the Stage-6 switch (see + /// ). A null loader today means an empty join, + /// where there is no pool to bound and the resident loops cost nothing. + /// + private Action> BuildResumePerRunSource(PipelineContext ctx) + { + if (!ScoringTaskShared.CanStreamStage7Join(ctx.Config)) + return null; + var loader = PublishedSurvivorLoader(ctx); + if (loader == null) + return null; + // Both answers taken ONCE, here, for the reason RescoredPoolPlan gives: the + // reconciled-parquet judgement stops being true the moment this task returns, when + // the driver stamps a fresh validity sidecar onto every declared output that merely + // exists. The answer travels; the question does not. + var reconciledPaths = CurrentReconciledPaths(ctx); + // LAZY, and on this arm usually never read at all. PerFileGapFillForRescore is + // published DEFERRED, and its own header records what pulling it costs: "dotTrace + // 69.7s total in ReadGapFillAndCalibrations ... an in-code probe at 92.7s; and a + // night run's log gap at 111s" - order 10 GB of envelope JSON at 446 runs. Pulling + // it here made the whole-run read unconditional inside a method whose surrounding + // comment says the work should wait for the consumer that folds. It is needed ONLY + // by the overlay branch, which a run with a survivor loader does not take. + var gapFill = new Lazy>>( + () => ctx.Get().Value); + // Says which shape Stage 7 took, for the reason its --task SecondPassFDR sibling + // gives: without it the only evidence is a memory profile, and "the gate is green so + // the new path must have run" is the inference that lets a resident path pass as a + // streamed one. + ctx.LogInfo(string.Format( + @"Second-pass join: folding over {0} run(s), each rebuilt from its own " + + @"reconciled parquet and dropped (no all-runs survivor pool). {1} of {0} run(s) " + + @"carry a current reconciled parquet; a run without one is an error, not a " + + @"run that keeps its 1st-pass boundaries.", + _perFileEntries.Count, reconciledPaths.Count)); + return (fileName, survivors) => + MaterializeResumedFile(fileName, survivors, loader, reconciledPaths, gapFill, ctx); + } + private static Action> BuildStage7PerRunSource( IReadOnlyDictionary perFileParquetPaths, OspreyConfig config, @@ -2144,18 +2241,26 @@ private static Action> BuildStage7PerRunSource( @"Second-pass join hydrate: no scores parquet path published for {0}", fileName)); } - // Resolve to the RECONCILED sibling, exactly as every other reader on this leg - // does. perFileParquetPaths holds whatever --input-scores named, and the - // documented fallback form names .scores.parquet - so rebuilding from the - // path verbatim would give this arm the PRE-reconciliation rows (first-pass - // boundaries, no Stage-6 gap-fill) while the resident arm on the same command - // line reads the reconciled file. Two different .blib files from one command - // line is precisely what the byte-identity oracle exists to prevent, and it - // would not have caught it: both arms would be self-consistent. - string effectivePath = - ParquetScoreCache.EffectiveScoresPathFromScoresPath(parquetPath); + // The published path IS the reconciled parquet on this leg: it is reached only + // under --task SecondPassFDR, and ScoringTaskShared.ScoresPathsForInputs + // resolves that task's paths to the reconciled artifact by task membership + // rather than by probing disk. Asserted rather than re-resolved, because + // rebuilding from a Stage 4 path would give this arm the PRE-reconciliation + // rows (first-pass boundaries, no Stage-6 gap-fill) while the resident arm on + // the same command line reads the reconciled file. Two different .blib files + // from one command line is precisely what the byte-identity oracle exists to + // prevent, and it would not have caught it: both arms would be self-consistent. + if (!ParquetScoreCache.IsReconciledScoresPath(parquetPath)) + { + throw new InvalidDataException(string.Format( + @"Second-pass join hydrate: run '{0}' published '{1}', which is not a " + + @".scores-reconciled.parquet. The join rebuilds every run from its " + + @"reconciled artifact; a Stage 4 path here would silently produce " + + @"1st-pass boundaries with no gap-fill rows.", + fileName, parquetPath)); + } bool overlayFirstPass = !haveSecondPass.Contains(fileName); - RescoreHydration.RefillOneRunSurvivors(fileName, effectivePath, survivors, + RescoreHydration.RefillOneRunSurvivors(fileName, parquetPath, survivors, retainedBaseIds, overlayFirstPass ? experimentRecords.Value : null, (name, path) => ParquetScoreCache.LoadFdrStubsFromParquet(path, null, sequencePool), overlayFirstPass); @@ -2182,11 +2287,12 @@ private static Action> BuildStage7PerRunSource( /// /// Build the file_name -> input_files index map used to pick the - /// right mzML path for the spectra-cache load + sibling - /// .calibration.json. For the worker, config.InputFiles was - /// synthesized from --input-scores parquet stems by Program.Main; - /// for in-process it's the user's -i mzML list. Either way the - /// stem matches the file_name keys in perFileEntries. + /// right data-file path for the spectra-cache load + sibling + /// .calibration.json. One source on every route: the user's -i list. + /// A worker's used to be SYNTHESIZED from --input-scores parquet stems by + /// Program.Main, which is the round trip that flag forced; both routes now + /// name the data files, so the stem matches the file_name keys in + /// perFileEntries without a second derivation. /// private static Dictionary BuildFileNameToIndex(IReadOnlyList inputFiles) { @@ -2531,6 +2637,23 @@ private static FirstPassSurvivorLoader StreamedSurvivorLoader(PipelineContext ct { if (!OspreyEnvironment.Stage6StreamSurvivors) return null; + return PublishedSurvivorLoader(ctx); + } + + /// + /// The per-run survivor loader FirstPassFDR published, WITHOUT the Stage-6 switch + /// applies. + /// + /// One switch per stage. OSPREY_STAGE6_STREAM_SURVIVORS=0 is the A/B oracle + /// for the RESCORE window - it asks Stage 6 to keep the buffer it would have drained - + /// and it says nothing about how Stage 7 should fold. Reading it through the Stage-6 + /// gate would have made that oracle silently decide the Stage-7 arm as well, so the + /// stage whose arm OSPREY_STAGE7_STREAM governs asks for the loader directly. + /// The object is the same one either way - the gate withholds it, it does not + /// unbuild it. + /// + private static FirstPassSurvivorLoader PublishedSurvivorLoader(PipelineContext ctx) + { return ctx.TryGet(out var source) ? source?.Value : null; } @@ -2564,8 +2687,10 @@ private sealed class RescoredPoolPlan /// plan is a refill and nothing else). EMPTY is not null: a rescore that ran and /// skipped every file still overlays, exactly as a cold run does. /// file name -> the .scores-reconciled.parquet - /// judged CURRENT while Run still held the answer. Files absent from this map keep - /// their 1st-pass boundaries. + /// judged CURRENT while Run still held the answer. A file absent from this map is a + /// run Stage 6 did not persist, and it is a hard failure - see + /// . Null only on the refill-only plan, which + /// does not overlay at all. /// The planner's per-file gap-fill targets, for the /// overlay. /// Per file, the entry_ids whose scores the rescore @@ -2602,6 +2727,102 @@ public static RescoredPoolPlan RefillOnly( public IReadOnlyDictionary> ResetEntryIds { get; } } + /// + /// The per-run source Stage 7 folds through on the straight-through COMPUTE path, or + /// null when this run cannot supply one and the whole-run + /// stays the only route. + /// + /// Its body is - the per-file half the + /// whole-run build already loops over - so run-at-a-time is the same work in the same + /// order, which is why the two arms write byte-identical output. + /// + /// A third condition cannot be asked here, and is enforced at fold time instead: + /// a run whose reconciled parquet never reached disk keeps its re-scored entries in + /// memory, and a fold that drops them loses the only copy. Only the rescore this + /// method runs BEFORE can decide that, so the source raises it when asked. + /// + /// Two conditions here, and neither is a preference. A run with no survivor loader kept + /// its resident buffer and has nothing on disk to rebuild a run from, so folding would + /// DROP the only copy on its way past. And every list must ALREADY be empty: that is + /// what makes repeatable, because an empty list + /// takes its rebuild-from-the-reconciled-parquet branch and skips the gap-fill-appending + /// overlay that a second call would apply twice. Both hold together exactly when + /// FirstPassFDR released the survivors, which is the default; the checked form is used + /// rather than that inference because the inference is the kind that stops being true + /// quietly. + /// + /// Asked WITHOUT the reconciled-parquet term + /// (): this decision is + /// taken at the top of Stage 6, and the parquets the full predicate asks about are what + /// the rescore below is about to write. This arm does not need them - a run without a + /// reconciled sibling is rebuilt from its Stage 4 parquet plus its 1st-pass sidecar, + /// which is the no-work file's normal path here. + /// + private Action> BuildRunPerRunSource( + PipelineContext ctx, FirstPassSurvivorLoader survivorLoader) + { + if (survivorLoader == null || + !ScoringTaskShared.Stage7StreamAdmittedBeforeRescore( + ctx.Config, OspreyEnvironment.Stage7Stream)) + { + return null; + } + foreach (var kv in _perFileEntries) + { + if (kv.Value.Count > 0) + return null; + } + // The SAME opening words as the other two arms' markers, deliberately: the gate + // asserts the per-run fold by that phrase, and an arm that streams under a different + // sentence is an arm no verifier can see. Said HERE, at the top of Stage 6, because + // this is where the decision is taken - a worker that exits before Stage 7 has still + // made it, and the alternative (report it when the fold starts) is a fact about the + // consumer rather than about this task. + ctx.LogInfo(string.Format( + @"Second-pass join: folding over {0} run(s), each rebuilt from its own artifacts " + + @"and dropped (no all-runs survivor pool). Decided at Stage 6, where the " + + @"survivors were released.", _perFileEntries.Count)); + return (fileName, entries) => + { + var plan = PoolPlanForBuild(); + // A run with no current reconciled parquet fails in MaterializeRescoredFile + // below, on BOTH arms, rather than being caught here for the fold alone. + // ExecuteRescore drops a run's entries only when its reconciled parquet reached + // disk, KEEPING them when the write no-opped or failed - so on the resident arm + // that run's rescore survives in memory and the output looks right while + // nothing was persisted. A fold drops every run it hands over, so the same run + // would be rebuilt from a parquet that is not there. Neither is a run that + // finished: one arm cannot be allowed to treat as routine what the other has + // to fail on, or the missing artifact is a property of the arm. + // + // CLEARED here rather than relying on the caller having dropped the run. + // StreamFiles does drop it, but MaterializeFile leaves that to its caller, and + // the whole repeatability argument above rests on the list being empty - so the + // source establishes that itself instead of inheriting it from a call site. + entries.Clear(); + // plan.Loader is non-null by construction: both plan branches are built from + // the same survivorLoader this method already refused to proceed without. + MaterializeRescoredFile(ctx, plan, fileName, entries); + }; + } + + /// + /// The plan parked, or a hard failure. + /// + /// A pull before Run decided is a programming defect, not a case to guess at: + /// every guess available here - refill-only, or an overlay against paths not yet judged - + /// silently produces a wrong reported set rather than an error. + /// + private RescoredPoolPlan PoolPlanForBuild() + { + if (_poolPlan == null) + { + throw new InvalidOperationException( + @"RescoredEntries was pulled before PerFileRescoring decided how to build the survivor pool."); + } + return _poolPlan; + } + /// /// Bring the shared buffer to the whole-run post-rescore state the /// milestone promises, on the first read of that @@ -2620,15 +2841,7 @@ public static RescoredPoolPlan RefillOnly( /// private void BuildRescoredPool(PipelineContext ctx) { - // A pull before Run decided is a programming defect, not a case to guess at: every - // guess available here (refill-only, or an overlay against paths not yet judged) - // silently produces a wrong reported set rather than an error. - var plan = _poolPlan; - if (plan == null) - { - throw new InvalidOperationException( - @"RescoredEntries was pulled before PerFileRescoring decided how to build the survivor pool."); - } + var plan = PoolPlanForBuild(); if (plan.Loader == null) return; var sw = Stopwatch.StartNew(); @@ -2669,20 +2882,27 @@ private void BuildRescoredPool(PipelineContext ctx) private void MaterializeRescoredFile(PipelineContext ctx, RescoredPoolPlan plan, string fileName, List entries) { - // ONE parquet, not two. When this file's reconciled parquet was judged - // current, it already holds the survivor subset with Stage 6's boundaries - // applied and the gap-fill rows merged - so reading it makes both the - // Stage 4 read and the overlay that put those values back unnecessary - // (#4486). Stage 6 originally OVERWROTE the Stage 4 parquet, which is why - // one read used to give both; splitting the files left Stage 7 reading one - // for the rows and the other for the values. - string reconciledPath = null; - plan.ReconciledPaths?.TryGetValue(fileName, out reconciledPath); - bool loadedReconciled = reconciledPath != null && entries.Count == 0; - MaterializeFileSurvivors(fileName, entries, plan.Loader, ctx, - loadedReconciled ? reconciledPath : null); + // The refill-only plan FIRST, because it is the one route here that legitimately + // never reads a reconciled parquet - not a run missing one. The resident arm does + // nothing at all on this route: it leaves the buffer as Stage 5 compacted it and + // SecondPassFDR reloads the rescored features from the reconciled parquets by + // identity, so overlaying here would apply Stage-6 boundaries twice (see the + // RefillOnly call site). Its one job is to put back what FirstPassFDR released. if (plan.RescoredFiles == null) + { + MaterializeFileSurvivors(fileName, entries, plan.Loader, ctx); return; + } + // ONE parquet, not two. This file's reconciled parquet already holds the survivor + // subset with Stage 6's boundaries applied and the gap-fill rows merged - so + // reading it makes both the Stage 4 read and the overlay that put those values + // back unnecessary (#4486). Stage 6 originally OVERWROTE the Stage 4 parquet, + // which is why one read used to give both; splitting the files left Stage 7 + // reading one for the rows and the other for the values. + string reconciledPath = ReconciledPathOrFail(fileName, plan.ReconciledPaths, ctx); + bool loadedReconciled = entries.Count == 0; + MaterializeFileSurvivors(fileName, entries, plan.Loader, ctx, + loadedReconciled ? reconciledPath : null); // BEFORE the overlay, which appends gap-fill rows: the planner's indices // address the survivor list as loaded, and appending shifts nothing but // would be indexed if the reset ran after. The overlay preserves Score / @@ -2693,7 +2913,7 @@ private void MaterializeRescoredFile(PipelineContext ctx, RescoredPoolPlan plan, // copy of the gap-fill rows already merged into them. if (!loadedReconciled) { - OverlayReconciledIntoFile(fileName, entries, plan.ReconciledPaths, + OverlayReconciledIntoFile(fileName, entries, reconciledPath, plan.GapFill?.Value, canonicalize: false); } } @@ -2722,10 +2942,15 @@ private IReadOnlyDictionary CurrentReconciledPaths(PipelineConte } /// - /// Refill every file whose survivor list was released, leaving files that already - /// hold entries untouched so a second call is a no-op. Throws if any file's parquet - /// or 1st-pass sidecar cannot be read - Stage 5 wrote both, so a failure here is a - /// fault rather than an absence. + /// Bring every file to its post-rescore state on a RESUME: the whole-run shape of + /// , which is also what + /// folds through one run at a time. One loop + /// where there were two - a refill pass over every file followed by an overlay pass + /// over every file - because both passes now resolve to the same single parquet, so + /// splitting them only decided how long each file's list stayed alive. + /// + /// Throws if any file's artifacts cannot be read - Stage 5 and Stage 6 wrote + /// them, so a failure here is a fault rather than an absence. /// /// Logs the fault and sets the exit code before throwing. The throw is what the /// deferred needs (a pull has no bool channel back to @@ -2735,15 +2960,22 @@ private IReadOnlyDictionary CurrentReconciledPaths(PipelineConte /// the message and a stack trace. Returning false and letting Stage 7 build a partial /// pool is the one option not on the table. /// - private static void MaterializeAllSurvivors( + private static void MaterializeAllResumedFiles( List>> perFileEntries, - FirstPassSurvivorLoader loader, PipelineContext ctx) + FirstPassSurvivorLoader loader, + IReadOnlyDictionary reconciledPaths, + Lazy>> gapFill, PipelineContext ctx) { // Reported, not silent: this is a per-file parquet + sidecar read across every file // in the run, landing in the quiet window where Stage 7 starts (or, on resume, in // the rehydrate) with nothing else printing. An unreported sequential loop of // exactly this shape has twice read as a hung run in this codebase (#4513, // Pass2FdrSidecar). Console-only. + // The SAME label the cold arm's loop uses, because it is now the same operation on + // the same artifact - one name, so a log reads the same whichever arm produced it, + // and the resident-join landmark in ai/scripts/Osprey/CHS/README.md keeps meaning + // what it says. It names WHAT is rebuilt (the first-pass survivor subset), not + // which file supplied the rows. using (var progress = new ProgressReporter(string.Format( @"Rebuilding first-pass survivors from {0} file(s)", perFileEntries.Count), perFileEntries.Count)) @@ -2752,16 +2984,102 @@ private static void MaterializeAllSurvivors( foreach (var kv in perFileEntries) { progress.Report(++done); - MaterializeFileSurvivors(kv.Key, kv.Value, loader, ctx); + MaterializeResumedFile(kv.Key, kv.Value, loader, reconciledPaths, gapFill, ctx); } } } + /// + /// Bring ONE file's list to its post-rescore state on a RESUME, from that file's OWN + /// .scores-reconciled.parquet. The per-file half both resume arms call - the + /// whole-run loop above and the per-run source Stage 7 folds through - so the two + /// cannot disagree about what a run's post-rescore state is. + /// + /// ONE parquet, not two, which is the Boundary 3 -> 4 contract: the reconciled + /// parquet holds the survivor subset with Stage 6's boundaries already applied and the + /// gap-fill rows already merged in canonical position, so reading it makes both the + /// Stage 4 read and the overlay that put those values back unnecessary. It also lands + /// the rows in canonical order ( sorts, and its + /// callers must not re-order), where the overlay had to sort afterwards because it + /// APPENDED the gap-fill rows. + /// + /// The overlay survives for one case, and it is not a fallback for a missing + /// artifact: a list that still HOLDS its entries has its rows already, and re-reading + /// them would be the duplicate build OSPREY_STAGE6_STREAM_SURVIVORS=0 exists to + /// avoid - so those rows take the reconciled values through the overlay instead. Both + /// routes read the same one parquet. + /// + private static void MaterializeResumedFile(string fileName, List entries, + FirstPassSurvivorLoader loader, + IReadOnlyDictionary reconciledPaths, + Lazy>> gapFill, PipelineContext ctx) + { + string reconciledPath = ReconciledPathOrFail(fileName, reconciledPaths, ctx); + if (loader != null) + { + // CLEARED here rather than relying on the caller having dropped the run - the + // same establish-your-own-precondition BuildRunPerRunSource states, and for the + // same reason. StreamFiles does drop it, but RescoredEntries.MaterializeFile + // invokes the source WITHOUT clearing and leaves dropping to its caller, whose + // own doc names "merely finished one of several passes over it" as legitimate. + // On a non-empty list the overlay below would APPEND this run's gap-fill rows a + // second time - duplicating precursors in the pool Stage 7 writes the .blib + // from, silently, exit 0. That is the duplication BuildRunPerRunSource's comment + // records once exiting a straight-through Stellar run on AssertSidecarDescribesPool. + // + // Guarded on the loader, which is what makes the clear safe: a run cleared here + // is rebuilt from disk on the next line. Where there is NO loader the entries + // are the only copy - the OSPREY_STAGE6_STREAM_SURVIVORS=0 resident oracle - + // and they take the reconciled values through the overlay instead. + entries.Clear(); + MaterializeFileSurvivors(fileName, entries, loader, ctx, reconciledPath); + return; + } + OverlayReconciledIntoFile(fileName, entries, reconciledPath, gapFill.Value, + canonicalize: true); + } + + /// + /// This run's CURRENT .scores-reconciled.parquet, or a hard failure naming the + /// run. Absence is a fault, never a case to work around: Stage 6 writes this artifact + /// for every run - covers the run it did no work + /// on - so a run missing from the current set means the write never landed or what + /// landed is not what this run would produce. That is principle P13 read from the + /// consumer's end: the producer writes unconditionally so that absence stays + /// unambiguous, and the consumer is then entitled to treat it as failure. + /// + /// The Stage 4 parquet is NOT a substitute, which is what this method exists to + /// stop being re-derived. It carries pre-reconciliation boundaries and none of the + /// gap-fill rows, so substituting it writes 1st-pass boundaries for that run into the + /// blib from a process that exits 0 - and on the resident arm, where the entries are + /// still in memory, it hides that the run was never persisted at all. + /// + private static string ReconciledPathOrFail(string fileName, + IReadOnlyDictionary reconciledPaths, PipelineContext ctx) + { + string reconciledPath = null; + if (reconciledPaths != null && reconciledPaths.TryGetValue(fileName, out reconciledPath)) + return reconciledPath; + // Logged as well as thrown, for the reason MaterializeAllResumedFiles gives: the + // top-level handler prints the message and a stack trace, where an operator needs + // the run named in the log beside the phase that failed. + string error = string.Format( + @"Second-pass join: run '{0}' has no current .scores-reconciled.parquet. Stage 6 " + + @"writes one for every run, so this run was not persisted - the write no-opped, " + + @"failed, or produced a file this run rejects as stale. Its .scores.parquet is " + + @"not a substitute: it holds 1st-pass boundaries and none of the gap-fill rows. " + + @"Re-run Stage 6 for it.", + fileName); + ctx.LogError(error); + ctx.ExitCode = 1; + throw new InvalidDataException(error); + } + /// /// Refill ONE file's survivor list, or leave it alone when it already holds entries - /// so a second call is a no-op. The per-file half of - /// , separate because the resume overlay loop - /// needs it per file with a reconciled-parquet override. + /// so a second call is a no-op. Every caller now supplies the reconciled-parquet + /// override except the refill-only plan, which deliberately does not overlay at all + /// (see ). /// private static void MaterializeFileSurvivors(string fileName, List entries, FirstPassSurvivorLoader loader, PipelineContext ctx, @@ -2850,100 +3168,57 @@ private static void ResetRescoredTargetsForFile(RescoredPoolPlan plan, string fi } /// - /// Bring EVERY file's list to its post-rescore state by overlaying that file's - /// .scores-reconciled.parquet, canonicalizing the order, and releasing the - /// re-fattened payload. This is the state a fresh - /// leaves behind, rebuilt from disk. + /// Bring ONE file's list to its post-rescore state by overlaying that file's + /// .scores-reconciled.parquet onto rows it ALREADY holds, canonicalizing the + /// order, and releasing the re-fattened payload. /// - /// Two callers, one body. The resume uses it because - /// the driver skipped when the reconciled parquets were already - /// valid. The streamed rescore uses it because it deliberately dropped each file's - /// entries after writing that file's parquet, so the - /// milestone SecondPassFDR reads has to be rebuilt at - /// the end (issue #4526). Sharing the body is what makes the streamed buffer - /// identical to the resumed one. + /// The route for a list that still has its entries. Where the list is empty the + /// callers load from that same reconciled parquet instead, which is one read rather + /// than a Stage 4 read plus this overlay - see + /// and . Either way exactly one parquet is read + /// per run, and it is the reconciled one. /// - /// The two callers differ in WHEN the reconciled parquets were judged current, - /// which is why that judgement is a parameter: the resume path asks now, the deferred - /// streamed build asks during and carries the answer (see - /// ). + /// The path is a parameter, already resolved and already judged CURRENT by + /// , because the two callers differ in WHEN that + /// judgement was made: the resume path asks now, the deferred streamed build asks + /// during and carries the answer (see + /// ). Validity, not mere existence - testing + /// File.Exists would accept a reconciled parquet this run REJECTED and + /// re-scored, overlaying another run's boundaries into this one's blib. /// - /// The shared per-file buffer to bring to its - /// post-rescore state, updated in place. - /// file name -> the reconciled parquet to overlay. - /// Files absent from the map keep their 1st-pass boundaries, matching a fresh run's - /// no-work files. + /// The run whose list is being brought forward. + /// That run's list, updated in place. + /// The run's current reconciled parquet. /// The planner's per-file gap-fill targets. - /// Re-sort each file by the canonical + /// Re-sort the file by the canonical /// (EntryId, Charge, ScanNumber, ParquetIndex) key. TRUE on resume, where the /// buffer has to be brought to the order a cold run ends in. FALSE on the streamed /// rebuild, which is REPRODUCING a cold run: a cold rescore appends gap-fill at the /// END of the list and never re-sorts, so sorting here would move those rows into /// EntryId order and change the buffer order Stage 7 writes its 2nd-pass sidecars /// in - which changes the protein-compact competition and the reported set. - private static void OverlayReconciledIntoFiles( - List>> perFileEntries, - IReadOnlyDictionary reconciledPaths, - IReadOnlyDictionary> gapFill, - bool canonicalize = true) - { - // Reported for the same reason as the survivor rebuild above: a per-file parquet - // read across the whole run, in the silent window after the parallel rescore. - using (var progress = new ProgressReporter(string.Format( - @"Overlaying reconciled results from {0} file(s)", perFileEntries.Count), - perFileEntries.Count)) - { - int done = 0; - foreach (var kv in perFileEntries) - { - progress.Report(++done); - OverlayReconciledIntoFile(kv.Key, kv.Value, reconciledPaths, gapFill, - canonicalize); - } - } - } - - /// - /// The per-file half of - overlay one file's - /// reconciled parquet, optionally canonicalize, and release the re-fattened payload. - /// Separate because the resume overlay loop also applies it per file, and the - /// whole-run loop is only one of its callers. - /// private static void OverlayReconciledIntoFile(string fileName, List entries, - IReadOnlyDictionary reconciledPaths, + string reconciledPath, IReadOnlyDictionary> gapFill, bool canonicalize) { - // Overlay this file's reconciled boundaries when the caller judged its - // .scores-reconciled.parquet present AND CURRENT; no-work files (none on - // disk) keep their 1st-pass boundaries, matching a fresh run. - // - // Validity, not mere existence. The rescore's own per-file gate - // (TryResumeRescoredFile) asks PerFileResumeDriver.IsCurrent, so testing - // File.Exists accepted a reconciled parquet this run would have REJECTED - // and re-scored - one left by a run with different reconciliation - // parameters, say. That overlays stale boundaries onto a cold run's buffer, - // which is worse than the no-work fallback of leaving 1st-pass values. - if (reconciledPaths != null && - reconciledPaths.TryGetValue(fileName, out string reconciledPath)) - { - IReadOnlyList gapFillForFile = null; - if (gapFill != null && gapFill.TryGetValue(fileName, out var gfList)) - gapFillForFile = gfList; - OverlayReconciledIntoBuffer(entries, reconciledPath, gapFillForFile); - } - // Canonical sort for EVERY file (incl. no-work files) so the WARM - // buffer order matches the order COLD establishes in - // RunPercolatorFdr, independent of whether the file was rescored. + IReadOnlyList gapFillForFile = null; + if (gapFill != null && gapFill.TryGetValue(fileName, out var gfList)) + gapFillForFile = gfList; + OverlayReconciledIntoBuffer(entries, reconciledPath, gapFillForFile); + // Canonical sort so the WARM buffer order matches the order COLD establishes + // in RunPercolatorFdr. Needed on THIS route because the overlay APPENDS the + // gap-fill rows; the load-from-reconciled route gets canonical order from the + // loader itself and does not re-sort. if (canonicalize) SortFileEntriesCanonical(fileName, entries); // Same release the rescore path does once a file's reconciled parquet is // on disk: the overlay above re-fattened this file's entries straight // from that parquet, and holding those arrays for every file is the - // O(files) Stage-6 growth term. A no-work file was never fattened, so - // this is a no-op there. Nothing downstream reads them off the buffer - - // SecondPassFDR's 2nd pass reloads PIN features from the reconciled parquet - // by identity - and this leaves the same buffer shape COLD leaves. + // O(files) Stage-6 growth term. Nothing downstream reads them off the + // buffer - SecondPassFDR's 2nd pass reloads PIN features from the + // reconciled parquet by identity - and this leaves the same buffer shape + // COLD leaves. ReleaseRescoredPayload(entries); } @@ -2952,12 +3227,14 @@ private static void OverlayReconciledIntoFile(string fileName, List en /// the exact order a COLD run establishes via /// FirstPassFdrTask.RunPercolatorFdr (run by SecondPassFDR's 2nd-pass, /// which a WARM straight-through resume skips when the .2nd-pass sidecars - /// are already valid on disk). Both resume paths apply this to EVERY file's - /// list, including no-work files with no reconciled parquet, so the WARM buffer - /// order matches COLD regardless of whether the file was rescored -- otherwise - /// SecondPassFDR's BuildSharedBoundaries could iterate a different order and, - /// on a q-value tie between charge states of a peptide, pick a different shared - /// (modseq, file) boundary. A no-work file already lands in this order today via + /// are already valid on disk). Applied to EVERY file the resume OVERLAYS, so the + /// WARM buffer order matches COLD regardless of whether the file was rescored -- + /// otherwise SecondPassFDR's BuildSharedBoundaries could iterate a different + /// order and, on a q-value tie between charge states of a peptide, pick a different + /// shared (modseq, file) boundary. A file the resume LOADS from its reconciled + /// parquet instead arrives in this order already, from + /// 's own canonical sort. A file with no + /// reconciliation work already lands in this order today via /// the single-key compaction sort (compacted EntryIds are unique per file), but /// sorting unconditionally future-proofs the tie-break against any later change /// that retains multiple rows per EntryId. diff --git a/pwiz_tools/Osprey/Osprey.Tasks/PerFileScoringTask.cs b/pwiz_tools/Osprey/Osprey.Tasks/PerFileScoringTask.cs index 6dd4808bf4..77ca75555f 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/PerFileScoringTask.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/PerFileScoringTask.cs @@ -77,15 +77,14 @@ internal sealed class PerFileScoringTask : OspreyTask public override string Name => @"PerFileScoring"; /// - /// Computes per-file scores from spectra only when no per-file scores - /// were supplied via --input-scores. Under --input-scores it is - /// excluded: a downstream task lazy-rehydrates the supplied scores - /// through ctx.Demand<PerFileScoringTask>(). + /// Computes per-file scores from spectra for every task except the three that + /// start after Stage 4. For those it is excluded and a downstream task + /// lazy-rehydrates each run's scores through + /// ctx.Demand<PerFileScoringTask>(). /// public override bool IsIncluded(PipelineContext ctx) { - bool inputs = ctx.Config.InputScores != null && ctx.Config.InputScores.Count > 0; - return !inputs; + return !ScoringTaskShared.StartsAfterPerFileScoring(ctx.Config); } // Stage 1-4 byproducts this task publishes for downstream consumers to @@ -394,7 +393,7 @@ public override bool Run(PipelineContext ctx) // // NOT reachable today, and the claim that it was is wrong: Program.cs rejects // --task SecondPassFDR combined with --input and requires --input-scores, so - // ExpectReconciledInput implies InputScores.Count > 0 and IsIncluded returns false - + // ExpectReconciledInput means this task is excluded and IsIncluded returns false - // Run is never entered on that config. This is aligned with its two siblings so the // one decision has one predicate, not so that a live defect is closed. bool needsResidentPool = !CanUseLeanProjection(ctx.Config, hasReconSidecars: false, @@ -496,12 +495,13 @@ public override bool Rehydrate(PipelineContext ctx) // (CanRehydrate), and a downstream task is the first to touch its // state. Load those valid parquets straight from disk (never // compute) so Rehydrate stays pure -- Run is outer-loop-only. The - // worker-mode join-only disk-load below applies only when - // --input-scores actually supplied the per-file scores. - if (ctx.Config.InputScores == null || ctx.Config.InputScores.Count == 0) + // worker-mode join-only disk-load below applies only to the tasks that + // start after Stage 4. That used to be asked as "were parquets supplied"; + // one seam now answers it, and it is the task. + if (!ScoringTaskShared.StartsAfterPerFileScoring(ctx.Config)) return RehydrateFromOwnOutputs(ctx); - // Disk-load path for worker-mode entry (--input-scores): the + // Disk-load path for a node that starts after Stage 4: the // per-file Stage 2-4 scores already exist on disk, so load the // FdrEntry stubs + PIN features straight from the parquets // (Stage 1 library still loads -- Stage 5+ needs it) instead of @@ -524,12 +524,7 @@ public override bool Rehydrate(PipelineContext ctx) // knows each input parquet path and fills this in. var perFileParquetPaths = new Dictionary(); - int nFiles = config.InputScores.Count; - - // InputFiles is synthesized from the --input-scores parquet stems - // once at pipeline entry (AnalysisPipeline.Run), so downstream code - // (Stage 6 rescore's fileNameToIdx in particular) already has the - // synthetic input paths by the time this load runs. + int nFiles = config.InputFiles.Count; // Mirror Run's EffectiveFileParallelism bookkeeping via the shared // resolver (unused by the disk-load path, which never calls @@ -688,7 +683,7 @@ private bool RehydrateFromOwnOutputs(PipelineContext ctx) // This site gated on bare NeedsResidentPool, which no longer excludes // ExpectReconciledInput, so an ExpectReconciledInput config arriving here would // take the lean branch and add an EMPTY entry list per file. Not reachable today - // (Run routes InputScores runs away from Rehydrate), but re-deriving the rule at + // (Run routes a post-Stage-4 task away from Rehydrate), but re-deriving the rule at // one call site and importing it at the other is precisely the drift this change // exists to remove. hasReconSidecars is false here: this path has no bundle. // ARM THE GUARD ON THE SAME DECISION, for the reason the branch below states: the @@ -1303,8 +1298,13 @@ private FdrProjectionSet LoadJoinOnlyScores( PipelineContext ctx) { hydrationFailed = false; + // Each run's parquet, derived from its input stem: the reconciled sibling where + // Stage 6 wrote one, else the Stage 4 file. This list used to arrive ready-made + // on --input-scores, and the pipeline's first act was to convert it BACK into + // input stems so the sidecar helpers could work. + var scoresPaths = ScoringTaskShared.ScoresPathsForInputs(config); // --task FirstPassFDR: load per-file FdrEntry stubs directly from - // each .scores.parquet listed via --input-scores. Skips the + // each run's .scores.parquet, derived from its input stem. Skips the // per-file Stage 2-4 scoring (Stage 1 library load already ran // in Run). Also loads a best-effort calibration JSON sibling // per file (the loop below) for Stage 6 reconciliation, like @@ -1313,13 +1313,12 @@ private FdrProjectionSet LoadJoinOnlyScores( // Aborts with a clear, file-named error if the operator points // SecondPassFDR at parquets from a different scoring run. string validationError = ParquetScoreCache.ValidateScoresParquetGroup( - config.InputScores, config, OspreyVersion.Current); + scoresPaths, config, OspreyVersion.Current); if (validationError != null) throw new InvalidDataException(validationError); ctx.LogInfo(string.Format( - @"--input-scores: loading {0} per-file score parquet(s)", - config.InputScores.Count)); + @"Loading {0} per-file score parquet(s)", scoresPaths.Count)); // Lean on the HPC merge/join too (#4400): a large FirstPassFDR node // loading every worker's .scores.parquet used to rebuild the full fat // FdrEntry stubs + PIN features (~53 GB at 82 files) -- the same Stage-5 @@ -1346,9 +1345,9 @@ private FdrProjectionSet LoadJoinOnlyScores( // go lean" additionally excludes the reconciled-input merge. Do not "fix" this by // copying the builder decision: the merge does not read Features off these stubs - // both pass-2 shapes reload them per file from the reconciled parquet - // (ComputePass2TransferCompeteFull's own read, or ComputePass2Resident's), and - // EffectiveScoresPathFromScoresPath falls back to the original parquet when no - // reconciled one exists, so the reload does not depend on hasReconSidecars either. + // (ComputePass2TransferCompeteFull's own read, or ComputePass2Resident's), which + // Stage 6 writes for every run, so the reload does not depend on hasReconSidecars + // either. bool loadFeatures = needsResidentPool; // The --input-files paths at :381 and :644 THROW on the same O(files) situation. @@ -1435,7 +1434,7 @@ private FdrProjectionSet LoadJoinOnlyScores( // non-zero exit code, not an unhandled stack trace. _rescoreInputs = HydrateRescoreBundleOrNull( () => RescoreHydration.HydrateCompactedStreaming( - perFileEntries, config.InputScores, + perFileEntries, scoresPaths, (fileIdx, fileName, parquetPath) => LoadJoinOnlyScoresForFile( config, fileIdx, fileName, parquetPath, perFileParquetPaths, perFileCalibrations, perFileIsolationMz, _sequencePool.Value, ctx), @@ -1462,17 +1461,18 @@ private FdrProjectionSet LoadJoinOnlyScores( return null; } - for (int fileIdx = 0; fileIdx < config.InputScores.Count; fileIdx++) + for (int fileIdx = 0; fileIdx < scoresPaths.Count; fileIdx++) { - string parquetPath = config.InputScores[fileIdx]; - // Derive the bare input stem via the single shared suffix-strip - // helper so a .scores-reconciled.parquet input maps to the same - // fileName key as its .scores.parquet sibling (a naive trailing - // ".scores" strip would leave the bogus key ".reconciled"). + string parquetPath = scoresPaths[fileIdx]; + // The input's own stem, not one recovered from the parquet name. The + // recovery existed because the parquet was all this path was given; it had + // to strip ".scores" or ".scores-reconciled" to get back to a key that + // matches the rest of the pipeline, and a naive trailing strip left the + // bogus key ".reconciled". string fileName = Path.GetFileNameWithoutExtension( - RescoreHydration.SyntheticInputFromParquet(parquetPath)) ?? string.Empty; + config.InputFiles[fileIdx]) ?? string.Empty; ctx.LogInfo(string.Format(@"Loading file {0}/{1}: {2} (from {3})", - fileIdx + 1, config.InputScores.Count, fileName, parquetPath)); + fileIdx + 1, scoresPaths.Count, fileName, parquetPath)); if (builder != null) { // Lean: stream 32 B projection rows straight from the parquet; no @@ -1701,7 +1701,7 @@ private static List LoadJoinOnlyScoresForFile( // of its own child. The counter here is this file within the bundle; the percentage // above it is the bundle's own. ctx.LogInfo(string.Format(@" Loading file {0}/{1}: {2} (from {3})", - fileIdx + 1, config.InputScores.Count, fileName, parquetPath)); + fileIdx + 1, config.InputFiles.Count, fileName, parquetPath)); var stubs = ParquetScoreCache.LoadFdrStubsFromParquet(parquetPath, null, sequencePool); // Keep the fail-fast the feature load used to provide: a foreign or truncated // parquet missing the PIN schema must stop here, not surface downstream. @@ -1752,13 +1752,15 @@ private static void LoadJoinOnlyPerRunNames( string consumer, PipelineContext ctx) { + var scoresPaths = ScoringTaskShared.ScoresPathsForInputs(config); ctx.LogInfo(string.Format( - @"--input-scores: {0} run(s) will be hydrated one at a time by {1}; " + - @"no all-runs pre-load.", config.InputScores.Count, consumer)); - foreach (string parquetPath in config.InputScores) + @"{0} run(s) will be hydrated one at a time by {1}; " + + @"no all-runs pre-load.", scoresPaths.Count, consumer)); + for (int i = 0; i < scoresPaths.Count; i++) { + string parquetPath = scoresPaths[i]; string fileName = Path.GetFileNameWithoutExtension( - RescoreHydration.SyntheticInputFromParquet(parquetPath)) ?? string.Empty; + config.InputFiles[i]) ?? string.Empty; perFileEntries.Add(new KeyValuePair>(fileName, new List())); perFileParquetPaths[fileName] = parquetPath; LoadJoinOnlyCalibration(fileName, parquetPath, perFileCalibrations, @@ -1767,20 +1769,16 @@ private static void LoadJoinOnlyPerRunNames( } /// - /// The --input-scores per-file names in input order, each derived from its - /// parquet stem through the same shared suffix-strip helper - /// 's resident loop and - /// use, so index i here names - /// the file the streaming hydrate reports at index i. + /// The per-file names in input order, so index i here names the file the streaming + /// hydrate reports at index i. Straight off the input stems: the shared suffix-strip + /// helper this used to route through existed only to recover a stem from a parquet + /// name, which is the round trip --input-scores forced. /// private static string[] JoinOnlyFileNames(OspreyConfig config) { - var fileNames = new string[config.InputScores.Count]; + var fileNames = new string[config.InputFiles.Count]; for (int i = 0; i < fileNames.Length; i++) - { - fileNames[i] = Path.GetFileNameWithoutExtension( - RescoreHydration.SyntheticInputFromParquet(config.InputScores[i])) ?? string.Empty; - } + fileNames[i] = Path.GetFileNameWithoutExtension(config.InputFiles[i]) ?? string.Empty; return fileNames; } @@ -1900,6 +1898,10 @@ private bool HydrateRescoreBundleIfPresent( } if (hasReconSidecars) { + // Each run's parquet, derived from its input stem - the same derivation the + // loader above makes, from the same source, so the two cannot name different + // files for one run. + var scoresPaths = ScoringTaskShared.ScoresPathsForInputs(config); // Already hydrated when the loader took the file-count-bounded streaming // path (ShouldStreamCompaction): it has to own the hydrate, because the // sidecar overlay and the compaction have to happen inside its per-file @@ -1911,7 +1913,7 @@ private bool HydrateRescoreBundleIfPresent( { _rescoreInputs = HydrateRescoreBundleOrNull( () => RescoreHydration.HydrateReconciliationOverlay( - perFileEntries, config.InputScores, + perFileEntries, scoresPaths, FdrExperimentSidecar.ReadMap( FdrExperimentSidecar.PathFor(config.OutputBlib, ScoringTaskShared.ArtifactSiblingPath(config), FdrScoresSidecar.Pass.FirstPass), @@ -1997,9 +1999,8 @@ private static RescoreInputs HydrateRescoreBundleOrNull( /// private static bool AllHaveReconSidecars(OspreyConfig config) { - foreach (var parquetPath in config.InputScores) + foreach (var syntheticInput in config.InputFiles) { - string syntheticInput = RescoreHydration.SyntheticInputFromParquet(parquetPath); // Version-fenced like every other sidecar gate: a v3 file left by an older build // is present but unreadable, and answering "yes, all sidecars are here" off // File.Exists keeps the fat pool on a path whose overlay then cannot load it. diff --git a/pwiz_tools/Osprey/Osprey.Tasks/PipelineContext.cs b/pwiz_tools/Osprey/Osprey.Tasks/PipelineContext.cs index fd9f634f35..9870da762a 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/PipelineContext.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/PipelineContext.cs @@ -124,10 +124,14 @@ public sealed class PipelineContext /// must /// remain stable for the life of the run, so a worker can /// reproduce the same hash a straight-through invocation would - /// stamp into its parquet footers. Pipeline-populated fields - /// that do NOT feed those hashes (e.g. the worker-mode - /// synthesis of InputFiles from InputScores) may be - /// written once at pipeline entry. Run-time state that is not parsed + /// stamp into its parquet footers. NOTHING is written to the config at + /// pipeline entry any more: it is complete when parsing ends + /// (OspreyCommandArgs.ToConfig, which is also where + /// --input-list is expanded into InputFiles). The carve-out + /// that stood here - pipeline-populated fields that do not feed the hashes + /// may be written once at entry - existed for the worker-mode synthesis of + /// InputFiles from --input-scores parquet stems, and it went + /// with that flag. Run-time state that is not parsed /// config (e.g. file parallelism) lives on /// instead. For per-file scratch that /// mutates hash-affecting fields (e.g. the MS2-calibrated diff --git a/pwiz_tools/Osprey/Osprey.Tasks/RescoreHydration.cs b/pwiz_tools/Osprey/Osprey.Tasks/RescoreHydration.cs index adaa5e8687..68855a062e 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/RescoreHydration.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/RescoreHydration.cs @@ -1208,6 +1208,20 @@ private static bool StemsEqual(IList a, IList b) /// reconciliation JSON) without duplicating them. The synthetic /// path is never opened — only its components are inspected. /// Mirrors Rust's synthetic_input_from_parquet. + /// + /// Its REASON is gone. It existed because --input-scores named parquets + /// on the command line, so the pipeline's first act was to convert them back into + /// data-file names for the sidecar helpers - a round trip, and the clearest evidence + /// that the flag was a second way of saying what --task already said. That + /// flag has retired; every task is given the data-file names directly. + /// + /// What is left is internal: the hydrate methods below still take a PARQUET + /// path per run (from PerFileParquetPaths, which is how the pipeline carries + /// them), and derive the stem back from it. Inverting those signatures to take the + /// input and derive the parquet is the remaining half of the retirement - a + /// no-behaviour-change refactor, deliberately not folded into the CLI change so the + /// gate can attribute a failure to one of them. See + /// TODO-20260908_osprey_input_scores_retirement.md. /// public static string SyntheticInputFromParquet(string parquetPath) { diff --git a/pwiz_tools/Osprey/Osprey.Tasks/ScoringTaskShared.cs b/pwiz_tools/Osprey/Osprey.Tasks/ScoringTaskShared.cs index a0104ea85b..98caff453d 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/ScoringTaskShared.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/ScoringTaskShared.cs @@ -253,11 +253,14 @@ internal static SpectraWindowIndex EnsureSpectraCache(string inputFile, bool ser /// /// Resolve a path whose stem matches , used /// only as the base for sidecar file naming (the path itself need - /// not exist). In normal mode this is the input mzML; in - /// --task FirstPassFDR mode where InputFiles is empty we synthesize the - /// path from the matching .scores.parquet by replacing the - /// `.scores.parquet` suffix with `.mzML`. Mirrors the Rust - /// `synthetic_input_from_parquet` helper. + /// not exist). This is the input data file, on every route. + /// + /// The parquet-derived fallback below is UNREACHABLE now and is kept + /// only because removing it is a behaviour change that belongs in its own + /// commit. It existed for --task FirstPassFDR, which took + /// --input-scores and so arrived with InputFiles empty; every + /// task now requires --input, and the fileName keys are + /// derived from those same inputs, so the loop always matches. /// /// Lives here rather than on because /// needs the same resolution to find a @@ -284,9 +287,16 @@ internal static string ResolveSidecarBasePath( } } } - // --task FirstPassFDR fallback: derive a synthetic mzML path from the - // matching parquet stem so all the existing sidecar path - // helpers keep working without conditional branches. + // KEPT, and the "unreachable since --input-scores retired" note that stood here + // is not safe to act on. The argument for unreachability is that every task + // requires --input and the fileName keys are derived from those same inputs, so + // the loop above always matches. But PerFileRescoreTask documents a SUPPORTED + // state in which a run's file_name has no input_files stem (WriteUnchangedReconciled + // returns silently for it), and that is exactly the case this branch answers - + // deleting it would turn a synthesized sidecar path into null for the one shape + // that needs it. Establish which of the two is true before removing this; it is a + // behaviour change either way, and it belongs with the finding that owns that + // state rather than with the flag retirement that made it look dead. if (perFileParquetPaths != null && perFileParquetPaths.TryGetValue(fileName, out string parquetPath)) { @@ -331,19 +341,18 @@ internal static void TallyPreCompaction( /// to, for naming the analysis-wide experiment-scope FDR sidecar /// (). /// - /// Prefers InputScores over InputFiles because a distributed - /// --task node is given its inputs as scores parquets and may have no mzML list - /// at all; both resolve to the same directory, since the parquets are written beside the - /// inputs. What matters is only that every phase of one analysis picks a path that - /// resolves the SAME way - the blib's own directory does not, which is the bug this - /// exists to avoid. + /// The FIRST input, on every route. It used to prefer InputScores, + /// because a distributed --task node was given its inputs as scores parquets + /// and might have had no data-file list at all; every node is given the same list + /// now, and both forms resolved to the same directory anyway since the parquets are + /// written beside the inputs. What matters is only that every phase of one analysis + /// picks a path that resolves the SAME way - the blib's own directory does not, which + /// is the bug this exists to avoid. /// internal static string ArtifactSiblingPath(OspreyConfig config) { if (config == null) return null; - if (config.InputScores != null && config.InputScores.Count > 0) - return config.InputScores[0]; if (config.InputFiles != null && config.InputFiles.Count > 0) return config.InputFiles[0]; return null; @@ -435,6 +444,124 @@ internal static bool CanHydratePerRun(OspreyConfig config) return !string.IsNullOrEmpty(path) && RetainedBaseIdSidecar.IsCurrentFormat(path); } + /// + /// Every task that starts AFTER Stage 4 - the two joins and the rescore worker. They + /// are handed a directory of per-run artifacts rather than spectra, so + /// does not run for them; a consumer materializes + /// its state through ctx.Demand, which routes to its disk load. + /// + /// This used to be asked as "were parquets supplied on the command line", which + /// is the INPUT KIND - the Rust pipeline's way of saying Stage 1-4 was done. The port + /// says it with --task, and the two seams disagreeing is what let + /// --task ModelDiagnostics join the pipeline and demand state a diagnostics + /// fold never publishes. One question, asked of the task. + /// + /// ModelDiagnostics is deliberately NOT here. It is neither a fan-out nor + /// a join but a render over retained products, and it needs the per-file load to have + /// happened - which it did by taking -i even while the others took parquets. + /// That asymmetry was the first symptom of the two seams, and it survives the + /// retirement as an ordinary membership fact rather than as an input-kind accident. + /// + internal static bool StartsAfterPerFileScoring(OspreyConfig config) + { + switch (config.SelectedTask) + { + case HpcTask.FirstPassFdr: + case HpcTask.PerFileRescore: + case HpcTask.SecondPassFdr: + return true; + default: + return false; + } + } + + /// + /// Which per-run parquet THIS task reads its rows from: the Stage 6 + /// .scores-reconciled.parquet for SecondPassFDR, the Stage 4 + /// .scores.parquet for the two tasks that run before Stage 6 has written one. + /// + /// A property of the TASK, not of what happens to be on disk. It used to be + /// decided by probing for the reconciled sibling and taking it where it existed, + /// which gives the right answer only because the pipeline happens to run the stages + /// in order - the file is absent before Stage 6 and present after. Re-run + /// --task FirstPassFDR over a directory a previous run completed and the same + /// probe hands the FIRST pass the survivor SUBSET, roughly 1/52 of its rows, with + /// nothing to reject it: the version, search and library hashes all match. It then + /// writes cohort-wide boundary artifacts from that subset and exits 0. + /// + /// The two answers are also what an HPC node is SHIPPED. Boundary 3 -> 4 sends + /// a SecondPassFDR node the reconciled parquets and not the Stage 4 originals - + /// regression.ps1's mode-3 chain deletes them from the worker directory before + /// staging phase 4, so reaching for one fails on a missing file rather than passing + /// quietly. A FirstPassFDR or PerFileRescoring node gets the originals + /// and no reconciled sibling exists yet. So on a correct node each task has exactly + /// one of the two present, and asking the disk cannot distinguish "the artifact for + /// my pass" from "the only artifact here". + /// + internal static bool ReadsReconciledScores(OspreyConfig config) + { + return config.SelectedTask == HpcTask.SecondPassFdr; + } + + /// + /// True when THIS process runs Stage 7's join, i.e. when a per-run source published for + /// that join will actually be folded by something. + /// + /// Names what is ADMITTED, so it fails closed: the straight-through pipeline (no + /// --task, which runs every stage), the SecondPassFDR node, and + /// ModelDiagnostics - which is not an HPC fan-out node but does let + /// SecondPassFDR compute the pass-2 view, so it folds the same join and must not + /// be pushed back onto the resident pool. A task added later is excluded until someone + /// decides otherwise, which is the direction a predicate guarding a memory shape - and, + /// since , a correctness one - should + /// fail in. + /// + /// The excluded tasks each have a consumer that never arrives. + /// PerFileScoring and SpectraCache stop before Stage 5. + /// FirstPassFDR would publish empty per-run lists for a fold that never runs. + /// A PerFileRescoring worker exits after Stage 6, so a source built there is + /// never pulled - it only pays for a retained-sidecar read the "entering + /// PerFileRescoring must cost the same for 1 run as for 446" contract forbids, and + /// emits a streamed-join marker into a log for a join that did not happen. + /// + internal static bool RunsStage7Join(OspreyConfig config) + { + if (!config.SelectedTask.HasValue) + return true; + return config.SelectedTask == HpcTask.SecondPassFdr || + config.SelectedTask == HpcTask.ModelDiagnostics; + } + + /// + /// Each input's scores parquet for THIS task, in input order - see + /// for which one that is. Reached only by the + /// three tasks names, so every case has an + /// answer. + /// + /// The derivation --input-scores used to be handed ready-made. Its + /// directory form globbed a directory and preferred the reconciled sibling per stem; + /// this is the same list built from the runs the command line names instead of from + /// whatever a directory happened to hold. The difference matters three times: a + /// directory with a stray parquet no longer changes the cohort; ORDER is now the + /// caller's (FirstPassFDR reconciliation is order-sensitive, so a chain must pass a + /// deterministically sorted list - which is what it already did to get a stable + /// directory sort); and the per-stem preference is no longer part of it. + /// + internal static List ScoresPathsForInputs(OspreyConfig config) + { + var paths = new List(config.InputFiles?.Count ?? 0); + if (config.InputFiles == null) + return paths; + bool reconciled = ReadsReconciledScores(config); + foreach (string input in config.InputFiles) + { + paths.Add(reconciled + ? ParquetScoreCache.GetReconciledScoresPath(input) + : ParquetScoreCache.GetScoresPath(input)); + } + return paths; + } + /// /// True when the --task SecondPassFDR merge may hand Stage 7 a per-run source /// instead of every run's survivors at once. @@ -447,9 +574,18 @@ internal static bool CanHydratePerRun(OspreyConfig config) /// that leg. Widening the first would have told the rescore it may stream on a leg where /// it does not run at all. /// - /// Three requirements, and the third is the one that is easy to miss. The leg has - /// to be the reconciled-input merge, whose parquets already hold the survivor subset. - /// No consumer may read PIN features off these stubs + /// Three requirements, and the first is the one that is easy to miss. Every run's + /// .scores-reconciled.parquet has to be on disk in the survivor-subset shape, + /// because that parquet IS what a run is rebuilt from and dropped again. It used to be + /// asked as config.ExpectReconciledInput, a PROXY for it: only + /// --task SecondPassFDR sets that flag, so the one route the #4486 repro used + /// became the only route that could stream - while a straight-through run, whose + /// Stage 6 had just written those same parquets, met the requirement and was refused + /// anyway (91.1 GB private, measured on a 446-run resume). Asked of the disk it is + /// route-independent, which is also what lets --input-scores retire without + /// taking the streamed join with it. + /// + /// No consumer may read PIN features off these stubs /// (PerFileScoringTask.NeedsResidentPool: --fdrbench-pass 1, a /// non-Percolator FDR method, OSPREY_FDR_PROJECTION=0) - a streamed pool drops /// the entries those consumers index. And the analysis-wide retained base_id summary has @@ -471,7 +607,46 @@ internal static bool CanStreamStage7Join(OspreyConfig config) /// internal static bool CanStreamStage7Join(OspreyConfig config, bool stage7Stream) { - if (!config.ExpectReconciledInput || !stage7Stream) + // LAST, because AllReconciledParquetsCurrent is the only term that opens a file per + // run. Every cheaper disqualifier returns first, so a run that was never going to + // stream does not pay 446 footer reads to be told so. + return Stage7StreamAdmittedBeforeRescore(config, stage7Stream) && + AllReconciledParquetsCurrent(config); + } + + /// + /// Every CanStreamStage7Join term EXCEPT the reconciled parquets, i.e. the half a + /// run can answer BEFORE Stage 6 has written them. + /// + /// Split out for exactly one caller: the straight-through Run arm of + /// PerFileRescoreTask, which decides whether to publish a per-run source at the + /// TOP of Stage 6, hours before the rescore it is about to perform writes the parquets + /// the full predicate asks about. Asking the full question there answers "no" on every + /// cold run - not because the run cannot stream, but because it has not got there yet. + /// It does not need the term either: by the time that arm's source is pulled, Stage 6 + /// has written a reconciled parquet for every run, so the question the full predicate + /// asks has an answer - and a run still missing one fails there rather than falling + /// back to its Stage 4 parquet. + /// + /// The retained base_id summary IS in this half even though it is an artifact: + /// FirstPassFDR writes it before any caller of either form runs, so it is answerable on + /// every route at every point either question is asked. + /// + internal static bool Stage7StreamAdmittedBeforeRescore(OspreyConfig config, bool stage7Stream) + { + // FIRST, and it is a correctness term rather than an optimisation. Every other term + // here describes the SHAPE of a Stage 7 join; none of them asks whether this process + // runs one. Without this, `--task FirstPassFDR` re-run over a COMPLETED directory + // satisfies all of them - the reconciled parquets are present because a previous + // pass wrote them - and PerFileScoringTask's `perRunJoin` branch then publishes one + // EMPTY list per run for a fold that never comes. FirstPassFDR computes its pass over + // nothing and rewrites both boundary sidecars and the retained base_id summary as + // empty, exit 0. The `ExpectReconciledInput` term this predicate replaced made that + // unreachable for anything but `--task SecondPassFDR`, so the hole opened when the + // proxy went and nothing took over the question it had been answering incidentally. + if (!RunsStage7Join(config)) + return false; + if (!stage7Stream) return false; if (PerFileScoringTask.NeedsResidentPool(config, OspreyEnvironment.UseFdrProjection)) return false; @@ -505,6 +680,42 @@ internal static bool CanStreamStage7Join(OspreyConfig config, bool stage7Stream) RetainedBaseIdSidecar.IsCurrentFormat(retainedPath); } + /// + /// True when EVERY input has a .scores-reconciled.parquet on disk that this build + /// can read in the survivor-subset shape - the disk-side question + /// config.ExpectReconciledInput used to stand in for. + /// + /// ALL, not any. The fold rebuilds each run from its own reconciled parquet, so + /// one run without a readable one is a run the fold cannot produce - and admitting the + /// stage on "some run has one" would fail at that run, hours in. The sibling question + /// "did Stage 6 rescore anything", which decides whether a second Percolator pass is + /// owed, is SecondPassFdrTask.AnyReconciledParquet and is deliberately not this: + /// a file with no rescore work still gets a faithful copy written for it, which is what + /// makes "all present" reachable on any route. + /// + /// Empty or absent inputs return FALSE rather than vacuously true. There is no + /// pool to bound on a run with no inputs, so the streamed arm buys nothing there, and + /// vacuous truth would hand the fold an empty file set on a configuration nothing else + /// in this predicate examines. + /// + internal static bool AllReconciledParquetsCurrent(OspreyConfig config) + { + if (config.InputFiles == null || config.InputFiles.Count == 0) + return false; + foreach (var input in config.InputFiles) + { + // From the INPUT stem, the same derivation AnyReconciledParquet uses, so this + // reads identically in the in-process pipeline (Stage 6 has just written the + // parquets) and on a --task SecondPassFDR node (a Stage 6 worker wrote them). + if (!ParquetScoreCache.IsCurrentReconciledSurvivorSubset( + ParquetScoreCache.GetReconciledScoresPath(input))) + { + return false; + } + } + return true; + } + /// /// Fail fast when the RESIDENT Stage-7 join was CHOSEN over an admissible streamed one, /// unless the operator named . The Stage-7 @@ -537,6 +748,14 @@ internal static string Stage7ResidentGuardError( { return null; } + // OSPREY_STAGE6_STREAM_SURVIVORS=0 withholds the survivor loader, so the cold arm + // publishes no per-run source however this run answers - which makes the remedy + // below ("unset OSPREY_STAGE7_STREAM") unachievable, and refusing on it would demand + // a token for a choice the operator does not have. The A/B oracle that switch exists + // to provide asks for BOTH stages resident; this is the one combination where + // `streamingAvailable` is true and streaming is nonetheless unreachable. + if (!OspreyEnvironment.Stage6StreamSurvivors) + return null; // The SUPPLIED value is quoted, matching the two sibling guards: a stale or // misspelled token otherwise reads exactly like an unset one, and the operator // cannot tell "you named nothing" from "you named the wrong path". diff --git a/pwiz_tools/Osprey/Osprey.Tasks/SecondPassFdrTask.cs b/pwiz_tools/Osprey/Osprey.Tasks/SecondPassFdrTask.cs index 9e977ffc49..a2b359bd73 100644 --- a/pwiz_tools/Osprey/Osprey.Tasks/SecondPassFdrTask.cs +++ b/pwiz_tools/Osprey/Osprey.Tasks/SecondPassFdrTask.cs @@ -58,14 +58,10 @@ internal sealed class SecondPassFdrTask : OspreyTask public override bool IsIncluded(PipelineContext ctx) { var c = ctx.Config; - bool inputs = c.InputScores != null && c.InputScores.Count > 0; - // StopAfterStage5 on BOTH input routes, for the reason PerFileRescoreTask.IsIncluded - // states: it appeared only in the --input-scores clause because --task FirstPassFDR - // was the only setter and that task rejects -i. --task ModelDiagnostics sets it too - // and takes -i. - return (!inputs && !c.NoJoin && !c.StopAfterStage5) - || (inputs && c.ExpectReconciledInput) - || (inputs && !c.NoJoin && !c.StopAfterStage5 && !c.ExpectReconciledInput); + // Its own node always, and the full pipeline unless something stops earlier. + // StopAfterStage5 means that boundary whatever the inputs look like, for the + // reason PerFileRescoreTask.IsIncluded states. + return c.ExpectReconciledInput || (!c.NoJoin && !c.StopAfterStage5); } // Phase B resume surface. Reads each file's reconciled @@ -76,13 +72,13 @@ public override bool IsIncluded(PipelineContext ctx) public override IEnumerable Inputs(PipelineContext ctx) { if (ctx.Config.InputFiles == null) yield break; - // Stage 7 reads the reconciled parquet when Stage 6 produced one, - // else the original Stage 4 parquet (no-work files). Recorded for - // provenance only -- the driver validates tasks by output sidecar + // Stage 7 reads the reconciled parquet, which Stage 6 writes for every run - + // a run without one fails in UnusableReconciledParquets rather than being read + // from its Stage 4 file, which a SecondPassFDR node is not even shipped. + // Recorded for provenance only -- the driver validates tasks by output sidecar // key, never by re-checking Inputs() existence (TaskValiditySidecar). foreach (var input in ctx.Config.InputFiles) - yield return ParquetScoreCache.EffectiveScoresPathFromScoresPath( - ParquetScoreCache.GetScoresPath(input)); + yield return ParquetScoreCache.GetReconciledScoresPath(input); // Under the frozen modes the per-run 2nd-pass FDR sidecars are this task's INPUTS: // the rescore worker computed and wrote them, and the join folds the per-base_id @@ -285,39 +281,26 @@ public override string ValidityKey(PipelineContext ctx) public override bool Run(PipelineContext ctx) { - bool couldStream = ScoringTaskShared.CanStreamStage7Join(ctx.Config, stage7Stream: true); - string residentError = ScoringTaskShared.Stage7ResidentGuardError( - couldStream, OspreyEnvironment.Stage7Stream, OspreyEnvironment.AllowUnfixedResident); + // Refuse a resident Stage-7 join that was CHOSEN over an admissible streamed one, + // before anything is written or any pool is pulled. The first-pass guard cannot see + // this pool - it stops at the compaction line - so without this the fat path was + // reachable with no token at all, which is the one shape the named-token ratchet is + // supposed to make impossible. (The paragraph sat above the marker wipe below, far + // from the call it describes.) + // + // The switch is tested FIRST because `couldStream` is not free: its + // AllReconciledParquetsCurrent term opens a footer per run, and the guard's own + // first line discards the answer whenever the switch is on - so on the default + // path that was an O(files) sweep of a network artifact directory computed only to + // be thrown away. Nothing else here needs it. + string residentError = OspreyEnvironment.Stage7Stream + ? null + : ScoringTaskShared.Stage7ResidentGuardError( + ScoringTaskShared.CanStreamStage7Join(ctx.Config, stage7Stream: true), + OspreyEnvironment.Stage7Stream, OspreyEnvironment.AllowUnfixedResident); if (residentError != null) throw new InvalidOperationException(residentError); - // The run that CANNOT stream is not refused - there is no alternative to choose, and - // demanding a token would make the ordinary path unusable. But it must not be - // silent either. Until now the only statement of this deficiency lived in - // regression.ps1's end-of-run table, which prints for the developers who already - // know and never for the operator whose run is about to take it: what they get - // today is an OOM at a file count nothing warned them about. - // - // FIRST in Run, ahead of the diagnostics fold arm below. That arm returns early, and - // when the join cannot stream it is ITSELF a resident-pool path - it pulls the same - // survivor buffer, which is where 91.1 GB was measured at 446 files. Placing this - // after it silenced the warning on exactly the run that most needed it. - // - // One line, naming the shape, the issue and the cost model, so the ceiling is - // predictable from the run's own output rather than from a projection in a gate - // nobody outside this repo executes. - if (!couldStream) - { - ctx.LogWarning(string.Format( - @"Stage 7 is taking the RESIDENT join: every run's survivors are rebuilt at " + - @"once and held for the whole stage, which is O(files) (issue #4486). " + - @"Measured cost is ~4.4 GB plus ~0.197 GB per file, so {0} file(s) needs " + - @"~{1:F0} GB. The streamed join is admitted only for --task SecondPassFDR " + - @"today; this run does not qualify, so there is nothing to switch on.", - ctx.Config.InputFiles?.Count ?? 0, - 4.4 + 0.197 * (ctx.Config.InputFiles?.Count ?? 0))); - } - // The pass-2 diagnostics product is the ONLY outstanding output: every // computational artifact this task produces is already on disk and key-current, and // the driver reached Run solely because the pass-2 diagnostics JSON is missing - @@ -343,11 +326,6 @@ public override bool Run(PipelineContext ctx) return FoldPass2DiagnosticsOnly(ctx); } - // Refuse a resident Stage-7 join that was CHOSEN over an admissible streamed one, - // before anything is written or any pool is pulled. The first-pass guard cannot see - // this pool - it stops at the compaction line - so without this the fat path was - // reachable with no token at all, which is the one shape the named-token ratchet is - // supposed to make impossible. // Mid-Run crash safety: see FirstPassFdrTask.Run for rationale. foreach (var output in Outputs(ctx)) TaskValiditySidecar.Delete(output, Name); @@ -365,6 +343,7 @@ public override bool Run(PipelineContext ctx) // the work lands, and a worker that never reaches this line never pays it. // Taken as a TOKEN first, so the probes below can bracket that build. var rescored = ctx.Get(); + WarnResidentStage7Join(rescored, ctx); // Stage 7's INHERITED baseline, post-GC, before this stage does any work // (#4486). Every figure that issue has ever quoted came from --memstamp, i.e. @@ -404,8 +383,19 @@ public override bool Run(PipelineContext ctx) // refuses outright no longer pays for 289 M survivors it is about to discard. // Its own transients are footer metadata, which is why it can sit between the // stage7-inherited and stage7-pool probes without distorting either. - var stale = StaleReconciledParquets(rescored.FileNames, perFileParquetPaths); - if (stale.Count > 0) + var unusable = UnusableReconciledParquets(rescored.FileNames, perFileParquetPaths); + if (unusable.Missing.Count > 0) + { + throw new InvalidOperationException(string.Format( + "{0} of {1} run(s) have no .scores-reconciled.parquet. Stage 6 writes one " + + "for every run, so these were not persisted - the write no-opped, failed, " + + "or the artifacts were not shipped to this node. Their .scores.parquet is " + + "not a substitute: it holds 1st-pass boundaries and none of the gap-fill " + + "rows. Re-run Stage 6 for them. Missing: [{2}].", + unusable.Missing.Count, rescored.FileCount, + string.Join(", ", unusable.Missing))); + } + if (unusable.Stale.Count > 0) { throw new InvalidOperationException(string.Format( "{0} of {1} reconciled parquet(s) predate the survivor-subset format, so " + @@ -414,7 +404,7 @@ public override bool Run(PipelineContext ctx) "unusable, so a parquet-only rewrite would leave the directory " + "inconsistent. Re-run the analysis from Stage 5 over this directory. " + "Stale: [{2}].", - stale.Count, rescored.FileCount, string.Join(", ", stale))); + unusable.Stale.Count, rescored.FileCount, string.Join(", ", unusable.Stale))); } // NO .Value here any more (#4486). Every consumer below folds through @@ -657,6 +647,39 @@ private bool OnlyDiagnosticsProductOutstanding(PipelineContext ctx) return true; } + /// + /// Say so when Stage 7 is about to build the whole-run survivor pool, naming the shape, + /// the issue and the cost model. + /// + /// The run that CANNOT stream is not refused - there is no alternative to choose, + /// and demanding a token would make the ordinary path unusable. But it must not be + /// silent either: until #4642 the only statement of this deficiency lived in + /// regression.ps1's end-of-run table, which prints for the developers who already know + /// and never for the operator whose run is about to take it, and what they got instead + /// was an OOM at a file count nothing had warned them about. + /// + /// Keyed on the MILESTONE, not on CanStreamStage7Join. The predicate says + /// the run is ADMISSIBLE; only the milestone says a per-run source was actually built, + /// and the two stopped being the same statement once the admission was derived from disk + /// rather than named by one CLI flag. Reading the fact off the thing that decides it is + /// what keeps this line honest as each remaining arm is converted. + /// + /// Called from both arms that pull the milestone, each right after its pull. The + /// diagnostics-only fold returns before reaches its own call, so a + /// single site would go silent on exactly the run that most needs it. + /// + private static void WarnResidentStage7Join(RescoredEntries rescored, PipelineContext ctx) + { + if (rescored.Streams) + return; + int nFiles = ctx.Config.InputFiles?.Count ?? 0; + ctx.LogWarning(string.Format( + @"Stage 7 is taking the RESIDENT join: every run's survivors are rebuilt at " + + @"once and held for the whole stage, which is O(files) (issue #4486). " + + @"Measured cost is ~4.4 GB plus ~0.197 GB per file, so {0} file(s) needs " + + @"~{1:F0} GB.", nFiles, 4.4 + 0.197 * nFiles)); + } + /// /// Produce the pass-2 diagnostics product and nothing else, from a second pass that is /// already complete on disk. The pass-2 sibling of @@ -686,6 +709,10 @@ private bool FoldPass2DiagnosticsOnly(PipelineContext ctx) { var config = ctx.Config; var rescored = ctx.Get(); + // This arm is ITSELF a resident-pool path when the source is absent - it pulls the + // same survivor buffer, which is where 91.1 GB was measured at 446 files - and it + // returns before Run's own call, so it has to make the statement itself. + WarnResidentStage7Join(rescored, ctx); var libraryById = ctx.Get().Value; var perFileParquetPaths = ctx.Get().Value; @@ -1068,29 +1095,40 @@ private static bool AnyReconciledParquet(OspreyConfig config) } /// - /// The per-file keys whose .scores-reconciled.parquet is on disk but predates - /// the survivor-subset format, so Stage 7 cannot read it. + /// The per-file keys whose .scores-reconciled.parquet Stage 7 cannot read, + /// split by WHY, because the two have different remedies: Missing means Stage 6 + /// never persisted the run, and Stale means what it wrote predates the + /// survivor-subset format. /// - /// The run refuses rather than converting: this branch changed the FDR - /// sidecars too, so an old directory has no self-consistent artifact set to + /// Absence is reported, not skipped. Stage 6 writes this artifact for every run - + /// WriteUnchangedReconciled covers the run it did no work on - so P13 makes + /// absence unambiguous at the producer, and this is the consumer end of the same + /// principle. Exempting a missing file here let a run whose write never landed pass + /// this gate and be silently rebuilt from its Stage 4 parquet, i.e. at 1st-pass + /// boundaries with no gap-fill rows, in a run that exits 0. + /// + /// The run refuses rather than converting a stale one: this branch changed the + /// FDR sidecars too, so an old directory has no self-consistent artifact set to /// convert toward and has to be re-run from Stage 5 (issue #4486). /// - private static List StaleReconciledParquets( + private static (List Missing, List Stale) UnusableReconciledParquets( IReadOnlyList fileNames, IReadOnlyDictionary perFileParquetPaths) { + var missing = new List(); var stale = new List(); if (perFileParquetPaths == null) - return stale; + return (missing, stale); foreach (var fileName in fileNames) { if (!perFileParquetPaths.TryGetValue(fileName, out string scoresPath)) continue; string reconciledPath = ParquetScoreCache.ReconciledPathFromScoresPath(scoresPath); if (!File.Exists(reconciledPath)) + { + missing.Add(fileName); continue; - var metadata = ParquetScoreCache.LoadFooterMetadata(reconciledPath); - metadata.TryGetValue(@"osprey.reconciled", out string marker); + } // Stale is EITHER an older generation (marker mismatch) OR the interim // #4486 shape - survivor subset with no score_index column - which the // per-file loaders would otherwise read by POSITION, silently binding @@ -1098,14 +1136,15 @@ private static List StaleReconciledParquets( // IsSubsetWithoutScoreIndex documents). Only FirstPassSurvivorLoader // carried that refusal; the pass-2 feature loaders reach the same file // through this gate, so it has to ask the same question. - if (!string.Equals(marker, ParquetScoreCache.RECONCILED_SURVIVORS, - StringComparison.Ordinal) || - ParquetScoreCache.IsSubsetWithoutScoreIndex(reconciledPath)) - { + // + // Through the shared predicate rather than re-testing the footer here: + // ScoringTaskShared.CanStreamStage7Join ADMITS a run to the per-run fold on + // exactly this question, and a second copy of it is the drift that admits a + // run to a fold this refusal then aborts. + if (!ParquetScoreCache.IsCurrentReconciledSurvivorSubset(reconciledPath)) stale.Add(fileName); - } } - return stale; + return (missing, stale); } /// /// Write passing entries to a BiblioSpec blib file. diff --git a/pwiz_tools/Osprey/Osprey.Test/IOTest.cs b/pwiz_tools/Osprey/Osprey.Test/IOTest.cs index 69998184a6..fad55fc1a5 100644 --- a/pwiz_tools/Osprey/Osprey.Test/IOTest.cs +++ b/pwiz_tools/Osprey/Osprey.Test/IOTest.cs @@ -2227,35 +2227,40 @@ public void TestReconciledNamingUnambiguousForReconciledStem() } /// - /// Verifies EffectiveScoresPathFromScoresPath returns the reconciled - /// sibling when it exists on disk, else the original -- the per-file - /// read contract that makes the separate-reconciled-file design - /// byte-equivalent to the former in-place overwrite. + /// Which parquet a task reads is decided by the TASK, and NOT by what is on disk. + /// + /// The predecessor of this test pinned the opposite - a probe that took the + /// reconciled sibling wherever it existed. That gives the right answer only because + /// the pipeline runs the stages in order, and it silently gives the wrong one on a + /// re-run: --task FirstPassFDR over a directory a previous run completed found + /// the reconciled parquets and would have trained the FIRST pass on the survivor + /// SUBSET, with every version, search and library hash matching. So the assertion + /// that matters is the negative one - BOTH files present, and the task still decides. + /// A test that laid down only one file would pass against the probe as well. /// [TestMethod] - public void TestEffectiveScoresPathFromScoresPath() + public void TestScoresPathsDependOnTaskNotDisk() { string dir = Path.Combine(Path.GetTempPath(), "osprey_eff_" + Guid.NewGuid().ToString("N")); Directory.CreateDirectory(dir); try { + string input = Path.Combine(dir, "sample1.mzML"); string original = Path.Combine(dir, "sample1.scores.parquet"); string reconciled = Path.Combine(dir, "sample1.scores-reconciled.parquet"); + // BOTH on disk, which is the state a completed run leaves behind. + File.WriteAllText(input, "x"); File.WriteAllText(original, "x"); - - // No reconciled sibling -> original (no-work file). - Assert.AreEqual(original, - ParquetScoreCache.EffectiveScoresPathFromScoresPath(original)); - - // Reconciled sibling present -> reconciled (rescored file). File.WriteAllText(reconciled, "y"); - Assert.AreEqual(reconciled, - ParquetScoreCache.EffectiveScoresPathFromScoresPath(original)); - // An already-reconciled input that exists is returned as-is. - Assert.AreEqual(reconciled, - ParquetScoreCache.EffectiveScoresPathFromScoresPath(reconciled)); + // The two passes that run BEFORE Stage 6 read the Stage 4 file even though + // the reconciled sibling is sitting beside it. + AssertScoresPathForTask(input, HpcTask.FirstPassFdr, original); + AssertScoresPathForTask(input, HpcTask.PerFileRescore, original); + + // The join reads the reconciled one - the only artifact its node is shipped. + AssertScoresPathForTask(input, HpcTask.SecondPassFdr, reconciled); } finally { @@ -2263,6 +2268,22 @@ public void TestEffectiveScoresPathFromScoresPath() } } + /// + /// One input, one task, one expected parquet - through the same helper the pipeline + /// calls, so the test cannot agree with a rule the tasks do not use. + /// + private static void AssertScoresPathForTask(string input, HpcTask task, string expected) + { + var config = new OspreyConfig + { + SelectedTask = task, + InputFiles = new List { input } + }; + var paths = ScoringTaskShared.ScoresPathsForInputs(config); + Assert.AreEqual(1, paths.Count); + Assert.AreEqual(expected, paths[0]); + } + /// /// Verifies that writing an empty list does not create a file. /// @@ -2807,6 +2828,74 @@ private static void AssertBitEqual(double expected, double actual, string label) BitConverter.DoubleToInt64Bits(actual), label + " bit mismatch"); } + /// + /// The survivor-subset currency predicate + /// (), against real + /// artifacts rather than a hand-built footer. + /// + /// It answers whether a reconciled parquet can be READ in the shape a per-run + /// survivor rebuild needs, and two callers ask it: the Stage 7 refusal that names the + /// stale files, and the admission that decides whether the second-pass join may fold a + /// run at a time. Both of them turn a wrong answer into a whole-cohort outcome - a run + /// admitted to a fold it then aborts, or an O(files) pool nobody asked for - so the + /// three states are pinned here: absent, present-without-the-marker (the Stage 4 + /// original, which is exactly the file a path-derivation slip would hand it), and + /// present-and-current. + /// + [TestMethod] + public void TestIsCurrentReconciledSurvivorSubset() + { + string dir = Path.Combine(Path.GetTempPath(), + "osprey_recon_current_" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(dir); + try + { + string originalPath = Path.Combine(dir, "sample1.scores.parquet"); + string reconciledPath = Path.Combine(dir, "sample1.scores-reconciled.parquet"); + + // Absent: false, and no throw. This is the no-work state on a cold cohort, so it + // has to be an answer rather than an error. + Assert.IsFalse(ParquetScoreCache.IsCurrentReconciledSurvivorSubset(reconciledPath)); + + var original = new List(); + foreach (uint id in new uint[] { 3, 1, 2 }) + original.Add(MakeStreamEntry(id, id * 10.0)); + ParquetScoreCache.WriteScoresParquet(originalPath, original, null, null, "f.mzML"); + + // The Stage 4 original is a well-formed parquet with neither the marker nor the + // score_index column, and it must not pass: it is the file a stem-derivation + // slip substitutes, and its rows are the PRE-reconciliation ones. + Assert.IsFalse(ParquetScoreCache.IsCurrentReconciledSurvivorSubset(originalPath)); + + // Written the way Stage 6 writes it - the marker in the footer, score_index in + // the schema - which is the only combination that passes. + // + // Metadata from the REAL producer, not hand-fed. StreamReconciledScoresParquet + // writes the caller's map verbatim, so a test that supplies the marker itself + // asserts only that the reader can read what the test wrote: let + // ReconciledParquetWriter stop stamping it and this would still pass while + // AllReconciledParquetsCurrent returned false for every real run and the whole + // cohort fell back to the resident join. Going through the producer is what + // makes the two sides able to disagree. + var metadata = ReconciledParquetWriter.BuildReconciliationMetadata( + new OspreyConfig(), null); + Assert.AreEqual(ParquetScoreCache.RECONCILED_SURVIVORS, + metadata["osprey.reconciled"]); + ParquetScoreCache.StreamReconciledScoresParquet( + originalPath, reconciledPath, null, null, metadata, null, "f.mzML", null, + null, null); + Assert.IsTrue(ParquetScoreCache.IsCurrentReconciledSurvivorSubset(reconciledPath)); + + // And it is the POSITIVE form of the interim-shape refusal, not a second + // opinion: a file this accepts is one that one rejects. + Assert.IsFalse(ParquetScoreCache.IsSubsetWithoutScoreIndex(reconciledPath)); + } + finally + { + try { Directory.Delete(dir, true); } catch { /* best-effort */ } + } + } + /// /// Stage-6 streaming reconciled transfer: streaming the original parquet /// group-by-group with an overlay map + gap-fill list diff --git a/pwiz_tools/Osprey/Osprey.Test/LibraryFragmentReleaseTest.cs b/pwiz_tools/Osprey/Osprey.Test/LibraryFragmentReleaseTest.cs index f203376c0f..c6081185f2 100644 --- a/pwiz_tools/Osprey/Osprey.Test/LibraryFragmentReleaseTest.cs +++ b/pwiz_tools/Osprey/Osprey.Test/LibraryFragmentReleaseTest.cs @@ -175,11 +175,11 @@ private static void ValidateIdentityFieldsSurvive() private static void ValidateEveryLegThatHoldsTheLibraryReleasesIt() { AssertRunsOnLeg(true, @"straight-through", new OspreyConfig()); - AssertRunsOnLeg(true, @"--task SecondPassFDR", - WithInputScores(c => c.ExpectReconciledInput = true)); - AssertRunsOnLeg(true, @"--input-scores full pipeline", WithInputScores(_ => { })); - AssertRunsOnLeg(false, @"--task FirstPassFDR", - WithInputScores(c => c.StopAfterStage5 = true)); + AssertRunsOnLeg(true, @"--task SecondPassFDR", ForTask(HpcTask.SecondPassFdr)); + // The `--input-scores full pipeline` leg that stood here is gone with the flag: + // a single-node full pipeline started from parquets IS the straight-through leg + // above now, asserted once rather than twice under two input kinds. + AssertRunsOnLeg(false, @"--task FirstPassFDR", ForTask(HpcTask.FirstPassFdr)); // --fdrbench-pass 1 forces the RESIDENT first-pass pool, which never computes a // surviving base_id set, so there is nothing to release against. @@ -226,22 +226,22 @@ private static void ValidateValidityKeySuffixTracksWhetherTheReleaseRan() { AssertSuffix(true, @"straight-through, released", new OspreyConfig()); AssertSuffix(true, @"--task SecondPassFDR, released", - WithInputScores(c => c.ExpectReconciledInput = true)); + ForTask(HpcTask.SecondPassFdr)); AssertSuffix(true, @"--task FirstPassFDR cannot release", - WithInputScores(c => c.StopAfterStage5 = true)); + ForTask(HpcTask.FirstPassFdr)); OspreyEnvironment.UseFdrProjection = false; AssertSuffix(false, @"could have released, Stage 5 went resident instead", new OspreyConfig()); // SecondPassFDR's release is its own and does not ride the Stage 5 path. AssertSuffix(true, @"--task SecondPassFDR ignores OSPREY_FDR_PROJECTION", - WithInputScores(c => c.ExpectReconciledInput = true)); + ForTask(HpcTask.SecondPassFdr)); OspreyEnvironment.UseFdrProjection = savedProjection; OspreyEnvironment.ReleaseLibraryFragments = false; AssertSuffix(false, @"opted out where a release was possible", new OspreyConfig()); AssertSuffix(true, @"opted out where it was not possible anyway", - WithInputScores(c => c.StopAfterStage5 = true)); + ForTask(HpcTask.FirstPassFdr)); } finally { @@ -270,11 +270,23 @@ private static PipelineContext MakeContext(OspreyConfig config) return new PipelineContext(config, AnalysisPipeline.CanonicalPipeline(), null, null, null); } - private static OspreyConfig WithInputScores(Action set) + /// + /// One task's config, built the way Program.Main builds it: the task, and the + /// three membership flags DERIVED from it. It used to carry an input KIND as well - a + /// parquet list standing for --input-scores - which the release predicate read + /// alongside the flags; that seam has retired. + /// + private static OspreyConfig ForTask(HpcTask task) { - var config = new OspreyConfig { InputScores = new List { @"a.scores.parquet" } }; - set(config); - return config; + return new OspreyConfig + { + SelectedTask = task, + NoJoin = task == HpcTask.PerFileScoring || task == HpcTask.PerFileRescore, + // EXACTLY Program.cs's single assignment. Naming ModelDiagnostics here built + // a config the CLI cannot produce - see PipelineMembershipTest.ForTask. + StopAfterStage5 = task == HpcTask.FirstPassFdr, + ExpectReconciledInput = task == HpcTask.SecondPassFdr, + }; } /// diff --git a/pwiz_tools/Osprey/Osprey.Test/PipelineMembershipTest.cs b/pwiz_tools/Osprey/Osprey.Test/PipelineMembershipTest.cs index 4f25ef9524..e8737003a3 100644 --- a/pwiz_tools/Osprey/Osprey.Test/PipelineMembershipTest.cs +++ b/pwiz_tools/Osprey/Osprey.Test/PipelineMembershipTest.cs @@ -22,7 +22,7 @@ */ using System; -using System.Collections.Generic; +using System.Linq; using Microsoft.VisualStudio.TestTools.UnitTesting; using pwiz.Osprey.Core; using pwiz.Osprey.Tasks; @@ -45,11 +45,29 @@ namespace pwiz.Osprey.Test [TestClass] public class PipelineMembershipTest { - private static OspreyConfig WithInputScores(Action set) + /// + /// One task's config, built the way Program.Main builds it: the task, and the + /// three membership flags DERIVED from it. Nothing else - which is the change these + /// rows record. Each row used to carry an input KIND too (a parquet list standing for + /// --input-scores), and every predicate read both; the kind is gone and the + /// expected memberships below are unchanged, which is the claim worth pinning. + /// + private static OspreyConfig ForTask(HpcTask task) { - var config = new OspreyConfig { InputScores = new List { @"a.scores.parquet" } }; - set(config); - return config; + return new OspreyConfig + { + SelectedTask = task, + NoJoin = task == HpcTask.PerFileScoring || task == HpcTask.PerFileRescore, + // EXACTLY Program.cs's assignment, which is the only one in the tree: + // `config.StopAfterStage5 = selectedTask == HpcTask.FirstPassFdr;`. This + // helper also named ModelDiagnostics, building a config the CLI cannot + // produce - so the row below asserted a membership no real run has, while + // ProgramTests pinned the real flags and stated the opposite design. Two + // tests in one assembly asserting incompatible things is worse than either + // being wrong alone, because whichever you read first looks corroborated. + StopAfterStage5 = task == HpcTask.FirstPassFdr, + ExpectReconciledInput = task == HpcTask.SecondPassFdr, + }; } [TestMethod] @@ -61,20 +79,23 @@ public void TestIsIncludedMembershipTable() { (@"straight-through", new OspreyConfig(), new[] { true, true, true, true }), - (@"PerFileScoring", new OspreyConfig { NoJoin = true }, + (@"PerFileScoring", ForTask(HpcTask.PerFileScoring), new[] { true, false, false, false }), - (@"FirstPassFDR", WithInputScores(c => c.StopAfterStage5 = true), + (@"FirstPassFDR", ForTask(HpcTask.FirstPassFdr), new[] { false, true, false, false }), - (@"PerFileRescoring", WithInputScores(c => c.NoJoin = true), + (@"PerFileRescoring", ForTask(HpcTask.PerFileRescore), new[] { false, false, true, false }), - (@"SecondPassFDR", WithInputScores(c => c.ExpectReconciledInput = true), + (@"SecondPassFDR", ForTask(HpcTask.SecondPassFdr), new[] { false, false, false, true }), - // --input-scores with no --task: the single-node full pipeline. - // PerFileScoring lazy-rehydrates the supplied scores rather than - // computing them, so it is excluded; FirstPassFDR..SecondPassFDR compute - // Stages 5-8. - (@"input-scores-full", WithInputScores(_ => { }), - new[] { false, true, true, true }), + // --task ModelDiagnostics is a RENDER over retained products, and it reaches + // AnalysisPipeline with all three membership flags FALSE - it sets none of + // them (see ForTask, and Program.cs's single StopAfterStage5 assignment). So + // it is in every task, exactly like the straight-through run, and suppresses + // artifact writes rather than membership. The row here used to read + // {true,true,false,false}, which was the shape of a config the CLI cannot + // build; ProgramTests.cs pins the real flags and now agrees with this. + (@"ModelDiagnostics", ForTask(HpcTask.ModelDiagnostics), + new[] { true, true, true, true }), }; foreach (var c in cases) @@ -91,5 +112,42 @@ public void TestIsIncludedMembershipTable() } } } + + /// + /// Only a process that RUNS Stage 7's join may be admitted to the streamed one. + /// + /// The case that matters is --task FirstPassFDR, and it is not + /// hypothetical: re-run over a directory a previous analysis COMPLETED, every + /// disk-side term of CanStreamStage7Join is satisfied by that previous run's + /// own output. PerFileScoringTask would then take its per-run-join branch, + /// publish one EMPTY list per run for a fold that never comes, and FirstPassFDR would + /// compute its pass over nothing - rewriting both boundary sidecars and the retained + /// base_id summary as empty, exit 0. + /// + /// Asserted with the switch passed as TRUE and against the two-argument form, so + /// this pins the membership term alone and cannot pass merely because the environment + /// happens to have streaming off. + /// + [TestMethod] + public void TestOnlyStage7JoinTasksAdmitTheStreamedJoin() + { + var admitted = new[] { HpcTask.SecondPassFdr, HpcTask.ModelDiagnostics }; + foreach (HpcTask task in Enum.GetValues(typeof(HpcTask))) + { + bool expected = admitted.Contains(task); + Assert.AreEqual(expected, ScoringTaskShared.RunsStage7Join(ForTask(task)), + string.Format(@"--task {0}: RunsStage7Join must be {1}", task, expected)); + // A task that does not run the join must be refused BEFORE any disk term, + // which is what makes the refusal free and unconditional. + if (!expected) + { + Assert.IsFalse( + ScoringTaskShared.Stage7StreamAdmittedBeforeRescore(ForTask(task), true), + string.Format(@"--task {0} must not be admitted to the streamed join", task)); + } + } + // The straight-through pipeline runs every stage, so it is admitted. + Assert.IsTrue(ScoringTaskShared.RunsStage7Join(new OspreyConfig())); + } } } diff --git a/pwiz_tools/Osprey/Osprey.Test/ProgramTests.cs b/pwiz_tools/Osprey/Osprey.Test/ProgramTests.cs index 499b4a069b..31056603c0 100644 --- a/pwiz_tools/Osprey/Osprey.Test/ProgramTests.cs +++ b/pwiz_tools/Osprey/Osprey.Test/ProgramTests.cs @@ -24,7 +24,6 @@ using System; using System.Collections.Generic; using System.Globalization; -using System.IO; using Microsoft.VisualStudio.TestTools.UnitTesting; using pwiz.Osprey.Core; using pwiz.Osprey.IO; @@ -34,8 +33,9 @@ namespace pwiz.Osprey.Test { /// /// Tests for Osprey Program-level helpers: the HPC scoring split - /// flag validation (Program.ValidateArgs) and the --input-scores - /// directory expansion (Program.ResolveInputScores). + /// flag validation (Program.ValidateArgs). The --input-scores directory + /// expansion it also covered went with that flag: every task takes the data + /// files and derives its parquets from their stems. /// /// These are unit tests of CLI argument plumbing only. End-to-end /// scoring round-trip (Stages 1-4 → parquet → Stage 5+) is exercised @@ -67,7 +67,41 @@ public void RestoreExperimentAgg() OspreyEnvironment.MeanBestN = _savedMeanBestN; } - // --- ValidateArgs: --task is authoritative over input type -------- + /// + /// Two inputs sharing a file-name STEM are refused, whatever directories they sit in. + /// + /// Every per-run artifact is <stem>.<suffix> and every per-run + /// map is keyed the same way, so a shared stem is two runs the pipeline cannot tell + /// apart. Left to be discovered downstream it takes two shapes and neither names the + /// cause: an ArgumentException about a duplicate key mid-Stage-6/7, or - with + /// --output-dir, where both stems resolve into one directory - two runs quietly + /// sharing one parquet, no error at all. + /// + /// Asserted with DIFFERENT directories, which is the case that matters and the + /// one --input-list makes routine at cohort scale; identical paths would be + /// caught by cruder means. + /// + [TestMethod] + public void TestValidateRejectsDuplicateInputStems() + { + var config = TaskConfig(HpcTask.PerFileScoring); + config.LibrarySource = LibrarySource.FromPath("ref.blib"); + config.InputFiles = new List { @"plateA\run1.mzML", @"plateB\run1.mzML" }; + string err = Program.ValidateArgs(config); + Assert.IsNotNull(err, "two inputs sharing a stem must be refused"); + // The stem and BOTH colliding paths, so the operator can act without re-deriving + // which of several hundred inputs collided. + StringAssert.Contains(err, "run1"); + StringAssert.Contains(err, @"plateA\run1.mzML"); + StringAssert.Contains(err, @"plateB\run1.mzML"); + + // Distinct stems in one directory remain fine - the check is on the stem, not the + // directory, and a cohort in one folder is the ordinary case. + config.InputFiles = new List { @"plateA\run1.mzML", @"plateA\run2.mzML" }; + Assert.IsNull(Program.ValidateArgs(config)); + } + + // --- ValidateArgs: what each task requires ------------------------- private static OspreyConfig TaskConfig(HpcTask task) { @@ -103,8 +137,6 @@ public void TestValidateSpectraCache() Assert.IsNull(Program.ValidateArgs(config), "a library should be tolerated"); AssertSpectraCacheError(c => { }, "--input c.InputScores = new List { "a.scores.parquet" }, - "not --input-scores"); } private static void AssertSpectraCacheError(Action mutate, string expected) @@ -150,35 +182,20 @@ public void TestValidatePerFileScoringRequiresLibrary() StringAssert.Contains(err, "--library"); } - [TestMethod] - public void TestValidatePerFileScoringRejectsInputScores() - { - // --task is authoritative: PerFileScoring + --input-scores must - // error, not silently dispatch PerFileRescore. - var config = TaskConfig(HpcTask.PerFileScoring); - config.InputScores = new List { "a.scores.parquet" }; - config.LibrarySource = LibrarySource.FromPath("ref.blib"); - config.OutputBlib = "out.blib"; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "--task PerFileScoring"); - StringAssert.Contains(err, "not --input-scores"); - } - - // - PerFileRescore (--input-scores in) -- + // - PerFileRescore (one run in, its reconciled parquet out) -- [TestMethod] public void TestValidatePerFileRescoreHappyPath() { var config = TaskConfig(HpcTask.PerFileRescore); - config.InputScores = new List { "a.scores.parquet" }; + config.InputFiles = new List { "a.mzML" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; Assert.IsNull(Program.ValidateArgs(config)); } [TestMethod] - public void TestValidatePerFileRescoreRequiresInputScores() + public void TestValidatePerFileRescoreRequiresInput() { var config = TaskConfig(HpcTask.PerFileRescore); config.LibrarySource = LibrarySource.FromPath("ref.blib"); @@ -186,78 +203,49 @@ public void TestValidatePerFileRescoreRequiresInputScores() string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task PerFileRescoring"); - StringAssert.Contains(err, "--input-scores"); + StringAssert.Contains(err, "--input"); } [TestMethod] public void TestValidatePerFileRescoreRequiresLibraryAndOutput() { - var config = TaskConfig(HpcTask.PerFileRescore); - config.InputScores = new List { "a.scores.parquet" }; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "--task PerFileRescoring"); - StringAssert.Contains(err, "--library and --output"); - } - - [TestMethod] - public void TestValidatePerFileRescoreRejectsInputMzml() - { - // Authoritative: PerFileRescore + -i mzML must error, not silently - // dispatch PerFileScoring. Error must name the task the user typed. var config = TaskConfig(HpcTask.PerFileRescore); config.InputFiles = new List { "a.mzML" }; - config.LibrarySource = LibrarySource.FromPath("ref.blib"); - config.OutputBlib = "out.blib"; string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task PerFileRescoring"); - StringAssert.Contains(err, "not -i "); + StringAssert.Contains(err, "--library and --output"); } - // - FirstPassFDR (--input-scores in, 2+ files, reconciliation on) -- + // - FirstPassFDR (2+ runs in, reconciliation on) -- [TestMethod] public void TestValidateFirstPassFdrHappyPath() { var config = TaskConfig(HpcTask.FirstPassFdr); - config.InputScores = new List { "a.scores.parquet", "b.scores.parquet" }; + config.InputFiles = new List { "a.mzML", "b.mzML" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; Assert.IsNull(Program.ValidateArgs(config)); } [TestMethod] - public void TestValidateFirstPassFdrRequiresInputScores() - { - var config = TaskConfig(HpcTask.FirstPassFdr); - config.LibrarySource = LibrarySource.FromPath("ref.blib"); - config.OutputBlib = "out.blib"; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "--task FirstPassFDR"); - StringAssert.Contains(err, "--input-scores"); - } - - [TestMethod] - public void TestValidateFirstPassFdrRejectsInputMzml() + public void TestValidateFirstPassFdrRequiresInput() { var config = TaskConfig(HpcTask.FirstPassFdr); - config.InputFiles = new List { "a.mzML" }; - config.InputScores = new List { "a.scores.parquet", "b.scores.parquet" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task FirstPassFDR"); - StringAssert.Contains(err, "cannot be combined with --input"); + StringAssert.Contains(err, "--input"); } [TestMethod] public void TestValidateFirstPassFdrRequiresLibraryAndOutput() { var config = TaskConfig(HpcTask.FirstPassFdr); - config.InputScores = new List { "a.scores.parquet", "b.scores.parquet" }; + config.InputFiles = new List { "a.mzML", "b.mzML" }; string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task FirstPassFDR"); @@ -270,20 +258,20 @@ public void TestValidateFirstPassFdrRejectsSingleFile() // FirstPassFDR writes the Stage 5 -> Stage 6 boundary pair, only // meaningful with siblings; a single-file run errors fast. var config = TaskConfig(HpcTask.FirstPassFdr); - config.InputScores = new List { "only.scores.parquet" }; + config.InputFiles = new List { "only.mzML" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task FirstPassFDR"); - StringAssert.Contains(err, "2+ parquet files"); + StringAssert.Contains(err, "2+ files"); } [TestMethod] public void TestValidateFirstPassFdrRequiresReconciliationEnabled() { var config = TaskConfig(HpcTask.FirstPassFdr); - config.InputScores = new List { "a.scores.parquet", "b.scores.parquet" }; + config.InputFiles = new List { "a.mzML", "b.mzML" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; config.Reconciliation.Enabled = false; @@ -292,69 +280,42 @@ public void TestValidateFirstPassFdrRequiresReconciliationEnabled() StringAssert.Contains(err, "Reconciliation.Enabled"); } - // - SecondPassFDR (reconciled --input-scores in) -- + // - SecondPassFDR (every run in, reading their reconciled parquets) -- [TestMethod] public void TestValidateSecondPassFdrHappyPath() { - var config = TaskConfig(HpcTask.SecondPassFdr); - config.InputScores = new List { "a.scores-reconciled.parquet" }; - config.LibrarySource = LibrarySource.FromPath("ref.blib"); - config.OutputBlib = "out.blib"; - Assert.IsNull(Program.ValidateArgs(config)); - } - - [TestMethod] - public void TestValidateSecondPassFdrRequiresInputScores() - { - // Uncontested gap from ultrareview: --task SecondPassFDR without - // --input-scores (even with -i mzML) used to pass validation and - // silently run the full pipeline. It must now fail fast. var config = TaskConfig(HpcTask.SecondPassFdr); config.InputFiles = new List { "a.mzML" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "--task SecondPassFDR"); - // -i present -> the cross is reported first; either way it must not pass. + Assert.IsNull(Program.ValidateArgs(config)); } [TestMethod] - public void TestValidateSecondPassFdrRequiresInputScoresNoMzml() + public void TestValidateSecondPassFdrRequiresInput() { + // Uncontested gap from ultrareview: --task SecondPassFDR with no inputs at + // all used to pass validation and silently run the full pipeline. It must + // fail fast, and the message must name the task the user typed. var config = TaskConfig(HpcTask.SecondPassFdr); config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task SecondPassFDR"); - StringAssert.Contains(err, "--input-scores"); + StringAssert.Contains(err, "--input"); } [TestMethod] public void TestValidateSecondPassFdrRequiresLibraryAndOutput() - { - var config = TaskConfig(HpcTask.SecondPassFdr); - config.InputScores = new List { "a.scores-reconciled.parquet" }; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "--task SecondPassFDR"); - StringAssert.Contains(err, "--library and --output"); - } - - [TestMethod] - public void TestValidateSecondPassFdrRejectsInputMzml() { var config = TaskConfig(HpcTask.SecondPassFdr); config.InputFiles = new List { "a.mzML" }; - config.InputScores = new List { "a.scores-reconciled.parquet" }; - config.LibrarySource = LibrarySource.FromPath("ref.blib"); - config.OutputBlib = "out.blib"; string err = Program.ValidateArgs(config); Assert.IsNotNull(err); StringAssert.Contains(err, "--task SecondPassFDR"); - StringAssert.Contains(err, "cannot be combined with --input"); + StringAssert.Contains(err, "--library and --output"); } // - ModelDiagnostics (the completed run's own command line, replayed) -- @@ -368,7 +329,7 @@ public void TestValidateModelDiagnosticsTakesTheFullPipelineArgs() // ModelDiagnostics - so it must validate exactly as that command line does, and // adding a task-specific rule here would reject the invocation it exists to serve. var config = TaskConfig(HpcTask.ModelDiagnostics); - config.InputScores = new List { "a.scores.parquet", "b.scores.parquet" }; + config.InputFiles = new List { "a.mzML", "b.mzML" }; config.LibrarySource = LibrarySource.FromPath("ref.blib"); config.OutputBlib = "out.blib"; Assert.IsNull(Program.ValidateArgs(config)); @@ -389,7 +350,7 @@ public void TestValidateModelDiagnosticsTakesTheFullPipelineArgs() StringAssert.Contains(err, "No input files"); } - // - Default (no --task): full pipeline from -i mzML or --input-scores -- + // - Default (no --task): the full pipeline -- [TestMethod] public void TestValidateDefaultFullHappyPath() @@ -416,54 +377,6 @@ public void TestValidateDefaultRejectsMissingInput() StringAssert.Contains(err, "No input files"); } - [TestMethod] - public void TestValidateFullFromScoresHappyPath() - { - // No --task + --input-scores: the full pipeline started from scores - // (PerFileScoring lazy-rehydrates). A single file is a legal, - // degenerate case. - var config = new OspreyConfig - { - InputScores = new List { "only.scores.parquet" }, - LibrarySource = LibrarySource.FromPath("ref.blib"), - OutputBlib = "out.blib" - }; - Assert.IsNull(Program.ValidateArgs(config)); - } - - [TestMethod] - public void TestValidateFullFromScoresRequiresLibraryAndOutput() - { - // No --task: the error references --input-scores, not a task the - // user never selected. - var config = new OspreyConfig - { - InputScores = new List { "a.scores.parquet", "b.scores.parquet" }, - LibrarySource = LibrarySource.FromPath("ref.blib"), - // missing OutputBlib - }; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "--input-scores"); - StringAssert.Contains(err, "--library and --output"); - Assert.IsFalse(err.Contains("--task"), "full-from-scores error must not name a --task: " + err); - } - - [TestMethod] - public void TestValidateFullFromScoresRejectsInputMzml() - { - var config = new OspreyConfig - { - InputFiles = new List { "a.mzML" }, - InputScores = new List { "a.scores.parquet" }, - LibrarySource = LibrarySource.FromPath("ref.blib"), - OutputBlib = "out.blib" - }; - string err = Program.ValidateArgs(config); - Assert.IsNotNull(err); - StringAssert.Contains(err, "cannot be combined with --input"); - } - // --- ResolveTask (--task) ----------------------------------------- [TestMethod] @@ -599,7 +512,7 @@ public void TestModelDiagnosticsDeclaresNoOutputs() private static List SecondPassFdrOutputs(HpcTask task) { var config = TaskConfig(task); - config.InputScores = new List { @"a.scores.parquet", @"b.scores.parquet" }; + config.InputFiles = new List { @"a.mzML", @"b.mzML" }; config.LibrarySource = LibrarySource.FromPath(@"ref.blib"); config.OutputBlib = @"out.blib"; var tasks = AnalysisPipeline.CanonicalPipeline(); @@ -701,140 +614,6 @@ public void TestParseArgsRejectsValueFlagsWithoutValue() } } - // --- ResolveInputScores ------------------------------------------- - - [TestMethod] - public void TestResolveExplicitFilesPassThrough() - { - string dir = NewTempDir(); - try - { - string a = Path.Combine(dir, "a.scores.parquet"); - string b = Path.Combine(dir, "b.scores.parquet"); - File.WriteAllText(a, string.Empty); - File.WriteAllText(b, string.Empty); - var resolved = Program.ResolveInputScores(new List { a, b }); - CollectionAssert.AreEqual(new List { a, b }, resolved); - } - finally - { - Directory.Delete(dir, true); - } - } - - [TestMethod] - public void TestResolveExplicitMissingFileErrors() - { - string dir = NewTempDir(); - try - { - string missing = Path.Combine(dir, "does-not-exist.scores.parquet"); - try - { - Program.ResolveInputScores(new List { missing }); - Assert.Fail("Expected ArgumentException for missing file"); - } - catch (ArgumentException ex) - { - StringAssert.Contains(ex.Message, "not found"); - } - } - finally - { - Directory.Delete(dir, true); - } - } - - [TestMethod] - public void TestResolveDirectoryScansAndSorts() - { - string dir = NewTempDir(); - try - { - File.WriteAllText(Path.Combine(dir, "z.scores.parquet"), string.Empty); - File.WriteAllText(Path.Combine(dir, "a.scores.parquet"), string.Empty); - File.WriteAllText(Path.Combine(dir, "m.scores.parquet"), string.Empty); - File.WriteAllText(Path.Combine(dir, "readme.txt"), string.Empty); - var resolved = Program.ResolveInputScores(new List { dir }); - Assert.AreEqual(3, resolved.Count); - Assert.AreEqual("a.scores.parquet", Path.GetFileName(resolved[0])); - Assert.AreEqual("m.scores.parquet", Path.GetFileName(resolved[1])); - Assert.AreEqual("z.scores.parquet", Path.GetFileName(resolved[2])); - } - finally - { - Directory.Delete(dir, true); - } - } - - [TestMethod] - public void TestResolveDirectoryPrefersReconciledPerStem() - { - // The directory holds both Stage 4 .scores.parquet and Stage 6 - // .scores-reconciled.parquet files. For any stem that has both, - // only the reconciled file is returned; never both. - string dir = NewTempDir(); - try - { - File.WriteAllText(Path.Combine(dir, "a.scores.parquet"), string.Empty); - File.WriteAllText(Path.Combine(dir, "a.scores-reconciled.parquet"), string.Empty); - File.WriteAllText(Path.Combine(dir, "b.scores.parquet"), string.Empty); // no reconciled sibling - File.WriteAllText(Path.Combine(dir, "c.scores-reconciled.parquet"), string.Empty); // no original - // An input stem ending in ".reconciled" stays an original (Copilot - // ambiguity regression guard) - its Stage 4 file must be returned - // as an original, not misread as a reconciled output. - File.WriteAllText(Path.Combine(dir, "d.reconciled.scores.parquet"), string.Empty); - var resolved = Program.ResolveInputScores(new List { dir }); - CollectionAssert.AreEqual( - new[] { "a.scores-reconciled.parquet", "b.scores.parquet", - "c.scores-reconciled.parquet", "d.reconciled.scores.parquet" }, - resolved.ConvertAll(Path.GetFileName)); - // The superseded original must not appear. - CollectionAssert.DoesNotContain(resolved.ConvertAll(Path.GetFileName), "a.scores.parquet"); - } - finally - { - Directory.Delete(dir, true); - } - } - - [TestMethod] - public void TestResolveEmptyDirectoryErrors() - { - string dir = NewTempDir(); - try - { - File.WriteAllText(Path.Combine(dir, "not-a-match.txt"), string.Empty); - try - { - Program.ResolveInputScores(new List { dir }); - Assert.Fail("Expected ArgumentException for directory with no parquets"); - } - catch (ArgumentException ex) - { - StringAssert.Contains(ex.Message, "No *.scores.parquet"); - } - } - finally - { - Directory.Delete(dir, true); - } - } - - [TestMethod] - public void TestResolveEmptyListErrors() - { - try - { - Program.ResolveInputScores(new List()); - Assert.Fail("Expected ArgumentException for empty list"); - } - catch (ArgumentException ex) - { - StringAssert.Contains(ex.Message, "at least one path"); - } - } - // --- OspreyConfig defaults ---------------------------------------- [TestMethod] @@ -842,7 +621,6 @@ public void TestConfigDefaultsDisableHpcMode() { var cfg = new OspreyConfig(); Assert.IsFalse(cfg.NoJoin, "NoJoin should default to false"); - Assert.IsNull(cfg.InputScores, "InputScores should default to null"); } // --- ParquetScoreCache.CheckParquetMetadata ----------------------- @@ -1092,14 +870,5 @@ public void TestParseArgsDecoysInLibraryDefaultsFalse() Assert.IsTrue(string.IsNullOrEmpty(config.DecoyPairingManifestPath)); } - // --- helpers ------------------------------------------------------- - - private static string NewTempDir() - { - string dir = Path.Combine(Path.GetTempPath(), - "osprey_test_program_" + Guid.NewGuid().ToString("N")); - Directory.CreateDirectory(dir); - return dir; - } } } diff --git a/pwiz_tools/Osprey/Osprey.Test/ResidentPoolGuardTest.cs b/pwiz_tools/Osprey/Osprey.Test/ResidentPoolGuardTest.cs index 932da8e882..0d98eb3469 100644 --- a/pwiz_tools/Osprey/Osprey.Test/ResidentPoolGuardTest.cs +++ b/pwiz_tools/Osprey/Osprey.Test/ResidentPoolGuardTest.cs @@ -21,6 +21,9 @@ * limitations under the License. */ +using System; +using System.Collections.Generic; +using System.IO; using System.Linq; using Microsoft.VisualStudio.TestTools.UnitTesting; using pwiz.Osprey.Core; @@ -207,6 +210,12 @@ public void TestResidentPoolGuardError() // buffer SecondPassFDR rebuilds was refused by neither and no token could name it. AssertStage7JoinGuard(); + // The Stage-7 join ADMISSION, which is what decides whether that guard has a + // subject at all. It used to be config.ExpectReconciledInput - one CLI flag - and + // is now the disk question that flag stood in for, so the rules it must not lose + // are pinned here rather than left to the end-to-end gate. + AssertStage7StreamAdmission(); + // The trigger SET itself, not just the message it produces. Each of these takes the // O(files) resident pool and so arms the guard above. AssertNeedsResidentPool(true, fdrbench1); @@ -283,6 +292,40 @@ private static void AssertStage7JoinGuard() streamingAvailable: false, stage7Stream: false, allowUnfixedResident: null)); } + /// + /// The all-runs reconciled-parquet admission: ALL, never any, and never vacuously + /// true. + /// + /// The fold rebuilds each run from its own reconciled parquet, so one run without + /// a readable one is a run it cannot produce - and "some run has one" would fail at that + /// run, hours in. An empty or absent input list is refused for the opposite reason: + /// there is no pool to bound, so vacuous truth would admit a fold over nothing on a + /// configuration no other term in the predicate examines. + /// + /// Only the negative half is asserted here. The positive one needs real Stage 6 + /// artifacts, which is TestIsCurrentReconciledSurvivorSubset's job per file and + /// the regression gate's per cohort; what CANNOT be seen there is a predicate that says + /// yes when it has been handed nothing. + /// + private static void AssertStage7StreamAdmission() + { + Assert.IsFalse(ScoringTaskShared.AllReconciledParquetsCurrent(new OspreyConfig())); + Assert.IsFalse(ScoringTaskShared.AllReconciledParquetsCurrent( + new OspreyConfig { InputFiles = new List() })); + // Paths under a directory that does not exist: every run is missing its parquet, + // which is the cold cohort's state before Stage 6 has written any. + string absent = Path.Combine(Path.GetTempPath(), + "osprey_no_such_dir_" + Guid.NewGuid().ToString("N")); + Assert.IsFalse(ScoringTaskShared.AllReconciledParquetsCurrent( + new OspreyConfig + { + InputFiles = new List + { + Path.Combine(absent, "a.mzML"), Path.Combine(absent, "b.mzML") + } + })); + } + private static void AssertStage6HandoffGuard() { // Streaming: no error, whatever the token says. @@ -359,30 +402,24 @@ private static void AssertStage6HandoffGuard() [TestMethod] public void TestFirstPassMembershipAcrossTasks() { - var scores = new[] { "a.scores.parquet" }; - // Straight-through (-i, no --task): FirstPassFDR runs. Assert.IsTrue(FirstPassFdrTask.IsIncludedFor(new OspreyConfig())); // --task PerFileScoring / PerFileRescoring set NoJoin: excluded, they stop before - // the join. + // the join. One row each is enough now: these used to be asserted twice, once + // with a parquet list and once without, because the predicate read the input KIND + // as well as the flags and the two could disagree. Assert.IsFalse(FirstPassFdrTask.IsIncludedFor( new OspreyConfig { NoJoin = true })); - Assert.IsFalse(FirstPassFdrTask.IsIncludedFor( - new OspreyConfig { NoJoin = true, InputScores = scores.ToList() })); // --task FirstPassFDR sets StopAfterStage5: it IS the first-pass node. Assert.IsTrue(FirstPassFdrTask.IsIncludedFor( - new OspreyConfig { StopAfterStage5 = true, InputScores = scores.ToList() })); - - // The full --input-scores pipeline (no --task): runs. - Assert.IsTrue(FirstPassFdrTask.IsIncludedFor( - new OspreyConfig { InputScores = scores.ToList() })); + new OspreyConfig { StopAfterStage5 = true })); // --task SecondPassFDR: NoJoin FALSE, so the old !NoJoin proxy said "runs" - but // ExpectReconciledInput excludes it. This single row is the whole change. Assert.IsFalse(FirstPassFdrTask.IsIncludedFor( - new OspreyConfig { ExpectReconciledInput = true, InputScores = scores.ToList() }), + new OspreyConfig { ExpectReconciledInput = true }), "--task SecondPassFDR must not be treated as running first-pass Percolator"); // And the consequence the loader draws from it: the merge no longer demands the @@ -397,8 +434,7 @@ public void TestFirstPassMembershipAcrossTasks() // near-empty .blib with no error. Streaming hydrate and lean projection are // different routes; only the first is what this row unlocks. Assert.IsFalse(PerFileScoringTask.NeedsResidentPool( - new OspreyConfig { ExpectReconciledInput = true, InputScores = scores.ToList() }, - useFdrProjection: true)); + new OspreyConfig { ExpectReconciledInput = true }, useFdrProjection: true)); } } } diff --git a/pwiz_tools/Osprey/Osprey/AnalysisPipeline.cs b/pwiz_tools/Osprey/Osprey/AnalysisPipeline.cs index 2d2811c9b9..3d233feab5 100644 --- a/pwiz_tools/Osprey/Osprey/AnalysisPipeline.cs +++ b/pwiz_tools/Osprey/Osprey/AnalysisPipeline.cs @@ -61,26 +61,13 @@ public int Run(OspreyConfig config) // OSPREY_DUMP_* / OSPREY_DIAG_* env var is set. OspreyDiagnostics.Initialize(config.Diagnostics); - // Worker-mode entry normalization: in --input-scores modes - // without explicit -i, synthesize InputFiles from the parquet - // stems ONCE here, at pipeline entry, so the driver's - // Outputs/IsTaskAlreadyDone skip checks and every per-task - // accessor see a populated InputFiles regardless of which task - // the run starts at. (Mutation-contract: InputFiles is a - // pipeline-populated field that does NOT feed any identity - // hash, so it may be written once at entry -- see - // PipelineContext.Config. Previously this lived inside - // PerFileScoringTask's join-only load, which the driver never - // reached when PerFileScoring was the StartAt task, e.g. - // `--task PerFileScoring --input-scores`.) - if (config.InputScores != null && config.InputScores.Count > 0 - && (config.InputFiles == null || config.InputFiles.Count == 0)) - { - var synthetic = new List(config.InputScores.Count); - foreach (var p in config.InputScores) - synthetic.Add(RescoreHydration.SyntheticInputFromParquet(p)); - config.InputFiles = synthetic; - } + // No worker-mode entry normalization any more, and its absence is the + // point. A --input-scores run arrived here with parquet paths and no + // InputFiles, so the pipeline's FIRST act was to convert them back into + // data-file names - a round trip through a synthetic .mzML that does + // not exist, purely so the sidecar helpers could derive from a stem. Every + // task now receives the stems it needs on -i, which is the direction the + // derivation was always going. // --task SpectraCache stages data rather than analyzing it: it runs // its own one-task pipeline instead of the canonical four. Selecting diff --git a/pwiz_tools/Osprey/Osprey/OspreyCommandArgs.cs b/pwiz_tools/Osprey/Osprey/OspreyCommandArgs.cs index deeda0cc70..234f2851fd 100644 --- a/pwiz_tools/Osprey/Osprey/OspreyCommandArgs.cs +++ b/pwiz_tools/Osprey/Osprey/OspreyCommandArgs.cs @@ -85,7 +85,8 @@ static OspreyCommandArgs() // deeper path tree reaches it sooner. Past it the failure is a CreateProcess error or a // truncated argument list, neither of which says "too many inputs". // - // --input-scores already avoids this by accepting a directory; -i had no equivalent. + // --input-list is the answer, and since --input-scores retired it is the ONLY one: + // that flag used to accept a directory, which is how the HPC tasks avoided the wall. // One path per line, blank lines and #-comments ignored, composable with -i and with // itself (both append, exactly as repeated -i does). public static readonly OspreyArgument ARG_INPUT_LIST = new OspreyArgument(@"input-list", @@ -223,22 +224,15 @@ static OspreyCommandArgs() public static readonly OspreyArgument ARG_TASK = new OspreyArgument(@"task", new[] { @"SpectraCache", @"PerFileScoring", @"FirstPassFDR", @"PerFileRescoring", @"SecondPassFDR", @"ModelDiagnostics" }, (c, p) => true); - public static readonly OspreyArgument ARG_INPUT_SCORES = new OspreyArgument(@"input-scores", - () => @"", (c, p) => true) { Variadic = true, ProcessVariadic = (c, toks) => - { - // Accumulate across repeated --input-scores flags and re-resolve, matching the - // former switch exactly (Rust clap Vec). ResolveInputScores expands a - // single directory and validates explicit paths. - var scorePaths = new List(); - if (c._config.InputScores != null) - scorePaths.AddRange(c._config.InputScores); - scorePaths.AddRange(toks); - c._config.InputScores = Program.ResolveInputScores(scorePaths); - return true; - } }; + // --input-scores is GONE. It named an input KIND - "you handed me parquets" - which is + // how the Rust pipeline said "Stage 1-4 is already done"; the C# port says that with + // --task plus the per-run validity sidecars, and two seams answering one question is + // what let --task ModelDiagnostics join the pipeline and demand state a diagnostics + // fold never publishes. Every task now takes -i and derives its parquets from the + // input stem, which is the direction every other sidecar already derives in. private static readonly ArgumentGroup GROUP_HPC = new ArgumentGroup(() => @"Distributed / HPC", true, - ARG_TASK, ARG_INPUT_SCORES); + ARG_TASK); // --- Performance ------------------------------------------------------------------ // OUTER vs INNER parallelism, kept deliberately separate. --parallel-files is the @@ -337,7 +331,7 @@ public static IEnumerable UsageBlocks new ParaUsageBlock(@"EXAMPLES:"), new ParaUsageBlock(@" osprey -i sample.mzML -l library.tsv -o results.blib"), new ParaUsageBlock(@" osprey -i *.mzML -l library.tsv -o results.blib --resolution hram"), - new ParaUsageBlock(@"HPC SPLIT (one node = one --task): see --task / --input-scores above."), + new ParaUsageBlock(@"HPC SPLIT (one node = one --task): see --task above."), }; } } @@ -805,23 +799,27 @@ private static void AppendUsageHtmlHpcExamples(StringBuilder sb) sb.AppendLine(@"# split 1 - one process per mzML (writes <stem>.scores.parquet, <stem>.calibration.json beside each input)"); sb.AppendLine(@"Osprey --task PerFileScoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); sb.AppendLine(); - sb.AppendLine(@"# join 1 - one process over ALL parquets (pass a directory so the order is deterministic)"); - sb.AppendLine(@"Osprey --task FirstPassFDR --input-scores ./scores_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); + sb.AppendLine(@"# join 1 - one process over ALL runs (pass a sorted list so the order is deterministic)"); + sb.AppendLine(@"Osprey --task FirstPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); sb.AppendLine(@"# writes beside each parquet: <stem>.1st-pass.fdr_scores.bin, <stem>.reconciliation.json"); sb.AppendLine(); sb.AppendLine(@"# split 2 - one process per file (parquet + its two sidecars co-located)"); - sb.AppendLine(@"Osprey --task PerFileRescoring --input-scores s1.scores.parquet -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); + sb.AppendLine(@"Osprey --task PerFileRescoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); sb.AppendLine(@"# writes: <stem>.scores-reconciled.parquet"); sb.AppendLine(); - sb.AppendLine(@"# join 2 - one process over ALL reconciled parquets (writes out.blib)"); - sb.AppendLine(@"Osprey --task SecondPassFDR --input-scores ./reconciled_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); + sb.AppendLine(@"# join 2 - one process over ALL runs, reading their reconciled parquets (writes out.blib)"); + sb.AppendLine(@"Osprey --task SecondPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01"); sb.AppendLine(@""); - sb.AppendLine(@"

--input-scores takes a directory (globbed and sorted internally) " + - @"or an explicit file list (used in the order given). FirstPassFDR reconciliation is " + - @"order-sensitive, so for FirstPassFDR and SecondPassFDR pass a directory or a deterministically sorted " + - @"list. The rehydration sidecars must travel with their parquet into each worker's " + - @"working directory. Let the scheduler do the fan-out (one file per split process) rather " + - @"than --parallel-files, which is the single-node multi-file mode.

"); + sb.AppendLine(@"

EVERY task takes -i, naming the DATA files - the same names " + + @"the first split was given. A join task derives each run's parquet and sidecars from " + + @"the input stem, so the data file itself need not still exist: what has to be in the " + + @"worker's working directory (or under --output-dir) is that run's " + + @"artifacts. FirstPassFDR reconciliation is order-sensitive, so pass a " + + @"deterministically sorted list - --input-list takes one path per line and " + + @"is what a cohort past a few hundred runs needs, since -i spends the " + + @"command line at O(files). Let the scheduler do the fan-out (one file per split " + + @"process) rather than --parallel-files, which is the single-node " + + @"multi-file mode.

"); } /// @@ -860,7 +858,6 @@ private class OspreyArgUsageProvider : IArgUsageProvider { @"decoy-pairing-manifest", @"FDRBench 5-column pairing manifest (TSV), used with --decoys-in-library" }, { @"write-pin", @"Write PIN files for external tools" }, { @"task", @"HPC: run exactly one pipeline task (one node = one task). Omit for the full pipeline. SpectraCache stages the .spectra.bin caches; ModelDiagnostics regenerates only the --model-diagnostics report for a COMPLETED run, writing no other artifact." }, - { @"input-scores", @"HPC: one or more .scores.parquet files, or a single directory (non-recursive). Mutex with --input." }, { @"parallel-files", @"Input files scored concurrently (OUTER). Absent: one at a time (default). No value: auto from free RAM and cores. : exactly N regardless of RAM/cores. Distinct from --threads." }, { @"threads", @"Per-file main-search threads (INNER; default: all cores), divided across files run concurrently by --parallel-files" }, { @"timestamp", @"Prefix each output line with [yyyy/MM/dd HH:mm:ss]" }, diff --git a/pwiz_tools/Osprey/Osprey/Program.cs b/pwiz_tools/Osprey/Osprey/Program.cs index d165845497..64b9b00762 100644 --- a/pwiz_tools/Osprey/Osprey/Program.cs +++ b/pwiz_tools/Osprey/Osprey/Program.cs @@ -24,9 +24,12 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; +using System.Text; using pwiz.Common.SystemUtil; using pwiz.Osprey.Core; using pwiz.Osprey.IO; +using pwiz.Osprey.Tasks; using pwiz.Osprey.Tasks.ModelDiagnostics; namespace pwiz.Osprey @@ -76,7 +79,7 @@ static int Main(string[] args) try { // Scan args for the HPC task selector up front so error - // messages fire before --input-scores resolution. A single + // messages name the task before any other argument is parsed. A single // `--task ` runs exactly one pipeline task (HPC: one // node = one task) by setting the (NoJoin, StopAfterStage5, // ExpectReconciledInput) config flags the four tasks' @@ -120,10 +123,10 @@ static int Main(string[] args) OspreyConfig config = ParseArgs(args); // --task selects one pipeline task; derive the membership flags // the tasks' IsIncluded methods read. ExpectReconciledInput also - // arms the strict-reconciled-input gate (every --input-scores - // parquet must carry osprey.reconciled = "true"). Mirrors Rust's - // main.rs wiring. SelectedTask is kept so ValidateArgs can enforce - // the task<->input-type contract and name the typed task. + // arms the strict-reconciled-input gate (every run's reconciled + // parquet must carry osprey.reconciled = "true"). All three are + // derived from --task and from nothing else, which is what let the + // input KIND retire: it was the OTHER seam saying the same thing. config.SelectedTask = selectedTask; // --task ModelDiagnostics IS the request for the report; without the flag the // run would recompute the pass-2 view and write nothing, a silent no-op. @@ -186,56 +189,84 @@ static int Main(string[] args) Directory.CreateDirectory(config.OutputDir); if (!string.IsNullOrEmpty(config.CacheDir)) Directory.CreateDirectory(config.CacheDir); - // Runs that consume --input-scores (FirstPassFDR, PerFileRescore, - // SecondPassFDR, or the default full pipeline started from scores) - // have no mzML inputs to validate and ignore --output handling - // differently from per-file scoring. - bool fromInputScores = config.InputScores != null && config.InputScores.Count > 0; - // --task PerFileScoring ignores --output (it writes per-file // .scores.parquet, not a blib), but that is expected single-task / // HPC-worker behavior -- wrapper scripts routinely pass a placeholder // --output -- so it is NOT warned about. The settings block below // reports the real per-file parquet output for this task instead. - // Validate input files exist on disk (skip when consuming - // --input-scores, where there are no mzML inputs; --input-scores - // paths were already validated by ResolveInputScores during parsing). - if (!fromInputScores) + // Validate input files exist on disk. EVERY run reaches this now: a task + // that starts after Stage 4 used to be handed parquets and skipped the + // check entirely, and it is handed the same data-file names as every other + // task instead. + int cacheOnlyInputs = 0; + int artifactOnlyInputs = 0; + foreach (string inputFile in config.InputFiles) { - int cacheOnlyInputs = 0; - foreach (string inputFile in config.InputFiles) + // A directory counts as present. Several vendor formats ARE + // directories (Agilent .d, Bruker .d, Waters .raw), so testing + // File.Exists alone rejected every one of them here, before any + // reader was consulted, on builds with and without the vendor + // reader. It also blocked reusing a raw-derived .spectra.bin, + // which must work on a build that cannot read the raw itself. + if (File.Exists(inputFile) || Directory.Exists(inputFile)) + continue; + // An absent source is fine once its cache is built: Stage 1 is + // the only stage that reads a source, and SpectraCache already + // treats a missing one as "trust the cache". That makes + // delete-the-sources-after-caching a supported way to halve the + // disk a large cohort needs. + if (File.Exists(SpectraCache.GetCachePath(inputFile))) { - // A directory counts as present. Several vendor formats ARE - // directories (Agilent .d, Bruker .d, Waters .raw), so testing - // File.Exists alone rejected every one of them here, before any - // reader was consulted, on builds with and without the vendor - // reader. It also blocked reusing a raw-derived .spectra.bin, - // which must work on a build that cannot read the raw itself. - if (!File.Exists(inputFile) && !Directory.Exists(inputFile)) - { - // An absent source is fine once its cache is built: Stage 1 is - // the only stage that reads a source, and SpectraCache already - // treats a missing one as "trust the cache". That makes - // delete-the-sources-after-caching a supported way to halve the - // disk a large cohort needs. - if (File.Exists(SpectraCache.GetCachePath(inputFile))) - { - cacheOnlyInputs++; - continue; - } - LogError(string.Format("Input file not found: {0}", inputFile)); - return 1; - } + cacheOnlyInputs++; + continue; } - // Announced, not silent: a run whose sources are gone cannot rebuild a - // cache that turns out to be wrong, so the log is the only provenance. - if (cacheOnlyInputs > 0) + // ...and so is an absent source with no cache, once its SCORES exist. + // A join node is shipped parquets and sidecars and nothing else - that + // is the whole point of the split - so demanding the data file back + // would refuse the configuration the HPC chain is built on. This is + // what --input-scores used to say by naming a different input KIND; + // said here it is one input kind and one question about it. + // ...and only for a task that STARTS AFTER Stage 4. A scores parquet + // stands an input in because such a task never opens the data file; it + // stands in for nothing at all for --task SpectraCache or PerFileScoring, + // whose whole product is decoded FROM that file. Without this term a + // mistyped or moved input on those tasks proceeds on a leftover parquet + // and logs that the run will be read from its scores parquet, "which is + // what a task after Stage 4 needs" - false for exactly the two tasks that + // could reach it. The old `if (!fromInputScores)` wrapper could not reach + // them structurally; nothing re-established that scoping when it went. + // + // EITHER parquet, named. WHICH one a task reads is that task's question + // (ScoringTaskShared.ReadsReconciledScores) and this runs before dispatch: + // a FirstPassFDR node is shipped .scores.parquet, a SecondPassFDR + // node only .scores-reconciled.parquet. + if (ScoringTaskShared.StartsAfterPerFileScoring(config) && + (File.Exists(ParquetScoreCache.GetScoresPath(inputFile)) || + File.Exists(ParquetScoreCache.GetReconciledScoresPath(inputFile)))) { - LogInfo(string.Format( - "{0} of {1} input(s) are absent but have a spectra cache; reading those from the cache.", - cacheOnlyInputs, config.InputFiles.Count)); + artifactOnlyInputs++; + continue; } + LogError(string.Format( + "Input file not found, and it has neither a spectra cache nor a scores " + + "parquet to stand in for it: {0}", inputFile)); + return 1; + } + // Announced, not silent: a run whose sources are gone cannot rebuild a + // cache that turns out to be wrong, so the log is the only provenance. + if (cacheOnlyInputs > 0) + { + LogInfo(string.Format( + "{0} of {1} input(s) are absent but have a spectra cache; reading those from the cache.", + cacheOnlyInputs, config.InputFiles.Count)); + } + if (artifactOnlyInputs > 0) + { + LogInfo(string.Format( + "{0} of {1} input(s) are absent and have no spectra cache; reading those from " + + "their scores parquet, which is what a task after Stage 4 needs.", + artifactOnlyInputs, config.InputFiles.Count)); } if (config.LibrarySource != null && !File.Exists(config.LibrarySource.Path)) { @@ -262,7 +293,7 @@ static int Main(string[] args) // writes --output.) if (config.SelectedTask == HpcTask.SpectraCache) LogInfo("Output: per-file .spectra.bin (no scoring; --output and --library are not used)"); - else if (config.NoJoin && !fromInputScores) + else if (config.NoJoin && config.SelectedTask == HpcTask.PerFileScoring) LogInfo("Output: per-file .scores.parquet (next to each input file)"); else if (config.DiagnosticsOnly) { @@ -355,7 +386,7 @@ static int Main(string[] args) } // Single entry point. The rescore worker (--task - // PerFileRescore, with --input-scores) includes only + // PerFileRescoring) includes only // PerFileRescoreTask (OspreyTask.IsIncluded); PerFileScoring's // lazy-rehydrate (via ctx.Demand) populates the upstream state // from the boundary files on disk. @@ -518,16 +549,49 @@ private static string TaskCliName(HpcTask task) /// /// Validate the parsed config against the selected /// (or the default full pipeline - /// when none was given). When a --task is selected the task is - /// authoritative: it dictates the input type, and the cross - /// (e.g. --task PerFileScoring --input-scores) is rejected rather - /// than silently dispatching the other task. Returns null on success or - /// an error message string on failure. Does not log warnings (those stay - /// in ). Internal so Osprey.Test can exercise it. + /// when none was given). Every task takes the SAME input kind now - the data + /// files, named with -i or --input-list - so what is validated is + /// presence and count, not kind. The cross this used to reject + /// (--task PerFileScoring --input-scores) cannot be expressed any more, + /// which is the point of retiring the second seam rather than teaching a third + /// predicate about it. Returns null on success or an error message string on + /// failure. Does not log warnings (those stay in ). Internal so + /// Osprey.Test can exercise it. + /// + /// + /// An error naming every input stem that appears more than once, with the paths that + /// collide, or null when all stems are distinct. Ordinal comparison, matching the + /// per-run maps this protects. /// + private static string DuplicateInputStemError(IReadOnlyList inputFiles) + { + var byStem = new Dictionary>(StringComparer.Ordinal); + foreach (string input in inputFiles) + { + string stem = Path.GetFileNameWithoutExtension(input) ?? string.Empty; + if (!byStem.TryGetValue(stem, out var paths)) + { + paths = new List(); + byStem[stem] = paths; + } + paths.Add(input); + } + var collisions = byStem.Where(kv => kv.Value.Count > 1).ToList(); + if (collisions.Count == 0) + return null; + var sb = new StringBuilder(); + sb.AppendFormat( + "{0} input stem(s) appear more than once. Every per-run artifact is named " + + "., so runs sharing a stem cannot be told apart and would " + + "overwrite each other's parquets and sidecars. Rename or stage them so each " + + "run has a distinct file name:", collisions.Count); + foreach (var kv in collisions) + sb.AppendFormat("\n '{0}': {1}", kv.Key, string.Join(", ", kv.Value)); + return sb.ToString(); + } + internal static string ValidateArgs(OspreyConfig config) { - bool hasInputScores = config.InputScores != null && config.InputScores.Count > 0; bool hasInputFiles = config.InputFiles != null && config.InputFiles.Count > 0; // OSPREY_EXPERIMENT_AGG family, before any I/O. Checked here rather than at the @@ -535,10 +599,31 @@ internal static string ValidateArgs(OspreyConfig config) // large run spends reaching FirstPassFDR, and so a warm resume - which skips // FirstPassFdrTask.Run entirely - is still checked. string aggErr = OspreyEnvironment.ValidateExperimentAggSettings( - ExperimentAggFileCount(config, hasInputScores, hasInputFiles)); + ExperimentAggFileCount(config, hasInputFiles)); if (aggErr != null) return aggErr; + // Every run is keyed on its input STEM - the per-file artifacts are + // ., and every per-run map in the pipeline is keyed the same way - + // so two inputs sharing a stem are two runs the pipeline cannot tell apart. It is + // not exotic: --input-list makes it routine at cohort scale, where the same + // acquisition name recurs under different directories. + // + // Refused here rather than surviving to be discovered downstream, where it takes + // two shapes and neither says what happened. Without --output-dir the join appends + // two rows under one key while the parquet map keeps only the second, and + // CurrentReconciledPaths dies with "An item with the same key has already been + // added" mid-Stage-6/7. WITH --output-dir it is worse and silent: both stems + // resolve into the same directory, so the two runs share one .scores.parquet and + // one .scores-reconciled.parquet, each overwriting the other, with no error at all. + // The retired --input-scores form made stems unique by construction. + if (hasInputFiles) + { + string dupErr = DuplicateInputStemError(config.InputFiles); + if (dupErr != null) + return dupErr; + } + if (config.SelectedTask.HasValue) { switch (config.SelectedTask.Value) @@ -548,20 +633,12 @@ internal static string ValidateArgs(OspreyConfig config) // does NOT require --library: caching depends only on the // input file, and demanding one would make staging a dataset // wait on a library that is often chosen later. - if (hasInputScores) - { - return "--task SpectraCache takes -i , not --input-scores " + - "(it builds spectra caches from raw inputs, not from scores)."; - } if (!hasInputFiles) return "--task SpectraCache requires --input ."; return null; case HpcTask.PerFileScoring: // Stage 1-4 worker: mzML in, per-file .scores.parquet out. - if (hasInputScores) - return "--task PerFileScoring takes -i , not --input-scores " + - "(did you mean --task PerFileRescoring?)."; if (!hasInputFiles) return "--task PerFileScoring requires --input ."; if (config.LibrarySource == null) @@ -569,32 +646,31 @@ internal static string ValidateArgs(OspreyConfig config) return null; case HpcTask.PerFileRescore: - // Stage 6 worker: --input-scores in, reconciled per-file out. - if (hasInputFiles) - return "--task PerFileRescoring takes --input-scores, not -i " + - "(mzML paths are derived from the parquet stems)."; - if (!hasInputScores) - return "--task PerFileRescoring requires --input-scores ."; + // Stage 6 worker: one run's scores in, its reconciled parquet out. + // Named by its DATA file like every other task; the parquet and + // sidecars are derived from the stem, and the data file itself need + // not exist (Main's input check accepts a run whose scores are on + // disk). + if (!hasInputFiles) + return "--task PerFileRescoring requires --input ."; if (config.LibrarySource == null || string.IsNullOrEmpty(config.OutputBlib)) return "--task PerFileRescoring requires --library and --output."; return null; case HpcTask.FirstPassFdr: - if (hasInputFiles) - return "--task FirstPassFDR cannot be combined with --input. Use --input-scores instead."; - if (!hasInputScores) - return "--task FirstPassFDR requires --input-scores ."; + if (!hasInputFiles) + return "--task FirstPassFDR requires --input ."; if (config.LibrarySource == null || string.IsNullOrEmpty(config.OutputBlib)) return "--task FirstPassFDR requires --library and --output."; // FirstPassFDR writes the Stage 5 → Stage 6 boundary file // pair, only meaningful with 2+ siblings to reconcile // against and reconciliation enabled. Reject early. - if (config.InputScores.Count < 2) + if (config.InputFiles.Count < 2) return string.Format( - "--task FirstPassFDR requires --input-scores with 2+ parquet files " + - "(got {0}). The Stage 5 → Stage 6 boundary file pair is only meaningful for " + + "--task FirstPassFDR requires --input with 2+ files " + + "(got {0}). The Stage 5 -> Stage 6 boundary file pair is only meaningful for " + "multi-file fan-back-in.", - config.InputScores.Count); + config.InputFiles.Count); if (!config.Reconciliation.Enabled) return "--task FirstPassFDR requires Reconciliation.Enabled = true " + "(got false from config). The Stage 5 → Stage 6 boundary file pair is " + @@ -602,26 +678,17 @@ internal static string ValidateArgs(OspreyConfig config) return null; case HpcTask.SecondPassFdr: - if (hasInputFiles) - return "--task SecondPassFDR cannot be combined with --input. Use --input-scores instead."; - if (!hasInputScores) - return "--task SecondPassFDR requires --input-scores ."; + if (!hasInputFiles) + return "--task SecondPassFDR requires --input ."; if (config.LibrarySource == null || string.IsNullOrEmpty(config.OutputBlib)) return "--task SecondPassFDR requires --library and --output."; return null; } } - // No --task: the full pipeline, started from either -i mzML or - // --input-scores (PerFileScoring lazy-rehydrates the supplied scores). - if (hasInputScores) - { - if (hasInputFiles) - return "--input-scores cannot be combined with --input. Use one or the other."; - if (config.LibrarySource == null || string.IsNullOrEmpty(config.OutputBlib)) - return "--input-scores requires --library and --output."; - return null; - } + // No --task: the full pipeline. A cold run scores from Stage 1; a resume over a + // directory that already holds each run's artifacts skips to whichever stage is + // outstanding, which the per-task validity sidecars decide - not the input kind. if (!hasInputFiles) return "No input files specified. Use -i [file2.mzML ...]"; if (config.LibrarySource == null) @@ -638,8 +705,7 @@ internal static string ValidateArgs(OspreyConfig config) /// never compute an experiment-wide score, so reporting their input count would refuse /// every worker of a legitimate distributed mean(best-N) run. /// - private static int ExperimentAggFileCount( - OspreyConfig config, bool hasInputScores, bool hasInputFiles) + private static int ExperimentAggFileCount(OspreyConfig config, bool hasInputFiles) { switch (config.SelectedTask) { @@ -648,65 +714,9 @@ private static int ExperimentAggFileCount( case HpcTask.PerFileRescore: return 0; } - if (hasInputScores) - return config.InputScores.Count; return hasInputFiles ? config.InputFiles.Count : 0; } - /// - /// Expand --input-scores arguments: a single directory becomes the - /// non-recursive list of *.scores.parquet files in it; explicit file - /// paths are passed through unchanged. Throws if the directory is - /// empty or any explicit path doesn't exist. - /// - /// Directory mode collects both the Stage 4 *.scores.parquet files - /// and the Stage 6 *.scores-reconciled.parquet siblings, then - /// dedupes per stem: for any stem that has both, only the reconciled file - /// is returned (the authoritative later pass; the --task SecondPassFDR - /// reconciled-input gate expects reconciled parquets). A stem with only an - /// original is returned as-is. The two suffixes are unambiguous, so this - /// never returns both files for one stem (see - /// ). - /// - internal static List ResolveInputScores(List paths) - { - if (paths == null || paths.Count == 0) - throw new ArgumentException("--input-scores requires at least one path."); - - if (paths.Count == 1 && Directory.Exists(paths[0])) - { - string dir = paths[0]; - // Glob *.parquet and classify by suffix in code rather than - // relying on multi-dot search-pattern matching (which differs - // across platforms). Keep only the two known scores suffixes. - var originals = new List(); - var reconciledSet = new HashSet(StringComparer.Ordinal); - foreach (string f in Directory.GetFiles(dir, "*.parquet", SearchOption.TopDirectoryOnly)) - { - if (ParquetScoreCache.IsReconciledScoresPath(f)) - reconciledSet.Add(f); - else if (f.EndsWith(ParquetScoreCache.ScoresParquetSuffix, StringComparison.Ordinal)) - originals.Add(f); - } - if (originals.Count == 0 && reconciledSet.Count == 0) - throw new ArgumentException(string.Format( - "No *.scores.parquet files found in --input-scores directory: {0}", dir)); - var result = new List(reconciledSet); // reconciled: authoritative - foreach (string f in originals) - if (!reconciledSet.Contains(ParquetScoreCache.ReconciledPathFromScoresPath(f))) - result.Add(f); // original with no reconciled sibling - result.Sort(StringComparer.Ordinal); // Array.Sort OK: unique filenames, so the comparator never ties - return result; - } - - foreach (string p in paths) - { - if (!File.Exists(p)) - throw new ArgumentException(string.Format("--input-scores path not found: {0}", p)); - } - return paths; - } - internal static void LogInfo(string message) { OspreyOutput.Out.WriteLine(message); diff --git a/pwiz_tools/Osprey/Osprey/RescoreWorker.cs b/pwiz_tools/Osprey/Osprey/RescoreWorker.cs index ff7a0221df..846dd3f8de 100644 --- a/pwiz_tools/Osprey/Osprey/RescoreWorker.cs +++ b/pwiz_tools/Osprey/Osprey/RescoreWorker.cs @@ -73,15 +73,15 @@ namespace pwiz.Osprey public static class RescoreWorker { /// - /// Run the per-file rescore worker on the boundary files - /// referenced by . + /// Run the per-file rescore worker on the boundary files beside the run named by + /// . /// Returns 0 on success, non-zero on failure. /// public static int Run(OspreyConfig config) { // Phase C: the worker is now an alias for the canonical // pipeline entry. The driver runs only the included tasks - // (OspreyTask.IsIncluded): a NoJoin+InputScores config includes + // (OspreyTask.IsIncluded): a --task PerFileRescoring config includes // PerFileRescoreTask, while PerFileScoringTask's probe-the-disk // joinOnly Rehydrate (reached via ctx.Demand) hydrates the upstream // state (stubs, 1st-pass overlay, reconciliation actions, refined diff --git a/pwiz_tools/Osprey/README.md b/pwiz_tools/Osprey/README.md index deb218fc61..2bb2e9f2de 100644 --- a/pwiz_tools/Osprey/README.md +++ b/pwiz_tools/Osprey/README.md @@ -109,9 +109,16 @@ fan-out boundaries into four single-task workers — one node = one | `--task` | shape | reads | writes (next to the input) | |----------|-------|-------|-----------------------------| | `PerFileScoring` | split 1 — per file | mzML (`-i`) + library (`-l`) | `.scores.parquet`, `.calibration.json` | -| `FirstPassFDR` | join 1 — all files | every `.scores.parquet` (`--input-scores`) | `.1st-pass.fdr_scores.bin`, `.reconciliation.json` | +| `FirstPassFDR` | join 1 — all files | every `.scores.parquet` | `.1st-pass.fdr_scores.bin`, `.reconciliation.json` | | `PerFileRescoring` | split 2 — per file | `.scores.parquet` + co-located `.1st-pass.fdr_scores.bin`, `.reconciliation.json` | `.scores-reconciled.parquet` | -| `SecondPassFDR` | join 2 — all files | every `.scores-reconciled.parquet` (`--input-scores`) | `.blib` (+ `.2nd-pass.fdr_scores.bin` when protein FDR is on) | +| `SecondPassFDR` | join 2 — all files | every `.scores-reconciled.parquet` | `.blib` (+ `.2nd-pass.fdr_scores.bin` when protein FDR is on) | + +Every task names its runs with `-i` / `--input-list`, giving the **data files**, and derives +each run's parquet and sidecars from the input stem plus `--output-dir`. Which parquet a task +reads is a property of the task — `FirstPassFDR` and `PerFileRescoring` read `.scores.parquet`, +`SecondPassFDR` reads `.scores-reconciled.parquet` — not of what happens to be in the +directory. The data file itself need not still exist: a node whose `.spectra.bin` or scores +parquet is staged is accepted without it. The driver also writes a `..osprey.task` validity sidecar next to each output; re-running a task whose outputs already exist @@ -128,17 +135,22 @@ Osprey -i *.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 Osprey --task PerFileScoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 # -> s1.scores.parquet, s1.calibration.json (next to s1.mzML; -o is ignored here) -# Join 1 — FirstPassFDR, one process over ALL parquets (pass a DIRECTORY so order is fixed): -Osprey --task FirstPassFDR --input-scores ./scores_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +# Join 1 — FirstPassFDR, one process over ALL runs (name them in a fixed order): +Osprey --task FirstPassFDR -i s1.mzML -i s2.mzML -i s3.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +# reads s1.scores.parquet, s2..., s3... # -> .1st-pass.fdr_scores.bin, .reconciliation.json (next to each parquet) # Split 2 — PerFileRescoring, one process per file (parquet + its two sidecars co-located): -Osprey --task PerFileRescoring --input-scores s1.scores.parquet -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +Osprey --task PerFileRescoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 # -> s1.scores-reconciled.parquet -# Join 2 — SecondPassFDR, one process over ALL reconciled parquets (DIRECTORY again): -Osprey --task SecondPassFDR --input-scores ./reconciled_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 -# -> out.blib +# Join 2 — SecondPassFDR, one process over ALL runs (same order): +Osprey --task SecondPassFDR -i s1.mzML -i s2.mzML -i s3.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +# reads s1.scores-reconciled.parquet, s2..., s3... -> out.blib + +# Past a few hundred runs, --input-list takes one path per line and composes with -i +# (446 -i paths measured ~28,600 characters against a 32,767 command-line limit): +Osprey --task SecondPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 ``` The example above shows the *commands*. What each node must actually be **shipped** at each @@ -151,14 +163,16 @@ often proceed without them and produce a plausible wrong answer - is the relay c - **Same parameters on every task.** Pass an identical `-l ` and identical search flags (`--resolution`, `--protein-fdr`, ...) to all four tasks. The parquet integrity check (`osprey.search_hash` footer - metadata) rejects `--input-scores` files whose search/library hash does - not match the current invocation. -- **`--input-scores` ordering is significant.** A *directory* argument is - globbed and sorted internally (deterministic). An explicit *file list* - is consumed in the order given. FirstPassFDR reconciliation is - order-sensitive, so for `FirstPassFDR` and `SecondPassFDR` pass a directory or a + metadata) rejects parquets whose search/library hash does not match the + current invocation. +- **Input ORDER is significant, and it is now yours.** Runs are consumed in + the order given to `-i` / `--input-list`. FirstPassFDR reconciliation is + order-sensitive, so for `FirstPassFDR` and `SecondPassFDR` pass a deterministically sorted list — a workflow engine's channel order is - otherwise nondeterministic and would cause run-to-run drift. + otherwise nondeterministic and would cause run-to-run drift. The retired + `--input-scores` sorted a globbed directory on your behalf; naming the runs + means the order is stated rather than inherited from a directory listing, + and a stray parquet in that directory can no longer change the cohort. - **Outputs land next to inputs; sidecars travel with the parquet.** Each task writes its outputs and sidecars beside the input file (not into cwd or a separate output dir). `PerFileRescoring` rehydrates from @@ -171,7 +185,7 @@ often proceed without them and produce a plausible wrong answer - is the relay c several files concurrently in one process) and would double-parallelize under a scheduler. - **`--help` is the authoritative flag reference** (`Osprey --help`), - with a Distributed / HPC group covering `--task` and `--input-scores`. + with a Distributed / HPC group covering `--task`, `-i` and `--input-list`. - **Exit codes**: a failing task returns a non-zero process exit code, so a workflow engine can gate on it normally. diff --git a/pwiz_tools/Osprey/docs/00-pipeline-architecture.md b/pwiz_tools/Osprey/docs/00-pipeline-architecture.md index 3897730ebb..0c6c1ece6c 100644 --- a/pwiz_tools/Osprey/docs/00-pipeline-architecture.md +++ b/pwiz_tools/Osprey/docs/00-pipeline-architecture.md @@ -32,7 +32,7 @@ layer, and none repeats another: |---|---|---| | **00** (this doc) | Scope, contract, principles, relay | Which file, whose, when, and who may read it | | [14-intermediate-files](14-intermediate-files.md) | Bytes | Headers, versions, schemas, hashing, invalidation mechanics | -| [15-hpc-scoring-split](15-hpc-scoring-split.md) | Operations | CLI flags, `--input-scores` ordering, orchestration recipes | +| [15-hpc-scoring-split](15-hpc-scoring-split.md) | Operations | CLI flags, how a task names its runs and in what order, orchestration recipes | If you are asking "what does this file's header look like?", you want 14. "How do I launch the third worker?" is 15. "Is this task allowed to read that file?" is here. @@ -150,7 +150,7 @@ as the worked one, because for a long time every step in it *was* a fold and the held the pool. The fragment release, the pass-2 competition, protein parsimony, the experiment-q re-clamp and all three `.blib` gates each reduce to `O(distinct)` and each visits every run - but the stage was **handed** every run's survivors before the first of -them started, by the `--input-scores` merge, so nothing they did could bring the peak down. +them started, by the `--task SecondPassFDR` merge, so nothing they did could bring the peak down. At 446 CHS runs that load reached 68.0 GB and was killed at run 381 with 0.34 GB free, having computed nothing. **A fold does not bound anything unless its SOURCE is per-run too**: the runs are now rebuilt one at a time from their own @@ -621,12 +621,12 @@ node running that task needs a copy, whatever batch it was handed. | `.libcache` | experiment cache | library load, any task | all tasks | rebuild locally | | `.spectra.bin` | per-run cache | `PerFileScoring` (Stage 2), or `--task SpectraCache` | `PerFileScoring`, `PerFileRescoring` | with the run | | `.calibration.json` | per-run product | `PerFileScoring` (Stage 3) | `PerFileScoring`, `PerFileRescoring`, `FirstPassFDR`, `SecondPassFDR` | with the run, on **every** leg | -| `.scores.parquet` | per-run product | `PerFileScoring` (Stage 4) | `FirstPassFDR`, `PerFileRescoring`, **`SecondPassFDR`** (fallback for runs with no reconciled sibling) | with the run | +| `.scores.parquet` | per-run product | `PerFileScoring` (Stage 4) | `FirstPassFDR`, `PerFileRescoring` | with the run | | `.1st-pass.fdr_scores.bin` | per-run product | `FirstPassFDR` (pass 1) | `PerFileRescoring`; `SecondPassFDR` only under `OSPREY_PASS2_VERIFY_WORKER` or where no worker answer exists | with the run | | `.reconciliation.json` | per-run product | `FirstPassFDR` (Stage 6 planning) | `PerFileRescoring`, `SecondPassFDR` (gap-fill entry ids) | with the run | | `.1st-pass.fdr_experiment.bin` | experiment product | `FirstPassFDR` | `PerFileRescoring`, `SecondPassFDR`, `PerFileScoring` (rehydrate) | **every node** | | `.1st-pass.model.json` | experiment product, replicated | `FirstPassFDR` (training) | `PerFileRescoring`, `SecondPassFDR` | **every node** (any one copy) | -| `.scores-reconciled.parquet` | per-run product | `PerFileRescoring` (Stage 6) | `SecondPassFDR`, and `PerFileRescoring` itself on its per-run resume arm | with the run | +| `.scores-reconciled.parquet` | per-run product, written for **every** run | `PerFileRescoring` (Stage 6) | `SecondPassFDR` - the join's only row source, one parquet per run; and `PerFileRescoring` itself on its per-run resume arm | with the run | | `.2nd-pass.fdr_decoys.bin` | per-run product | `PerFileRescoring` (pass-2 worker) | `SecondPassFDR` | with the run | | `.2nd-pass.fdr_scores.bin` | per-run product | `PerFileRescoring` (pass-2 worker), else `SecondPassFDR` | `SecondPassFDR` | with the run | | `.2nd-pass.fdr_experiment.bin` | experiment product | `SecondPassFDR` | `SecondPassFDR` on a resume | n/a | @@ -852,7 +852,7 @@ regardless, and adding a third path to a hash would narrow it further. A warm resume across builds is a separate matter: the version stamp is compared for exact equality (`YEAR.ORDINAL.BRANCH.DOY`) - **but only where it is checked, which is narrower -than it sounds.** That comparison guards the `--input-scores` parquet load. The +than it sounds.** That comparison guards the per-run parquet load. The `.osprey.task` resume path does not do it: `TaskValiditySidecar.IsValid` compares the `validity_key` only, and the `version` field it records is provenance. No version component is in the base key either. So re-invoking the same straight-through command line the next @@ -981,7 +981,7 @@ are functions of all runs: - `.scores.parquet` for **every** run in the cohort - `.calibration.json` for **every** run -- the library, and `--input-scores` naming the parquets +- the library, and `-i` naming the runs whose parquets it reads `.calibration.json` must travel, which is easy to get wrong because the join reads parquets rather than spectra. It supplies RT calibration and the isolation-scheme windows @@ -1050,6 +1050,16 @@ inputs. The run log says which shape it took - "folding over N run(s), each rebu own artifacts and dropped" - and that line is the evidence, because a resident pool and a fold produce identical output and differ only in a memory profile. +**One parquet per run, and it is the reconciled one.** `.scores.parquet` is not an +input to this boundary in any form - not as a fallback, not for a run Stage 6 did no work on. +Stage 6 writes a reconciled parquet for *every* run (P13; `WriteUnchangedReconciled` covers +the no-work run), so a missing one means the write never landed and the run is not finished. +Substituting the Stage 4 file would put 1st-pass boundaries and no gap-fill rows into the +blib for that run from a process that exits 0, which is exactly the ambiguity P13 exists to +remove - so every consumer here **fails** on absence instead. The rule survived one earlier +round as "read the reconciled parquet, Stage 4's only as the per-file fallback"; the fallback +half is retired, and the code carries no path to it. + Not needed on the default path: `.1st-pass.fdr_scores.bin`. Establishing that is what issue #4486 was for - an orchestrator hands a `SecondPassFDR` node the per-run second-pass artifacts and the analysis-wide experiment sidecar, and nothing per-run from @@ -1115,17 +1125,34 @@ the text says so rather than describing the current shape as though it were the survivors instead of rebuilding one run at a time through `StreamFiles`. Both arms are required to produce identical bytes. - **It is NOT the in-place A/B its Stage 6 sibling is, and must not be described as one.** - `CanStreamStage7Join` short-circuits on `!config.ExpectReconciledInput` *before* it reads - the switch, and that flag is set only for `--task SecondPassFDR`. So on a straight-through - run the switch changes nothing - while `SecondPassFdrTask.ValidityKey` appends - `;stage7stream=0` unconditionally, invalidating the `.blib` and every 2nd-pass sidecar and - forcing a full Stage 7 re-run for a setting that cannot change the arm. Comparing the two - shapes means comparing two `--task SecondPassFDR` runs over the same linked bed. + **It IS the in-place A/B its Stage 6 sibling is, and it did not used to be.** + `CanStreamStage7Join` opened on `!config.ExpectReconciledInput`, a flag only + `--task SecondPassFDR` sets, so on a straight-through run the switch changed nothing - + while `SecondPassFdrTask.ValidityKey` appended `;stage7stream=0` regardless, forcing a + full Stage 7 re-run for a setting that could not change the arm. That term is now the + question it stood in for: does every run have a `.scores-reconciled.parquet` on disk in + the survivor-subset shape (`ScoringTaskShared.AllReconciledParquetsCurrent`). Asked of + the disk, it is route-independent - a straight-through run's Stage 6 has just written + those parquets - so the cold run, both resume arms and the `--task SecondPassFDR` merge + all fold run by run, and the switch compares two arms of whichever one you are running. + + The per-run source is not one implementation reached four ways: each arm hands the fold + the per-file half of the whole-run loop it would otherwise have run + (`PerFileRescoreTask.BuildRunPerRunSource` / `BuildResumePerRunSource` / + `BuildStage7PerRunSource`), so run-at-a-time is the same work in the same order as + all-runs-at-once. That is why the arms are required to produce identical bytes, and why + an arm is a call-shape change rather than a second algorithm. + + One route still cannot stream: a pass-2 mode whose per-file half has no worker + (`OSPREY_PASS2_QVALUE=transfer` still competes over the whole pool in Stage 7). Until + `TransferOneFile` moves into `Pass2PerFileWorker`, `Stage7ResidentGuardError` keeps its + `streamingAvailable` exemption - a run with no streamed alternative has no choice for a + token to record. Because nothing in the output distinguishes the arms, the shape that ran is asserted from - the marker line `Second-pass join: folding over N run(s)` rather than inferred - which is - what mode 3 does, scoped to the configurations that can actually stream. + the marker line `Second-pass join: folding over N run(s)` rather than inferred - + `regression.ps1` demands it per leg (the cold run, both resumes, and mode 3's phase 4), + scoped to the configurations that can actually stream. 5. **Whether the 500-run / 64 GB target is met.** It is not yet, and which stage binds is itself moving as each is fixed. The two TODOs above carry the current measurements; diff --git a/pwiz_tools/Osprey/docs/11-boundary-overrides.md b/pwiz_tools/Osprey/docs/11-boundary-overrides.md index b2faa42d0f..a87c5ff788 100644 --- a/pwiz_tools/Osprey/docs/11-boundary-overrides.md +++ b/pwiz_tools/Osprey/docs/11-boundary-overrides.md @@ -291,7 +291,7 @@ computed by Stage 6 planning. The flags that affect this stage: | Flag / field | Default | Effect on this stage | |--------------|---------|----------------------| | `--task {PerFileScoring\|FirstPassFDR\|PerFileRescoring\|SecondPassFDR}` | (in-process, all stages) | `PerFileRescoring` runs this stage as a standalone worker (internal `HpcTask.PerFileRescore`). `SecondPassFDR` (`HpcTask.SecondPassFdr`) rehydrates reconciled parquets instead of re-scoring. | -| `--input-scores ` | — | Supplies the boundary `.scores.parquet` files the worker rescores; drives `IsIncluded` (`PerFileRescoreTask.cs:123`). | +| `-i ` | — | Names the run the worker rescores; its boundary `.scores.parquet` and sidecars derive from the stem. Membership is `--task` alone (`PerFileRescoreTask.IsIncluded`). | | `--reconciliation-compaction-fdr ` | 0.01 (`OspreyConfig.ReconciliationCompactionFdr`) | First-pass compaction predicate applied upstream in FirstPassFDR; determines which entries survive into the rescore set. | | `ReconciliationConfig.Enabled` | true | Gates reconciliation planning + `reconciliation.json` inputs (`PerFileRescoreTask.cs:158`). Disabling leaves only multi-charge consensus rescore. | | `ReconciliationConfig.ConsensusFdr` | 0.01 (`ReconciliationConfig.cs:39`) | Threshold for consensus peptide selection, calibration refit, and reconciliation planning (`Stage6Planner.cs`). Not a CLI flag; config field. | diff --git a/pwiz_tools/Osprey/docs/14-intermediate-files.md b/pwiz_tools/Osprey/docs/14-intermediate-files.md index ae3ecb1fdf..fde3284581 100644 --- a/pwiz_tools/Osprey/docs/14-intermediate-files.md +++ b/pwiz_tools/Osprey/docs/14-intermediate-files.md @@ -312,11 +312,18 @@ Stage 6 (`PerFileRescoreTask`) writes `.scores-reconciled.parquet` (`GetReconciledScoresPath`, `ParquetScoreCache.cs:1055`) rather than overwriting the Stage 4 `.scores.parquet`. The `.scores-reconciled.parquet` suffix is appended **after** the `.scores` token so it is an unambiguous "Stage 6 output" signal (`ParquetScoreCache.cs:1036`). -`EffectiveScoresPathFromScoresPath` (`ParquetScoreCache.cs:1103`) is the read-side contract: a -post-Stage-6 reader consumes the reconciled sibling when it exists on disk, else the original — -making the split-file design byte-equivalent to the former in-place overwrite while surviving a -partial Stage 6 crash. This is a C# infrastructure refinement over the Rust doc's single-file -model. +The read-side contract is `ScoringTaskShared.ReadsReconciledScores`: which of the two a reader +consumes is decided by the **task**, not by which file happens to be on disk. `SecondPassFDR` +reads the reconciled parquet; `FirstPassFDR` and `PerFileRescoring` read the Stage 4 file. +This is a C# infrastructure refinement over the Rust doc's single-file model, and it survives a +partial Stage 6 crash. + +It was a disk probe until 2026-09-08 - take the reconciled sibling wherever it exists, else the +original - which reads as equivalent and is not. It is right only while the stages run in order, +because the artifact is absent before Stage 6 and present after; re-run `--task FirstPassFDR` +over a completed directory and the same probe hands the FIRST pass the survivor SUBSET, with +every version, search and library hash matching. The task always knew which artifact it wanted; +the probe was inferring it from a side effect. --- @@ -667,9 +674,9 @@ default resume mechanism. - **[INTENTIONAL-CSHARP-DESIGN] Reconciled parquet is a separate `.scores-reconciled.parquet`, not an in-place overwrite** - Rust doc's model rewrites `.scores.parquet` in place during Stage - 6; C# writes a distinct sibling and selects it on read via - `EffectiveScoresPathFromScoresPath`, surviving a partial Stage 6 crash. Evidence: - `ParquetScoreCache.cs:1036,1055,1103`; `ReconciledParquetWriter.cs`. Severity: minor. + 6; C# writes a distinct sibling and selects it on read by task membership + (`ScoringTaskShared.ReadsReconciledScores`), surviving a partial Stage 6 crash. Evidence: + `ParquetScoreCache.cs`; `ReconciledParquetWriter.cs`. Severity: minor. - **[INTENTIONAL-CSHARP-DESIGN] FDR sidecar loader matches records by `entry_id`, tolerating `count < entries.len()`** - Rust doc says `entry_count` must equal `entries.len()` and records diff --git a/pwiz_tools/Osprey/docs/15-hpc-scoring-split.md b/pwiz_tools/Osprey/docs/15-hpc-scoring-split.md index 25edc1d2cf..15540828b9 100644 --- a/pwiz_tools/Osprey/docs/15-hpc-scoring-split.md +++ b/pwiz_tools/Osprey/docs/15-hpc-scoring-split.md @@ -6,7 +6,7 @@ For large experiments (hundreds to thousands of mzML files) the Osprey pipeline The C# port implements this split as **four pipeline tasks** driven by a single `--task ` CLI selector, rather than the Rust doc's `--no-join` / `--join-at-pass` / `--join-only` flag family. Each task is a subclass of `OspreyTask` (`Osprey.Tasks/OspreyTask.cs`), and the orchestration model is a per-task membership predicate walked by a driver loop (`Osprey/AnalysisPipeline.cs:99-112`) rather than a contiguous `[start..stop]` stage window. -> **This document owns operations**: CLI flags, the membership truth table, `--input-scores` resolution and ordering, footer-hash validation, and concurrency. *Why* the split has this shape - the scope of each artifact, which task may read what, and the exact file list a node must be shipped at each boundary - is owned by [00-pipeline-architecture.md](00-pipeline-architecture.md), and the byte formats by [14-intermediate-files.md](14-intermediate-files.md). Read 00 before changing what any task writes. +> **This document owns operations**: CLI flags, the membership truth table, how a task names its runs and in what order, footer-hash validation, and concurrency. *Why* the split has this shape - the scope of each artifact, which task may read what, and the exact file list a node must be shipped at each boundary - is owned by [00-pipeline-architecture.md](00-pipeline-architecture.md), and the byte formats by [14-intermediate-files.md](14-intermediate-files.md). Read 00 before changing what any task writes. > **In flight** - this document describes `--task PerFileRescoring` as rehydrating `FirstPassFdrTask` and reading an all-runs `CompactedEntries` buffer (the membership truth table below, and the Stage 6 section). That is what the branch `Skyline/work/20260901_osprey_firstpass_resume` replaces with a per-run hydrate, so both statements change when it lands. **Deviations from the target architecture are tracked in one place - 00's `## In flight` section - not per document**; this note exists so a reader of 15 alone knows to look there. @@ -53,11 +53,11 @@ The exact per-task membership per mode is pinned by `Osprey.Test/PipelineMembers | straight-through (no `--task`, `-i mzML`) | run | run | run | run | | `--task PerFileScoring` (`NoJoin`) | run | – | – | – | | `--task FirstPassFDR` (`StopAfterStage5`) | rehydrate | run | – | – | -| `--task PerFileRescoring` (`NoJoin`+`InputScores`) | rehydrate | rehydrate | run | – | +| `--task PerFileRescoring` (`NoJoin`, `SelectedTask`) | rehydrate | rehydrate | run | – | | `--task SecondPassFDR` (`ExpectReconciledInput`) | rehydrate | (skipped) | rehydrate | run | -| `--input-scores`, no `--task` (single-node full) | rehydrate | run | run | run | +| `--task ModelDiagnostics` (no flag - sets NONE of the three) | run | run | run | run | -("rehydrate" = excluded from the driver loop but lazily materialized on demand from disk; "–" = never touched.) The predicates live in `PerFileScoringTask.IsIncluded` (`:84-88`), `FirstPassFdrTask.IsIncluded` (`:80-95`), `PerFileRescoreTask.IsIncluded` (`:123-130`), and `SecondPassFdrTask.IsIncluded` (`:57-64`). +("rehydrate" = excluded from the driver loop but lazily materialized on demand from disk; "–" = never touched.) `--task ModelDiagnostics` sets none of the three flags — `StopAfterStage5` has exactly one assignment in the tree and it names `FirstPassFdr` alone — so it is a member of every task, like the straight-through run, and suppresses artifact WRITES rather than membership. It is listed here because a truth-table row claiming otherwise stood in this file and in a unit test. The predicates live in `PerFileScoringTask.IsIncluded` (`:84-88`), `FirstPassFdrTask.IsIncluded` (`:80-95`), `PerFileRescoreTask.IsIncluded` (`:123-130`), and `SecondPassFdrTask.IsIncluded` (`:57-64`). ## Stage 1-4 — Per-file scoring (`--task PerFileScoring`) @@ -67,9 +67,9 @@ The exact per-task membership per mode is pinned by `Osprey.Test/PipelineMembers - `.calibration.json` — RT + MS1/MS2 mass calibration (`CalibrationIO.CalibrationPathForInput`). - `.spectra.bin` — the decoded-spectrum cache. No *join* reads it, but Stage 6 rescore does, and it is what lets a search run at all once the input has been deleted (see 14-intermediate-files.md). -The parquet footer is stamped once against the unmutated outer config (`:226-232`) with `osprey.version`, `osprey.search_hash`, `osprey.library_hash`, and `osprey.reconciled = "false"`. Under `--task PerFileScoring` (`config.NoJoin` with no `--input-scores`) the task stops after writing the parquets and returns false with `ExitCode = 0` (`FinalizeAndCheck`, `:649-658`) — Stage 5+ is skipped, no blib is written. `--output` is accepted but not used (`Osprey/Program.cs:228-232` reports the real per-file parquet output instead of warning). +The parquet footer is stamped once against the unmutated outer config (`:226-232`) with `osprey.version`, `osprey.search_hash`, `osprey.library_hash`, and `osprey.reconciled = "false"`. Under `--task PerFileScoring` (`config.NoJoin`, `SelectedTask == PerFileScoring`) the task stops after writing the parquets and returns false with `ExitCode = 0` (`FinalizeAndCheck`, `:649-658`) — Stage 5+ is skipped, no blib is written. `--output` is accepted but not used (`Osprey/Program.cs:228-232` reports the real per-file parquet output instead of warning). -Under `--task PerFileScoring` the task's `IsIncluded` requires **no** `--input-scores` (`:84-88`); `ValidateArgs` rejects `--task PerFileScoring --input-scores` (`Osprey/Program.cs:357-366`). +Under `--task PerFileScoring` the task's `IsIncluded` is true because the task does not start after Stage 4 (`ScoringTaskShared.StartsAfterPerFileScoring`). The cross `ValidateArgs` used to reject here - `--task PerFileScoring --input-scores` - cannot be typed any more, which is the point of retiring the second seam rather than teaching a third predicate about it. `ProcessFile` always writes the parquet regardless of task, matching Rust's end-to-end behavior (the sidecar is needed by Stage 6 reconciliation to lazy-load CWT candidates). @@ -86,7 +86,7 @@ Under `--task PerFileScoring` the task's `IsIncluded` requires **no** `--input-s The boundary file pair per file is thus `.1st-pass.fdr_scores.bin` + `.reconciliation.json`. Each reconciliation.json carries `search_hash`, `library_hash`, the sorted join-wide file-stem set, and the global first-pass passing base_id set (`:970-996`), so a single-file Stage 6 worker can reconstruct the join-wide compaction set. -Under `--task FirstPassFDR` (`config.StopAfterStage5`), `PlanStage6` writes the boundary pair and returns true with `ExitCode = 0` before Stage 6 rescore (`:775-797`). `IsIncluded` requires `--input-scores` with 2+ parquets (`ValidateArgs`, `Osprey/Program.cs:379-399`) and `Reconciliation.Enabled = true`. +Under `--task FirstPassFDR` (`config.StopAfterStage5`), `PlanStage6` writes the boundary pair and returns true with `ExitCode = 0` before Stage 6 rescore (`:775-797`). `ValidateArgs` requires `--input` with 2+ runs and `Reconciliation.Enabled = true`: the Stage 5 -> Stage 6 boundary pair is only meaningful for multi-file fan-back-in. ## Stage 6 — Per-file rescore (`--task PerFileRescoring`) @@ -96,7 +96,7 @@ Under `--task FirstPassFDR` (`config.StopAfterStage5`), `PlanStage6` writes the Reconciled output goes to a **separate** `.scores-reconciled.parquet` sibling, leaving the Stage 4 `.scores.parquet` intact (`ParquetScoreCache.GetReconciledScoresPath`; `WriteReconciledAndStamp`, `:944-987`). Its footer carries `osprey.reconciled = "true"` plus `osprey.reconciliation_hash` (`Osprey.Tasks/ReconciledParquetWriter.cs:198-205`). This differs from the Rust doc, which says Stage 6 "rewrites each `.scores.parquet`" in place (see Divergences). -Under `--task PerFileRescoring` (`config.NoJoin` + `--input-scores`), `IsIncluded` (`:123-130`) includes only this task; `PerFileScoringTask` and `FirstPassFdrTask` lazy-rehydrate the upstream state from the boundary files via `ctx.Demand`. `RescoreWorker.Run` (`Osprey/RescoreWorker.cs:80-91`) is now a thin alias that just calls `new AnalysisPipeline().Run(config)` — the hand-rolled worker path was collapsed into the canonical driver. `ValidateArgs` forbids `-i` (mzML paths are derived from the parquet stems) and requires `--library` + `--output` (`Osprey/Program.cs:368-377`). +Under `--task PerFileRescoring` (`config.NoJoin`, and `SelectedTask` is what distinguishes it from `PerFileScoring` - the input KIND used to), `IsIncluded` includes only this task; `PerFileScoringTask` and `FirstPassFdrTask` lazy-rehydrate the upstream state from the boundary files via `ctx.Demand`. `RescoreWorker.Run` (`Osprey/RescoreWorker.cs:80-91`) is now a thin alias that just calls `new AnalysisPipeline().Run(config)` — the hand-rolled worker path was collapsed into the canonical driver. `ValidateArgs` requires `--input` (the run this worker rescores, whose parquet and sidecars derive from its stem) plus `--library` + `--output`. ## Stages 7-8 — Second-pass FDR (`--task SecondPassFDR`) @@ -107,25 +107,31 @@ Under `--task PerFileRescoring` (`config.NoJoin` + `--input-scores`), `IsInclude 3. Re-clamp experiment q to best run q (`PercolatorEngine.ClampExperimentQToBestRun`, `:177`). 4. Write the BiblioSpecLite `.blib` (`WriteBlibOutput`, `:299-370`; see 13-blib-output-schema.md). -Under `--task SecondPassFDR` (`config.ExpectReconciledInput`), `Rehydrate` (`:317-455`) hydrates from the reconciled parquets + sidecars **without** materializing `FirstPassFdrTask` (which would wrongly re-run Stage 5 Percolator on the reconciled parquets), applies its own compaction, and lets `SecondPassFdrTask.Run` do 2nd-pass FDR + protein FDR + blib. The strict reconciled-input gate asserts every `--input-scores` parquet carries `osprey.reconciled = "true"` (`ParquetScoreCache.ValidateScoresParquetGroup`, `Osprey.IO/ParquetScoreCache.cs:1292-1305`). `ValidateArgs` forbids `-i` and requires `--library` + `--output` (`Osprey/Program.cs:401-408`). +Under `--task SecondPassFDR` (`config.ExpectReconciledInput`), `Rehydrate` (`:317-455`) hydrates from the reconciled parquets + sidecars **without** materializing `FirstPassFdrTask` (which would wrongly re-run Stage 5 Percolator on the reconciled parquets), applies its own compaction, and lets `SecondPassFdrTask.Run` do 2nd-pass FDR + protein FDR + blib. The strict reconciled-input gate asserts every run's reconciled parquet carries `osprey.reconciled = "true"` (`ParquetScoreCache.ValidateScoresParquetGroup`). `ValidateArgs` requires `--input` plus `--library` + `--output`. `SecondPassFdrTask.Rehydrate` returns `true` as a no-op (`:113`): nothing consumes SecondPassFDR's state in-memory, so it is never demanded. -## Full pipeline (default) and `--input-scores` full run +## Full pipeline (default) -With no `--task`, all four tasks run in one process (`straight-through` row of the truth table): output is identical to running the four workers in sequence over the same files. `--input-scores` with no `--task` (the `input-scores-full` row) runs Stages 5-8 in one process from existing per-file parquets — `PerFileScoringTask` is excluded (`IsIncluded` returns false when `InputScores` is non-empty, `:84-88`) and lazy-rehydrates the supplied scores instead of recomputing them. +With no `--task`, all four tasks run in one process (`straight-through` row of the truth table): output is identical to running the four workers in sequence over the same files. A run whose per-file artifacts are already on disk resumes to whichever stage is outstanding - the per-task validity sidecars decide that, not the input kind. -## `--input-scores` resolution and ordering +The row that used to sit beside it, `--input-scores` with no `--task` (a single-node full run started from parquets), retired with the flag. It was the same run: the pipeline resumes from whatever is current in the output directory either way. -`Program.ResolveInputScores` (`Osprey/Program.cs:446-483`), wired through `ARG_INPUT_SCORES` (`Osprey/OspreyCommandArgs.cs:208-220`): +## How a task names its runs, and ordering -- A single **directory** argument is globbed **non-recursively** for `*.parquet`, then classified by suffix: `*.scores-reconciled.parquet` vs `*.scores.parquet` (`:459-465`). -- **Reconciled wins per stem**: a stem with both files returns only the reconciled parquet (the authoritative later pass); a stem with only the original returns the original (`:469-472`). -- The resulting list is **sorted Ordinal** (`:473`), so the file order is deterministic and stable across nodes. -- Explicit path lists are passed through unchanged after existence validation (`:477-482`); repeated `--input-scores` flags accumulate and re-resolve (`OspreyCommandArgs.cs:211-218`). -- Empty directory or missing explicit path throws (`:466-468`, `:479-480`). +Every task takes `-i` / `--input-list`, naming the **data files**, and derives each run's parquet and sidecars from the input stem plus `--output-dir`. The data file need not still exist: `Program.Main`'s input check accepts a run whose `.spectra.bin` is on disk (delete-the-sources-after-caching), and also one whose `.scores.parquet` - or its reconciled sibling - is on disk, which is the state a staged worker directory is in. -At pipeline entry, `AnalysisPipeline.Run` synthesizes `config.InputFiles` from the parquet stems once (`:76-83`, via `RescoreHydration.SyntheticInputFromParquet`) so every per-task accessor sees a populated `InputFiles` regardless of which task the run starts at. +- **The TASK picks the parquet, not the directory.** `ScoringTaskShared.ScoresPathsForInputs` resolves each input through `ReadsReconciledScores`: `SecondPassFDR` reads `.scores-reconciled.parquet`, `FirstPassFDR` and `PerFileRescoring` read `.scores.parquet`. That is also what each node is shipped (Boundary 3 -> 4), so on a correct node only one of the two is present - which is exactly why this must not be decided by looking. The rule was "reconciled wins per stem", inherited from `--input-scores`' directory form; it is right only while the stages run in order, and a `--task FirstPassFDR` re-run over a completed directory would take the survivor subset as its first-pass population. +- **Order is the caller's.** FirstPassFDR reconciliation is order-sensitive, so a chain must pass a deterministically sorted list. `--input-scores` used to sort a globbed directory Ordinal on the caller's behalf; naming the runs explicitly means an orchestrator states the order rather than inheriting it from a directory listing, and a stray parquet in that directory can no longer change the cohort. +- `--input-list` takes one path per line (blank lines and `#` comments ignored) and composes with `-i`. It is what a cohort past a few hundred runs needs: 446 `-i` paths measured ~28,600 characters against a 32,767 limit. + +**Why the flag went.** The pipeline is a set of BASENAMES. Every per-run artifact is +`.` and every per-run map is keyed on the stem, so naming the runs names +everything they own. `--input-scores` named that same set with a different extension - it was +redundant by construction, not merely superseded, and the two spellings could disagree about +which runs the cohort contained. + +It also named an input KIND - "you handed me parquets" - which is how the Rust pipeline said *Stage 1-4 is done*. The C# port says that with `--task` plus the per-run validity sidecars, and two seams answering one question is what let `--task ModelDiagnostics` (a C#-era signal, while its inputs were mzML stems, a Rust-era one) join the pipeline and demand state a diagnostics fold never publishes. The clearest evidence it was a round trip: the pipeline's first act was `RescoreHydration.SyntheticInputFromParquet`, rebuilding a synthetic `.mzML` that does not exist, purely so the sidecar path helpers could work. ## Parquet footer hash validation @@ -183,7 +189,6 @@ a corrupt cache a downstream stage must reject. See principle P8 in | Flag / field | Default | Effect on this stage | |---|---|---| | `--task {PerFileScoring\|FirstPassFDR\|PerFileRescoring\|SecondPassFDR}` | none (full pipeline in one process) | Selects one HPC worker; sets `NoJoin` / `StopAfterStage5` / `ExpectReconciledInput` (`Program.cs:126-128`). Resolved case-insensitively (`ResolveTask`). | -| `--input-scores ` | none | One or more `.scores.parquet` files or a single directory (globbed non-recursively, reconciled-wins-per-stem, Ordinal-sorted). Mutually exclusive with `-i/--input`. Consumed by `FirstPassFDR` / `PerFileRescoring` / `SecondPassFDR` and the `--input-scores`-only full run. | | `-i/--input ` | none | Required by `PerFileScoring` and the default full pipeline; forbidden by `FirstPassFDR` / `PerFileRescoring` / `SecondPassFDR`. | | `-l/--library`, `-o/--output` | none | Required by `FirstPassFDR`, `PerFileRescoring`, and `SecondPassFDR`. `--output` is accepted-but-unused by `--task PerFileScoring` (writes per-file parquets, not a blib). | | `--reconciliation-compaction-fdr ` | 0.01 | Peptide-q gate for Stage 5 compaction (`FirstPassFdrTask.cs:689`). | @@ -202,7 +207,7 @@ a corrupt cache a downstream stage must reject. See principle P8 in - **[INTENTIONAL-CSHARP-DESIGN] One name per task, describing the FDR pass** - The CLI name, the `HpcTask` member, the task class, the `[TASK]` log token, and the `.osprey.task` stamp are all one string per task, describing the FDR pass rather than the join topology. Two of them used to describe the topology instead (`FirstJoinTask`/`FirstPassFDR` and `MergeNodeTask`/`SecondPassFDR`), which cost a reader a mapping table and once produced a resume leg that keyed off the class names, matched zero sidecars, and passed green having resumed nothing; issue #4535 renamed them. The residual mapping is `PerFileRescoring` vs `PerFileRescore`, plus the `Fdr`/`FDR` casing that follows this codebase's type convention (`FdrEntry`, `FdrController`) rather than the all-caps `pwiz.Osprey.FDR` namespace. Folding those two in as well would let `ResolveTask` and `TaskCliName` be deleted outright. Evidence: the `HpcTask` enum in `Osprey.Core/OspreyConfig.cs`, `ResolveTask` / `TaskCliName` in `Osprey/Program.cs`. Severity: info. -- **[INTENTIONAL-CSHARP-DESIGN] Stage 6 writes a separate `.scores-reconciled.parquet`, not an in-place rewrite** - Rust doc says Stage 6 "rewrites each `.scores.parquet`" with reconciled scores; C# writes a separate `.scores-reconciled.parquet` sibling and leaves the Stage 4 parquet intact (crash-safety: a partial Stage 6 crash cannot half-rewrite the Stage 4 output). `--input-scores` directory resolution then prefers the reconciled sibling per stem. Evidence: `Osprey.Tasks/PerFileRescoreTask.cs:163-177,944-954`, `Osprey/Program.cs:459-472`. Severity: minor. +- **[INTENTIONAL-CSHARP-DESIGN] Stage 6 writes a separate `.scores-reconciled.parquet`, not an in-place rewrite** - Rust doc says Stage 6 "rewrites each `.scores.parquet`" with reconciled scores; C# writes a separate `.scores-reconciled.parquet` sibling and leaves the Stage 4 parquet intact (crash-safety: a partial Stage 6 crash cannot half-rewrite the Stage 4 output). Which of the two a run is read from is decided by the task (`ScoringTaskShared.ReadsReconciledScores`). Evidence: `Osprey.Tasks/PerFileRescoreTask.cs`, `Osprey.Tasks/ScoringTaskShared.ScoresPathsForInputs`. Severity: minor. - **[INTENTIONAL-CSHARP-DESIGN] Orchestration is membership-predicate + lazy-rehydrate, not a stage window** - Rust doc frames each mode as "run stages X through Y, load the rest from disk"; C# implements a fixed four-task canonical pipeline where each task's `IsIncluded` decides participation and excluded/valid tasks lazy-rehydrate their state on demand through the typed byproduct registry. Behavior/outputs match the Rust modes (pinned by the membership truth table). Evidence: `Osprey/AnalysisPipeline.cs:99-148`, `Osprey.Test/PipelineMembershipTest.cs:55-93`. Severity: info. @@ -212,4 +217,4 @@ a corrupt cache a downstream stage must reject. See principle P8 in - **[STALE-RUST-DOC] Stage 4 parquet footer omits `osprey.reconciliation_hash`** - Rust doc's hash table lists `osprey.reconciliation_hash` as parquet footer metadata generally; in C# the Stage 4 `.scores.parquet` footer carries only `version` / `search_hash` / `library_hash` / `reconciled = "false"`, and `reconciliation_hash` is written **only** on the Stage 6 reconciled parquet. This matches the semantic intent (the hash is meaningful only post-reconciliation) but the field is not present on every parquet. Evidence: `Osprey.Tasks/PerFileScoringTask.cs:226-232` vs `Osprey.Tasks/ReconciledParquetWriter.cs:198-205`. Severity: info. -Verified as matching the Rust doc: the four-phase split (per-file scoring / FirstPassFDR / per-file rescore / SecondPassFDR) and which stages run vs load-from-disk in each mode; the boundary file pair (`.1st-pass.fdr_scores.bin` + `.reconciliation.json`); the SHA-256 footer-hash validation (version / search_hash / library_hash / reconciled) aborting early with a file-named error; the `--task SecondPassFDR` strict `reconciled = "true"` gate; the `--input-scores` directory being scanned non-recursively; the mutual-exclusion validation errors (`Program.ValidateArgs`); the reconciliation.json carrying `search_hash`/`library_hash`; the copy-and-verify safe-write pattern; and the env-var-gated cross-impl bisection dumps. +Verified as matching the Rust doc: the four-phase split (per-file scoring / FirstPassFDR / per-file rescore / SecondPassFDR) and which stages run vs load-from-disk in each mode; the boundary file pair (`.1st-pass.fdr_scores.bin` + `.reconciliation.json`); the SHA-256 footer-hash validation (version / search_hash / library_hash / reconciled) aborting early with a file-named error; the `--task SecondPassFDR` strict `reconciled = "true"` gate; the per-task input requirements (`Program.ValidateArgs`); the reconciliation.json carrying `search_hash`/`library_hash`; the copy-and-verify safe-write pattern; and the env-var-gated cross-impl bisection dumps. diff --git a/pwiz_tools/Osprey/docs/16-determinism.md b/pwiz_tools/Osprey/docs/16-determinism.md index 5f198eb6d9..27ac5b01d7 100644 --- a/pwiz_tools/Osprey/docs/16-determinism.md +++ b/pwiz_tools/Osprey/docs/16-determinism.md @@ -134,8 +134,10 @@ is seeded and deterministic. There are **two**, structurally different: which run survives follows FILE ORDER. A cross-run maximum was commutative and did not. Re-running the same file list in the same order reproduces the same model; re-running it in a different order does not, and file order is not part of the task validity key. - `Program.ResolveInputScores` sorts the single-directory form but preserves caller order - for the explicit multi-path `--input-scores` form. + Order is the CALLER's: `--input-scores` used to sort a globbed directory on the caller's + behalf, and with it retired an orchestrator states the order explicitly (`--input-list` + takes a sorted file). A stray parquet in a directory can no longer change the cohort + either, which is the other half of the same trade. `XorShift64` (`Osprey.ML/LinearSvmClassifier.cs:266`) matches the Rust generator exactly (`x ^= x << 13; x ^= x >> 7; x ^= x << 17`). @@ -192,11 +194,12 @@ on it. The scoring task orders entries and writes them to the per-file `PerFileRescoreTask.SortFileEntriesCanonical` (`Osprey.Tasks/PerFileRescoreTask.cs:1301`) re-imposes the exact `(EntryId, Charge, ScanNumber, ParquetIndex)` order a cold run establishes, with `ParquetIndex` as a unique terminal key so the sort never -ties (`:1306-1315`). The comment at `:1287-1299` explains why this is applied to -**every** file (even no-work files with no reconciled Parquet): otherwise +ties (`:1306-1315`). Its comment explains why this is applied to **every** file the +resume overlays, including a file with no reconciliation work: otherwise `SecondPassFDR`'s `BuildSharedBoundaries` could iterate a different order and, on a q-value tie between charge states, pick a different shared `(modseq, file)` -boundary. Parquet preserves exact IEEE-754 values, so a rehydrated entry is +boundary. A file the resume loads from its reconciled Parquet instead arrives in +that order already, from `FirstPassSurvivorLoader`'s own canonical sort. Parquet preserves exact IEEE-754 values, so a rehydrated entry is bit-identical to the in-memory original (see 14-intermediate-files.md). The PEP estimator is fed a `base_id`-ascending-sorted union so its diff --git a/pwiz_tools/Osprey/docs/19-testing.md b/pwiz_tools/Osprey/docs/19-testing.md index c0c1934206..705f051017 100644 --- a/pwiz_tools/Osprey/docs/19-testing.md +++ b/pwiz_tools/Osprey/docs/19-testing.md @@ -72,8 +72,8 @@ inputs (spectra, library entries, feature vectors) with no external data dependency. Additional C# test files with no direct Rust analog cover port-specific -infrastructure: `ProgramTests.cs` (CLI `--task` argument validation and -`--input-scores` directory expansion — see 15-hpc-scoring-split.md), +infrastructure: `ProgramTests.cs` (CLI `--task` argument validation — see +15-hpc-scoring-split.md), `ByproductContextTest.cs`, `DiagnosticsTest.cs`, `ModelDiagnosticsDataTest.cs` (the `--model-diagnostics` HTML report), `FileSaverTest.cs` (the safe copy-and-verify NAS-write pattern), `DecoyPairingManifestTest.cs` and diff --git a/pwiz_tools/Osprey/docs/20-command-line.md b/pwiz_tools/Osprey/docs/20-command-line.md index f992eaea8c..153e8228eb 100644 --- a/pwiz_tools/Osprey/docs/20-command-line.md +++ b/pwiz_tools/Osprey/docs/20-command-line.md @@ -139,8 +139,7 @@ Defaults and value lists are from `Osprey/OspreyCommandArgs.cs`; the parser acce | Option | Value | Effect | |--------|-------|--------| -| `--task` | `PerFileScoring \| FirstPassFDR \| PerFileRescoring \| SecondPassFDR` | Run exactly one pipeline task (one node = one task). Omit for the whole pipeline. See [15-hpc-scoring-split.md](15-hpc-scoring-split.md). | -| `--input-scores` | `` | One or more `.scores.parquet` files, or a single directory (non-recursive). Mutually exclusive with `--input`. | +| `--task` | `PerFileScoring \| FirstPassFDR \| PerFileRescoring \| SecondPassFDR` | Run exactly one pipeline task (one node = one task). Omit for the whole pipeline. EVERY task takes `-i`/`--input-list` naming the data files; the parquets and sidecars are derived from their stems. See [15-hpc-scoring-split.md](15-hpc-scoring-split.md). | ### Logging @@ -181,21 +180,26 @@ check rejects inputs whose search/library hash does not match. # split 1 — one process per mzML (writes .scores.parquet, .calibration.json beside each input) osprey --task PerFileScoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 -# join 1 — one process over ALL parquets (pass a DIRECTORY so order is deterministic) -osprey --task FirstPassFDR --input-scores ./scores_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +# join 1 — one process over ALL runs (pass a sorted list so order is deterministic) +osprey --task FirstPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 # writes beside each parquet: .1st-pass.fdr_scores.bin, .reconciliation.json # split 2 — one process per file (parquet + its two sidecars co-located) -osprey --task PerFileRescoring --input-scores s1.scores.parquet -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +osprey --task PerFileRescoring -i s1.mzML -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 # writes: .scores-reconciled.parquet -# join 2 — one process over ALL reconciled parquets (writes out.blib) -osprey --task SecondPassFDR --input-scores ./reconciled_dir -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 +# join 2 — one process over ALL runs, reading their reconciled parquets (writes out.blib) +osprey --task SecondPassFDR --input-list runs.txt -l hela.tsv -o out.blib --resolution unit --protein-fdr 0.01 ``` -- `--input-scores` takes a **directory** (globbed and sorted internally) or an explicit - file list (consumed in the order given). FirstPassFDR reconciliation is order-sensitive, - so for `FirstPassFDR` and `SecondPassFDR` pass a directory or a deterministically sorted list. +- Every task names its runs by their **data files**, and the data file itself need not + still exist: a task after Stage 4 is accepted when the run's `.scores.parquet` (or its + reconciled sibling) is on disk, which is the state a staged worker directory is in. + `--input-scores`, which named parquets instead, has retired - it was a second way of + saying what `--task` already says. +- FirstPassFDR reconciliation is **order-sensitive**, so pass a deterministically sorted + list. `--input-list` (one path per line) is what a cohort past a few hundred runs needs: + 446 `-i` paths is ~87% of the Windows command-line limit. - Rehydration sidecars must travel with their parquet into each worker's working directory. Let the scheduler fan out (one file per split process) rather than `--parallel-files`, which is the single-node multi-file mode. diff --git a/pwiz_tools/Osprey/docs/DIVERGENCES.md b/pwiz_tools/Osprey/docs/DIVERGENCES.md index a26bc0c1b3..6d995bd19a 100644 --- a/pwiz_tools/Osprey/docs/DIVERGENCES.md +++ b/pwiz_tools/Osprey/docs/DIVERGENCES.md @@ -275,7 +275,7 @@ Legend — Classification: **STALE** = STALE-RUST-DOC, **INTENT** = INTENTIONAL- |---|---|---|---|---|---| | INTENT | CLI is `--task `, not `--no-join`/`--join-at-pass`/`--join-only` | Orchestrated by `--join-at-pass` + modifiers | Single `--task {PerFileScoring\|FirstPassFDR\|PerFileRescoring\|SecondPassFDR}`; old flags retired, fail fast | `Program.cs:86-128`; `OspreyCommandArgs.cs:206-207` | major | | INTENT | One name per task, describing the FDR pass | Named by pass/join topology | CLI name, enum member and class are one name per task; residual `PerFileRescoring` vs `PerFileRescore` | `OspreyConfig.cs` (`HpcTask`); `Program.cs` (`ResolveTask`) | info | -| INTENT | Stage 6 separate `.scores-reconciled.parquet` | Rewrites `.scores.parquet` | Separate sibling; `--input-scores` prefers reconciled | `PerFileRescoreTask.cs:163-177,944-954` | minor | +| INTENT | Stage 6 separate `.scores-reconciled.parquet` | Rewrites `.scores.parquet` | Separate sibling; each run's effective parquet prefers reconciled | `PerFileRescoreTask.cs:163-177,944-954` | minor | | INTENT | Membership-predicate + lazy-rehydrate, not stage window | Each mode runs stages X..Y | Fixed 4-task pipeline; `IsIncluded` + typed byproduct registry; pinned by truth table | `AnalysisPipeline.cs:99-148`; `PipelineMembershipTest.cs:55-93` | info | | INTENT | No `--parquet-compression`; ZSTD unconditional | `--parquet-compression snappy` for OspreySharp interop | Writes ZSTD; read auto-dispatches; cross-impl ZSTD/Snappy read compat is follow-up | `ParquetScoreCache.cs:270,462` | minor | | FLAG | `OSPREY_DUMP_PREDICT_RT` declared but disabled | Stage 6 worker bisection dump | `DumpPredictRt` declared; call site commented out (scoring hotspot) → produces nothing | `IOspreyDiagnostics.cs:80`; `PerFileRescoreTask.cs:715-726` | minor | diff --git a/pwiz_tools/Osprey/docs/README.md b/pwiz_tools/Osprey/docs/README.md index 78f798c819..7cc7f94af0 100644 --- a/pwiz_tools/Osprey/docs/README.md +++ b/pwiz_tools/Osprey/docs/README.md @@ -25,7 +25,7 @@ architecture the C# pipeline is built to, rather than porting a Rust source doc. Three documents divide the file-and-orchestration subject, and none repeats another: **00** owns scope, contract, principles and relay (which file, whose, when, who may read it); **14** owns the bytes (headers, versions, schemas, hashing, invalidation mechanics); **15** -owns operations (CLI flags, `--input-scores` ordering, orchestration recipes). +owns operations (CLI flags, how a task names its runs and in what order, orchestration recipes). | # | Doc | What it covers | |---|-----|----------------| @@ -44,7 +44,7 @@ owns operations (CLI flags, `--input-scores` ordering, orchestration recipes). | 12 | [second-pass-fdr](12-second-pass-fdr.md) | Stage-7 second-pass FDR and the frozen-model q-value modes — transfer-compete and protein-compact — selected by `OSPREY_PASS2_QVALUE`. | | 13 | [blib-output-schema](13-blib-output-schema.md) | BiblioSpec SQLite schema plus Osprey extension tables and the nullable `retentionTime` convention for Skyline ID lines. | | 14 | [intermediate-files](14-intermediate-files.md) | On-disk caches / sidecars (calibration JSON, spectra cache, `.scores.parquet`, FDR sidecars), SHA-256 footer hashing, and the tiered memory architecture. | -| 15 | [hpc-scoring-split](15-hpc-scoring-split.md) | The four `--task` workers, their input/output files, `--input-scores` ordering rules, and validity sidecars for HPC / NextFlow orchestration. | +| 15 | [hpc-scoring-split](15-hpc-scoring-split.md) | The four `--task` workers, their input/output files, the ordering rules a chain must observe, and validity sidecars for HPC / NextFlow orchestration. | | 16 | [determinism](16-determinism.md) | Patterns that keep results bit-identical across runs: thread-order independence, float stability, cross-validation fold assignment. | | 17 | [vectorization](17-vectorization.md) | Performance-critical vectorization — the SIMD / BLAS-equivalent paths for XCorr and matrix operations. | | 18 | [peptide-trace](18-peptide-trace.md) | The per-peptide diagnostic dump facility (C# `OSPREY_DUMP_*` / `OSPREY_DIAG_*` in place of the Rust `OSPREY_TRACE_PEPTIDE`). | diff --git a/pwiz_tools/Osprey/regression-parallel.ps1 b/pwiz_tools/Osprey/regression-parallel.ps1 index 76671b74f3..569cf697c2 100644 --- a/pwiz_tools/Osprey/regression-parallel.ps1 +++ b/pwiz_tools/Osprey/regression-parallel.ps1 @@ -158,17 +158,29 @@ Write-Host '=== Parallel regression summary ===' -ForegroundColor Cyan $totalPass = 0; $totalFail = 0; $totalSkip = 0; $worst = 0 foreach ($r in $running) { $text = if (Test-Path $r.Log) { Get-Content $r.Log } else { @() } - $pass = @($text | Select-String -Pattern ': PASS').Count - $fail = @($text | Select-String -Pattern ': FAIL').Count - $skip = @($text | Select-String -Pattern ': SKIP').Count + # -CaseSensitive, and it is not a nicety. Select-String is case-INSENSITIVE by default, + # so ': FAIL' matched the ': fail' inside any "WARN: failed to ..." line the lane emitted - + # and one of those (a prune racing a previous run's directory) turned a lane that exited 0, + # passed all 23 legs and printed "Osprey regression PASSED" into "1 FAIL" and an overall + # FAILED. A gate that cries wolf about its own warnings is worse than one that stays quiet: + # the next red gets read as this one. The leg lines these count are emitted in upper case by + # regression.ps1, so requiring that costs nothing. + $pass = @($text | Select-String -CaseSensitive -Pattern ': PASS').Count + $fail = @($text | Select-String -CaseSensitive -Pattern ': FAIL').Count + $skip = @($text | Select-String -CaseSensitive -Pattern ': SKIP').Count $totalPass += $pass; $totalFail += $fail; $totalSkip += $skip $code = $r.Proc.ExitCode if ($code -gt $worst) { $worst = $code } $colour = if ($code -eq 0 -and $fail -eq 0) { 'Green' } else { 'Red' } Write-Host (" {0,-45} exit={1} {2} PASS / {3} FAIL / {4} SKIP" -f $r.Name, $code, $pass, $fail, $skip) -ForegroundColor $colour - foreach ($line in ($text | Select-String -Pattern ': (PASS|FAIL|SKIP)')) { + foreach ($line in ($text | Select-String -CaseSensitive -Pattern ': (PASS|FAIL|SKIP)')) { Write-Host (" " + $line.Line.Trim()) } + # Warnings still surface - they were only ever miscounted, not unwanted - but as + # warnings, in their own colour, where nothing tallies them as legs. + foreach ($line in ($text | Select-String -CaseSensitive -Pattern '^\s*WARN:')) { + Write-Host (" " + $line.Line.Trim()) -ForegroundColor Yellow + } } Write-Host '' Write-Host (" TOTAL {0} PASS / {1} FAIL / {2} SKIP in {3:hh\:mm\:ss} wall" -f diff --git a/pwiz_tools/Osprey/regression.ps1 b/pwiz_tools/Osprey/regression.ps1 index 6c1dd1b00f..032e68eb3f 100644 --- a/pwiz_tools/Osprey/regression.ps1 +++ b/pwiz_tools/Osprey/regression.ps1 @@ -302,7 +302,16 @@ $knownResidentGaps = @( # the 91.1 GB private measured on the 446-run CHS cohort (2026-09-08), which is the # first endpoint past 82 files. Quoted as a check on the model, not as a second model - # see the note above about three numbers no single model reproduced. - Legs = 'Every leg EXCEPT the streamed Stage-7 join (mode 3''s SecondPassFDR phase, which sets ExpectReconciledInput). ~4.4 GB library + 0.197 GB/file live post-GC: ~20 GB at 82 files, ~103 GB projected at 500, and 92.3 GB predicted vs 91.1 GB measured at 446.' + # The set has SHRUNK from "every leg except mode 3's join phase" to one pass-2 mode. + # CanStreamStage7Join's first term was config.ExpectReconciledInput, which only + # --task SecondPassFDR sets, so the ordinary run could not stream BY CONSTRUCTION; + # derived from the reconciled parquets on disk it is route-independent, and the cold + # run and both resume arms now fold run by run (asserted per leg, not projected). + # What is left is the pass-2 mode with no per-file worker: transfer still computes + # its per-file half in Stage 7, over the whole pool. Moving TransferOneFile into + # Pass2PerFileWorker is what empties this row - and then the guard's + # streamingAvailable exemption has no subject either, so the two go together. + Legs = 'ONLY a pass-2 mode with no per-file worker (OSPREY_PASS2_QVALUE=transfer, i.e. mode 10''s transfer arm). Every default leg - cold straight-through, both resumes, and mode 3''s SecondPassFDR phase - folds run by run. ~4.4 GB library + 0.197 GB/file live post-GC where it is still taken: ~20 GB at 82 files, and 92.3 GB predicted vs 91.1 GB measured at 446.' } ) # Reachable only outside this gate, tokened, each with an open issue: @@ -1156,6 +1165,12 @@ $firstPassFdrRehydrateMarker = 'Resume rehydrate: streaming the first-pass bundl # Asserting the outcome alone is exactly what let an earlier resume fix report success while # testing the old path (defect (b2), TODO-20260901_osprey_firstpassfdr_resume). $perRunHydrateMarker = 'Per-run rescore: hydrating each of' +# The line every arm of the streamed Stage-7 join logs - the reconciled-input merge, the +# straight-through cold run and the straight-through resume all open with these words on +# purpose. Output is IDENTICAL whichever arm runs, so this line is the only evidence the +# bounded join happened at all; giving each arm its own wording would need three markers +# and would let a fourth arm ship unwatched. +$stage7StreamMarker = 'Second-pass join: folding over ' # FirstPassFDR's half of the same shape: on a rehydrate where the analysis-wide summary is on # disk it publishes the survivor loader and builds no experiment-wide bundle, so it emits this @@ -1399,7 +1414,7 @@ function Invoke-HpcChain { # staged dir as CWD. $manifestName = if ($Manifest) { Split-Path -Leaf $Manifest } else { $null } $extraArgs = Get-DatasetCliArgs -Spec $Spec -Manifest $manifestName - # Stable, file-order stem list (NOT hashtable key order) so the --input-scores + # Stable, file-order stem list (NOT hashtable key order) so the -i # argument order matches the straight-through's file order deterministically. $stemList = @($Mzmls | ForEach-Object { [IO.Path]::GetFileNameWithoutExtension($_) }) $mzmlByStem = @{} @@ -1473,8 +1488,12 @@ function Invoke-HpcChain { Copy-Item (Join-Path $ph1Dirs[$s] "$s.calibration.json") (Join-Path $ph2 "$s.calibration.json") } Copy-LibraryInto -Library $Library -Dir $ph2 -Manifest $Manifest + # -i names the DATA files, exactly as phase 1 was given them, even though this + # directory holds no data file at all: Osprey accepts an absent input whose scores + # parquet is on disk, which is precisely the state a staged join node is in. That + # tolerance is what --input-scores used to express by naming a different input KIND. $a2 = @('--task', 'FirstPassFDR') - foreach ($s in $stemList) { $a2 += @('--input-scores', "$s.scores.parquet") } + foreach ($s in $stemList) { $a2 += @('-i', "$s.mzML") } $a2 += @('-l', $libName, '-o', 'output.blib', '--resolution', $Resolution, '--protein-fdr', '0.01', '--threads', $Threads.ToString()) $a2 += $extraArgs @@ -1543,7 +1562,7 @@ function Invoke-HpcChain { $ph2diag = Join-Path $ph2 'output.1st-pass.model-diagnostics.json' if (Test-Path $ph2diag) { Copy-Item $ph2diag (Join-Path $ph3 'output.1st-pass.model-diagnostics.json') } Copy-LibraryInto -Library $Library -Dir $ph3 -Manifest $Manifest - $a3 = @('--task', 'PerFileRescoring', '--input-scores', "$s.scores.parquet", + $a3 = @('--task', 'PerFileRescoring', '-i', "$s.mzML", '-l', $libName, '-o', 'output.blib', '--resolution', $Resolution, '--protein-fdr', '0.01', '--threads', $Threads.ToString()) $a3 += $extraArgs @@ -1682,8 +1701,13 @@ function Invoke-HpcChain { # worker dirs are done. foreach ($d in $ph3Dirs.Values) { Remove-Scratch $d } Copy-LibraryInto -Library $Library -Dir $ph4 -Manifest $Manifest + # -i again, and the RECONCILED parquet is what each run resolves to - because the TASK + # says so (ScoringTaskShared.ReadsReconciledScores), not because it is the only file + # here. That this directory also holds only the reconciled sibling is the enforcement + # above, and the two are deliberately independent: if the resolution ever regressed to + # a disk probe, the staging would hide it. $a4 = @('--task', 'SecondPassFDR') - foreach ($s in $stemList) { $a4 += @('--input-scores', "$s.scores-reconciled.parquet") } + foreach ($s in $stemList) { $a4 += @('-i', "$s.mzML") } $a4 += @('-l', $libName, '-o', 'output.blib', '--resolution', $Resolution, '--protein-fdr', '0.01', '--threads', $Threads.ToString()) $a4 += $extraArgs @@ -1737,6 +1761,37 @@ foreach ($name in $selected) { $dataFp = Get-DirFingerprint -Dir $inputs.Dir $straightDir = Join-Path $runRoot "$name\straight" + # Whether this run was ASKED for a configuration that cannot stream the Stage-7 join, so + # the legs below can tell "took the resident join" from "was told to". These are + # CanStreamStage7Join's OWN terms, not a mode list: the switch that forces the resident + # join, the two that make NeedsResidentPool true, and any pass-2 mode other than + # protein-compact (transfer still computes its per-file half in Stage 7). Enumerated + # rather than inferred from the log, because a resident run says nothing about WHY it was + # resident - and a silent SKIP for the wrong reason is what these legs exist to prevent. + # + # ExpectReconciledInput is not among them, and used to be the whole answer: it was + # CanStreamStage7Join's first term, and only --task SecondPassFDR sets it, so the ordinary + # run could not stream by construction. Deriving the admission from the reconciled parquets + # on disk is what puts every leg under one question. + # + # OSPREY_STAGE6_STREAM_SURVIVORS=0 belongs here too, and its absence made this list + # wrong rather than merely incomplete. Under that switch BuildRunPerRunSource returns + # null (no loader, and the per-run lists are never cleared), so no marker reaches + # straight.log and the mode1 leg FAILS a run behaving exactly as instructed - while + # mode2/mode5 PASS, because BuildResumePerRunSource reads the loader through + # PublishedSurvivorLoader, which deliberately bypasses the Stage-6 switch. A mode1-only + # red that reads like a genuine regression is the worst shape a gate can have. + # + # STILL INCOMPLETE, deliberately, and worth knowing: this reads env vars only, while two + # of the three NeedsResidentPool triggers are CLI/config (--fdrbench-pass 1, a + # non-Percolator --fdr-method). A dataset spec setting either would red all three legs. + # No spec does today; if one is added, this has to grow a $cfg term. + $cannotStreamJoin = + ($env:OSPREY_STAGE7_STREAM -eq '0') -or + ($env:OSPREY_STAGE6_STREAM_SURVIVORS -eq '0') -or + ($env:OSPREY_FDR_PROJECTION -eq '0') -or + (-not [string]::IsNullOrWhiteSpace($env:OSPREY_PASS2_QVALUE) -and + $env:OSPREY_PASS2_QVALUE -ne 'protein-compact') $proteinDump = Join-Path $straightDir 'cs_stage7_protein_fdr.tsv' # GoldenFolder, not Folder: StellarLibDecoy shares the stellar mzML folder, # so keying the golden on Folder alone would collide with Stellar's. @@ -1881,7 +1936,7 @@ foreach ($name in $selected) { # pool, so no leg of this chain arms the guard at all. Keeping the opt-in would be # actively harmful: # it wrapped the whole chain and would mask a genuine guard regression on any - # --input-scores worker (--task PerFileScoring / PerFileRescoring), which is exactly + # per-file worker (--task PerFileScoring / PerFileRescoring), which is exactly # what mode 3 exists to exercise. $chainBlib = Invoke-HpcChain -Mzmls $inputs.Mzmls -Library $inputs.Library ` -Resolution $cfg.Resolution -ChainRoot $chainRoot -Spec $cfg -Manifest $inputs.Manifest ` @@ -2047,21 +2102,12 @@ foreach ($name in $selected) { # needs a narrower rule should name the condition rather than restore a blanket skip. # ...unless the run was asked for a configuration that cannot stream, in which case it # is doing exactly what it was told and demanding the marker would fail it for - # complying. These are CanStreamStage7Join's OWN terms, not a mode list: the switch that - # forces the resident join, the two that make NeedsResidentPool true, and any pass-2 - # mode other than protein-compact (transfer still computes its per-file half in Stage 7). - # ExpectReconciledInput is not among them because phase 4 always sets it. - # Enumerated rather than inferred from the log, because a resident run says nothing - # about WHY it was resident - and a silent SKIP for the wrong reason is what this leg - # exists to prevent. - $chainCannotStream = - ($env:OSPREY_STAGE7_STREAM -eq '0') -or - ($env:OSPREY_FDR_PROJECTION -eq '0') -or - (-not [string]::IsNullOrWhiteSpace($env:OSPREY_PASS2_QVALUE) -and - $env:OSPREY_PASS2_QVALUE -ne 'protein-compact') + # complying. $cannotStreamJoin, computed once per dataset above, is that question; it + # governs every leg's copy of this assertion rather than each one enumerating the + # terms again. $chainStreamed = Select-String -Path (Join-Path (Join-Path $chainRoot 'logs') 'phase4.log') ` -Pattern 'Second-pass join: folding over \d+ run\(s\)' -Quiet - if ($chainCannotStream) { + if ($cannotStreamJoin) { $summaryLines.Add("$name mode3 (streamed join): SKIP (this configuration cannot stream the join)") } elseif (-not $chainStreamed) { $overallFail = $true @@ -2646,6 +2692,71 @@ foreach ($name in $selected) { } } + # ---- mode 1/2/5: the IN-PROCESS legs took the streamed Stage-7 join ---- + # Here, beside mode 6, for mode 6's own reason: it reads the logs of the legs above and + # they have to have been written. Mode 3's chain carries the same assertion inside its own + # block, where its phase-4 log lives. + # + # This could not be asserted before. CanStreamStage7Join's admission WAS + # config.ExpectReconciledInput, which only --task SecondPassFDR sets, so every in-process + # leg was resident by construction and demanding the marker would have failed each of them + # for behaving correctly. Derived from the reconciled parquets on disk the admission is + # route-independent, and these three legs - the cold run and the two resumes - are exactly + # the shapes an operator runs. The 91.1 GB measured on a 446-run cohort was an ORDINARY + # `-i ... --output-dir` resume, i.e. the third of them. + # + # Three separate arms, not one: the cold run publishes its source from + # PerFileRescoreTask.Run, and the two resumes from its Rehydrate, having arrived there by + # different routes (mode 2 re-runs FirstPassFDR, mode 5 rebuilds its bundle from its own + # sidecars, which leaves the survivor lists POPULATED where mode 2 leaves them released). + # A marker on one says nothing about the others. + # + # A leg that did not run is SKIP, not FAIL - -SkipResume / -SkipRehydrate are legitimate. + foreach ($streamLeg in @( + @{ Log = 'straight.log'; Mode = 'mode1'; What = 'the cold straight-through run' }, + @{ Log = 'resume.log'; Mode = 'mode2'; What = 'the resume' }, + @{ Log = 'rehydrate.log'; Mode = 'mode5'; What = 'the own-sidecar rehydrate' })) { + $legPath = Join-Path $straightDir $streamLeg.Log + if ($cannotStreamJoin) { + $summaryLines.Add(("$name $($streamLeg.Mode) (streamed join): SKIP " + + '(this configuration cannot stream the join)')) + } elseif (-not (Test-Path -LiteralPath $legPath)) { + $summaryLines.Add("$name $($streamLeg.Mode) (streamed join): SKIP (leg not run)") + } else { + $legStream = Test-LogMarker -LogPath $legPath -Marker $stage7StreamMarker ` + -Description ("$($streamLeg.What) folding Stage 7 one run at a time instead " + + 'of rebuilding every run''s survivors at once') + # PRESENCE OF THE MARKER IS NOT ENOUGH, and this is the hole it left. The marker is + # logged when the per-run source is BUILT, not when anything folds through it. A + # consumer that reads RescoredEntries.Value instead of streaming routes to + # MaterializeAllFromSource, which builds every run at once - the exact O(runs x + # entries) peak, 91.1 GB at 446 - while Streams stays true, so the marker is present, + # WarnResidentStage7Join stays silent, and this leg reported PASS on a resident run. + # MaterializeAllFromSource has always logged its own warning; nothing asserted its + # absence. Assert it here: the marker says a source was offered, this says nothing + # took the whole pool anyway. + $legPooled = Select-String -LiteralPath $legPath -SimpleMatch -Quiet ` + -Pattern 'a consumer asked for the whole-run survivor pool' + if ($legPooled) { + $overallFail = $true + Write-Problem-Tc ("$name $($streamLeg.Mode) (streamed join): FAIL - a per-run " + + 'source was published AND a consumer then pulled the whole pool through it, ' + + 'so the fold did not bound anything. The marker alone cannot see this.') + $summaryLines.Add("$name $($streamLeg.Mode) (streamed join): FAIL") + } elseif ($legStream.Pass) { + $summaryLines.Add(("$name $($streamLeg.Mode) (streamed join): PASS " + + '(per-run fold, no all-runs pool)')) + } else { + $overallFail = $true + Write-Problem-Tc ("$name $($streamLeg.Mode) (streamed join): FAIL - Stage 7 " + + 'built the whole-run survivor pool. Output is unchanged either way; only ' + + 'this line distinguishes them.') + $legStream.Issues | ForEach-Object { Write-Host " $_" -ForegroundColor Red } + $summaryLines.Add("$name $($streamLeg.Mode) (streamed join): FAIL") + } + } + } + # ---- mode 6: the library-fragment release engaged on every leg that holds the library ---- # Runs LAST because it reads the logs of all the legs above -- straight-through, # resume, and every phase of the HPC chain -- and they have to have been written.