Skip to content

Commit ea2dc19

Browse files
lguerardclaude
andcommitted
fix(cluster): 🐛 size work against the SLURM allocation, not the whole node
os.cpu_count() and psutil.virtual_memory().available describe the machine, so on a shared node every worker-count and memory decision was made against resources the job never had: a 32-core, 128 GB allocation on a 128-core, 512 GB node sized itself for 128 cores and 512 GB. That mismatch is how a job walks into an OOM kill while its own accounting says it has room. cpu_allocation() prefers SLURM_CPUS_PER_TASK, then the process' affinity mask, then the machine. _get_available_memory() takes the smallest of SLURM_MEM_PER_NODE, SLURM_MEM_PER_CPU x cpus, the cgroup limit (v1 and v2 — the one that actually triggers the kill) and the node's free RAM. Both are now used everywhere a worker count was derived. convert.py pins dask's threaded scheduler to the allocation; it previously ran one worker per machine core, the same bug the profile worked around by raising mem_mb rather than fixing. merge.py sizes its pool from the RAM budget and logs what it detected, so `seff` output can be read against it — merge_workers: null also silently left merge_tile_labels capped at 4 while the profile's comment claimed the full allocation was in use. The profile gains retries: 2 with attempt-scaled mem_mb, so a transient OOM resubmits larger instead of failing the run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 3da3d0b commit ea2dc19

10 files changed

Lines changed: 244 additions & 36 deletions

File tree

src/patchworks/__init__.py

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,13 @@
2929
from importlib.metadata import PackageNotFoundError
3030
from importlib.metadata import version as _pkg_version
3131

