refactor(virtual-stain): rewrite CLI on cytoland predict_sliding_windows#267
Conversation
Replace the viscy-CLI subprocess pipeline (preprocess/predict/combine via conda) with in-process cytoland inference: - Add cytoland (modular VisCy) under the `stain` extra, sourced from the modular-viscy-staging branch. - Validate the predict config against VisCy's own VSUNet/HCSDataModule signatures via jsonargparse, so biahub keeps no copy of the model schema. - Run predict_sliding_windows per position, looping over timepoints on a single GPU; read precomputed normalization stats from the store. - Rotation TTA (default on) is applied at the volume level with square padding so it is correct for non-square FOVs. - Add minimal example config pointing at the vscyto3d finetune checkpoint. Adopt the deskew --cluster/--init pattern: add cluster() and init_only() parsing decorators, extend get_submitit_cluster() to take an explicit cluster string, emit RESOURCES:, support --init, and run --cluster debug in-process in the foreground. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Delegate normalization and test-time augmentation to VisCy instead of re-implementing them, eliminating the divergence risk (the previous manual normalization omitted VisCy's divisor epsilon): - Normalization: apply the config's instantiated NormalizeSampled transforms (via Compose) to a per-timepoint sample dict, reading precomputed stats with viscy_data.read_norm_meta. Drops _normalization_params and _read_fov_statistics. - TTA: use AugmentedPredictionVSUNet.with_rotation_tta, which handles non-square FOVs natively. Drops the local _predict_volume square-pad workaround. Depends on the cytoland feat/vs-nonsquare-tta-and-helpers branch (mehta-lab/VisCy#450). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Picks up mehta-lab/VisCy#450's rename of _read_norm_meta to the public read_norm_meta (commit eb1ffa1). No biahub source change; the import already used the public name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Repoints to mehta-lab/VisCy#450 head after its lint fixes (commit 8f42e30). No functional change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arr_version Add the three optional orchestration knobs to the example config (defaults shown). output_ome_zarr_version (default None, mirroring DeskewSettings) now dictates the output store's OME-Zarr version, threaded through _init_output_plate to resolve_ome_zarr_version; None preserves the input store's version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Timepoints are processed sequentially on a single GPU, so resource needs no longer depend on dataset size. Use num_cpus=16, gb_ram=4 instead of estimate_resources. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Base the wall-clock budget on T * Z * 5s (per sliding window) with a 60-minute floor, assuming median TTA. Measured ~1.4 s/window with TTA, so this gives a ~3-4x margin and accounts for stack depth (number of z-windows), which the previous T*3 formula ignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
batch_size has no effect in biahub (predict_sliding_windows runs one z-window at a time); set to 1 with a clarifying comment for parity with viscy predict. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
example_virtual_stain_settings.yml is a viscy predict config validated against VisCy's classes, not a biahub pydantic Settings model, so it broke test_all_example_settings_tested's file count. Track it in a separate list, include it in the expected count, and add a dedicated validation test that skips when cytoland (the `stain` extra) is not installed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per-position SLURM logs previously showed only submitit start/complete lines. Emit position-prefixed progress: device, checkpoint, TTA status, geometry (T / Z,Y,X / windows-per-timepoint), per-timepoint timing, and a completion summary with total elapsed time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Ready for review. First tests in |
|
Based on rough first speed measurements, Claude says this is 2x faster than |
|
FYI @aofei-liu this workflow will significantly simplify how we run virtual staining. For now, we'll do |
|
P.S. When reviewing i'd suggest just looking at the new |
There was a problem hiding this comment.
Pull request overview
This PR rewrites the biahub virtual-stain command to run virtual staining in-process using cytoland/VisCy (via predict_sliding_windows) instead of shelling out to the viscy predict CLI in a separate environment. It also adopts the --cluster {slurm,local,debug} / --init orchestration pattern and adds an example VisCy-compatible predict config.
Changes:
- Replace the legacy VisCy CLI subprocess workflow with an in-process cytoland-based implementation and per-position GPU job dispatch.
- Add jsonargparse-based config validation against VisCy’s
VSUNet/HCSDataModule, plus a new example virtual-stain config file. - Add
stainextra dependency plumbing (cytoland source pinning viauvsources) and update example-settings tests to include the new YAML.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
biahub/virtual_stain.py |
Replaces subprocess-based workflow with cytoland/VisCy in-process prediction, output store init, and submitit-based per-position job dispatch. |
biahub/cli/utils.py |
Extends submitit cluster selection to support explicit cluster= selection (while still forcing debug in CI). |
biahub/cli/parsing.py |
Adds --cluster and --init click parsing decorators used by the new virtual-stain CLI. |
tests/test_example_settings.py |
Ensures the new example YAML is accounted for and validates it via jsonargparse when cytoland is installed. |
settings/example_virtual_stain_settings.yml |
Adds a minimal example VisCy-style predict configuration for virtual staining. |
pyproject.toml |
Adds cytoland to the stain extra and configures an uv git source for cytoland. |
uv.lock |
Locks cytoland + VisCy branch dependencies and related transitive dependency updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Suggestion on Slack from @mattersoflight: After thinking about our conversation at lab meeting and brainstorming with claude, I land on adding a tightly scoped
With a verifiable test set of (inputs, predictions) for VSCyto2D and DynaCLR, I think we can have a package. This is preferable over ONNX, because ONNX is its own runtime that can drift from features of core pytorch library. |
|
I have no strong opinion on this. I asked Claude to compare the two approaches. What each approach actually isThis PR (#267 + VisCy #450) — "library reuse / in-process." biahub depends on cytoland (modular VisCy) as a library. A The proposal — " Key framing: #267 changes where orchestration lives; the proposal changes how the model is packaged. They overlap as answers to "how should biahub run VS models," but optimize for different goals — fast iteration inside the VisCy ecosystem vs. portable, frozen, contract-bound distribution. A subtlety on the ONNX reasoning: "torch.export avoids a second runtime that drifts" is a correct argument for torch.export over ONNX (it keeps real ATen ops, lowers via AOTInductor, tracks PyTorch numerics far more faithfully than the ONNXRuntime op-set). But that's a comparison between two export technologies — it is not an argument against the library-reuse approach in this PR, which has zero drift precisely because it runs the real VisCy code. Pros and cons
The honest weak point of the proposal: the difficulty of VS is not the forward pass — it's the sliding-window blend, the per-FOV data-dependent normalization statistics ( Switching between VSCyto2D / VSCyto3D / VSNeuroMastLibrary reuse wins decisively for model-switching. Those three differ only in architecture hyperparameters, stack depth, and source/target channels — all in the YAML and validated against For
So export is least suited to exactly the multi-model-switching scenario, and most suited to its opposite (a small frozen flagship set distributed widely). RecommendationKeep this PR as biahub's production path — for HPC batch inference over plates with an evolving zoo of VS models inside the Biohub Python ecosystem, library reuse is correct and per-model export buys nothing against the real bottleneck. The one cost it carries (the VisCy version pin) is a dependency-hygiene problem, not an architecture problem. Pursue 🤖 Summary generated with Claude Code. |
|
P.S. This PR will be useful right away. I'll merge tomorrow, and then we can refactor again if needed. |
|
It makes sense to use core VisCy to re-use the test-time-augmentation and blending. Please merge. |
…ning # Conflicts: # biahub/cli/utils.py # pyproject.toml # uv.lock
The feat/vs-nonsquare-tta-and-helpers branch was removed; pin to the underlying commit 7fb3bc1 (VisCy PR #465) instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Remove module-level torch.multiprocessing.set_start_method("spawn",
force=True): it was a global import-time side effect and is unused. The
inference path has no DataLoader workers or torch multiprocessing, and
submitit launches jobs as fresh interpreters (or in-process for debug),
so the start method never applied.
- Delegate checkpoint loading to VisCy: route the top-level ckpt_path into
the model's init args so VSUNet.__init__ extracts the Lightning
checkpoint["state_dict"] itself, instead of biahub assuming that layout.
- Rename the misleading in_stack_depth log variable to z_window_depth and
document that it mirrors VisCy's predict_sliding_windows (which reads
model.out_stack_depth as the input Z-window depth); guard the window
count with max(0, ...).
- Reject cluster='local' with >1 position: local jobs run concurrently
without a parallelism cap and all target cuda:0, oversubscribing a single
GPU. Point users to slurm (multi-GPU) or debug (sequential).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- virtual_stain_cli: default monitor to False (matching deskew_cli / flat_field_correction_cli; the library functions keep monitor=True) and document the SLURM fan-out / --init / --cluster debug usage examples. - Fix flat_field_correction_cli docstring: it used a raw string, which turned click's `\b` no-wrap markers into literal text and rewrapped the usage examples. Use a normal docstring with `# noqa: D301`, matching deskew. virtual-stain follows the same (correct) pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Speed:
|
| biahub virtual-stain | viscy predict | |
|---|---|---|
| GPU | H100 / H200 | A6000 |
| Time/position | ~124 s (predict loop) · ~138 s (job) | ~1006 s · ~1038 s |
~8× — but not apples-to-apples: biahub set no --constraint and landed on H100/H200; the viscy control was constrained to a100\|a6000\|a40 and landed on A6000.
Same GPU class (A6000 vs A6000)
Comparing a prior biahub A6000 run against the viscy A6000 run (total job elapsed):
| biahub virtual-stain (A6000) | viscy predict (A6000) | |
|---|---|---|
| Time/position | ~368 s | ~960–1070 s |
→ ~2.5–2.8× faster on equal hardware.
Bottom line
biahub virtual-stain is ~2.5× faster than viscy predict on equal hardware. It loads the full ZYX volume once per timepoint and runs all 72 windows on-GPU from memory, eliminating viscy's per-window DataLoader round-trips.
Note on GPU constraints: the ~8× wall-clock gap in the as-run numbers came almost entirely from hardware — biahub grabbed H100/H200 because it was unconstrained. We should not pin reconstruction/prediction jobs to a specific (weaker) GPU when a more powerful one is available; leaving them unconstrained lets SLURM schedule the fastest free GPU. (The --constraint on the viscy run was only there to set up a controlled comparison.)
Caveats: the A6000 biahub number is n=1 and from a slightly older revision (pre per-timepoint logging), so it's total job elapsed rather than the isolated predict loop — if anything that understates biahub, since viscy's predict loop (~1006 s) is nearly its full job time.
|
Side-by-side comparison here: Other clear wins - Effect of TTA bug fixed in mehta-lab/VisCy#465
green - @edyoshikun It looks like the TTA fix helps reduce artifacts at the edge of the images. |
* feat(nextflow): integrate virtual-stain into mantis-v2 pipeline Recreate the Nextflow virtual-stain integration against the current in-process cytoland CLI (post-#267), replacing the #259 approach which was built on the old `viscy predict` + `--copy` temp-zarr flow. - New `nextflow/modules/virtual_stain.nf`: init → preprocess → fan-out per-position GPU prediction. Per-position work is a single `biahub virtual-stain --cluster debug` call (no temp zarr, no --copy). - `viscy preprocess` runs over the whole input plate to compute the normalization statistics the model reads via read_norm_meta. - Both `biahub virtual-stain` and `viscy` run under biahub's `stain` extra (cytoland → viscy-utils provides the `viscy` console script). - Add a `gpu` process label (queue=gpu) to nextflow.config. - Wire virtual_stain_wf after reconstruct in mantis-v2.nf. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix biahub virtual-stain mem request * refactor: unify per-position resource requests across step CLIs Every step CLI (deskew, flat-field, apply-inv-tf, virtual-stain) now emits its per-position resource request through a single shared helper, biahub.cli.utils.echo_resources, as a JSON payload: RESOURCES:{"cpus": 16, "mem_gb": 64, "time_min": 480} The same (cpus, total mem_gb, time_min) values feed both the CLI's own slurm_* submission args and, via parse_resources in nextflow, the Nextflow per-position task directives — so the SLURM fan-out and the Nextflow fan-out can no longer request different resources. - Add echo_resources() and parse the JSON in common.nf::parse_resources, which now returns time_min alongside cpus/mem_gb. - Emit memory as a TOTAL (mem_gb) and request it via slurm_mem rather than slurm_mem_per_cpu, fixing virtual-stain's per-cpu over-request. - Wire each CLI's slurm_time from time_min; drop the hardcoded `time` literals in the run_* processes for `meta.time_min * task.attempt`. - virtual-stain computes time_min before the init_only return so --init emits it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * payload bugfix * debug viscy predict * refactor: rename time_min to time_minutes for clarity Avoids confusion with "minimum time"; renames the resource payload key and Nextflow meta field consistently across both languages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * update iohub dep --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adopt merged-PR decisions: JSON echo_resources protocol (cpus/mem_gb/ time_minutes) migrated across stabilize/track/concatenate/estimate-crop/ nf init-resources; take main's deskew/flat-field/reconstruct/virtual-stain (#255/#257/#258/#267/#271); nextflow.config preempted-partition hardening (#275); iohub>=0.3.8 (#272). Remove obsolete virtual-stain CLI test; move example_pipeline_steps.yml to nextflow/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bring PR #260 (track --init/--cluster + rename-channels + cellpose) up to date with main. main had already merged the deskew/flat-field/reconstruct (#255/#257/ #258), cytoland virtual-stain (#267/#271), and stitch-vendor (#238) refactors, which the refactor-* stack implemented in parallel; those subsystems are resolved in favour of main. Conflict resolution: - Took main's versions of deskew.py, flat_field_correction.py, apply_inverse_transfer_function.py, virtual_stain.py (cytoland rewrite, supersedes the stack's VisCy --init/--copy), cli/utils.py, and the deskew/flat_field/reconstruct/virtual_stain Nextflow modules + tests. - Took main's nextflow.config (adds the gpu label/queue that tracking.nf's run_track needs) and common.nf (JSON resources contract), and mantis-v2.nf. - pyproject.toml/uv.lock resolved to main (identical after taking iohub>=0.3.8). - Adopted main's JSON resources contract in track.py: --init now calls echo_resources(num_cpus, mem_gb, time_minutes) instead of the old space-separated 'RESOURCES:' line, so parse_resources (common.nf) works. - Preserved PR #260's unique work (tracking.nf, rename_channels.nf, the refactored/cellpose track.py, example_track_settings.yml).


Summary
Rewrites
biahub virtual-stainto run virtual staining in-process via cytoland (modular VisCy) instead of shelling out to theviscy predictCLI through a separate conda env. Normalization and test-time augmentation are delegated to VisCy so behavior stays in sync withviscy predict.cytolandunder thestainextra (sourced from VisCy'sfeat/vs-nonsquare-tta-and-helpersbranch via[tool.uv.sources]).VSUNet/HCSDataModulesignatures usingjsonargparse— biahub keeps no copy of the model schema, so it can't drift from VisCy.AugmentedPredictionVSUNet.predict_sliding_windows, looping over timepoints on a single GPU (GPU-bound, no per-timepoint parallelism).NormalizeSampledtransforms +viscy_data.read_norm_meta(reads precomputed stats from the store; errors with a "runviscy preprocessfirst" message if missing).AugmentedPredictionVSUNet.with_rotation_tta(rotation TTA, correct for non-square FOVs); configurable, default on.--cluster {slurm,local,debug}/--initpattern:cluster()+init_only()parsing decorators,get_submitit_cluster(cluster=...),RESOURCES:emission, and--cluster debugrunning jobs in-process in the foreground.settings/example_virtual_stain_settings.yml) pointing at the vscyto3d finetune checkpoint.Dependencies
Depends on mehta-lab/VisCy#450 (non-square rotation TTA +
rotation_tta_transforms/with_rotation_tta/read_norm_meta). Pins iohub 0.3.6 (PyPI). Intentionally not built onrefactor-deskew-clean, whose iohub fork (oldzarr/tensorstorecodec architecture) is runtime-incompatible with cytoland's modern iohub; the--cluster/--initinfra was instead replicated locally. Once VisCy#450 merges and releases, repoint[tool.uv.sources]cytoland to a stable ref.Test plan
ruffclean;biahub virtual-stain --helpshows--cluster/--init.uv lock/uv sync --extra allresolve (cytoland + viscy-* from the VisCy branch).--cluster debugon a non-square (64×48) 2-position plate with TTA on → correct(T, 2, Z, 64, 48)finite output;--initinitializes the store and exits;RESOURCES:emitted.🤖 Generated with Claude Code