Skip to content

Commit 6c99dda

Browse files
lguerardclaude
andcommitted
perf(workflow): ⚡ run the segmentation configs concurrently
run_multi.py ran the configs strictly one after another, so the GPU partition sat idle through each config's prepare and its multi-hour CPU-only merge before the next config submitted a single tile. The conversion now runs once up front -- it writes with overwrite=True, so concurrent runs must not race on it -- and the segmentations then run together. They already namespace everything under work_dir/<label_name>/, so they touch disjoint files; each gets its own --directory because Snakemake's lock lives in the working directory rather than the config. Note that `jobs:` is per Snakemake process, so a 3-config run can have 3x that many jobs in flight. A failing config no longer aborts its siblings: check=True used to kill the remaining configs and the relations step, discarding hours of finished GPU work over an unrelated failure. Each config's status is reported and the exit code is non-zero if any failed. Cross-config invariants are now checked before anything is submitted, not hours later as a shape mismatch from label_relations: shared work_dir, identical tile_shape and level, and unique label_name -- duplicates silently overwrote each other's namespace. Previously only work_dir was checked, and only when relations were configured and it was not a dry run. Relations read the ids from the n_objects/sequential_labels attrs the merge writes instead of two full-volume da.unique scans, which matters because that step runs on the login node. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent ea2dc19 commit 6c99dda

2 files changed

Lines changed: 162 additions & 27 deletions

File tree

workflow/profile/slurm/config.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,10 @@ executor: slurm
1111
# real GPU quota (MaxTRESPU) and backfills the rest, so it's safe to raise.
1212
# Check your actual per-user GPU cap with:
1313
# sacctmgr show assoc user=$USER format=account,partition,maxjobs,GrpTRES
14+
#
15+
# NOTE: this is per Snakemake process. run_multi.py runs the segmentation
16+
# configs concurrently, so a 3-config multi run can have up to 3 x this many
17+
# jobs in flight.
1418
jobs: 128
1519
latency-wait: 60
1620
rerun-incomplete: true

workflow/scripts/run_multi.py

