Add image QC and transcript QC subworkflow (ported from nf-xenium-processing dev HEAD) - #205
Add image QC and transcript QC subworkflow (ported from nf-xenium-processing dev HEAD)#205an-altosian wants to merge 7 commits into
Conversation
|
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 4.0.3. For more documentation on how to update your pipeline, please see the Synchronisation documentation. |
|
I will review. It might be that there needs to be some slight changes, because I will open up a PR for our QC tool as well, which brings some changes to the pipeline also. I will ping you in slack @an-altosian. |
❌ nf-test failed with latest Nextflow versionNote Tests with Nextflow's latest version failed but it will not cause a CI workflow failure.
See the full run for details. |
nnie-altos
left a comment
nnie-altos
left a comment
There was a problem hiding this comment.
Thanks for pulling this across.
I am reviewing this as one of the people who contributed the original QC code, so I focused on the three things @an-altosian asked about plus whether the port carried the source over faithfully. Haven't looked at nf-core conventions, module structure or the Quarto renderer - @heylf those are yours.
Adaptive morphology loading - all good, nothing to change.
GPU handling - needs changes. The num_gpus rework dropped two things that were quietly making an upstream limitation safe. Comment on conf/base.config:107.
Transcript QC - the port itself is good. I diffed it against the source: 73 lines, all dependency-inlining and renaming, no maths changed. There are some potential technical points, but they're not from this PR and several are already fixed on our side. I'd rather work through those with @nmalwinka @ChristelKrueger and re-port once they're confirmed and corrected upstream, than patch them here.
The subworkflow split is clean and moving the params into ext.args keeps the modules parameter-agnostic.
And separately a few things that came in with the port:
- Snapshots are out of date. run_qc = true by default, so QC runs in every pipeline test, but no snapshot mentions qc/ and tests/ isn't touched. The failing nf-test shard might be this, not flake.
- Nothing runs the Python in CI. Both module tests are -stub only, no unit tests. None of my comments would've been caught.
- Commit message mentions pytest tests for the transcript QC maths - can't find them in the diff?
- params.tile_size is gone. --roi-size 35 is hardcoded (modules/local/image_qc/main.nf:65). Intentional?
- QC reports never reach MultiQC. ch_qc_reports is empty at workflows/spatialaxe.nf:114 and never filled, so :773-779 is dead.
- Containers: pinned twice (modules/local/image_qc/main.nf:8, conf/modules.config:404), and missing from conf/containers_*.config - arm64/singularity fall back to the amd64 image, which won't work with cupy-cuda12x.
- No docs. docs/usage.md and CHANGELOG.md not updated. nf-core lint will want the CHANGELOG entry.
One upstream issue that's newly reachable here:
image_qc.py only looks for morphology inside morphology_focus/ (:8894-8913), but workflows/spatialaxe.nf:236-256 says spatialaxe also supports morphology_focus.ome.tif at bundle root. Those bundles have no such folder, so :8910 raises. bin/transcript_qc_processing.py:142 fails them too by requiring the folder - which it never opens.
QC is on by default, so that's a pipeline failure for v1-bundle users. workflows/spatialaxe.nf:246-256 already resolves the layout - could that be reused and the path passed in?
@an-altosian - this matches an upstream snapshot from mid-May, and we've since fixed the threshold scaling, GPU concurrency capping, memory bounds, and unified the two morphology loaders. Any reason for sourcing that rather than current upstream?
@heylf - you mentioned a QC tool PR coming that touches this area. Worth settling ordering before these fixes land?
| // knob — null requests no accelerator (image_qc.py runs its CPU path), an | ||
| // integer N requests N GPUs (the script auto-detects the visible devices). | ||
| withLabel:process_gpu_qc { | ||
| accelerator = { params.num_gpus ? (params.num_gpus as int) : null } |
There was a problem hiding this comment.
params.num_gpus doesn't work. The script never looks at it - it calls detect_gpu_ids() (bin/image_qc.py:8964) and grabs every GPU it can see.
Upstream this was harmless because the label was gated on params.use_gpu and pinned to GPU queues, so "no GPU requested" meant the task landed on a CPU queue with no devices visible. This PR drops both.
Consider wiring --num-gpus into image_qc.py and setting CUDA_VISIBLE_DEVICES (precedent at modules/local/segger/train/main.nf:28), or drop the param and document that image QC uses whatever the scheduler gives it.
| containerOptions = { "--shm-size ${task.memory.toGiga()}g" } | ||
| queue = { params.gpu_queue ?: null } | ||
| } | ||
| withLabel:process_gpu_single { |
There was a problem hiding this comment.
The aws profile routes process_gpu (:286) and process_gpu_single (:293) but not process_gpu_qc. The comment at :286 notes these blocks replace rather than merge, so image QC ends up requesting an accelerator on the default CPU queue - the job is either unschedulable or lands somewhere without GPUs.
Suggest a matching block after :298 with queue = { params.gpu_queue ?: null }.
| "default": false | ||
| }, | ||
| "num_gpus": { | ||
| "type": "integer", |
There was a problem hiding this comment.
num_gpus has no default and no allowed range in the schema, so nothing stops --num_gpus -1. That would reach conf/base.config:107 and set accelerator -1 on the task. Adding "minimum": 0 rejects it at launch instead.
Also worth moving num_gpus and the image_qc_* params out of segmentation_options (:119-122) - they're QC settings sitting under a "Segmentation options" heading.
| "type": "integer", | |
| "type": "integer", | |
| "minimum": 0, | |
| "default": null, |
| ].findAll { it }.join(' ') | ||
| } | ||
| publishDir = [ | ||
| path: { "${params.outdir}/${params.mode}/qc/image_qc" }, |
There was a problem hiding this comment.
publishDir double-nests. ext.prefix = 'image_qc' (:385) and outdir = prefix (modules/local/image_qc/main.nf:24), so the published folder is itself named image_qc - files end up at .../qc/image_qc/image_qc/.
Same pattern for transcript QC at :414-419.
Minor knock-on: modules/local/quarto/main.nf:37 passes SAMPLE_PUBLISHED_OUTDIR as .../qc/image_qc, so it's one level off. Harmless today since the notebook never reads that param, but it'll bite whoever wires it up.
|
|
||
| # EXACT CODE FROM ORIGINAL NOTEBOOK - Save metrics | ||
| metrics = { | ||
| "total_molecules": int(num_molecules), |
There was a problem hiding this comment.
The rename didn't reach the outputs. The script and module are transcript now, but the files it writes and the keys in the metrics JSON still say molecule:
- JSON keys: total_molecules, selected_molecules (:663), min_molecules_per_cell (:669) - the report reads these at assets/notebooks/transcript_qc.qmd:70-75
- Figure files: num_molecules_per_feature.* (:402), num_molecules_per_cell.* (:600), nucleus_molecule_fraction_per_cell_distribution.* (:504)
These are the bits users and downstream scripts actually read, so changing them after release would break people. Nothing depends on them yet, so this is the cheap moment to finish it.
- GPU: wire --num-gpus into image_qc.py + set CUDA_VISIBLE_DEVICES in the module so num_gpus actually caps device use; add process_gpu_qc to the aws profile GPU-queue routing; add schema minimum:0. - publishDir: stop double-nesting (qc/image_qc/image_qc -> qc/image_qc) for both QC analysis steps. - Finish molecule->transcript rename in outputs (metrics JSON keys + figure filenames) and the report's reads; fix an exposed figure-name collision. - Wire ch_qc_reports from the QC reports so the MultiQC collection isn't dead. - Schema: move QC params into a dedicated qc_options group. - Make ROI tile size configurable via params.image_qc_roi_size. - Commit the previously-missing pytest unit tests for the transcript QC maths. - Docs: CHANGELOG entry + usage.md QC-mode section. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
- GPU: wire --num-gpus into image_qc.py + set CUDA_VISIBLE_DEVICES in the module so num_gpus actually caps device use; add process_gpu_qc to the aws profile GPU-queue routing; add schema minimum:0. - publishDir: stop double-nesting (qc/image_qc/image_qc -> qc/image_qc) for both QC analysis steps. - Finish molecule->transcript rename in outputs (metrics JSON keys + figure filenames) and the report's reads; fix an exposed figure-name collision. - Wire ch_qc_reports from the QC reports so the MultiQC collection isn't dead. - Schema: move QC params into a dedicated qc_options group. - Make ROI tile size configurable via params.image_qc_roi_size. - Commit the previously-missing pytest unit tests for the transcript QC maths. - Docs: CHANGELOG entry + usage.md QC-mode section. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
7a3cce1 to
f1dd859
Compare
|
Thank you @nnie-altos , this is completely on me. I did not pull the latest commits when I was working on the PR. I have re-made the PR using our latest internal commits. Please check! |
CI on PR nf-core#205 surfaced two lint failures and container failures: - nf-core lint: the three Altos contributor entries in the manifest had a trailing comma after their last field. Groovy tolerates it but nf-core's linter converts the manifest to JSON and choked (JSONDecodeError). This also cascaded into spurious files_exist igenomes failures because the config-aware exemption depends on a parseable manifest. Removing the trailing commas clears both. - nf-test: QC processes died with exit status 1 (`Command 'ps' required by nextflow to collect task metrics cannot be found`). The micromamba-based QC containers lacked procps. Added conda-forge::procps-ng to all three QC environment.yml files (source of truth for docker/singularity/conda). Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
…utors parsing The two failing lint tests on PR nf-core#205 were template_strings: the QC Quarto notebooks use doubled braces as Python f-string escapes (emitting pandoc callout-note divs), which nf-core lint flags as leftover Jinja. Add the documented per-file exclusions in .nf-core.yml. Verified locally with nf-core/tools 4.0.3 (the CI-pinned version): 678 passed, 0 failed. Also reorder the Altos manifest.contributors entries so a scalar field (affiliation) is last: tools 4.0.3 converts the contributors block to JSON with naive bracket replacements, and an entry ending in a list field (contribution) gets its closing brackets corrupted, breaking RO-Crate metadata generation (reproduced offline against the 4.0.3 transform). This was a non-fatal log error, not the failing lint test. Add .mypy_cache/ to .gitignore (local cache; its binary content trips merge_markers when linting a dirty working tree). Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
There was a problem hiding this comment.
I think its better to move this to bin/ because it is directly linked to the .py script.
There was a problem hiding this comment.
I think its better to move this to bin/ because it is directly linked to the .py script.
| cpus = { 2 * task.attempt } | ||
| memory = { 42.GB * task.attempt } |
There was a problem hiding this comment.
Doesnt look like ery specific resource. Can you pick or define a label from/for base.config.
| cpus = { 2 * task.attempt } | ||
| memory = { 42.GB * task.attempt } |
There was a problem hiding this comment.
Doesnt look like ery specific resource. Can you pick or define a label from/for base.config.
| // ---------------------------- image + transcript QC ----------------------- | ||
|
|
||
| withName: '.*IMAGE_QC:ANALYSIS' { | ||
| ext.prefix = 'image_qc' |
| tuple( | ||
| meta, | ||
| // parameters (NON_GENE_PREFIX is the key the module's arg builder reads) | ||
| ["NON_GENE_PREFIX", params.neg_control_prefix], |
There was a problem hiding this comment.
all params.* in the main.nf needs to go into here
Lines 41 to 45 in b05a853
or the conf/modules.config
There was a problem hiding this comment.
sorry, is this used somewhere? Or is this just a script you use internally for testing the tool? I would generally place it in bin/tests/ for now.
| [ | ||
| name: 'Malwina Prater', | ||
| contribution: ['contributor'], | ||
| affiliation: 'Altos Labs' |
There was a problem hiding this comment.
if you have orcid please add
| [ | ||
| name: 'Nell Nie', | ||
| contribution: ['contributor'], | ||
| affiliation: 'Altos Labs' |
There was a problem hiding this comment.
if you have orcid please add
| [ | ||
| name: 'Christel Krueger', | ||
| contribution: ['contributor'], | ||
| affiliation: 'Altos Labs' |
There was a problem hiding this comment.
if you have orcid please add
|
Following up on @nnie-altos's review, where she flagged the inherited transcript QC issues as ours Items 1 to 11 below are @nnie-altos's findings, from her code read of the ported scripts. My part was confirming each one against current upstream, working out which already have a fix there, and measuring the size of the first one. The only item that's mine is the Three items from that list are already resolved: the GPU focus-map concurrency cap, the GPU OOM Summary: 4 of the 11 items below already have a fix upstream. A transcript QC re-sync closes them
Fix already exists upstream, so a re-sync picks these up1. Noise threshold is under-scaledStatus: fixed upstream. Worth flagging that the upstream fix is @an-altosian's own commit, from
The bias is toward passing QC. I measured it across our own sections: it bites on roughly half Upstream extracts the scaling into its own function, which is also the only way to test it: a test 2. Host memory is unbounded on the default pathStatus: fixed upstream. Upstream restructured this around a streaming reader and retired the flag, since streaming reads 3. Figure names don't match their contentsStatus: fixed upstream. Upstream splits these into three distinctly named outputs, one each for cell size, genes per cell 4. Unassigned% and per-FoV quality aren't computedStatus: fixed upstream. Upstream computes a per-gene unassigned breakdown and per-FoV Not fixed upstream either, so worth fixing in both repos
One new thing, post-dating that reviewCI is red on It came in with the Happy to open issues for any of these that shouldn't gate this PR, and happy to help with the |
…v HEAD Ports the QC layer from the internal nf-xenium-processing repo at dev HEAD 5e35cae onto nf-core/spatialaxe dev, alongside the existing spoQC subworkflow. Faithfulness to the source was the priority, because the previous attempt shipped a stale, partial snapshot: its transcript QC was 695 lines against upstream's 1344, its report notebook 333 against 1861, and transcript_stream.py and the threshold config were missing entirely. Reviewers spent their time re-deriving issues that current upstream had already fixed. Provenance, verified rather than asserted: - bin/snr_metrics.py, bin/xenium_image_qc_report.qmd, bin/roi_image_qc_thresholds.yaml, bin/transcript_stream.py: byte-identical to upstream 5e35cae. - bin/image_qc.py: exactly three diff hunks vs upstream, none touching logic — a typing import, removal of the xenium_helpers import region, and the inlined helper block replacing it. The seven inlined symbols are byte-identical to upstream's xenium_helpers.utils (AST-compared). - bin/transcript_qc_processing.py, bin/transcript_qc.qmd, bin/transcript_qc_thresholds.yaml: upstream plus a mechanical molecule -> transcript rename. The rename is recorded as an ordered, replayable rule set; replaying it on pristine upstream reproduces the notebook and threshold YAML byte-for-byte, so re-syncing stays a mechanical step rather than a merge. xenium_helpers is dropped completely: the needed helpers are inlined into each script verbatim, with no shared shim module. This picks up four upstream fixes the stale port missed: the under-scaled noise threshold (now a separate scaled_noise_threshold function), unbounded host memory (now a streaming reader via transcript_stream.py), figure names that did not match their contents, and uncomputed unassigned% / per-FoV QV. Pipeline integration: - nf-core quarto/notebook renders both reports. The older quartonotebook module is deprecated upstream and now hard-asserts, so it is not used. - The versions topic collector now keeps only (process, tool, version) tuples: quarto/notebook also publishes a versions.yml path into that topic, which broke the destructuring. - Resources come from dev's own labels rather than ad-hoc numbers. Image QC carries process_gpu_qc (GPU request only) plus process_xl, which matches what a 230K-cell production bundle actually needed; transcript QC uses process_high, matching upstream's allocation. - process_gpu_qc is defined in conf/base.config and routed in the aws profile; it previously existed unrouted and, on the old branch, inside an unclosed block, so it had never actually applied. - The QC processes read no params.* — every knob arrives as ext.* from conf/modules.config. Four of those keys must stay explicit because the module tests them negated or interpolates them unconditionally. - publishDir no longer double-nests: analysis and report both land in <mode>/qc/<step>/. - 18 QC parameters are declared under the schema's QC options group, with a minimum on image_qc_gpus so a negative device count is rejected at launch; run_qc moved out of Segmentation options. Also fixes four pre-existing mypy failures unrelated to QC (Pillow 12 dropped Image.NEAREST; a rebound tuple annotation; a headerless-CSV None; two str arguments declared as Path), so `make check` passes. mypy.ini excludes only the four vendored QC scripts, since annotating them locally would fork them from upstream. Validated: full -stub run in qc mode executes all four QC processes and publishes reports and metrics; make check clean (ruff, mypy over 27 files, pre-commit); nf-core schema lint 468 passed / 0 failed. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
Reviewer items that needed real fixes (numbering from the PR review):
- 6: --non-gene-prefix and --stain-names had no `nargs`, so the multi-value
form that nextflow_schema.json documents and the module emits (it splits on
';' into separate tokens) exited with argparse error 2. Added nargs="+".
Verified by driving parse_args: two values previously raised SystemExit(2)
and now parse to a list.
- 9: the CuPy import guard caught only ImportError. The container ships
cupy-cuda12x with CUDA runtime wheels but no driver, which raises
RuntimeError/CUDARuntimeError at import — neither an ImportError — so the
intended CPU-only default path aborted instead of falling back. Broadened
the guard. Verified with a stub cupy raising RuntimeError at import.
- 7: estimate_min_mols_per_cell raises from np.quantile on an empty `> mode`
slice, reachable with zero/one cell or zero retained genes (numpy raises
IndexError, not ValueError). Guarded at the two call sites rather than inside
the vendored helper, so the helper stays byte-identical to upstream. A
degenerate input now yields "not computable" (None -> null in the metrics
JSON, cutoff line omitted) instead of an exception; returning the helper's
min_value would have published a real-looking noise floor for empty data.
Normal inputs are unchanged (verified: 48 before and after).
- 5: --non-gene-prefix was parsed and then ignored — the selection hard-coded
startswith("NegControl"). Threaded the prefixes through
scaled_noise_threshold and changed the script default from "NegControlProbe"
to "NegControl" so the default reproduces the previous hard-coded behaviour
exactly and matches the pipeline default; wiring it without that change would
have moved the threshold. Verified: the parameter now changes the threshold
across prefix sets, and the default is identical to the old literal.
- 11: the ROI threshold channel keys were bridged by a hand-written
lowercase-to-mixed-case dict, and a drift between it and the YAML silently
fell back to a 15% WARN fraction where the YAML says 40% — 2.67x stricter
DAPI grading with nothing in the output revealing it. Replaced with a
case-insensitive resolve that raises when a populated config is missing an
expected channel. All resolved values verified unchanged for all three
channels, with and without YAML, and for every XOA-version branch. The same
pattern in _pick_critical was fixed too; without it every intensity floor
silently reverted to the defaults, discarding the XOA-4 values.
Items 8 and 10 needed no code change and are recorded for the reviewers:
10 (the report instructed filtering at QV<=20 while nothing filtered) is gone
in current upstream, and the notebook now states explicitly that the threshold
uses only negative-control features — so the claim matches the code. 8
(--stain-names parsed then discarded) is confirmed still dead: every figure
title and metric key is hard-coded, and the metric keys are the contract the
threshold YAML and the report read, so making stain names rewrite them would
break that contract. Left for a deliberate decision.
CI: nothing ran the QC Python code, which is why these defects survived. Added
.github/workflows/python-tests.yml. It builds a runtime environment rather than
the linting one, because both scripts import matplotlib/tifffile/scanpy at
module scope and cannot even be collected otherwise. The two QC module
environments cannot be co-installed (image QC pins zarr 2.x while scanpy pulls
zarr 3.x), so bin/tests/environment.yml declares one coherent set.
Also repairs the Python suite itself: tests/test_xenium_patch/test_stitch_-
transcripts.py imported the script from an upstream-only
modules/local/.../resources/usr/bin path that does not exist here, so it failed
collection and took the whole pytest run down with it (17 passed, 64 errors
before this). It now loads bin/stitch_transcripts.py, raises a clear error if
that ever moves, and skips cleanly where shapely/sopa are absent instead of
aborting collection.
Verified: 87 passed / 1 skipped in the CI-equivalent environment; make check
clean; the vendored helper blocks re-checked byte-identical to upstream
(7/7 and 11/11 symbols) so re-syncs stay mechanical.
Co-authored-by: Malwina Prater <mprater@altoslabs.com>
Co-authored-by: Nell Nie <nnie@altoslabs.com>
Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
…ns contract Three defects that only a real (non-stub) run could surface, since -stub never executes the analysis scripts nor renders the notebooks: - The four ported bin/ scripts were committed mode 644 while every other script in bin/ is 755. Nextflow runs bin/ scripts directly, so image QC failed immediately with exit 126 "Permission denied". Recorded the executable bit in the index, not just the working tree. - The nf-core quarto/notebook module requires the notebook to export a versions.csv (package,version) which it folds into the versions topic; the upstream notebooks do not, because upstream renders with its own quarto module. Both reports rendered and then the task failed. Added a small cell to each notebook that reads the versions at runtime via importlib.metadata, matching the pipeline's rule that versions are never hardcoded. This is a third documented divergence in the transcript notebook, so the recorded rename replay no longer reproduces it byte-for-byte and the cell has to be re-applied on re-sync. - The versions collector I had hardened to ignore non-tuple topic entries was discarding the versions.yml that quarto/notebook builds from that very versions.csv — requiring an artifact and then binning it. It now handles both shapes: tuples are formatted directly, and file entries are read in as YAML content (softwareVersionsToYAML parses content, not paths). Validated on a production Xenium bundle (126K cells, 7.8 GB): the pipeline completes, image QC reports status ok with its full metrics set, and both reports render with no failure banner — image_qc.html 58 MB / 19 figures, transcript_qc.html 3.8 MB / 11 figures. The report section of the software versions table now carries the notebook's own numpy, pandas and ipython versions. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
…ner env - Drop `params.stain_names` (reviewer item 8). The script parses `--stain-names` and then discards it: every figure title and metric key is hard-coded, and those keys are the contract the threshold YAML and the report read, so making stain names rewrite them would break that contract. Shipping a knob with no effect is worse than not having it, so the parameter, its schema entry and the `ext`/arg wiring in both modules are removed. The vendored scripts keep their now-unreachable CLI option rather than diverging further from upstream; each module records why it is not passed. - `modules/local/transcript_qc/environment.yml`: `pyarrow` 21.0.0 -> 20.0.0. The pinned set was unsolvable — pyarrow 21 conflicts with scanpy 1.11.4 on python 3.11.0 — so the declared environment could not build at all. Earlier runs passed only because they used the previously published image. pyarrow 20 still provides libarrow-acero, which transcript_stream.py needs. - Bump the transcript QC container to 1.1.0 and point both references at it. The environment genuinely changed (scanpy moved from pip to conda, numba and tqdm added, six pip packages dropped), so 1.0.0 no longer describes it; overwriting a published tag would make earlier runs irreproducible. Image QC stays at 1.0.0 — its environment is unchanged and does solve. - CHANGELOG and docs/output.md: describe the QC subworkflow, every published output file, and the new parameters, which were previously undocumented. - tests/.nftignore: ignore the QC report HTMLs and figure PDFs, which embed a render timestamp and a PDF CreationDate respectively. Names and existence are still asserted. - Rebaseline all six pipeline snapshots: the QC layer adds published outputs by design, and spoqc.nf.test runs `mode = 'qc'` so its output set changes too. Validated: full nf-test pipeline suite green under the CI-pinned Nextflow 26.04.6, with the snapshots stable across an update and a separate verify pass; make check clean; 87 pytest tests pass. Note for reviewers: SPOQC_ANALYSIS_OVERVIEW fails intermittently (three of seven runs here, on unmodified dev as well as on this branch) with `ValueError: Too many bins for data range` raised from spoqc/additional_analysis/cluster_analysis.py. It histograms clustering output derived from the spatialdata parquet writers, which tests/.nftignore already documents as not run-to-run reproducible, so the 5-bin histogram occasionally sees a degenerate range. It is unrelated to this PR — it reproduces with the QC subworkflow call removed — but it will flake in CI. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
…st env Two CI failures on this PR, both self-inflicted: - nf-core lint template_strings flagged bin/xenium_image_qc_report.qmd: the QC report notebooks use doubled braces as Python f-string escapes (they emit pandoc callout-note divs), which the check reads as leftover Jinja. Added the documented per-file exemptions. - The Python tests workflow could not create its environment. It still merged the two QC module environments, which cannot be co-installed — transcript QC's anndata 0.12.2 requires pyarrow <21 while image QC pins pyarrow 21.0.0. That is also the real constraint behind the unbuildable container environment fixed in the previous commit; anndata, not scanpy, is what caps pyarrow. bin/tests/environment.yml was already made self-contained for exactly this reason, so the workflow now uses it alone. Also ignore .mypy_cache/, whose binary cache files make the merge_markers lint check report false positives locally. Verified: template_strings passes (366 tests, 0 failed) and bin/tests/environment.yml solves standalone.
A shard failed with `docker: failed to register layer: write /xeniumranger-xenium4.0/external/cellranger/lib/bin/cr_ana: no space left on device` while pulling quay.io/nf-core/xeniumranger:4.0. The QC layer adds two sizeable images (image QC ~4.6 GB, transcript QC ~3 GB), and a shard that also pulls xeniumranger — which bundles cellranger — no longer fits in the 80 GB volume. Raise it to 120 GB. The same shard passed on latest-everything, whose runner had different residual disk, which is why this looked intermittent rather than systematic. Follow-up worth doing separately: the image QC container carries transitive weight it does not need (botocore ~121 MB, sympy ~81 MB, statsmodels ~56 MB and PySide6 ~43 MB, pulled in via the optional pysal/esda stack used only when Moran's I is enabled, which is off by default). Trimming those would cut the image without touching GPU support.
Carried over from the earlier QC branch, which had this fix; it is not on dev. The gene-panel fallback built `bundle + '/gene_panel.json'` with file(checkIfExists: true) inside a .map closure. Nextflow evaluates map closures for every element regardless of whether the resulting channel is consumed downstream, and the existence check fires when file() is constructed, so any bundle without the optional gene_panel.json — or a remote tarball input — failed at workflow init even with relabelling disabled. This is what broke every Atera-mode run internally. Guard the fallback with do_relabel: when relabelling is off, ch_gene_panel keeps its channel.empty() initialisation, and its only consumer already sits inside `if (do_relabel)`. Behaviour with relabelling enabled is unchanged. Also untrack .vscode/settings.json and ignore .vscode/ — editor settings should not be version-controlled.
cfff775 to
caba42f
Compare
an-altosian
changed the title
4 participants
Description
Adds the image QC and transcript QC layer, ported from the internal
nf-xenium-processingrepository at dev HEAD5e35cae, ontodevalongside the existing spoQC subworkflow.This branch was rebuilt from
devand from upstream HEAD, replacing the earlier contents of this PR. The previous version was built from a stale upstream snapshot: its transcript QC was 695 lines against upstream's 1344, its report notebook 333 against 1861, andtranscript_stream.pyand the threshold config were missing entirely. @nnie-altos and @nmalwinka spent their review time re-deriving issues that current upstream had already fixed, so those four items now close by provenance rather than by patching.Provenance, verified rather than asserted
5e35caebin/snr_metrics.py,bin/xenium_image_qc_report.qmd,bin/roi_image_qc_thresholds.yaml,bin/transcript_stream.pybin/image_qc.pyxenium_helpersimport, and the inlined helper block replacing itbin/transcript_qc_processing.py,bin/transcript_qc.qmd,bin/transcript_qc_thresholds.yamlmolecule→transcriptrenamexenium_helpersis dropped entirely: the needed helpers are inlined into each script byte-identically to upstream (AST-compared, 7/7 and 11/11 symbols), so a re-sync stays mechanical. The rename is recorded as an ordered, replayable rule set — replaying it on pristine upstream reproduces the notebook and threshold YAML byte-for-byte.Review items from #205
Closed by porting from upstream HEAD: 1 under-scaled noise threshold (now
scaled_noise_threshold), 2 unbounded host memory (now a streaming reader), 3 figure names not matching contents, 4 unassigned% / per-FoV QV, 10 the report's QV-filtering claim (gone upstream).Fixed here, each with before/after evidence:
--non-gene-prefixwas ignored (selection hard-coded toNegControl). Now threaded through, with the script default changed toNegControlso the default reproduces the previous behaviour exactly — wiring it naively would have moved the threshold.--non-gene-prefix/--stain-nameshad nonargs, so the documented multi-value form exited with argparse error 2.estimate_min_mols_per_cellraised fromnp.quantileon an empty slice (IndexError, notValueError). Guarded at the call sites so the vendored helper stays byte-identical; degenerate input now reports "not computable" rather than publishing a plausible-looking noise floor.ImportError; a driverless host raisesRuntimeError, so the intended CPU-only path aborted.intensity_warnis 0.40, not 0.35) with nothing in the output revealing it. Replaced with a case-insensitive resolve that raises on a genuinely missing channel; all resolved values verified unchanged. The same pattern in_pick_criticalwas also live.params.stain_namesis dropped. It was parsed and discarded, and the metric keys it would have to rewrite are the contract the threshold YAML and report read.Also addressed: GPU handling now gates on
use_gpuand caps devices via--max-gpus, withprocess_gpu_qcdefined inconf/base.configand routed in theawsprofile (it previously existed unrouted, and on the old branch inside an unclosed block, so it had never applied);publishDirno longer double-nests; QC parameters moved into the schema's QC group with aminimumonimage_qc_gpus; resources come fromdev's own labels rather than ad-hoc numbers; and the modules read noparams.*.Nothing ran the QC Python in CI
That is why several of the above survived. This adds
.github/workflows/python-tests.ymlandbin/tests/(upstream's own suite, adapted only for layout). It builds a runtime environment, because both scripts import matplotlib/tifffile/scanpy at module scope and cannot otherwise be collected.It also repairs the existing suite:
tests/test_xenium_patch/test_stitch_transcripts.pyimported the script from an upstream-onlymodules/local/.../resources/usr/binpath that does not exist here, so it failed collection and took the whole pytest run down (17 passed, 64 errors before). Result now: 87 passed, 1 skipped.Validation
status: okwith its full metrics set, both reports render with no failure banner (58 MB / 19 figures, 3.8 MB / 11 figures)make checkclean (ruff, mypy over 27 files, pre-commit);nf-core pipelines lintschema checks cleanFour pre-existing
mypyfailures unrelated to QC are fixed somake checkpasses (Pillow 12 droppedImage.NEAREST; a rebound tuple annotation; a headerless-CSVNone; twostrarguments declared asPath).mypy.iniexcludes only the four vendored QC scripts, since annotating them locally would fork them from upstream.Known issues, not introduced here
SPOQC_ANALYSIS_OVERVIEWflakes. It failed in three of seven runs withValueError: Too many bins for data rangefromspoqc/additional_analysis/cluster_analysis.py, and reproduces with the QC subworkflow call removed. It histograms clustering output derived from the spatialdata parquet writers, whichtests/.nftignorealready documents as not run-to-run reproducible, so the 5-bin histogram occasionally sees a degenerate range. @heylf — this will flake in CI regardless of this PR.quay.io/dongzehe/image_qc:1.0.0andtranscript_qc:1.1.0, both public. They need migrating to the nf-core org before release. The transcript tag is1.1.0because its environment genuinely changed and1.0.0was already published from a different one — and becausepyarrow=21.0.0made the declared environment unbuildable (it conflicts with scanpy 1.11.4), now pinned to 20.0.0.Reviewers
@nnie-altos @nmalwinka @ChristelKrueger for the QC content, @heylf for nf-core conventions and the spoQC interaction.