Add image QC and transcript QC subworkflow - #205
Open
an-altosian wants to merge 14 commits into
Open
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. |
Collaborator
|
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
suggested changes
Aug 7, 2026
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.
Suggested change
| "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.
an-altosian
added a commit
to an-altosian/spatialaxe
that referenced
this pull request
Aug 7, 2026
- 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>
IDE settings should not be version-controlled. Remove the tracked .vscode/settings.json from the index and ignore the directory going forward. Kept separate from the QC subworkflow feature.
Surfaced by adding mypy to the typecheck gate. Unrelated to the QC subworkflow feature; kept as a standalone commit. - xenium_patch_stitch_postprocess: DictWriter fieldnames could be None - utility_downscale_morphology: output_shape union of 2/3/N-tuples - baysor_create_dataset: sampled_transcripts must be a Path, not str
Add a quality-control layer to the pipeline: a QC subworkflow that runs image QC and transcript QC (renamed from molecule QC) on a Xenium bundle and emits their analysis directories and HTML reports. - New local modules: image_qc (IMAGE_QC_ANALYSIS, GPU-optional via params.num_gpus; morphology loading adapts to 1 or 3 channels), transcript_qc (TRANSCRIPT_QC_PROCESSING), and quarto (report renderer). Each ships main.nf + meta.yml + environment.yml + stub tests. - New subworkflows: image_qc, transcript_qc, and a thin qc wrapper. - environment.yml files are fully pinned and validated to build via conda. - Version reporting via topic channels; per-report versions.yml for the Quarto reports. - Wire QC(ch_bundle_path) into workflows/spatialaxe.nf under the qc layer. - Add num_gpus + image/transcript QC params (nextflow.config + schema). - pytest unit tests for the inlined transcript QC math. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
….yml Each module's Dockerfile builds its container (micromamba base + the pinned environment.yml, Quarto env vars set) so the images are reproducible and ready to be rebuilt / migrated to the nf-core org. 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>
… dev d71e0cd) The image QC module was ported from a stale mid-May upstream snapshot. Re-port it verbatim from current upstream, which since fixed threshold scaling, GPU concurrency capping (--max-gpus), host-memory bounds, tile streaming, and unified the morphology loaders. - bin/image_qc.py, bin/snr_metrics.py, assets/notebooks/xenium_image_qc_report.qmd: replaced verbatim with current upstream (byte-identical). - modules/local/image_qc/main.nf: re-ported faithfully; thin nf-core adaptations (topic-channel versions, conda + quay container, when: guard). - environment.yml: re-derived (cupy-cuda12x==14.0.1) + folded Quarto report stack. - params reconciled to upstream: add tile_size, image_qc_gpus, image_qc_stream_tiles, image_qc_figures, image_qc_figure_source_tables; drop num_gpus, image_qc_roi_size. process_gpu_qc accelerator gated on use_gpu + capped by image_qc_gpus. - mypy.ini: exclude the vendored QC analysis scripts from type-checking so they stay byte-identical to upstream and re-syncable. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
an-altosian
force-pushed
the
feat/qc-subworkflow-port
branch
from
August 7, 2026 21:17
7a3cce1 to
f1dd859
Compare
Collaborator
Author
|
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>
Adopt the official nf-core quartonotebook module (vanilla, no patch) instead of the bespoke local quarto module: - install modules/nf-core/quartonotebook (pinned in modules.json) - rewire IMAGE_QC and TRANSCRIPT_QC subworkflows to its 4-input contract: parameters as a map (params.yml / --execute-params) and the analysis output directory (+ ROI YAML for image QC) staged via input_files; all per-sample channels derive from one upstream channel so emission order stays aligned - pass the notebook as a plain path from the QC wrapper (meta2 tuple dropped) - per-caller container overrides remain in conf/modules.config (the stock quartonotebook container lacks pandas, which both reports import); publishDir scoped to *.html so params.yml / notebook copies are not published - emit report from REPORT.out.html; delete modules/local/quarto Validated: -preview DAG, full -stub run, and a real (non-stub) qc-mode run on the nf-core test bundle — both reports render with figures and no failure banner. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
… container Real-data validation (non-stub qc-mode runs) surfaced two failures that stub tests cannot catch: - image_qc.py failed at import: the verbatim upstream script depends on the xenium_helpers package, which this pipeline deliberately does not ship (QC scripts must be self-contained). Inlined the three segmentation-software label helpers (read_xenium_analysis_sw_version, read_xenium_major_version, resolve_segmentation_software, plus private helpers) verbatim from the same upstream commit (nf-xenium-processing dev d71e0cd); only annotations were modernised to X | None style. mypy.ini comment updated accordingly. - generate_tissue_mask crashed with 'OSError: GL ES 2.0 library not found': napari-simpleitk-image-processing lazily imports napari -> vispy, which dlopens a GL ES library at import even in headless runs. Added conda-forge::libgles to the image_qc environment and set ES2_LIBRARY=/opt/conda/lib/libGLESv2.so.2 in the Dockerfile (vispy checks ES2_LIBRARY before ctypes.util.find_library, which cannot see /opt/conda/lib). With both fixes the qc-mode run on the nf-core test bundle completes with status ok, the full metrics suite, and a banner-free report. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
Pillow 12 type stubs no longer declare the top-level Image.NEAREST alias, which fails mypy. Image.Resampling.NEAREST is the supported form since Pillow 9.1 and is available in the module's cellpose container.
Reprocessing a production Tower dataset (BII-002-715 R2_control, 126K cells) exposed a silent failure: QUARTONOTEBOOK's process_low label (12 GB) OOM-killed quarto's second pandoc pass (--embed-resources re-reads the entire rendered HTML) on the ~60 MB image QC report. The child is SIGKILLed, quarto exits 1 with an empty stderr, and the complete HTML is left in the work dir — a confusing signature. Confirmed by A/B: identical render succeeds unconstrained and fails under --memory 12288m. Give both REPORT processes explicit cpus/memory (42 GB, scaling with task.attempt) in conf/modules.config. Config-only: the nf-core module stays unpatched. With this fix the full qc-mode run on the production bundle completes: 58 MB image QC report with 19 figures, no failure banner. 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>
…unstable files The pipeline-level snapshots were baselined before the QC layer existed, so every 25.04.0 shard failed with additions-only diffs (qc/ directories, reports, versions.yml entries). Regenerated all six pipeline test snapshots under the CI-pinned toolchain (Nextflow 25.04.0, nf-test --profile=+docker). Local A/B/B reproducibility runs surfaced two flake classes, both fixed at the narrowest possible scope while keeping name/existence assertions strict: - content-unstable files (md5 changes every run): QC report HTMLs embed render dates and matplotlib figure PDFs embed CreationDate; MultiQC moved under <mode>/qc/ so its known-unstable outputs escaped the existing .nftignore patterns. Added QC-era paths to tests/.nftignore. - set-unstable files: the pre/post-XeniumRanger MultiQC _report_data stats tables and _report_plots exports are sometimes omitted within a run. Ignored those directories in the coordinate test's stable_name list (the report html itself remains asserted) and content-ignored them wholesale in .nftignore. Verified: coordinate test green 3 consecutive times, full suite green, under the exact CI invocation. ro-crate-metadata.json: regenerated; its embedded README copy now carries the Altos contributor credits (enabled by the manifest.contributors parse fix). Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
The gene-panel fallback channel built `bundle + '/gene_panel.json'` with file(checkIfExists: true) inside a .map closure that runs for every sample even when the channel is never consumed (Nextflow evaluates map closures regardless of downstream subscription, and the HTTP/file existence check fires at file() construction on all Nextflow versions — verified identically on 25.04.0 and 25.10.4). Any bundle without the optional gene_panel.json, or a remote tarball URL input, therefore killed the workflow at init even with relabelling disabled. Guard the fallback construction with do_relabel: when relabelling is off, ch_gene_panel keeps its channel.empty() initialisation, whose only consumer already sits inside if (do_relabel). Behavior with relabelling enabled is unchanged. Verified: with a tarball-URL bundle and no test profile, the gene_panel init error is gone (only the required-file checks remain); full nf-test suite green with unchanged snapshots. Co-authored-by: Malwina Prater <mprater@altoslabs.com> Co-authored-by: Nell Nie <nnie@altoslabs.com> Co-authored-by: Christel Krueger <ckrueger@altoslabs.com>
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
3 participants
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
Adds a quality-control layer to the pipeline: a
QCsubworkflow that runs image QC and transcript QC (renamed from "molecule QC") on a Xenium bundle and emits their analysis directories and rendered HTML reports.What's included
Modules (
modules/local/, each withmain.nf+meta.yml+environment.yml+ stub tests):image_qc—IMAGE_QC_ANALYSIS: focus / SNR / morphology metrics + figures. GPU-optional viaparams.num_gpus; morphology loading adapts to 1 or 3 channels.transcript_qc—TRANSCRIPT_QC_PROCESSING: per-transcript and per-cell QC metrics + figures.quarto— shared HTML report renderer.Subworkflows (
subworkflows/local/):image_qc,transcript_qc, and a thinqcwrapper that runs both. The wrapper holds no pre/post-segmentation logic — the caller (workflows/spatialaxe.nf) decides when QC runs.Config / wiring: new
params.num_gpus+ image/transcript QC params (withnextflow_schema.json), aprocess_gpu_qclabel,conf/modules.configpublish paths +ext.args,conf/roi_image_qc_thresholds.yaml,assets/notebooks/*.qmd, andQC(ch_bundle_path)wired into the QC layer ofworkflows/spatialaxe.nf.Design notes
params.num_gpus:null→ CPU (noaccelerator);N→accelerator = N.image_qc.pyauto-detects visible devices, so no script GPU flag is needed.versions.ymlthat its report displays.environment.yml, fully pinned and validated to build via conda/micromamba (R stack dropped — the reports use the Jupyter/Python engine).Containers
Two images are built from the module
environment.ymlfiles (each includes Quarto so it renders its own report):quay.io/dongzehe/image_qc:1.0.0quay.io/dongzehe/transcript_qc:1.0.0These are hosted on the author's quay.io namespace for now and should be migrated to the nf-core org before release (the
environment.ymlfiles are the source of truth).Validation
ruff,mypy,pre-commit, andnf-teststub tests pass; the full pipeline-stub -previewbuilds the DAG with the QC processes correctly wired.Credits
Contributed by the Altos Labs team — Malwina Prater, Nell Nie, Christel Krueger (added as
contributorsinnextflow.configand the README), building on the existing pipeline.