Lines changed: 158 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,17 @@
66
python scripts/run_multi.py --config config/multi.yaml -n # dry-run only
77
88
See config/multi.yaml and docs/guide/snakemake.md "Running two segmentations"
9-
for the config format. Each listed segmentation config is run as an ordinary
10-
`snakemake --configfile ...` invocation (this script is a thin sequencer, not
11-
a Snakemake rule — the segmentations already namespace their own paths under
12-
work_dir/<label_name>/, so running them one after another here is exactly
13-
equivalent to running each snakemake command by hand). Once all segmentations
14-
finish, each configured relation pair is computed via
9+
for the config format.
10+
11+
The conversion runs once up front, then every segmentation config runs
12+
**concurrently** as its own `snakemake --configfile ...` invocation. They
13+
namespace their paths under work_dir/<label_name>/ and so touch disjoint
14+
files; running them together keeps the GPU partition busy instead of idling
15+
through each config's prepare and multi-hour merge in turn. Each gets its own
16+
`--directory` because Snakemake's lock lives in the working directory, not in
17+
the config. A config that fails does not abort its siblings.
18+
19+
Once all segmentations succeed, each configured relation pair is computed via
1520
patchworks.label_relations and written as an Excel workbook in work_dir,
1621
with two sheets: one row per a-object (unmatched ones included, with an
1722
empty b-id and zeros) and one row per b-object (a-object count + total
@@ -32,29 +37,97 @@ def _load_yaml(path: Path) -> dict:
3237
return yaml.safe_load(path.read_text())
3338

3439

35-
def _run_snakemake(
40+
def _snakemake_cmd(
3641
configfile: Path,
3742
*,
3843
workflow_dir: Path,
3944
profile: str | None,
4045
cores: int,
4146
dry_run: bool,
42-
) -> None:
47+
state_dir: Path | None = None,
48+
targets: list[str] | None = None,
49+
) -> list[str]:
50+
"""Build one snakemake invocation.
51+
52+
Every path is absolutised because ``--directory`` moves the working
53+
directory: each config needs its own ``.snakemake`` state directory, or
54+
concurrent runs would contend for the same ``.snakemake/locks/``.
55+
"""
4356
cmd = [
4457
"snakemake",
4558
"-s",
4659
str(workflow_dir / "Snakefile"),
4760
"--configfile",
48-
str(configfile),
61+
str(configfile.resolve()),
4962
]
63+
if state_dir is not None:
64+
state_dir.mkdir(parents=True, exist_ok=True)
65+
cmd += ["--directory", str(state_dir.resolve())]
5066
if profile:
51-
cmd += ["--workflow-profile", profile]
67+
cmd += ["--workflow-profile", str((workflow_dir / profile).resolve())]
5268
else:
5369
cmd += ["--cores", str(cores), "--rerun-triggers", "mtime"]
5470
if dry_run:
5571
cmd += ["-n", "-p"]
72+
if targets:
73+
# "--" ends option parsing: --rerun-triggers takes a variable number
74+
# of values and would otherwise swallow the target path.
75+
cmd += ["--", *targets]
76+
return cmd
77+
78+
79+
def _run(cmd: list[str], workflow_dir: Path) -> int:
5680
print(f"[run_multi] $ {' '.join(cmd)}", flush=True)
57-
subprocess.run(cmd, check=True, cwd=workflow_dir)
81+
return subprocess.run(cmd, cwd=workflow_dir).returncode
82+
83+
84+
def _validate_configs(paths: list[Path], cfgs: list[dict]) -> str:
85+
"""Check the cross-config invariants before anything is submitted.
86+
87+
These used to surface hours later -- as a shape mismatch from
88+
label_relations, or not at all when two configs quietly overwrote each
89+
other's label group. Only ``work_dir`` was checked, and only when
90+
relations were configured and it was not a dry run.
91+
92+
Returns
93+
-------
94+
str
95+
The shared ``work_dir``.
96+
"""
97+
problems = []
98+
99+
def _spread(key):
100+
return {p.name: cfg.get(key) for p, cfg in zip(paths, cfgs)}
101+
102+
work_dirs = {cfg.get("work_dir") for cfg in cfgs}
103+
if len(work_dirs) != 1:
104+
problems.append(
105+
f"configs must share one work_dir (label_relations compares "
106+
f"against a single image.zarr); got {_spread('work_dir')}"
107+
)
108+
109+
for key in ("tile_shape", "level"):
110+
values = {repr(cfg.get(key)) for cfg in cfgs}
111+
if len(values) != 1:
112+
problems.append(
113+
f"{key} must be identical across configs so the label arrays "
114+
f"share a chunk layout; got {_spread(key)}"
115+
)
116+
117+
names = [cfg.get("label_name") for cfg in cfgs]
118+
duplicates = {n for n in names if names.count(n) > 1}
119+
if duplicates:
120+
problems.append(
121+
f"label_name must be unique per config -- duplicates silently "
122+
f"overwrite each other's work_dir/<label_name>/ and "
123+
f"image.zarr/labels/<name>/; repeated: {sorted(duplicates)}"
124+
)
125+
126+
if problems:
127+
for p in problems:
128+
print(f"[run_multi] ERROR: {p}", file=sys.stderr)
129+
sys.exit(1)
130+
return work_dirs.pop()
58131

59132

60133
def _resolve(workflow_dir: Path, path_str: str) -> Path:
@@ -96,37 +169,95 @@ def main() -> None:
96169
seg_config_paths = [
97170
_resolve(workflow_dir, c) for c in multi_cfg["segmentations"]
98171
]
99-
for cfg_path in seg_config_paths:
100-
_run_snakemake(
172+
seg_cfgs = [_load_yaml(p) for p in seg_config_paths]
173+
work_dir = _validate_configs(seg_config_paths, seg_cfgs)
174+
image_store = f"{work_dir}/image.zarr"
175+
176+
# Phase A: convert exactly once. The three runs are about to go concurrent
177+
# and `convert` writes with overwrite=True, so letting them race on it
178+
# would have them clobbering one store. Ask for its marker explicitly.
179+
rc = _run(
180+
_snakemake_cmd(
181+
seg_config_paths[0],
182+
workflow_dir=workflow_dir,
183+
profile=args.profile,
184+
cores=args.cores,
185+
dry_run=args.dry_run,
186+
state_dir=Path(work_dir) / ".snakemake_convert",
187+
targets=[f"{image_store}/zarr.json"],
188+
),
189+
workflow_dir,
190+
)
191+
if rc != 0:
192+
print("[run_multi] ERROR: conversion failed", file=sys.stderr)
193+
sys.exit(rc)
194+
195+
# Phase B: the segmentations touch disjoint files under
196+
# work_dir/<label_name>/, so run them together and let the GPU partition
197+
# stay busy instead of idling through each config's prepare and merge.
198+
# Each needs its own state directory: .snakemake/locks/ is per working
199+
# directory, not per config.
200+
procs = []
201+
for cfg_path, cfg in zip(seg_config_paths, seg_cfgs):
202+
cmd = _snakemake_cmd(
101203
cfg_path,
102204
workflow_dir=workflow_dir,
103205
profile=args.profile,
104206
cores=args.cores,
105207
dry_run=args.dry_run,
208+
state_dir=Path(cfg["work_dir"]) / cfg["label_name"] / ".snakemake",
106209
)
210+
print(f"[run_multi] $ {' '.join(cmd)}", flush=True)
211+
procs.append((cfg_path.name, subprocess.Popen(cmd, cwd=workflow_dir)))
107212

108-
relations = multi_cfg.get("relations", [])
109-
if args.dry_run or not relations:
110-
return
111-
112-
seg_cfgs = [_load_yaml(p) for p in seg_config_paths]
113-
work_dirs = {cfg["work_dir"] for cfg in seg_cfgs}
114-
if len(work_dirs) != 1:
213+
# Don't abort the siblings when one config fails: the others are
214+
# independent, and killing them would throw away hours of finished GPU
215+
# work over an unrelated failure.
216+
failed = [name for name, p in procs if p.wait() != 0]
217+
for name, p in procs:
218+
status = "FAILED" if p.returncode else "ok"
219+
print(f"[run_multi] {name}: {status}", flush=True)
220+
if failed:
115221
print(
116-
f"[run_multi] ERROR: segmentation configs use different work_dir "
117-
f"({sorted(work_dirs)}); label_relations needs one shared "
118-
"image.zarr to compare against.",
222+
f"[run_multi] ERROR: {len(failed)} config(s) failed: "
223+
f"{', '.join(failed)}; skipping relations.",
119224
file=sys.stderr,
120225
)
121226
sys.exit(1)
122-
work_dir = work_dirs.pop()
123-
image_store = f"{work_dir}/image.zarr"
227+
228+
relations = multi_cfg.get("relations", [])
229+
if args.dry_run or not relations:
230+
return
124231

125232
import dask.array as da
126233
import openpyxl
234+
import zarr
127235

128236
from patchworks import label_relations
129237

238+
def _label_ids(name: str) -> list[int]:
239+
"""Ids present in a label image, without scanning the volume.
240+
241+
The merge writes n_objects/sequential_labels into the label group's
242+
attrs precisely so consumers don't have to re-derive the id set; the
243+
ids are 1..n_objects by construction. Fall back to the full scan only
244+
for a label group written before those attrs existed -- that scan runs
245+
here on the login node, so it is worth avoiding.
246+
"""
247+
attrs = dict(zarr.open_group(f"{image_store}/labels/{name}").attrs)
248+
if (
249+
attrs.get("sequential_labels")
250+
and attrs.get("n_objects") is not None
251+
):
252+
return list(range(1, int(attrs["n_objects"]) + 1))
253+
print(
254+
f"[run_multi] {name}: no n_objects attr, falling back to a full "
255+
"scan for its id set",
256+
flush=True,
257+
)
258+
arr = da.from_zarr(image_store, component=f"labels/{name}/0")
259+
return sorted(int(x) for x in da.unique(arr[arr > 0]).compute())
260+
130261
for rel in relations:
131262
a_name, b_name = rel["a"], rel["b"]
132263
out_path = Path(work_dir) / rel.get(
@@ -141,8 +272,8 @@ def main() -> None:
141272
# Pull the full id sets so unmatched a-objects (zero overlap) and
142273
# b-objects with no matches at all still get a row -- otherwise
143274
# they'd silently vanish instead of counting as zero.
144-
a_ids = sorted(int(x) for x in da.unique(a[a > 0]).compute())
145-
b_ids = sorted(int(x) for x in da.unique(b[b > 0]).compute())
275+
a_ids = _label_ids(a_name)
276+
b_ids = _label_ids(b_name)
146277

147278
per_b = {b_id: {"count": 0, "overlap_voxels": 0} for b_id in b_ids}
148279
for m in table.values():

0 commit comments

Comments
 (0)