feat(qpx): split into diann + openmsconsensus subtools; add openms-consensus converter - #45
Conversation
…nsensus Restructure the bigbio/qpx module into subtools so it serves both pipelines from one shared module: - qpx/diann/ (was qpx/, process QPX_EXPORT -> QPX_DIANN) — DIA-NN - qpx/openmsconsensus/ (new, QPX_OPENMSCONSENSUS) — OpenMS consensusXML The openms-consensus converter reads the consensusXML directly (correct per-PSM run via map_index/id_merge_index; no -out_qpx run mis-assignment, OpenMS#9872), used by the quantms DDA (LFQ/TMT) pipeline. Container bumped to qpx 1.1.1 (openms-consensus + structured modifications / localization scores). Stale diann snapshot removed for regeneration.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR renames the DIANN QPX process and updates its container. It adds the ChangesQPX module extension
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Nextflow
participant QPX_OPENMSCONSENSUS
participant qpxc
participant MuDataGeneration
Nextflow->>QPX_OPENMSCONSENSUS: provide consensusXML, SDRF, and project accession
QPX_OPENMSCONSENSUS->>qpxc: convert consensus data
qpxc-->>QPX_OPENMSCONSENSUS: produce QPX dataset
QPX_OPENMSCONSENSUS->>MuDataGeneration: generate MuData file
MuDataGeneration-->>QPX_OPENMSCONSENSUS: return MuData output
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/bigbio/qpx/openmsconsensus/tests/main.nf.test (1)
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep an executable conversion test in CI.
Lines 21-22 skip the real-data test when no fixture parameter exists. Default CI then validates only stub artifacts. Add a small immutable consensusXML and SDRF fixture, configure its default URL, and remove this skip before downstream workflows rely on this module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/bigbio/qpx/openmsconsensus/tests/main.nf.test` around lines 13 - 22, Add small immutable consensusXML and SDRF fixtures for the OpenMS conversion test, configure the default value for params.test_data.proteomics.qpx.openms_consensusxml to reference the fixture archive, and remove the assume(zipUrl != null) skip in the test("Should convert OpenMS consensusXML to QPX Parquet + MuData") flow so CI always executes the conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/bigbio/qpx/openmsconsensus/environment.yml`:
- Around line 5-7: Pin the qpx dependency in the environment.yml pip
dependencies to an existing release, such as 1.0.2, so Conda execution is
reproducible and no longer installs the unconstrained latest version; keep it
aligned with the supported qpx release used by the workflow.
---
Nitpick comments:
In `@modules/bigbio/qpx/openmsconsensus/tests/main.nf.test`:
- Around line 13-22: Add small immutable consensusXML and SDRF fixtures for the
OpenMS conversion test, configure the default value for
params.test_data.proteomics.qpx.openms_consensusxml to reference the fixture
archive, and remove the assume(zipUrl != null) skip in the test("Should convert
OpenMS consensusXML to QPX Parquet + MuData") flow so CI always executes the
conversion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31e704c5-ce71-4739-a821-8ec5b6a85688
⛔ Files ignored due to path filters (1)
modules/bigbio/qpx/tests/main.nf.test.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
modules/bigbio/qpx/diann/environment.ymlmodules/bigbio/qpx/diann/main.nfmodules/bigbio/qpx/diann/meta.ymlmodules/bigbio/qpx/diann/tests/main.nf.testmodules/bigbio/qpx/diann/tests/nextflow.configmodules/bigbio/qpx/openmsconsensus/environment.ymlmodules/bigbio/qpx/openmsconsensus/main.nfmodules/bigbio/qpx/openmsconsensus/meta.ymlmodules/bigbio/qpx/openmsconsensus/tests/main.nf.testmodules/bigbio/qpx/openmsconsensus/tests/nextflow.config
…dentation - environment.yml: replace unpinned 'pip: qpx' with 'bioconda::qpx=1.1.1' so -profile conda resolves the same artifact as the biocontainers image (built from the bioconda recipe) and satisfies nf-core lint (tool=version). - openmsconsensus/main.nf: the versions.yml heredoc used space indentation, but <<- strips tabs not spaces, so it would emit an indented (malformed) top-level YAML key; align to col-0 like the diann subtool.
The detection matched only modules/org/module/ (2 levels) and looked for modules/org/module/tests/main.nf.test, so subtool modules like bigbio/qpx/diann and bigbio/qpx/openmsconsensus got zero CI. Walk up from each changed file to the nearest ancestor with tests/main.nf.test, which handles both flat and subtool layouts.
…b + snapshot nf-test has no assume() method (the real test errored with MissingMethodException). Remove the fixture-guarded real test (which also needs a hosted consensusXML+SDRF fixture) and keep the stub test, which passes and now has a committed snapshot. The real openms-consensus conversion + MuData build are validated out-of-band against the qpx 1.1.1 container; a real nf-test can be added once the fixture is hosted (mirroring the diann subtool).
qpx is published to ghcr.io/bigbio/qpx on every release (available immediately on tag), whereas the bioconda->biocontainers->Galaxy-depot chain lags by hours-to-days. Point both subtools at a single ghcr.io/bigbio/qpx:1.1.1 image (serves Docker natively and Singularity via docker://), matching quantms. -profile conda still resolves bioconda::qpx from environment.yml. Unblocks the module + downstream pipelines on release without waiting for biocontainers.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 108-110: Replace the `${TEST_FILES[@]}` regex membership check
with an explicit loop over `"${TEST_FILES[@]}"` that compares each existing
element to `test_file`, appending only when no match is found. Update the
deduplication logic in the surrounding test-file collection block while
preserving insertion order and validating distinct entries correctly.
In `@modules/bigbio/qpx/openmsconsensus/tests/main.nf.test`:
- Around line 13-18: Add a fixture-based execution test in the OpenMS consensus
test suite, replacing the TODO stub. Provide small consensusXML and SDRF
fixtures, run the real conversion and MuData build process, and assert the
resulting qpx_dataset, mudata, and versions outputs before release.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 366b9c03-8b6f-4a2d-b258-18d7046fc204
⛔ Files ignored due to path filters (1)
modules/bigbio/qpx/openmsconsensus/tests/main.nf.test.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
.github/workflows/ci.ymlmodules/bigbio/qpx/diann/environment.ymlmodules/bigbio/qpx/diann/main.nfmodules/bigbio/qpx/openmsconsensus/environment.ymlmodules/bigbio/qpx/openmsconsensus/main.nfmodules/bigbio/qpx/openmsconsensus/tests/main.nf.test
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/bigbio/qpx/openmsconsensus/main.nf
- modules/bigbio/qpx/diann/main.nf
- modules/bigbio/qpx/diann/environment.yml
| if [[ ! " ${TEST_FILES[@]} " =~ " ${test_file} " ]]; then | ||
| TEST_FILES+=("$test_file") | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use explicit element comparison for deduplication.
At Line 108, ${TEST_FILES[@]} expands one word per array element inside [[ ... =~ ... ]]. When TEST_FILES already contains multiple entries, the condition does not reliably evaluate one complete membership string. Processing a third unique module can fail or produce an invalid conditional expression.
Use an explicit loop over "${TEST_FILES[@]}".
Proposed fix
- if [[ ! " ${TEST_FILES[@]} " =~ " ${test_file} " ]]; then
+ already_seen=false
+ for existing_test_file in "${TEST_FILES[@]}"; do
+ if [[ "$existing_test_file" == "$test_file" ]]; then
+ already_seen=true
+ break
+ fi
+ done
+ if [[ "$already_seen" == false ]]; then
TEST_FILES+=("$test_file")
+ fiVerify the current expression with three distinct test files:
#!/usr/bin/env bash
set -euo pipefail
TEST_FILES=()
for test_file in \
modules/bigbio/qpx/diann/tests/main.nf.test \
modules/bigbio/qpx/openmsconsensus/tests/main.nf.test \
modules/bigbio/onsite/tests/main.nf.test
do
if [[ ! " ${TEST_FILES[@]} " =~ " ${test_file} " ]]; then
TEST_FILES+=("$test_file")
fi
done
test "${`#TEST_FILES`[@]}" -eq 3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 108 - 110, Replace the
`${TEST_FILES[@]}` regex membership check with an explicit loop over
`"${TEST_FILES[@]}"` that compares each existing element to `test_file`,
appending only when no match is found. Update the deduplication logic in the
surrounding test-file collection block while preserving insertion order and
validating distinct entries correctly.
| // TODO real-data test: mirror the diann subtool's test once a small OpenMS | ||
| // consensusXML + SDRF fixture (`<ACC>.zip` with `openms.consensusXML` + | ||
| // `sample.sdrf.tsv`) is hosted under the PRIDE pmultiqc example-projects, then | ||
| // add a test that downloads it and asserts qpx_dataset/mudata/versions. The | ||
| // openms-consensus conversion + MuData build have been validated out-of-band | ||
| // against the qpx 1.1.1 container on a real 2-replicate consensusXML. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add a fixture-based execution test before release.
The stub test does not execute the conversion command or the MuData build. Add a small consensusXML and SDRF fixture that runs the real process and validates qpx_dataset, mudata, and versions.
I can generate the fixture test when the hosted test data is available.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/bigbio/qpx/openmsconsensus/tests/main.nf.test` around lines 13 - 18,
Add a fixture-based execution test in the OpenMS consensus test suite, replacing
the TODO stub. Provide small consensusXML and SDRF fixtures, run the real
conversion and MuData build process, and assert the resulting qpx_dataset,
mudata, and versions outputs before release.
qpx 1.1.1 renamed --duckdb-threads/--duckdb-max-memory to
--max-cpus/--max-memory. Bumping the diann subtool to 1.1.1 exposed the
old flags ('No such option --duckdb-threads'); update the command.
CodeRabbit flagged the env pinning a version not yet available. qpx 1.1.1 is on PyPI now but bioconda hasn't autobumped it, so bioconda::qpx=1.1.1 doesn't resolve. Pin the PyPI release via pip to match the GHCR container (1.1.1) and keep -profile conda working; pinning 1.0.2 (the latest bioconda) would drop openms-consensus entirely. Revert to bioconda::qpx=1.1.1 once bioconda publishes it.
|
Thanks @coderabbitai — addressed both: 1. 2. Executable conversion test / fixture (Nitpick). The invalid |
|
Tip For best results, initiate chat on the files or code changes. @ypriverol Thanks for the detailed clarification.
The removal of 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
Summary
Restructure the shared
bigbio/qpxmodule into subtools so one shared module serves both pipelines:modules/bigbio/qpx/diann/— the existing DIA-NN converter (qpxc convert diann), process renamedQPX_EXPORT→QPX_DIANN(used by quantmsdiann).modules/bigbio/qpx/openmsconsensus/— new,QPX_OPENMSCONSENSUSrunningqpxc convert openms-consensusover an OpenMS consensusXML + SDRF (used by quantms DDA / LFQ+TMT).Why a new subtool: the openms-consensus converter reads the consensusXML directly and resolves the correct run per PSM (
map_index/id_merge_index/ feature-element runs), so it avoids the OpenMS-out_qpxPSM run mis-assignment (OpenMS#9872) and duplicate PSMs (OpenMS#9871). Both subtools share the same output contract (qpx_output/*+*.h5mu+versions.yml).Container bumped to qpx 1.1.1 (openms-consensus + structured modifications / localization scores).
Follow-up dependencies (flagged, not done here)
1.1.1--pyhdfd78af_0container resolves (the build number may need adjusting to the actual biocontainers build). Currently 1.1.0 is the latest release.openmsconsensusreal test needs a small consensusXML + SDRF zip hosted like the DIA-NN one (under the PRIDE pmultiqc example-projects), wired viaparams.test_data.proteomics.qpx.openms_consensusxml. Until then the real test is skipped (assume(...)) and only the stub test runs. The stalediannsnapshot was removed so nf-test regenerates it (container/process-name changed).bigbio/qpx→bigbio/qpx/diann(QPX_EXPORT→QPX_DIANN); quantms installsbigbio/qpx/openmsconsensusand drops its local module.Related: bigbio/qpx#237, bigbio/qpx#239, OpenMS#9871, OpenMS#9872.
Summary by CodeRabbit
New Features
Updates
Tests