Repository navigation
Preserve zarr version in register command - #198
Closed
mattersoflight wants to merge 3 commits into
Closed
mattersoflight wants to merge 3 commits into
mattersoflight wants to merge 3 commits into
Conversation
Add version parameter to create_empty_hcs_zarr() and detect input zarr version in register_cli() to ensure output version matches input (v2→v2, v3→v3). This maintains compatibility with both OME-NGFF v0.4 (zarr v2) and v0.5 (zarr v3) datasets. Changes follow the pattern from waveOrder PR#519: - Add optional version parameter to create_empty_hcs_zarr (default "0.4") - Pass version to open_ome_zarr when creating output plate - Detect input version from source plate in register_cli - Backward compatible: existing calls use default "0.4" Pair-coded-with: Soorya Pradeep <@Soorya19Pradeep> Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
8 of 10 tasks
- Convert shape, scale, and chunks to native Python types for zarr v3 compatibility (zarr v3 requires native Python ints/floats, not numpy types) - Fix position existence checking by using try/except instead of unreliable zgroup membership check - Fix channel appending to apply to all positions, not just the last one Fixes KeyError when accessing positions and TypeError when creating arrays with numpy int64 values in zarr v3 format. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Collaborator
|
I have tested the PR on a Hummingbird dataset with the following conditions: |
Soorya19Pradeep
force-pushed
the
preserve-zarr-version-register
branch
from
February 27, 2026 17:28
16b5a89 to
bea47c9
Compare
Collaborator
|
I have added the changes to concatenate to the same PR, as this is a very small number of lines of code as well. It was tested on zarr version 3 dataset with multiple positions. |
tayllatheodoro
marked this pull request as draft
April 1, 2026 21:58
mattersoflight
marked this pull request as ready for review
April 6, 2026 03:13
mattersoflight
added a commit
that referenced
this pull request
Apr 6, 2026
3 of 17 tasks
ieivanov
requested changes
Apr 7, 2026
Collaborator
There was a problem hiding this comment.
This PR modifies the biahub create_empty_hcs_zarr method which we've deprecated in favor of iohub create_empty_plate. Instead, we should merge #215 first and use the create_empty_plate version argument to create an output zarr store matching the input version.
This PR should not be merged, but it may be useful to include the added tests.
This was referenced Apr 13, 2026
Closed
ieivanov
marked this pull request as draft
April 21, 2026 20:01
5 tasks done
Collaborator
|
Superseded by #241 |
ieivanov
added a commit
that referenced
this pull request
Aug 20, 2026
Re-pinning imaging-qc to its merged main (06ec56d) brought in more than the iohub floor #199 was opened for: Phase 20 (#198) removed the wave mechanism, and its changelog names four things it breaks — "plan.json readers, `consolidate --wave-id`, a config declaring `scope:` on a metric group, and a driver that includes `finalize_wave` by name". This driver did all four, so QC was broken on the new pin until now. * plan.json is v5: `{version, stage, items[]}`, one flat list. `waves[]`, `wave_id`, `scope` and `finalize_after` are gone from the envelope, and `step_id` is the metric group's name with no `w{N}_` prefix. qc.nf reads items directly and still refuses an unreadable envelope rather than defaulting to an empty list — a default would turn the next schema bump into a silent no-op that re-gates the previous run's table at exit 0. * The three wave-branched compute_step aliases collapse to one fan-out. * `finalize_wave` is deleted. It published wave 0's shards for wave 1 to read; with one lane it was a second full merge immediately before the one that counts, and `consolidate --wave-id` no longer exists to run it. * `scope: position` comes out of both metric groups — it is exit 2 now. Per-position shard filenames change with the prefix (`..._position_w0_default` -> `..._position_default`), so a QC directory holding un-consolidated pre-upgrade shards must be cleared before re-running: both names match the merge predicate and the stale one sorts first, so it would win every shared key at exit 0. Verified on Bruno against the same subset (rerun_phase20/ in organelle_dynamics/qc_runs_debug_2026-08-18_current_pipeline): 11 tasks, 0 failed — two fewer than before, being the finalize_wave pair — one report with both tabs and all 7 plots, and metric values still identical to the 2026-08-12 run (360/360 and 134/134, max delta 0). `nextflow lint` reports no errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 of 6 tasks
ieivanov
added a commit
that referenced
this pull request
Sep 8, 2026
…ependency (#298) * refactor: QC generate-report-spec and Nextflow QC subworkflow modules Move generate-report-spec from nf.py into biahub/qc.py as a standalone `biahub generate-report-spec` command. Bring qc.nf and qc_processes.nf from wire-nextflow, updating the NF process to call the unified CLI instead of `biahub nf generate-report-spec`. QC is different from other steps: it wraps the external imaging-qc-pipeline CLI, so no --init/--cluster pattern is needed. Only the report-spec generation is biahub-owned orchestration logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(qc): reconcile qc_processes.nf with imaging-qc v0.4 CLI (ADR-003) (#295) * fix(qc): reconcile qc_processes.nf with imaging-qc v0.4 CLI (ADR-003) imaging-qc v0.4 (ADR-003) renamed the QC CLI surface. Update the process subcommands and pin: - run_step: run-step -> compute - finalize_wave: finalize-wave -> consolidate - finalize_stage: finalize-stage -> consolidate + gate (the removed verb was those two behind one name; report stays separate in qc_report_wf) - pin: v0.3.2 -> v0.4.0a1 Behaviour verified end-to-end against a local imaging-qc checkout (--qc_project): v3 plan (waves 0,1), compute/consolidate/gate all exit 0, QC_SUMMARY intact, gate failure stays exit 0 (D-08). TEMPLATE GAP: the QC config templates (qc_base.yaml + stage files) are not in the refactor chain yet -- they still carry top-level `mode: full`, which imaging-qc v0.4 rejects (exit 2). Deleting that line must land wherever the templates are brought into refactor-qc. Not fixable here; the files don't exist on this branch. PIN NOTE: v0.4.0a1 tag is not cut yet. Do not use the default (non --qc_project) path until it exists. See ~/docs/biahub-reconciliation/. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(qc): rename process run_step -> compute_step (Copilot review) The process invokes `imaging-qc compute`; `run_step` no longer names what it does, and it fans out as `run_step_w0/w1/w2` in qc.nf. Align the process and its aliases to the verb. finalize_wave/finalize_stage keep their names -- they finalize a wave/stage and finalize_stage runs two verbs, so neither maps to a single CLI verb. Include block re-aligned. `nextflow lint`: no errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * feat(qc): standalone QC entrypoint + DynaCell templates (pixel + cell-count) (#297) * feat(qc): standalone qc-standalone.nf entrypoint + qc params entrypoint or params, so the QC submodule could not run. Add: - nextflow/qc-standalone.nf — manifest-driven entry (CSV: zarr_path,config_path; one row = one config on one zarr). Runs qc_stage_wf then qc_report_wf. Adapted to refactor-qc conventions (params.output, not output_dir). - qc params in nextflow.config: stages_manifest, qc_project, qc_config_dir, qc_chunk_size, qc_report_static, qc_report_dir. Verified end-to-end on DynaCell mantis-v2 subsets via -profile local + a local imaging-qc v0.4 checkout (--qc_project): plan -> compute (renamed compute_step) -> finalize_wave -> finalize_stage -> report, all exit 0, QC_SUMMARY intact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(qc): DynaCell QC config templates (pixel + cell-count) Updated from the wire-nextflow qc templates for the imaging-qc v0.4 surface. - qc_base.yaml — shared pixel-metric base (intensity_stats, signal_to_noise, blank, tenengrad, bleach_rate). Dropped top-level `mode: full` (removed in ADR-003 Phase 4) and migrated deprecated outlier_method global_zscore -> zscore. - qc_assembled_pixel_metrics.yaml — pixel QC on the assembled `<name>_imaging.zarr` (inherits qc_base, stage 5). - qc_tracking_cell_count.yaml — instance_count on the segmentation/tracking `<name>_tracking.zarr` (nuclei_prediction_labels, hard_floor 1 flags empty frames). Self-contained: an external --config resolves Hydra defaults only against its own dir, so imaging-qc's /common+/segmentation groups are inlined. Validated on the DynaCell subsets against known ground truth: pixel run flags the blank position and passes the clean control; cell-count reproduces [19,0,0,0,22] (3 empty frames on the slice drop list) and [34,34,34,36,38] (clean). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(qc): per-channel SNR floors on the assembled pixel template Brightfield SNR is not comparable to fluorescence, so a single scalar floor is wrong for a mixed store. Override signal_to_noise with a {channel: value} hard_floor keyed to the store's fluorescence channels; "BF - Oblique" is left unlisted (no floor). Requires `granularity: channel` on the gate so it keys by channel — a channel-pooled gate refuses the mapping (D35). Values are per-experiment starting points, not calibrated from any dataset. Verified: re-gating the DynaCell imaging store now emits per-channel low_signal_to_noise_<channel> flags (GFP + mCherry), BF excluded, verdict unchanged (blank position fails, clean control passes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * docs(qc): correct plan.json version in qc_stage_wf comment plan-stage emits v3 as of imaging-qc-pipeline v0.4.0a1; the pinned tag adds run_generation additively and leaves the waves/items shape this module parses unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(qc): drive compute_step memory from imaging-qc estimate-resources The QC nextflow hardcoded compute_step memory to 16 GB and never invoked imaging-qc's estimate-resources CLI (meta.memory_gb was always empty). Add an estimate_resources process that runs `imaging-qc estimate-resources` per (zarr, config) and join its estimate_gb 1:1 into the plan, so every wave item carries a data-driven per-task memory request. Verified end-to-end on 2026_05_27 (1 position): estimate-resources emits 8 GB, compute_step Slurm jobs request mem=8G, metrics land correctly (image: blank/signal_to_noise/tenengrad; tracking: instance_count). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(qc): move QC configs to nextflow/configs/qc, drop the qc_ prefix The QC templates sat under settings/nextflow_templates/qc/ while every other Nextflow config lives under nextflow/configs/. Move them there and strip the now-redundant qc_ filename prefix, since the directory already says qc. Hydra resolves `defaults:` entries by filename, so assembled_pixel_metrics.yaml had to follow the rename: qc_base -> base. No code or docs referenced the old location — config paths reach the pipeline at runtime through the stages_manifest CSV and --qc_config_dir. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(qc): depend on imaging-qc-pipeline directly, and call it bare `qc_cmd()` prefixed every QC task with `uv run --from 'imaging-qc-pipeline @ git+…'` (or `--project` when `--qc_project` pointed at a checkout). That made each of up to `maxForks` tasks re-resolve an environment — and re-fetch a git dependency — concurrently against one shared site-packages. The rest of the pipeline stopped doing this when `biahub_cmd()` went away; QC is the last holdout, so it follows the same environment contract: resolve once before launching, then call `imaging-qc` and `biahub` as bare commands. `imaging_qc_pipeline` therefore becomes a real dependency, in a `qc` extra: uv sync --extra qc The extra is deliberately NOT in `all`. imaging-qc-pipeline is a private repo with no PyPI release, and CI's `uv sync --group dev` resolves `all` with no credentials for it. It is pinned to the tip of the branch behind imaging-qc-pipeline#199, which relaxes that project's `iohub==0.3.2` to a floor — biahub needs `iohub>=0.3.11` for the gapped-shard-write fix, and two exact pins in one environment have no solution. Repoint at a main commit once #199 merges. `check_environment()` now takes the tools the caller actually runs, so mantis-v2.nf is not held to an `imaging-qc` it never calls and qc-standalone.nf fails at launch — not N tasks in — if either CLI is missing. `params.qc_project` has no remaining meaning and is gone. Two related fixes fell out: qc_processes.nf still imported `biahub_cmd` from common.nf, which no longer defines it, and the qc-standalone usage comment named `nextflow/qc.nf`, which is not this file's name. Verified in one Python 3.13 environment (`uv sync --extra qc`): both `biahub` and `imaging-qc` on PATH, sharing iohub 0.3.11, with plotly present for the default interactive report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(qc): migrate the QC configs to the pinned imaging-qc's schema Pinning imaging-qc exposed that these configs were written against an older one. Running each through `plan-stage`, then the full compute → consolidate → gate → report chain against a synthetic store, found four keys the pinned version refuses or ignores: - `bleach_rate` is not a metric any more (imaging-qc Phase 19 removed it and replaced it with nothing). It and its `temporal` metric group, gate block and two report entries are gone. `metric_group: default` is what this pipeline selects, so no stage loses a metric it was running. - `blank.gate.aggregation` is refused. imaging-qc's changelog names this file: "`qc_base.yaml:39-41` still sets all three and will exit 2 until biahub drops them." It was read nowhere even when accepted — `blank` is boolean, so its gate collapses Z by `any()` regardless. - `granularity: z` was retired in v0.5.0 in favour of `slice`, the same scope under one spelling. - `report.mode` is not a `ReportConfig` key, so it was warned about and ignored. Nothing here changes a threshold or which metrics run; the gates evaluate as before. Confirmed by the chain above: stage 5 gates fire per channel off the {channel: value} SNR floors (`low_signal_to_noise_raw GFP EX488 EM525-45`), stage 6 passes on a 2-instance frame against `hard_floor: 1`, and the report renders 6 plots across both tabs. `plan-stage` now emits plan.json v4, not v3. Only the waves/items keys qc.nf reads are touched by the bump, so the parsing is unchanged — the comment is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(qc): generate a report spec imaging-qc can actually render Running the report step end to end surfaced two ways `generate-report-spec` produced a spec that fails or renders nothing — both only after every compute task in the run had already finished. `qc_dir` pointed unconditionally at the external sibling `<stem>_qc/`. That directory only exists when imaging-qc ran with `--output-dir`; this pipeline passes none, so its tables land in the store's own `tables/qc/` group. imaging-qc searches both locations, but a per-tab `qc_dir` overrides that search, so every tab was read from a directory that does not exist — "No stage*_position.parquet found … skipping stage", an empty report at exit 0. It now prefers the in-store group when present, which is imaging-qc's own precedence in `_stage_table_dir`, and keeps the sibling as the fallback. `label` came from the parent directory name, which is empty for a store with no parent to read — a manifest written with bare relative paths. imaging-qc requires a non-empty label and exits 2, so the stem is the fallback. With both fixed, the two-tab spec renders 6 plots and a 357 KB index.html against a synthetic store, with no skipped stages and no unrecognized-key warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(qc): QC the assembled store from mantis-v2, one report per store QC now runs as part of a reconstruction. mantis-v2.nf gains `--qc_config`, off by default; pointed at a stage config it QCs the ASSEMBLED plate — the one store carrying every channel the pipeline produced. It gates on `assemble_done`, the same signal `track_wf` waits on, so QC runs CONCURRENTLY with tracking and never extends the critical path. A QC verdict also cannot fail the run: `imaging-qc gate` exits 0 whether positions pass or fail (measured, with fail=4), recording the verdict in the store's `tables/qc/` tables and a QC_SUMMARY line. Only a broken config or a genuine compute error exits non-zero, which is what should stop a pipeline. One report per store, so the report-spec generator goes away. A spec exists to put N stores in one multi-tab report; with a single store per report, `imaging-qc report <store> <dir>` covers it — which is what imaging-qc's own qc_report.nf does. `biahub/qc.py`, its 7 tests, the `generate-report-spec` command and the `generate_report_spec` process are all deleted, and with them the `qc_dir` resolution problem that a generated spec created. `qc-standalone.nf` no longer calls `biahub` at all, and its reports no longer wait on a collect() barrier: each store's report follows that store, into a `<store>_report` sibling. `report` is handed a config DIRECTORY, not the config file. imaging-qc's report verb does not compose Hydra `defaults:` from a single file, so a config inheriting its `report:` block from base.yaml loses every metric plot and renders only the outlier heatmap — at exit 0, silently. Measured: file → 1 plot, directory → 5. Two defaults follow from where the parallelism actually is: - `qc_chunk_size` 10 → 0, one compute task per position. imaging-qc dispatches a position's (T, C) units through a thread pool sized from `max_concurrent`, which defaults to the task's CPU affinity, so a position is already parallel inside its node. Chunking T only multiplies jobs: 100 positions x T=100 is 100 tasks at 0 and 1000 at 10. - `compute_step` gains `cpus` (`qc_cpus`, default 8). It had none, so that thread pool saw one CPU and walked a position's timepoints serially. Verified end to end on a 4-position, 4-channel, T=6, Z=5 store using the exact commands these processes emit: plan-stage at chunk 0 yields one item per position with no chunk args, the four computes and both finalizes run clean, gate reports positions=4, and the report renders all 5 plots into the `<store>_report` sibling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(qc): one unified report, and QC of the tracking store in mantis-v2 Restores the single multi-tab report the 2026-08-12 DynaCell run produced, which the previous commit dropped when the scope was one store. mantis-v2 now QCs both the assembled plate (`--qc_config`) and the tracking store (`--qc_track_config`), and both land as tabs of one report at `${output}/qc/report`. Either param alone works; neither means no QC. Each store waits only on the step that wrote it — the assembled plate on assemble_done (the signal track also waits on, so image QC still runs concurrently with tracking), the tracking store on track_done — so adding tracking QC does not serialise anything that was parallel. THE SPEC IS WRITTEN AT LAUNCH, IN NEXTFLOW. `qc_report_spec()` in modules/qc.nf is a plain function, not a process and not a `biahub` CLI call: its content is fully determined before any compute runs, from params and the pipeline's own directory layout. So a malformed spec fails at launch instead of after every compute task has finished, which is how the two bugs in the old generator were found in the first place. `qc_dir` is DECLARED as each store's own `tables/qc/` group rather than probed on disk, since this pipeline passes no `--output-dir` and a probe run before the tables exist would guess wrong. Configs gain `stage{N}_` filename prefixes, which are load-bearing. imaging-qc resolves a tab's report settings with `_discover_config_in_dir`, which prefers files matching `*stage{N}*` and otherwise takes any YAML with a `report:` section — base.yaml. Without the prefix the stage-6 tab silently rendered stage 5's report block: no instance_count plot at all, and "Signal-to-Noise Ratio" labels in a cell-count tab, at exit 0. Verified on real data both ways. Tested on Bruno against an index-aligned subset of 2026_05_27_A549_SEC61B_TOMM20_G3BP1_ZIKV (2 positions; see organelle_dynamics/qc_runs_debug_2026-08-18_current_pipeline): 13 tasks, 0 failed, one report task instead of two. The report carries both tabs and 7 plots — 5 for stage 5, plus instance_count_timeseries and the heatmap for stage 6 — with all three metric labels present. Metric values still reproduce the 2026-08-12 run exactly: 360/360 stage-5 rows and 134/134 stage-6 rows identical, max delta 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(qc): group QC configs one directory per step, not by stage number The `stage{N}_` filename prefixes are gone. They encoded imaging-qc's internal table namespace in a filename, so renumbering a stage meant renaming files, and they said nothing about what a reader actually cares about: which pipeline step a config QCs. nextflow/configs/qc/assemble/{base,pixel_metrics}.yaml nextflow/configs/qc/track/cell_count.yaml This still fixes the bug the prefixes were introduced for, by a different route. `imaging-qc report` resolves a tab's report settings by scanning the directory it is handed and taking the first YAML with a `report:` section, so what matters is that a tab's directory holds only that step's configs. One shared directory made both tabs resolve to base.yaml — the cell-count tab rendered pixel metric labels and no instance_count plot, at exit 0. A per-step directory cannot, whatever the stage numbers are. The number now appears only in each config's `stage:` key: renumbering is that one edit, with no files to rename and no code to touch. Metrics still run on every channel in a store unless a config restricts them, so the assembled store is QC'd across all 6 channels. Note the consequence for the store-level summary, which imaging-qc aggregates across channels: prediction channels are near-binary and their SNR is enormous (nuclei_prediction mean 4500 vs 19 for GFP), so the aggregate reads ~880 rather than the ~17 of the raw channels. Per-channel `hard_floor` gates are unaffected — they key by name — and the per-slice values are all in the tables. Re-tested on the same subset (rerun_perstep/ in organelle_dynamics/qc_runs_debug_2026-08-18_current_pipeline): 13 tasks, 0 failed, one report with both tabs and all 7 plots including instance_count_timeseries, and metric values still identical to the 2026-08-12 run — 360/360 and 134/134, max delta 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * re-pin imaging-qc-pipeline * build(deps): constrain wasmtime<48 so imaging-qc imports Relocking after the merge moved wasmtime 47.0.1 -> 48.0.0, and every `imaging-qc` verb then failed before it started: ImportError: cannot import name 'DirPerms' from 'wasmtime' 48 removed `DirPerms`; itkwasm imports it unconditionally at module scope. The chain is entirely transitive — imaging_qc_pipeline -> ngff-zarr[validate] -> itkwasm -> wasmtime — and itkwasm asks for `wasmtime>=28.0.0` with no ceiling, with 1.0b200 its latest release, so there is nothing to upgrade to. A constraint rather than a dependency: nothing in biahub imports wasmtime. Reported upstream as imaging-qc-pipeline#205, where the ceiling belongs; drop this once itkwasm supports 48. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(qc): follow imaging-qc's flat plan and single merge (Phase 20) Re-pinning imaging-qc to its merged main (06ec56d) brought in more than the iohub floor #199 was opened for: Phase 20 (#198) removed the wave mechanism, and its changelog names four things it breaks — "plan.json readers, `consolidate --wave-id`, a config declaring `scope:` on a metric group, and a driver that includes `finalize_wave` by name". This driver did all four, so QC was broken on the new pin until now. * plan.json is v5: `{version, stage, items[]}`, one flat list. `waves[]`, `wave_id`, `scope` and `finalize_after` are gone from the envelope, and `step_id` is the metric group's name with no `w{N}_` prefix. qc.nf reads items directly and still refuses an unreadable envelope rather than defaulting to an empty list — a default would turn the next schema bump into a silent no-op that re-gates the previous run's table at exit 0. * The three wave-branched compute_step aliases collapse to one fan-out. * `finalize_wave` is deleted. It published wave 0's shards for wave 1 to read; with one lane it was a second full merge immediately before the one that counts, and `consolidate --wave-id` no longer exists to run it. * `scope: position` comes out of both metric groups — it is exit 2 now. Per-position shard filenames change with the prefix (`..._position_w0_default` -> `..._position_default`), so a QC directory holding un-consolidated pre-upgrade shards must be cleared before re-running: both names match the merge predicate and the stale one sorts first, so it would win every shared key at exit 0. Verified on Bruno against the same subset (rerun_phase20/ in organelle_dynamics/qc_runs_debug_2026-08-18_current_pipeline): 11 tasks, 0 failed — two fewer than before, being the finalize_wave pair — one report with both tabs and all 7 plots, and metric values still identical to the 2026-08-12 run (360/360 and 134/134, max delta 0). `nextflow lint` reports no errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * remove cytoland comment * refactor(qc): fold qc_processes.nf into qc.nf, as every other module does Every step module in nextflow/modules/ keeps its processes and its workflow in one file — assembly.nf (3 processes, 1 workflow), reconstruct.nf (3, 1), virtual_stain.nf (3, 1), deskew/flat_field/tracking (2, 1). QC was the only split one, and nothing but qc.nf ever included qc_processes.nf, so the split bought nothing. It came from imaging-qc's packaged layout, which does separate subworkflows/qc_processes.nf from qc_stage.nf. That reason is gone: this driver has diverged from theirs and was rewritten here for plan v5. The split also cost something. Its include list went stale twice during this work — first a `biahub_cmd` import of a function common.nf no longer defined, then the report-process rename — neither of which is expressible in one file. 268 lines: larger than the biggest step module (156) and well under notify.nf's 382, which is already one file. `nextflow lint` reports no errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(deps): upgrade imaging-qc to bd3e983 (Phases 21-22c) Four commits ahead of the previous pin, one of them breaking, and none of it touches this driver — checked rather than assumed, by running the pipeline against both revs on the same data. * Phase 21 (#200) is the breaking one: `METRIC_REGISTRY` goes 15 -> 11, dropping signal_to_noise_3d, michelson_contrast_3d, poisson_gaussian_noise_estimate and normalized_variance, and swapping max_intensity for frac_saturated. Our configs name none of them, so nothing hits its `Unknown metric` exit 2. The verdict changes it documents are for configs gating max_intensity or variance_of_laplacian; ours gate neither. * Phases 22/22b/22c rework the report. What the upgrade buys, measured on the subset: the report's plots directory goes 34.06 MB -> 4.96 MB, because chart fragments now share one plots/plotly.min.js instead of embedding 4.85 MB each (each fragment is 12-21 KB now); `_tmp_plots/` is cleaned up rather than left in every report; and the store table's compute status reaches the Dataset Overview. A per-tab slice drop-list table also lands, which this data cannot show — all 134 stage-6 slice rows pass, so there is nothing to render. What did not change: `--help` is byte-identical for all six verbs this pipeline calls, the consolidated table schema is identical, and the run is the same 11 tasks. Metric values still reproduce the 2026-08-12 DynaCell run exactly — 360/360 and 134/134, max delta 0 (rerun_bd3e983/ in organelle_dynamics/qc_runs_debug_2026-08-18_current_pipeline). The `wasmtime<48` constraint stays: imaging-qc-pipeline#205 is still open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(qc): QC is a reconstruction step, and Slack says so QC joins `reconstruction_steps()` when it is switched on, so the run-start announcement lists it and the per-step counters read N/7 instead of N/6. With `--qc_config`/`--qc_track_config` both unset the list is the six it always was, which is why the function takes the flag rather than hard-coding a seventh entry. ONE Slack message for the whole QC step, not one per store. The unified report is a single task gated on every store's finalize, so its completion is exactly "QC of all stores is done" — assemble and track both. The report directory stands in for the step's output zarr in the message, being the artifact worth naming. `qc_notify` is `Channel.empty()` when QC is off, so the mix stays unconditional. The run-end message needed no wiring to fire after QC: `onComplete` runs at session teardown, once every task in the DAG has finished, and the QC tasks are in the DAG like any other. Gating it on a QC channel would also be wrong — it has to fire on a failed run, where no QC channel ever emits. Said so in a comment, so it does not get "fixed" later. `directory_layout()` is untouched. QC writes its tables inside the stores it reads and its report to `${output}/qc/report`, so it owns no step directory. Tested with `nextflow run -preview`, which builds the DAG without executing tasks — the first time mantis-v2's QC block has been exercised at all. With QC on the graph constructs and the launch-time spec is written with both tabs, their in-store qc_dirs and their per-step config directories; with QC off the graph constructs and no spec is written. `nextflow lint` reports no errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(nextflow): assemble, track and QC are optional (#306) A step is now selected by the presence of its config, which is how QC already worked. Reconstruction proper — flat-field through virtual staining — is what this pipeline is for and stays mandatory; assemble, track and QC are deliverables some runs want and others do not. Tracking is the case from #306: it is tuned for A549 and its parameters (cellpose diameter, min/max area, linking distance) do not transfer to neuromasts, so a zebrafish run had to author a placeholder track.yml it did not want, pass it purely so the workflow would start, spend GPU wall time on a 4-track fan-out after assemble, and discard the result. Now it omits --track_config. A step cannot outlive the step whose output it reads, so three combinations are refused at launch, each naming the config to add or drop: --track_config needs --concatenate_config (tracking reads the assembled plate) --qc_config needs --concatenate_config (it QCs the assembled store) --qc_track_config needs --track_config (it QCs the tracking store) directory_layout() is untouched, as #306 asks: a skipped step does not renumber the ones around it, so `5-assemble` is `5-assemble` whether or not track runs and paths stay comparable across datasets. The Slack wiring follows the selection rather than a fixed list. `reconstruction_steps()` is gone; one `step_events` list is built from the wiring itself, carrying each performed step's label, done channel and artifact. The run-start announcement and the per-step messages both read it, so they cannot disagree about what ran, and the "i/n" counters are positions in that list rather than hard-coded numbers — a skipped step renumbers the messages without renumbering any directory. Verified with `nextflow run -preview`, which builds the DAG without executing tasks: A549 (assemble+track+QC) builds with a 2-tab report spec, neuromast (assemble+QC) builds with a 1-tab spec, reconstruction-only builds with no spec, and all three invalid combinations are refused with their message. `nextflow lint` reports no errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(skill): reconstruct-with-nextflow gains QC, and drops the track placeholder Now that steps are selected by config presence (#306), the skill says which set each family runs: A549 does assemble + track + QC, neuromast does assemble + QC and no tracking. The run template gains a `STEPS` variable that expands to the matching `--*_config` flags, so declining a step is deleting a word rather than authoring a config to satisfy a requirement. `nextflow/configs/zebrafish/track.yml` — a file whose own header said it existed only because mantis-v2 hard-required `--track_config` — is deleted, and caveats §4 now says to omit the flag instead of passing a placeholder and explaining away a discarded `4-track`. QC also has to be scaffolded as a TREE: `cp -r nextflow/configs/qc` keeps `assemble/` and `track/` apart, and flattening them would make every report tab render the same config's plots. The wrap-up step now reports the QC verdict — the per-store `QC_SUMMARY` line and the one-page report — and says explicitly that a failing gate does not fail the run, since `imaging-qc gate` exits 0 either way and the verdict is otherwise easy to miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(nextflow): step directory numbers are positions, not names `directory_layout()` hard-coded `0-flatfield` … `4-track`, `5-assemble`. With steps now optional (#306) that leaves holes — a neuromast run with no tracking produced `0,1,2,3,5` — and the numbers had already drifted from reality: `4-track` sorts before `5-assemble` while tracking runs AFTER assemble, since #282/#279 made it read the assembled plate. `step_directories(performed)` numbers each step by its position among the steps this run performs, in execution order. Only the order is written down; no number appears in the source. A run without tracking ends at `4-assemble`, and a pipeline that skips flat-field or deskew shifts everything after it down instead of leaving a gap. **This renames the A549 outputs**: `4-assemble` and `5-track`, where runs on disk today have `5-assemble` and `4-track`. That is the point rather than a side effect — under the fixed map the assembled store was `5-assemble` for A549 and would have been `4-assemble` for neuromast, so the same step had two names depending on whether an unrelated later step ran. Numbering by execution order gives it one. Existing outputs keep their names; nothing renames them, so a `-resume` against an old directory writes the new names and re-runs the steps. `notify_run_end` no longer builds `${output}/5-assemble/${dataset}.zarr` itself. The path is passed in, and is null when assemble did not run, in which case the "assembled:" line is simply absent. The skill follows: numbers stated in the plan, the deliverable described as `<N>-assemble`, the monitoring loop globbing `[0-9]-*/` rather than listing six fixed names, and a note that older A549 runs on disk say `5-assemble` so a comparison against one is not confusing. Verified with `-preview`: A549 emits 4-assemble + 5-track, neuromast emits 4-assemble, and both report specs carry the matching paths. `nextflow lint` finds no errors; the run template still parses as bash. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(skill): plain step flags in the run script, and the step list in the plan The `STEPS` variable and its case-to-flags loop are gone. The run script passes `--concatenate_config`, `--track_config`, `--qc_config` and `--qc_track_config` literally in the `nextflow run` call, so what a run performs is readable at the call site rather than assembled from a word list. Skipping a step is DELETING its line, not commenting it out, and the template says why: `#` inside a backslash-continued command does not start a comment — the continuation swallows it, every flag below is dropped (`-resume` included), and bash then tries to run the remainder as a command. Measured, not assumed: commenting one middle line turned `echo start --a 1 --b 2 --c 3` into `start --a 1` followed by `--c: command not found`. The agent notes the skip in a comment above the call instead, so the script still records what the run did. Defaults are per family and now stated as the step list itself: A549 runs assemble + track + QC (the template as written, no edit), zebrafish/neuromast runs assemble + QC and deletes the two tracking flags. The plan the agent presents before launching must write that list out in order, naming which optional steps are skipped and why, rather than naming a family and leaving the reader to infer it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * add .python-version and update run_mantis_v2 template * build(deps): fetch imaging-qc over HTTPS, so installing needs no SSH key The `qc` extra came from `ssh://git@github.com/...`, which asked every org member who wanted QC to generate an SSH key, upload it, and authorize it for SSO. Over HTTPS they authenticate with the GitHub CLI instead — already at /usr/bin/gh on Bruno — and uv picks the credential up through git: gh auth login # browser/device-code flow, SSO-aware gh auth setup-git # installs gh as git's credential helper Verified rather than assumed: with a COLD uv cache, no SSH agent and GIT_SSH_COMMAND disabled, `uv pip install "imaging_qc_pipeline @ git+https://..."` succeeds using only the gh token. The same install fails without a credential ("Could not read from remote repository"), so this changes who can install, not whether the repo is private. Nobody with keys loses anything: one local rewrite keeps ssh in use — git config --global url."ssh://git@github.com/".insteadOf "https://github.com/" The skill gains §1a-bis, which checks `gh auth status` BEFORE scaffolding rather than letting the failure surface as a raw git error mid-install, walks a new user through the two commands, verifies with `git ls-remote`, and — when the account simply lacks access to that repo, which no local setup can fix — says to ask for access and offers to continue without QC. Nothing else in the pipeline needs a GitHub credential, so this never blocks a reconstruction. The plan gains a matching item, so a run that cannot do QC says so instead of quietly dropping it. Re-verified end to end on the same subset after re-syncing from HTTPS: 11 tasks, 0 failed, and metric values still identical to the 2026-08-12 run (360/360 and 134/134). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build(deps): bump imaging-qc, and drop the local wasmtime constraint The new rev declares `wasmtime<48` itself, so biahub no longer needs a `constraint-dependencies` entry to keep itkwasm importable. The two changes are coupled and land together: with the old rev and no local constraint, `uv lock --upgrade-package wasmtime` resolves 48.0.0 and `imaging-qc` stops importing; with the new rev it resolves 47.0.1. `uv.lock` keeps wasmtime at 47.0.1 and repoints imaging-qc at the new rev. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(qc): the report spec names the config file, not its directory `qc_report_spec` passed each tab the config's parent directory because the report verb read a named file with one plain YAML load and never composed its `defaults:`, so a config inheriting its `report:` block from base.yaml arrived empty. The directory scan happened to find both files. That scan takes the FIRST YAML with a `report:` section, which made the directory layout load-bearing: point two tabs at one shared directory and both render the same block, so a cell-count tab gets pixel metric labels and no instance_count plot, at exit 0. The bumped imaging-qc composes a named file's defaults, so a tab can name the same file its compute steps were given. Each tab now resolves independently of what sits beside it on disk. Measured against this repo's own configs: with one directory per step the rendered archetypes are identical either way, so nothing changes today. With both steps' configs in one directory, the directory form gives the track tab `[intensity_stats, signal_to_noise, tenengrad]` and the file form gives it `[instance_count]`. Two comments went stale with the constraint they described. The config header's note that a `report:` block here would lose to base.yaml's is now inverted -- `_self_` follows `base`, so it merges over base key by key, verified against the composed config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(qc): map the QC trigger instead of combining it, and say what failed QC aborted a seven-hour run after every reconstruction step had finished. The trigger was joined with `combine`, which CONCATENATES the two items, so the producer's payload shape leaked into the tuple's arity: assemble emits one path -> [zarr, config, path] 3 params, fine track emits `| collect` -> [zarr, config, p1, p2, … p30] no such closure and Groovy could not spread that across `{ z, cfg, done -> }`: `MissingMethodException ... (LinkedList)`. Mapping the trigger reads nothing out of it, so no producer's payload can reach the tuple, and `.first()` makes a trigger a signal rather than a stream — one QC run per store however many items its producer emits. Reproduced in eight lines of Nextflow before fixing: with `combine`, assemble passes and track fails with exactly the production message; with `map`, both stores emit. `-preview` could not have caught this — it builds the DAG without executing operators, so the closure is never called with a real value. THE SLACK MESSAGE WAS WORSE THAN THE BUG. It read "reconstruction aborted: Possible solutions: any(), any(), any(groovy.lang.Closure), …" — Groovy's suggestion list, verbatim, as the entire diagnosis — for a run where 288 tasks succeeded, no task failed, and every output was on disk. Three fixes: * The title picks the last NON-BOILERPLATE line. Stack frames, `... N more` and Groovy's `Possible solutions:` list are dropped first, so a Groovy error yields "No signature of method: …" while a Python task failure still yields its last line ("ValueError: channel 'Phase3D' not in store") — the original reason for preferring the last line, kept. * The title says WHICH KIND of failure. A task failure names the process ("track_wf:run_track (D/3/001000) failed"); a pipeline error says "pipeline error, no task failed", so nobody goes looking for a broken position; and an interrupt is "interrupted", not "aborted", since Ctrl-C is the one case with no error message at all. * Stack frames are stripped from the detail. It is truncated from the front to keep a Python diagnosis at the tail, so a hundred `at java.base/…` lines would otherwise push out both the counts and the error itself. Each of the four shapes — Groovy, Python traceback, task exit, interrupt — was run through the helpers and checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style(nextflow): clear 42 of 50 lint warnings `nextflow lint` reported 50 warnings across 11 files. Cleared, by class: * 27 implicit closure parameters (`it`) given explicit names — `line`, `row`, `event`, `stdout_text`, `items`, `part`, `entry`, `arg`. * 9 unused parameters prefixed with `_`. Most are real: `_json`, `_summary`, `_n` and `_tf` are tuple slots the closure destructures past, and `_preprocess_done` is an ordering token the map deliberately drops. * 4 `Channel` factories lowercased to `channel`. * 1 unused variable in `collect_positions`, whose emit now carries the expression directly. Its caller already used the `collect_positions.out` form, so nothing downstream changed. Nothing here is inside a process `script:` block, so no task hash moves and a `-resume` of a run in flight is unaffected — checked, the diff contains no script-block lines. THE REMAINING 8 ARE LEFT DELIBERATELY. All are "Emit name should be omitted when there is only one emit", on the `done = …` emit of each step module. Omitting the name is not a local edit: a bare identifier in an `emit:` block is parsed as an emit NAME, not a variable reference, so the producer has to be inlined into the emit block, and every call site has to change. I tried it, and the call sites do not survive: `x = flat_field_wf(…)` returns a ChannelOut, and `.out` is not one of its properties — that form only works on the workflow NAME, as `collect_positions.out` does. Sixteen call sites would have to become `[0]` indexing or stop capturing the return value, to replace a self-describing `.done` with a positional idiom. `-preview` caught it; lint did not, since the mistake is a runtime property lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * update pixel_metrics.yaml config * fix(qc): qc-standalone reaches qc_stage_wf's emit by name again 226a83d reverted the `.out` experiment in mantis-v2.nf but missed qc-standalone.nf, which the same blanket regex had rewritten. `qc.out` is not a property of a ChannelOut, so every standalone run aborted at launch: No such property: out for class: DataflowBroadcast Shipped because the checks did not cover it: `nextflow lint` sees a property access, not a missing property, and the `-preview` I ran for qc-standalone never reached the DAG — `check_environment` refused first, since that shell had no `imaging-qc` on PATH, and I read the refusal as the script being fine. Caught by re-running the subset pipeline after merging main, which is the check that would have caught it the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(qc): one self-contained config file per store, beside its family `configs/qc/{assemble,track}/` with a shared `base.yaml` is gone. Each store's QC is one self-contained file, living with that family's other step configs: nextflow/configs/a549/qc.yaml assembled store (stage 5) nextflow/configs/a549/qc_track.yaml tracking store (stage 6) nextflow/configs/zebrafish/qc.yaml assembled store, no tracking The directory grouping existed for one reason: `imaging-qc report` resolved a tab's settings by SCANNING a directory, so a shared one made every tab render the same `report:` block. imaging-qc#201 fixed that — the report composes from the file it is named — so the grouping is no longer load-bearing, and dropping it also restores each config's `display_name` as its tab label, which the directory scan replaced with "Stage N". `base.yaml` existed to be shared and was shared by exactly one config, so inlining it duplicates nothing. Verified semantically inert: the merged file parses equal to base + overlay under Hydra's own merge rules. The extension must be `.yaml`. imaging-qc loads a stage config through Hydra, which strips the extension and looks for `<stem>.yaml`; the same content as `qc.yml` fails with "Cannot find primary config 'qc'". So the family directories now mix `.yml` step configs with `.yaml` QC ones, and the skill's scaffold copies both globs instead of special-casing a tree. zebrafish/qc.yaml deliberately carries NO per-channel SNR floor: a floor must be keyed to that family's channel names and read off a healthy run, and inventing either would be the unmeasured threshold the a549 config warns about. Blank detection, the tenengrad floor, the SNR outlier fence and the intensity_stats trends all still gate. Tested on the dataset the config is written for — an index-aligned subset of 2026_08_11_A549_SEC61B_DENV_debug/v3, whose stores carry the pre-rename channel names in-pipeline QC sees — with the repo configs unmodified: 11 tasks, 0 failed, one report, both tabs, 7 plots. Against v3's own tables from the pre-flatten run, all 720 per-slice metric rows are identical; the 312 volume rows differ because `intensity_stats` strides Z by `grid: 16`, sampling 6 planes of 86 but 1 of 10 — confirmed by reproducing the subset's value from the pixels exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: give the weekly relock a credential for the private QC dependency `uv lock --upgrade` discards the pinned rev and re-resolves every dependency, the private imaging-qc-pipeline one included, so this is the ONE workflow that needs a credential. PR CI does not: `uv sync --group dev` installs from the lockfile and never touches the `qc` extra, which is why the extra is kept out of `all`. And GITHUB_TOKEN cannot stand in — it is scoped to this repository, so it cannot read another private one. The token reaches git through a scoped `insteadOf` rewrite, offered to that one repository URL rather than to all of github.com, and reaches the script through job-level `env` rather than `${{ }}` inside `run:` — zizmor runs the pedantic persona here and only ignores template-injection for pull-request-ci.yml. Verified locally by replaying the rewrite against a throwaway HOME with a real token: `git ls-remote` over HTTPS resolves the private repo, and the config matches only that URL. Absent the secret the job fails with the cause named, rather than dying inside uv with "Could not read from remote repository", which reads as an outage. It fails rather than skips deliberately: a silently skipped relock looks like dependencies are being refreshed when they are not. One line moves it to a skip if that is preferred. Also records in the debug README that flattening the QC configs did not change any metric value — old and new configs produce byte-identical output over the same position (516/516 rows, 156/156 intensity_stats), so the volume-metric differences seen against v3 are the Z-subset fixture and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: authenticate the weekly relock with a read-only deploy key Replaces the token wiring. A deploy key cannot reach any repository other than the one it is attached to, which matters because biahub is PUBLIC, and it is not tied to a person's account, so it survives someone leaving where a personal PAT would not. No org PAT-approval policy is involved either. The dependency URL stays HTTPS, so an org member still needs only `gh auth login` and no SSH key of their own; CI rewrites that one repository's URL to ssh://, which is what lets a deploy key serve it. Verified end to end before writing this: with a COLD uv cache, no HTTPS credential and only the rewrite plus an ssh key, `uv lock --upgrade-package pandas` exits 0 and resolves all 322 packages. Measured while choosing the approach, and recorded in the comments because both facts are counter-intuitive: a TARGETED upgrade does not avoid the fetch (`uv lock --upgrade-package pandas` on a cold cache still reads the git source and fails without a credential), while `uv lock --check` needs no credential at all — it reads the metadata already in the lockfile, which is why PR CI is credential-free rather than accidentally so. No ssh-agent, and no third-party action: each `run:` is a fresh shell, so the key goes to a file and GIT_SSH_COMMAND is exported through $GITHUB_ENV. `webfactory/ssh-agent` would need a new entry in zizmor's forbidden-uses allow list, and this needs no dependency at all. The secret reaches the script through env, never `${{ }}` inside `run:`, as the pedantic persona requires. Host keys come from ssh-keyscan rather than a pin: GitHub rotates them (the RSA key was replaced in 2023) and a stale pin would fail this job closed for a reason nobody would look for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Ivan Ivanov <ivan.ivanov@czbiohub.org>
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.
Add version parameter to create_empty_hcs_zarr() and detect input zarr version in register_cli() to ensure output version matches input (v2→v2, v3→v3). This maintains compatibility with both OME-NGFF v0.4 (zarr v2) and v0.5 (zarr v3) datasets.
Changes follow the pattern from waveOrder PR#519:
Pair-coded-with: Soorya Pradeep <@Soorya19Pradeep>