32-
from ._chunks import auto_overlap, auto_tile_shape, auto_tile_shape_cellpose
32+
from ._chunks import (
33+
auto_overlap,
34+
auto_tile_shape,
35+
auto_tile_shape_cellpose,
36+
cpu_allocation,
37+
safe_worker_count,
38+
)
3339
from ._cluster import make_local_cluster
3440
from ._core import tile_process
3541
from ._distributed import (
@@ -56,6 +62,8 @@
5662
"auto_overlap",
5763
"auto_tile_shape",
5864
"auto_tile_shape_cellpose",
65+
"cpu_allocation",
66+
"safe_worker_count",
5967
"load_ome_zarr",
6068
"estimate_empty_tiles",
6169
"auto_empty_threshold",

src/patchworks/_chunks.py

Lines changed: 87 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import logging
66
import os
7+
from pathlib import Path
78
from typing import Any, Sequence, Union
89

910
import numpy as np
@@ -71,21 +72,100 @@ def auto_overlap(
7172
_GPU_MEMORY_FALLBACK = 8 * 1024**3
7273

7374

75+
def cpu_allocation() -> int:
76+
"""Return the number of CPUs this process may actually use.
77+
78+
``os.cpu_count()`` reports the machine's cores, which on a shared cluster
79+
node is wildly more than a job was granted -- a 4-core allocation on a
80+
128-core node would size itself for 128. Prefer what the scheduler says,
81+
then the process' CPU affinity mask, and only then the machine.
82+
83+
Returns
84+
-------
85+
int
86+
Usable CPU count (always >= 1).
87+
"""
88+
for var in ("SLURM_CPUS_PER_TASK", "SLURM_CPUS_ON_NODE"):
89+
try:
90+
value = int(os.environ[var])
91+
except (KeyError, ValueError):
92+
continue
93+
if value > 0:
94+
return value
95+
try:
96+
return max(1, len(os.sched_getaffinity(0)))
97+
except AttributeError: # not POSIX
98+
return max(1, os.cpu_count() or 1)
99+
100+
101+
def _cgroup_memory_limit() -> "int | None":
102+
"""Memory ceiling from the process' cgroup, if one applies.
103+
104+
SLURM confines jobs with cgroups, so this is the limit that actually gets
105+
the process OOM-killed -- unlike the node-wide figure ``psutil`` reports.
106+
"""
107+
for path in (
108+
"/sys/fs/cgroup/memory.max", # cgroup v2
109+
"/sys/fs/cgroup/memory/memory.limit_in_bytes", # cgroup v1
110+
):
111+
try:
112+
raw = Path(path).read_text().strip()
113+
except OSError:
114+
continue
115+
if raw == "max":
116+
continue
117+
try:
118+
value = int(raw)
119+
except ValueError:
120+
continue
121+
# v1 reports a sentinel near 2**63 when unlimited.
122+
if 0 < value < 2**62:
123+
return value
124+
return None
125+
126+
74127
def _get_available_memory() -> int:
75-
"""Return available system RAM in bytes.
128+
"""Return the memory this process may actually use, in bytes.
129+
130+
Takes the **smallest** of everything that can constrain it: the SLURM
131+
allocation, the cgroup limit, and the node's free RAM. A node with 512 GB
132+
free must never convince a 128 GB job that it has room -- that mismatch is
133+
exactly how a job sizes itself into an OOM kill.
76134
77135
Returns
78136
-------
79137
int
80-
Available memory via ``psutil``, or an 8 GiB fallback if it is not
81-
installed.
138+
Usable memory in bytes, or an 8 GiB fallback if nothing is knowable.
82139
"""
140+
limits = []
141+
142+
per_node = os.environ.get("SLURM_MEM_PER_NODE")
143+
if per_node:
144+
try:
145+
limits.append(int(per_node) * 1024**2) # SLURM reports MB
146+
except ValueError:
147+
pass
148+
per_cpu = os.environ.get("SLURM_MEM_PER_CPU")
149+
if per_cpu:
150+
try:
151+
limits.append(int(per_cpu) * 1024**2 * cpu_allocation())
152+
except ValueError:
153+
pass
154+
155+
cgroup = _cgroup_memory_limit()
156+
if cgroup is not None:
157+
limits.append(cgroup)
158+
83159
try:
84160
import psutil
85161

86-
return int(psutil.virtual_memory().available)
162+
limits.append(int(psutil.virtual_memory().available))
87163
except Exception:
164+
pass
165+
166+
if not limits:
88167
return 8 * 1024**3
168+
return max(1, min(limits))
89169

90170

91171
def safe_worker_count(
@@ -124,7 +204,7 @@ def safe_worker_count(
124204
int
125205
Worker-thread count (always >= 1).
126206
"""
127-
cpu_cap = max(1, (os.cpu_count() or 1) - 1)
207+
cpu_cap = max(1, cpu_allocation() - 1)
128208
if use_gpu:
129209
return 1
130210
avail = _get_available_memory()
@@ -204,7 +284,7 @@ def auto_tile_shape(
204284
>>> tile
205285
(128, 512, 512)
206286
"""
207-
n_workers = n_workers or os.cpu_count() or 1
287+
n_workers = n_workers or cpu_allocation()
208288
itemsize = np.dtype(dtype).itemsize
209289
n_spatial = min(3, len(shape))
210290

@@ -308,7 +388,7 @@ def auto_tile_shape_cellpose(
308388
>>> tile
309389
(1, 2048, 2048)
310390
"""
311-
n_workers = n_workers or os.cpu_count() or 1
391+
n_workers = n_workers or cpu_allocation()
312392
itemsize = np.dtype(dtype).itemsize
313393

314394
if use_gpu:

src/patchworks/_cluster.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
from __future__ import annotations
44

55
import logging
6-
import os
6+
7+
from ._chunks import cpu_allocation
78

89
logger = logging.getLogger(__name__)
910

@@ -95,7 +96,7 @@ def make_local_cluster(
9596
from dask.distributed import Client, LocalCluster
9697

9798
if n_workers is None:
98-
n_workers = 1 if use_gpu else min(8, os.cpu_count() or 1)
99+
n_workers = 1 if use_gpu else min(8, cpu_allocation())
99100

100101
cluster = LocalCluster(
101102
processes=True,

src/patchworks/_core.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import dask.array as da
1212
import numpy as np
1313

14-
from ._chunks import auto_tile_shape, safe_worker_count
14+
from ._chunks import auto_tile_shape, cpu_allocation, safe_worker_count
1515
from ._cluster import _client_is_in_process, _distributed_client
1616
from ._io import auto_empty_threshold, load_ome_zarr
1717
from ._merge import zarr_native_merge
@@ -537,7 +537,7 @@ def active_fn(block, block_info=None):
537537
if max_workers is not None
538538
else safe_worker_count(_tile_nbytes, use_gpu=use_gpu)
539539
)
540-
_workers = max(1, min(_workers, os.cpu_count() or 1))
540+
_workers = max(1, min(_workers, cpu_allocation()))
541541
logger.info("Staging with %d worker thread(s)", _workers)
542542
_sched_ctx: Any = _dask.config.set(
543543
scheduler="threads", num_workers=_workers

src/patchworks/_merge.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
import numpy as np
3333
import zarr
3434

35+
from ._chunks import cpu_allocation
36+
3537
try:
3638
from tqdm.auto import tqdm as _tqdm
3739
except ImportError:
@@ -746,7 +748,7 @@ def merge_tile_labels(
746748
"""
747749
import dask.array as da
748750

749-
nw = n_workers if n_workers is not None else min(4, os.cpu_count() or 1)
751+
nw = n_workers if n_workers is not None else min(4, cpu_allocation())
750752

751753
# -- Stage dask array to zarr if needed --
752754
stage_path: str | None = None

src/patchworks/_relations.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,15 @@
33
from __future__ import annotations
44

55
import logging
6-
import os
76
from concurrent.futures import ThreadPoolExecutor
87
from pathlib import Path
98
from typing import Union
109

1110
import dask.array as da
1211
import numpy as np
1312

13+
from ._chunks import cpu_allocation
14+
1415
logger = logging.getLogger(__name__)
1516

1617

@@ -94,7 +95,7 @@ def label_relations(
9495

9596
n_blocks = a.numblocks
9697
total = int(np.prod(n_blocks))
97-
nw = n_workers if n_workers is not None else min(4, os.cpu_count() or 1)
98+
nw = n_workers if n_workers is not None else min(4, cpu_allocation())
9899

99100
def _one(flat_idx: int) -> np.ndarray:
100101
idx = np.unravel_index(flat_idx, n_blocks)

tests/test_allocation.py

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
"""Tests for cluster-allocation-aware CPU and memory detection.
2+
3+
On a shared node ``os.cpu_count()`` and ``psutil.virtual_memory().available``
4+
describe the *machine*, not the slice of it this job was granted. Sizing work
5+
against the machine is how a job walks into an OOM kill, so these are the
6+
checks that would have caught it.
7+
"""
8+
9+
import numpy as np
10+
11+
from patchworks import cpu_allocation, safe_worker_count
12+
from patchworks._chunks import _get_available_memory
13+
14+
GIB = 1024**3
15+
16+
17+
def test_cpu_allocation_prefers_slurm(monkeypatch):
18+
"""SLURM's grant wins over the machine's core count."""
19+
monkeypatch.setenv("SLURM_CPUS_PER_TASK", "4")
20+
assert cpu_allocation() == 4
21+
22+
23+
def test_cpu_allocation_ignores_junk_and_falls_back(monkeypatch):
24+
"""A malformed or absent value falls through to the affinity mask."""
25+
monkeypatch.setenv("SLURM_CPUS_PER_TASK", "not-a-number")
26+
monkeypatch.delenv("SLURM_CPUS_ON_NODE", raising=False)
27+
assert cpu_allocation() >= 1
28+
29+
monkeypatch.delenv("SLURM_CPUS_PER_TASK", raising=False)
30+
assert cpu_allocation() >= 1
31+
32+
33+
def test_available_memory_takes_the_smallest_limit(monkeypatch):
34+
"""The node's free RAM must never override a smaller allocation."""
35+
monkeypatch.setenv("SLURM_MEM_PER_NODE", str(16 * 1024)) # 16 GiB, in MB
36+
monkeypatch.setattr(
37+
"patchworks._chunks._cgroup_memory_limit", lambda: 512 * GIB
38+
)
39+
assert _get_available_memory() == 16 * GIB
40+
41+
42+
def test_available_memory_respects_the_cgroup(monkeypatch):
43+
"""With no SLURM hint, the cgroup ceiling still bounds the answer."""
44+
monkeypatch.delenv("SLURM_MEM_PER_NODE", raising=False)
45+
monkeypatch.delenv("SLURM_MEM_PER_CPU", raising=False)
46+
monkeypatch.setattr(
47+
"patchworks._chunks._cgroup_memory_limit", lambda: 2 * GIB
48+
)
49+
assert _get_available_memory() <= 2 * GIB
50+
51+
52+
def test_mem_per_cpu_scales_with_the_allocation(monkeypatch):
53+
"""SLURM_MEM_PER_CPU is per core, so it multiplies by the core grant."""
54+
monkeypatch.delenv("SLURM_MEM_PER_NODE", raising=False)
55+
monkeypatch.setenv("SLURM_MEM_PER_CPU", str(2 * 1024)) # 2 GiB per cpu
56+
monkeypatch.setenv("SLURM_CPUS_PER_TASK", "4")
57+
monkeypatch.setattr(
58+
"patchworks._chunks._cgroup_memory_limit", lambda: 512 * GIB
59+
)
60+
assert _get_available_memory() == 8 * GIB
61+
62+
63+
def test_worker_count_is_bounded_by_the_allocation(monkeypatch):
64+
"""The merge-style sizing must fit the grant, not the machine.
65+
66+
This is the concrete failure: a 32 GiB job on a 512 GiB node, with chunks
67+
big enough that only a couple fit the grant.
68+
"""
69+
monkeypatch.setenv("SLURM_CPUS_PER_TASK", "32")
70+
monkeypatch.setenv("SLURM_MEM_PER_NODE", str(32 * 1024)) # 32 GiB
71+
monkeypatch.setattr(
72+
"patchworks._chunks._cgroup_memory_limit", lambda: 512 * GIB
73+
)
74+
75+
chunk_nbytes = int(np.prod((16, 1024, 1024))) * 4 # int32 tile ~64 MB
76+
n = safe_worker_count(chunk_nbytes * 40, fn_overhead=3)
77+
assert n < 32, "must not size itself to the core count when RAM is tighter"
78+
assert n >= 1

workflow/profile/slurm/config.yaml

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ rerun-incomplete: true
2020
rerun-triggers: mtime
2121
printshellcmds: true
2222
keep-going: true
23+
# Retry a failed job twice, asking for more memory each time (see the
24+
# attempt-scaled mem_mb below). A transient OOM or a node hiccup then costs a
25+
# resubmission instead of failing the run and requiring a manual rerun.
26+
retries: 2
2327

2428
# Defaults for every rule (CPU jobs).
2529
default-resources:
@@ -32,30 +36,40 @@ default-resources:
3236
# Per-rule overrides.
3337
set-resources:
3438
convert:
35-
# bioio/dask's default threaded scheduler uses every visible core, so
36-
# this scales conversion for free. Verify actual scaling with `seff
37-
# <jobid>` after a run and dial back if efficiency is low (scicore
38-
# fair-share asks you not to sit on idle cores).
39-
mem_mb: 64000
39+
# convert.py pins dask's threaded scheduler to cpus_per_task, so this
40+
# scales conversion without the scheduler quietly running one worker per
41+
# *machine* core. Verify actual scaling with `seff <jobid>` after a run
42+
# and dial back if efficiency is low (scicore fair-share asks you not to
43+
# sit on idle cores).
44+
mem_mb: "attempt * 64000"
4045
cpus_per_task: 32
4146
runtime: 360
4247
prepare:
43-
mem_mb: 32000
48+
# Also builds the max-pooled occupancy map on first use for an image.zarr
49+
# that predates it (one bounded streaming pass, then shared by every
50+
# config against that store).
51+
mem_mb: "attempt * 32000"
4452
runtime: 120
4553
segment:
46-
# one GPU per tile — this is what spreads Cellpose across GPUs.
54+
# one GPU per job — this is what spreads Cellpose across GPUs.
4755
# Use the plugin's native `gres` resource → emits --gres=gpu:1, which is
4856
# how scicore binds a CUDA device. (The `gpu:` resource emits --gpus and
4957
# leaves the job without a device.)
5058
slurm_partition: "rtx4090"
5159
gres: "gpu:1"
5260
qos: "rtx4090-6hours" # scicore: <partition>-<duration> QOS
53-
mem_mb: 32000 # plenty — a tile used ~1G
61+
# A job now processes `tiles_per_job` tiles sequentially, so both memory
62+
# and runtime scale with that setting — raise it there and re-check here.
63+
# The old "a tile used ~1G" note predates tile_shape: "auto", which sizes
64+
# tiles against the real GPU and makes them far bigger.
65+
mem_mb: "attempt * 32000"
5466
cpus_per_task: 4
5567
runtime: 360 # 6 hours — must match the QOS, NOT 120 (=2h → killed early)
5668
merge:
57-
# merge_tile_labels now uses n_workers = this allocation (see merge.py /
58-
# config.yaml's merge_workers) instead of silently capping at 4 threads.
59-
mem_mb: 128000
69+
# merge.py sizes its worker pool from the cgroup/SLURM budget rather than
70+
# the node's core count, and the pyramid is written zarr-natively (one
71+
# source chunk per task) instead of through a dask rechunk — so this no
72+
# longer has to cover an unbounded scheduler.
73+
mem_mb: "attempt * 64000"
6074
cpus_per_task: 32
6175
runtime: 240

0 commit comments

Comments
 (0)