Repository navigation
Improve benchmark result normalization and generic reporting - #1
Closed
nej1gotnochill wants to merge 32 commits into
Closed
nej1gotnochill wants to merge 32 commits into
nej1gotnochill wants to merge 32 commits into
Conversation
- Copy OpenSwathWorkflow_1 test fixtures from OpenMS repo (7 peptides, 5 SWATH windows) - Add run_openswath_benchmark.sh following smoke benchmark conventions - Include correctness metrics: feature count, overall quality, intensity sum, QC JSON - Update PROVENANCE.md with OpenSwath fixture provenance This benchmark exercises DIA proteomics (complementing DDA smoke benchmark), covering SWATH data reading, chromatogram extraction, MRM scoring, and feature picking. Fixtures: 320 KB total, deterministic, no external dependencies. Expected runtime: 5-15s in CI.
- Add run-openswath-benchmark job alongside smoke benchmark - Uses same OpenMS artifact from build-openms job - Installs runtime dependencies and sets up library paths - Uploads results as separate artifact This enables CI validation of the OpenSwath DIA benchmark prototype.
- Update normalize_smoke and normalize_proteobench to produce v2 output - Add v1->v2 auto-promotion in load_results - Support both v1 (openms/tools) and v2 (benchmark-specific) directory layouts - Add comprehensive test suite for v2 schema changes
- Remove duplicate v1 fields from normalize_smoke and normalize_proteobench output
- Renderer reads exclusively from v2 fields (identity, run, performance, metrics)
- Dynamic stage discovery: no hard-coded stage names (decoy_database, comet, percolator)
- Dynamic metric comparison: discovers shared metrics between runs
- Optional sections: handles missing stages/build/correctness/tool gracefully
- No benchmark-specific if/elif branches in renderer
- Add comprehensive test suite (11 tests) covering all benchmark types
- Add v2 data access helpers (_identity, _run, _performance, _metrics, etc.)
- Fix tool result labels to use _run_id() instead of raw get("run_id")
- Remove _v1_compat from promote output (clean v2 only)
The renderer now works with Smoke, ProteoBench, OpenSwath, and any future
benchmark type without code changes.
The comparison table lost all stage and build metrics when the renderer was refactored to use _metrics() only. Smoke comparison showed only "verdict" instead of stage wall times, build time, and stage statuses. Add _flat_compare() that generically flattens a result's metrics, performance.build, performance.stages, and correctness into a single comparable dict — without duplicating data in the stored schema. Also fix stage regression detection which was silently broken because stage.*.status keys never appeared in the comparison dict. Tests: 16/16 pass (5 new tests for _flat_compare, Smoke comparison, stage regression detection, and ProteoBench no-spurious-keys).
Add JSON definition files for Smoke, OpenSwath, and ProteoBench benchmarks. Each definition contains id, name, description, runner path, fixtures, and normalization function. Also add a lightweight validation script that checks referenced files exist and functions are defined. These definition files provide discovery and documentation benefits without hiding execution logic. The runner scripts and normalization functions remain unchanged. The renderer remains benchmark-agnostic.
- Add normalize_openswath: extract OpenSwath normalization from runner script into report_generate.py as a proper CLI subcommand - Remove tool_versions from canonical v2 result format (normalize_smoke, normalize_openswath, _promote_v1); identity.software.version remains the source for primary software version - Add CI normalization steps: Smoke and OpenSwath results are now normalized into the canonical v2 format after benchmark execution - Add 4 tests for normalize_openswath: basic, load_results integration, missing files graceful handling, required stage failure - Preserve raw benchmark output for debugging alongside canonical results - Generic renderer remains benchmark-agnostic; no schema migration introduced
|
Important Review skippedThe saved review history does not include the base for the last reviewed commit. This saved history cannot establish the base for an incremental review. Comment You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a ProSE and PeptDeep entrapment benchmark with input preparation, metric extraction, execution, v2 normalization, ONNX-enabled CI jobs, output validation, and tests. It also extends OpenSwath normalization coverage. ChangesProSE and PeptDeep benchmark flow
Sequence Diagram(s)sequenceDiagram
participant CI as Benchmark workflow
participant Build as ONNX-enabled OpenMS build
participant Runner as ProSE and PeptDeep runner
participant Metrics as ProSE metrics
participant Normalizer as v2 normalizer
participant Artifact as Results artifact
CI->>Build: build targets and package models and runtime libraries
Build-->>CI: provide ONNX benchmark artifact
CI->>Runner: run baseline and PeptDeep benchmark arms
Runner->>Metrics: parse idXML and validate benchmark features
Metrics-->>Runner: return metrics and verdict
Runner-->>CI: write prose.json
CI->>Normalizer: normalize ProSE and PeptDeep results
Normalizer-->>Artifact: write and upload v2 results
A rabbit reads each line, Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The OpenSwath normalization currently can emit an incorrect passing verdict when stages.tsv is missing (and has fragile TSV numeric parsing), and the workflow’s artifact-size output may be blank unless forced to a numeric default.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors OpenSwath result normalization into report_generate.py as a first-class CLI subcommand, aligns CI to normalize benchmark outputs into the canonical v2 result layout after execution, and adds test coverage to ensure the normalized OpenSwath output integrates cleanly with the generic renderer.
Changes:
- Added
normalize openswathCLI subcommand to convert raw OpenSwath benchmark outputs into v2 run results, and removedtool_versionsfrom v2 outputs/promotion. - Extended CI workflow to normalize smoke and OpenSwath results into v2 immediately after benchmarks run.
- Added 4 tests validating OpenSwath normalization output structure, renderer loading, missing optional files handling, and required-stage failure verdict.
File summaries
| File | Description |
|---|---|
benchmark/report/test_generic_renderer.py |
Adds OpenSwath normalization test coverage and verifies v2 loading behavior. |
benchmark/report/report_generate.py |
Implements normalize openswath and removes v1 tool_versions carryover to keep v2 canonical. |
.github/workflows/benchmark.yml |
Adds post-benchmark normalization steps (smoke + OpenSwath) and computes artifact size for smoke normalization. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+869
to
+885
| stages_path = os.path.join(results_dir, "stages.tsv") | ||
| if os.path.exists(stages_path): | ||
| with open(stages_path) as fh: | ||
| for row in csv.reader(fh, delimiter="\t"): | ||
| if not row: | ||
| continue | ||
| name, required, rc, wall, cpu, peak, status, reason = (row + [""] * 8)[:8] | ||
| stages.append({ | ||
| "name": name, | ||
| "required": required == "true", | ||
| "exit_code": int(rc), | ||
| "wall_time_s": float(wall), | ||
| "cpu_time_s": float(cpu), | ||
| "peak_rss_kb": int(peak), | ||
| "status": status, | ||
| "reason": reason, | ||
| }) |
| - name: Compute artifact size | ||
| id: artifact-size | ||
| shell: bash | ||
| run: echo "bytes=$(du -sb openms/build/bin openms/build/lib openms/share/OpenMS openms/_thirdparty 2>/dev/null | awk '{s+=$1} END{print s}')" >> "$GITHUB_OUTPUT" |
| normalize proteobench <local-proteobench.json> --label LABEL [--out PATH] | ||
| Convert a local ProteoBench scoring result into a tool result. | ||
|
|
||
| normalize openswath --run-id ID [--cache cold|warm|none] [--results-dir DIR] [--out PATH] |
Fix missing/empty stages producing incorrect pass verdicts via all([]), make TSV numeric parsing robust to empty/malformed values, and default CI artifact size to 0 when du fails. Add 4 regression tests.
Two arms over the same mzML/FASTA/config: baseline (ProSE -> Percolator) vs ProSE with PeptDeep MS2+RT prediction features (PR #9975). Extraction of q-values, target/entrapment classification and unique-peptide counting lives in benchmark/prose_metrics.py (shared with unit tests); the runner follows the OpenSwath runner conventions (/usr/bin/time -v per stage). The peptdeep arm fails the verdict unless all five PeptDeep features land in the idXML, so an ONNX-less build cannot silently pass as baseline. Includes the v2 normalization subcommand, benchmark definition, design report and synthetic-idXML unit tests; no dataset is vendored yet.
…ndling - Add make_entrapment_fasta.py (deterministic P02769 target + ENTRAPMENT_ E. coli K-12 targets, rev_ decoys excluded) and prepare_prose_inputs.sh (FileMerger BSA1+2+3 merge into git-ignored benchmark/generated/prose/) - Fix peptdeep:enable CLI handling: OpenMS maps the string param to a no-value flag, so the baseline arm must omit it and the peptdeep arm must pass it bare - Record a clean verdict=fail prose.json instead of crashing when a search produces no PeptideIdentification elements - Document dataset provenance in the definition note; include pending design-doc, normalizer, and prose_metrics test updates Validated against OpenMS PR #9975 (e4b9609c95): both arms pass, Percolator runs in both, all five PeptDeep features present, target-vs-entrapment separation clean (67 BSA vs 1 entrapment vs 2 decoy at q<=0.05).
Collaborator
Author
|
Added an opt-in ONNX-enabled CI path for the ProSE + PeptDeep benchmark. Added: build-openms-onnx job with pinned ONNX Runtime and model SHA256 verification Validation: Existing benchmark jobs remain unchanged The CI benchmark intentionally does not gate on 1% FDR because the current small BSA fixture is pipeline-validation-only; Day-32 showed that apparent 1% gains from duplicated spectra were scientifically invalid. The new workflow is currently workflow_dispatch only, so it can be validated without affecting normal CI. |
Reproducible preparation path for the scientific benchmark dataset (PXD028735 ProteoBench QE HF-X HYE DDA, CC0): pinned archive.openms.de downloads with byte-size + sha256 fail-closed verification, deterministic target + ENTRAPMENT_-prefixed S. cellulosum FASTA assembly, and a PROVENANCE.txt record. The FASTA generator refuses decoy-marked sources (including the SoCe file's _rev suffix decoys, which a rev_-prefix check misses) - ProSE generates decoys via -Search:decoys auto. Validated against the unmodified runner: verdict pass, both arms, Percolator q-values, five-feature coverage 79,113/79,113, +5.18% target PSMs at 1% FDR with PeptDeep on prepared inputs.
Collaborator
Author
|
Update: ProteoBench external comparison Added a standalone ProteoBench comparison tool in 333e71d:
I intentionally kept this separate from The full snapshot gives 46 submissions: 31 compatible, 13 incompatible, and 2 with insufficient/unusable metadata under the documented filter. The comparison is contextual rather than a leaderboard. Runtime/memory and historical FASTA provenance are not treated as directly comparable. Related: OpenMS Issue #8788 and OpenMS PR #9975. |
Rewrite the README as a project-level entry point following the main OpenMS README's information architecture: CI badge, project overview, table of contents, Features, per-benchmark sections (smoke, OpenSwath DIA, ProteoBench reference results), a layered architecture diagram, local run commands, results/reporting with schema v2 and v1 promotion, the three CI jobs with ccache and artifacts, a Development section on benchmark definitions and adding a benchmark, Testing, an updated repository structure, present-tense scope, and Resources.
The referenced notebook repository is personal and not publicly visible, so it does not belong in the public README.
- add `normalize openswath` subcommand: maps raw CI openswath.json into the common v2 schema (stages, correctness, verdict, identity), failing loudly on non-finite stage metrics - store the first OpenSwath baseline from real CI run 32995654115 (build time and artifact size measured from the same CI run) - point the OpenSwath definition's normalization at the real function so validate_definitions.py verifies the wiring - history table gains a generic Benchmark column (two benchmarks now share the results tree) - 3 new behavioral tests (19 total): normalization contract, non-finite rejection, two-run baseline comparison end-to-end OpenSwath now participates in the same lifecycle as smoke: runner -> raw JSON -> normalize -> stored v2 baseline -> generic renderer.
Store the second real OpenSwath run (CI run 35456974761, OpenMS 95fb768fa79298ca70ebadbf7fa08a8f54724ae4) alongside the existing baseline (run 32995654115, f1768367fa66f7901b4fa78a9ebece64b2ce9024), so the generic renderer now performs a real version-to-version comparison: correctness identical, no regression detected. Fix the comparison-card template line that emitted a trailing-whitespace line when no cache note is present; regenerated report.html is now git-diff-check clean.
OpenMS moved its CI dependency model to vcpkg ("Only use dependencies
from vcpkg"): newer SHAs no longer install libraries via their
deps-ubuntu.sh, so the classic build fails at CMake configure. Detect
the dependency model from the checked-out SHA itself (deps-script
content + vcpkg mechanism presence, no hardcoded boundary), and for
vcpkg-era SHAs initialize the vcpkg submodule, bootstrap it with the
same upstream mechanism (lukka/run-vcpkg), provide a manifest-hash-keyed
binary cache, and configure with OPENMS_USE_VCPKG=ON and the upstream
linux triplet. Pre-vcpkg SHAs keep the existing path unchanged; the
build/ output layout and all benchmark build flags are preserved, so
the runners need no changes. A SHA with neither working model fails
loudly.
Post-vcpkg OpenMS builds link TOPP tools against dynamic vcpkg libraries under build/vcpkg_installed/x64-linux-dynamic/lib, which did not cross the artifact boundary: Smoke and OpenSwath failed at their first TOPP invocation with exit 127 (loader cannot resolve libboost/libxerces shared objects). Upload the vcpkg runtime lib directory with the binary artifact and add it to LD_LIBRARY_PATH in both benchmark jobs, preserving the existing build/lib path.
…ization-and-cleanup # Conflicts: # .github/workflows/benchmark.yml # benchmark/report/test_generic_renderer.py
Static stdlib-only generator reusing the ProteoBench comparison layer: a pinned reference view over the vendored snapshot (default) and an optional latest view from a CI shallow clone of upstream HEAD, with local Exp-1/Exp-2 results rendered alongside external submissions.
The Phase-2 port made the vcpkg runtime-lib operand unconditional in the du -sb artifact-size steps, so on legacy (pre-vcpkg) builds du exited 2 on the missing directory and pipefail failed the benchmark jobs after they had already passed. Make the operand conditional on the directory existing.
OpenMS SHAs in the vcpkg model install the wnetalign headers (pylmcf/wnet/wnetalign) and the onnxruntime overlay port only through VCPKG_MANIFEST_FEATURES, matching upstream's linux-x64-ci preset. Without them the plain build fails at the CMake generate step (PYLMCF/WNET/WNETALIGN_INC NOTFOUND, seen in CI run 35700644257) and the ONNX build cannot satisfy find_package(onnxruntime CONFIG REQUIRED), which the vcpkg branch of OpenMS' CMakeLists requires instead of the module/tarball path used in legacy mode.
The onnxruntime vcpkg overlay port builds ORT from source (dbg+rel), which exhausted the runner disk (0 MB during the debug build) and the 90-minute job budget. Drop the onnxruntime manifest feature and resolve find_package(onnxruntime CONFIG REQUIRED) against the pinned, SHA256- verified prebuilt release archive instead, staged at $RUNNER_TEMP/ort-prefix so the generated config package's lib64 / include-onnxruntime layout resolves real paths.
Baseline selection (issue #2, task 1): - pick_baseline now prefers the latest stored run of the same benchmark whose OpenMS version is an ancestor of the current run's version, resolved via the GitHub compare API (compare(base...head) status "ahead"), with per-render caching and graceful fallback to the previous timestamp rule when ancestry is unknown, offline, or rate-limited (403/429 disables further API use). - render gains --baseline (explicit run id | OpenMS SHA | result path) and --no-sha-baseline. Normalize + render in CI (issue #2, task 2): - all five normalizer invocations record real --run-at timestamps; smoke/openswath derive cache state from the actual vcpkg cache restore instead of hardcoding "none" (new build-openms output vcpkg_cache_restored). - new render-report job merges all benchmark-* result artifacts, renders report.html, uploads it, and writes the verdict into the job summary. Cold-cache acceleration: - both vcpkg build jobs seed .vcpkg-cache from upstream's published binary cache at archive.openms.de on Actions-cache miss (same manifest hash: hashFiles over vcpkg.json + vcpkg-configuration.json reproduces the Linux-X64-g++-vcpkg-0fa92544... tree), cutting from-source provisioning from ~90 min to minutes. Misses and unreachable archives fall back to source builds. Tests: 6 new renderer tests (28/28); proteobench + prose suites and 4/4 definitions still pass.
5 tasks done
nej1gotnochill
force-pushed
the
feat/openswath-normalization-and-cleanup
branch
from
September 25, 2026 15:40
b332c52 to
8f291ce
Compare
Collaborator
|
Validated SHA ancestor baseline selection in CI with OpenMS e4b9609 … against baseline run 31881021123 at f1768367 …. 
|
Collaborator
Author
|
Really great work ! Good to see that you validated the SHA ancestor baseline selection in CI with |
timosachsenberg
added this pull request to stack #8
October 1, 2026 09:43
Collaborator
Author
|
Closing this PR as the work has been absorbed into main through #4 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests