Skip to content

Commit 4ec07f4

Browse files
authored
fix(running): jit-dict mutation, dict string monitors, multi-device parallel concat (#852)
fix(running): jit-dict mutation, dict-form string monitors, multi-device parallel concat - Runner.__init__ popped 'predict' from the caller's jit dict, mutating it and corrupting subclass jit config (predict JIT-on when user set it off); copy the dict (High) - dict-form string monitors (monitors={'a': 'V'}) were never resolved and invalid names not validated; fix the resolution branch + validation + result key (High) - jax_parallelize_map crashed concatenating a trailing partial chunk sharded on a device subset ("incompatible devices"); gather chunks to host before concat (High) - process_pool_lock injected the lock into the caller's param dicts via update(); submit a copy (Medium) Findings recorded in docs/issues-found-20260619-train-running.md
1 parent 3204311 commit 4ec07f4

7 files changed

Lines changed: 403 additions & 51 deletions

brainpy/running/jax_multiprocessing.py

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,10 @@ def jax_parallelize_map(
133133

134134
res_tree = None
135135
results = None
136-
# Build the pmapped function once and reuse it across all chunks. Re-applying
137-
# ``jax.pmap`` inside the loop forces a recompilation on every chunk, which is
138-
# both slow and unnecessary since the traced function does not change.
136+
# Build the pmapped function once and reuse it across all chunks. ``jax.pmap``
137+
# automatically re-traces for a chunk whose leading-axis size differs from the
138+
# device count (e.g. a trailing partial chunk), so a single cached function is
139+
# both correct and avoids redundant re-tracing of full chunks.
139140
pmap_func = pmap(func)
140141
for i in range(0, num_pars[0], num_parallel):
141142
if isinstance(arguments, dict):
@@ -145,16 +146,21 @@ def jax_parallelize_map(
145146
else:
146147
raise TypeError(f'"arguments" must be sequence or dict, but we got {type(arguments)}')
147148
res_values, res_tree = tree_flatten(r, is_leaf=lambda a: isinstance(a, bm.Array))
149+
# Gather each chunk's output to the host. A trailing partial chunk is
150+
# sharded on only a *subset* of devices, so leaving the outputs on-device
151+
# makes the final concatenation fail with "Received incompatible devices
152+
# for jitted computation". Pulling to numpy first sidesteps the placement
153+
# conflict (it also serves the ``clear_buffer`` path for free).
148154
if results is None:
149-
results = tuple([np.asarray(val) if clear_buffer else val] for val in res_values)
155+
results = tuple([np.asarray(val)] for val in res_values)
150156
else:
151157
for j, val in enumerate(res_values):
152-
results[j].append(np.asarray(val) if clear_buffer else val)
158+
results[j].append(np.asarray(val))
153159
if clear_buffer:
154160
bm.clear_buffer_memory()
155161
if res_tree is None:
156162
return None
157-
results = ([np.concatenate(res, axis=0) for res in results]
158-
if clear_buffer else
159-
[bm.concatenate(res, axis=0) for res in results])
163+
results = [np.concatenate(res, axis=0) for res in results]
164+
if not clear_buffer:
165+
results = [bm.asarray(res) for res in results]
160166
return tree_unflatten(res_tree, results)
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
# Copyright 2025 BrainX Ecosystem Limited. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
# ==============================================================================
15+
"""Tests for ``brainpy/running/jax_multiprocessing.py``.
16+
17+
Covers :func:`jax_vectorize_map` (``jax.vmap`` chunking) and
18+
:func:`jax_parallelize_map` (``jax.pmap`` chunking), including:
19+
20+
- the chunked vmap path with a trailing partial chunk and with dict-form args;
21+
- the length-mismatch guard;
22+
- (regression for P15-H3) ``jax_parallelize_map`` with a task count that is *not*
23+
a multiple of the device count, which produces a trailing partial chunk sharded
24+
on a *subset* of devices. The faulty version cached one ``pmap`` and then crashed
25+
in the closing ``bm.concatenate`` with "Received incompatible devices for jitted
26+
computation". The multi-device sub-test runs in a subprocess (devices must be
27+
configured before JAX initialises) and is skipped if extra host devices cannot be
28+
spun up.
29+
"""
30+
31+
import os
32+
import subprocess
33+
import sys
34+
35+
import numpy as np
36+
import pytest
37+
38+
import brainpy.math as bm
39+
from brainpy.running.jax_multiprocessing import jax_vectorize_map, jax_parallelize_map
40+
41+
42+
def _double(x):
43+
return x * 2.0
44+
45+
46+
# --------------------------------------------------------------------------- #
47+
# jax_vectorize_map (vmap)
48+
# --------------------------------------------------------------------------- #
49+
50+
def test_vectorize_map_partial_chunk():
51+
# 5 tasks, chunk size 2 -> chunks of 2, 2, 1 (trailing partial chunk).
52+
args = [np.arange(5.0)]
53+
r = np.asarray(jax_vectorize_map(_double, args, num_parallel=2))
54+
np.testing.assert_allclose(r, np.arange(5.0) * 2.0)
55+
56+
57+
def test_vectorize_map_partial_chunk_clear_buffer():
58+
args = [np.arange(5.0)]
59+
r = np.asarray(jax_vectorize_map(_double, args, num_parallel=2, clear_buffer=True))
60+
np.testing.assert_allclose(r, np.arange(5.0) * 2.0)
61+
62+
63+
def test_vectorize_map_dict_args():
64+
def add(x, y):
65+
return x + y
66+
67+
args = {'x': np.arange(4.0), 'y': np.arange(4.0) * 10}
68+
r = np.asarray(jax_vectorize_map(add, args, num_parallel=3))
69+
np.testing.assert_allclose(r, np.arange(4.0) * 11)
70+
71+
72+
def test_vectorize_map_length_mismatch_raises():
73+
with pytest.raises(ValueError):
74+
jax_vectorize_map(_double, [np.arange(4.0), np.arange(3.0)], num_parallel=2)
75+
76+
77+
def test_vectorize_map_bad_arguments_type_raises():
78+
with pytest.raises(TypeError):
79+
jax_vectorize_map(_double, 42, num_parallel=2)
80+
81+
82+
# --------------------------------------------------------------------------- #
83+
# jax_parallelize_map (pmap)
84+
# --------------------------------------------------------------------------- #
85+
86+
def test_parallelize_map_single_device():
87+
# On a single device num_parallel must be 1; chunks of size 1 each.
88+
args = [np.arange(3.0)]
89+
r = np.asarray(jax_parallelize_map(_double, args, num_parallel=1))
90+
np.testing.assert_allclose(r, np.arange(3.0) * 2.0)
91+
92+
93+
def test_parallelize_map_length_mismatch_raises():
94+
with pytest.raises(ValueError):
95+
jax_parallelize_map(_double, [np.arange(2.0), np.arange(1.0)], num_parallel=1)
96+
97+
98+
# Regression for P15-H3: trailing partial chunk across multiple devices.
99+
_MULTI_DEVICE_SNIPPET = r"""
100+
import numpy as np
101+
import jax
102+
assert jax.local_device_count() == 4, jax.local_device_count()
103+
from brainpy.running.jax_multiprocessing import jax_parallelize_map
104+
# 6 tasks, num_parallel == 4 devices -> chunks of 4 then 2 (partial, subset of devices).
105+
r = jax_parallelize_map(lambda x: x * 2.0, [np.arange(6.0)], num_parallel=4)
106+
r = np.asarray(r)
107+
expected = np.arange(6.0) * 2.0
108+
assert np.allclose(r, expected), (r, expected)
109+
print('OK')
110+
"""
111+
112+
113+
def test_parallelize_map_partial_chunk_multi_device():
114+
env = dict(os.environ)
115+
env['XLA_FLAGS'] = (env.get('XLA_FLAGS', '') + ' --xla_force_host_platform_device_count=4').strip()
116+
env.setdefault('JAX_PLATFORMS', 'cpu')
117+
proc = subprocess.run(
118+
[sys.executable, '-c', _MULTI_DEVICE_SNIPPET],
119+
env=env, capture_output=True, text=True, timeout=300,
120+
)
121+
if proc.returncode != 0:
122+
# Could not spin up 4 host devices in this environment -> skip rather than fail.
123+
if 'AssertionError' in proc.stderr and 'local_device_count' in proc.stderr:
124+
pytest.skip('Could not configure 4 host devices for the pmap test.')
125+
pytest.fail(f'multi-device pmap run failed:\nSTDOUT:\n{proc.stdout}\nSTDERR:\n{proc.stderr}')
126+
assert 'OK' in proc.stdout

brainpy/running/native_multiprocessing.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,8 @@ def some_func(..., lock, ...):
107107
if isinstance(net_params, (list, tuple)):
108108
results.append(pool.apply_async(func, args=tuple(net_params) + (lock,)))
109109
elif isinstance(net_params, dict):
110-
net_params.update(lock=lock)
111-
results.append(pool.apply_async(func, kwds=net_params))
110+
# Do not mutate the caller-owned dict; submit a copy with the lock added.
111+
results.append(pool.apply_async(func, kwds={**net_params, 'lock': lock}))
112112
else:
113113
raise ValueError('Unknown parameter type: ', type(net_params))
114114
pool.close()

brainpy/running/native_multiprocessing_coverage_test.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,3 +99,12 @@ def test_process_pool_lock_with_dict_params():
9999
def test_process_pool_lock_unknown_param_type_raises():
100100
with pytest.raises(ValueError):
101101
process_pool_lock(_add_lock, [42], num_process=1)
102+
103+
104+
def test_process_pool_lock_does_not_mutate_caller_dict():
105+
# P15-M1: the lock must not be injected into the caller-owned param dicts.
106+
params = [{'x': 1, 'y': 2}]
107+
results = process_pool_lock(_add_lock_kw, params, num_process=1)
108+
assert results == [3]
109+
assert params == [{'x': 1, 'y': 2}] # unchanged
110+
assert 'lock' not in params[0]

brainpy/running/runner.py

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,11 @@ def __init__(
9696
if isinstance(jit, bool):
9797
self.jit = {C.PREDICT_PHASE: jit}
9898
elif isinstance(jit, dict):
99+
# Operate on a shallow copy: never mutate the caller-owned dict. The
100+
# original ``jit`` is also kept as ``self._origin_jit`` and read by
101+
# subclasses (e.g. ``DSTrainer``/``BPTrainer``) which expect the
102+
# explicit ``predict`` setting to still be present.
103+
jit = dict(jit)
99104
for k, v in jit.items():
100105
self.jit[k] = v
101106
self.jit[C.PREDICT_PHASE] = jit.pop(C.PREDICT_PHASE, True)
@@ -238,32 +243,40 @@ def _find_dict_monitor_targets(self, _monitors):
238243
monitors = {}
239244
name2node = None
240245
for _key, _mon in _monitors.items():
241-
if isinstance(_mon, str):
246+
# A ``(name_str, index)`` value (produced by ``_format_dict_monitors``
247+
# from a string monitor such as ``{'a': 'V'}`` or ``{'a': ('sub.V', 2)}``)
248+
# must be resolved to its target Variable, exactly like the
249+
# sequence-form resolver. ``(Variable, index)`` / callable values are
250+
# already resolved and fall through to the ``else`` branch unchanged.
251+
if isinstance(_mon, (tuple, list)) and isinstance(_mon[0], str):
242252
if name2node is None:
243253
name2node = {node.name: node for node in list(self.target.nodes(level=-1).unique().values())}
244254

255+
# ``_key`` is the user-chosen monitor name (e.g. 'a' for
256+
# ``{'a': 'V'}``); the resolved (Variable, index) must be stored
257+
# under ``_key`` so that ``runner.mon[_key]`` works.
245258
key, index = _mon[0], _mon[1]
246259
splits = key.split('.')
247260
if len(splits) == 1:
248261
if not hasattr(self.target, splits[0]):
249262
raise RunningError(f'{self.target} does not has variable {key}.')
250-
monitors[key] = (getattr(self.target, splits[-1]), index)
263+
monitors[_key] = (getattr(self.target, splits[-1]), index)
251264
else:
252265
if not hasattr(self.target, splits[0]):
253266
if splits[0] not in name2node:
254267
raise MonitorError(f'Cannot find target {key} in monitor of {self.target}, please check.')
255268
else:
256269
master = name2node[splits[0]]
257270
assert len(splits) == 2
258-
monitors[key] = (getattr(master, splits[-1]), index)
271+
monitors[_key] = (getattr(master, splits[-1]), index)
259272
else:
260273
master = self.target
261274
for s in splits[:-1]:
262275
try:
263276
master = getattr(master, s)
264277
except KeyError:
265278
raise MonitorError(f'Cannot find {key} in {master}, please check.')
266-
monitors[key] = (getattr(master, splits[-1]), index)
279+
monitors[_key] = (getattr(master, splits[-1]), index)
267280
else:
268281
monitors[_key] = _mon
269282
return monitors

brainpy/running/runner_coverage_test.py

Lines changed: 48 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,23 @@ def test_jit_invalid_type_raises(target):
122122
Runner(target, monitors=None, jit='not-a-jit', progress_bar=False)
123123

124124

125+
def test_jit_dict_does_not_mutate_caller(target):
126+
# P15-H1: the caller's jit dict must not be mutated (no key popped).
127+
d = {'train': False, C.PREDICT_PHASE: False}
128+
Runner(target, monitors=None, jit=d, progress_bar=False)
129+
assert d == {'train': False, C.PREDICT_PHASE: False}
130+
assert C.PREDICT_PHASE in d
131+
132+
133+
def test_jit_dict_origin_jit_preserves_predict_key(target):
134+
# P15-H1: ``_origin_jit`` keeps the explicit predict setting so that
135+
# subclasses reading ``self._origin_jit.get('predict')`` see the user value.
136+
d = {C.PREDICT_PHASE: False, 'fit': True}
137+
r = Runner(target, monitors=None, jit=d, progress_bar=False)
138+
assert r._origin_jit.get(C.PREDICT_PHASE) is False
139+
assert r.jit[C.PREDICT_PHASE] is False
140+
141+
125142
# --------------------------------------------------------------------------- #
126143
# default / None monitors
127144
# --------------------------------------------------------------------------- #
@@ -224,32 +241,34 @@ def test_dict_monitor_variable_value(target):
224241
assert var is target.V and idx is None
225242

226243

227-
def test_dict_monitor_str_value_not_resolved():
228-
# NOTE: DEFECT - dict-form *string* monitors are NOT resolved to their
229-
# target Variable. ``_format_dict_monitors`` wraps a string value 'V' into
230-
# the tuple ('V', None); by the time it reaches
231-
# ``_find_dict_monitor_targets`` the value is a tuple, so the
232-
# ``isinstance(_mon, str)`` resolution branch (runner.py lines ~241-266) is
233-
# never taken and the value falls through to the ``else`` branch which
234-
# stores it verbatim. The recorded "variable" is therefore the literal
235-
# string 'V', not ``target.V``. (Sequence-form monitors resolve correctly.)
244+
def test_dict_monitor_str_value_resolved():
245+
# P15-H2 (was a documented DEFECT): dict-form *string* monitors must resolve
246+
# to their target Variable, exactly like sequence-form monitors.
236247
target = _Target()
237248
r = Runner(target, monitors={'a': 'V'}, progress_bar=False, jit=False)
238249
var, idx = r._monitors['a']
239-
assert var == 'V' # the bug: a string, not the Variable
250+
assert var is target.V # now resolved to the Variable
240251
assert idx is None
241252

242253

243-
def test_dict_monitor_str_value_nested_not_resolved():
244-
# NOTE: DEFECT (same root cause as above) - a dotted string value is also
245-
# stored verbatim and never resolved to ``target.sub.V``.
254+
def test_dict_monitor_str_value_nested_resolved():
255+
# P15-H2: a dotted string value resolves to the nested ``target.sub.V``.
246256
target = _Target()
247257
r = Runner(target, monitors={'a': 'sub.V'}, progress_bar=False, jit=False)
248258
var, idx = r._monitors['a']
249-
assert var == 'sub.V'
259+
assert var is target.sub.V
250260
assert idx is None
251261

252262

263+
def test_dict_monitor_str_with_index_resolved():
264+
# P15-H2: ``(name, index)`` dict values resolve the name and keep the index.
265+
target = _Target()
266+
r = Runner(target, monitors={'a': ('V', 2)}, progress_bar=False, jit=False)
267+
var, idx = r._monitors['a']
268+
assert var is target.V
269+
assert np.asarray(bm.as_jax(idx)).tolist() == [2]
270+
271+
253272
def test_dict_monitor_var_index_tuple(target):
254273
r = Runner(target, monitors={'a': (target.spike, 0)}, progress_bar=False, jit=False)
255274
_, idx = r._monitors['a']
@@ -274,15 +293,12 @@ def test_dict_monitor_callable_value(target):
274293
assert r._monitors['a'] is fn
275294

276295

277-
def test_dict_monitor_str_missing_var_not_validated():
278-
# NOTE: DEFECT (same root cause) - because dict string monitors are never
279-
# resolved, an *invalid* variable name like 'nope' is silently accepted and
280-
# stored verbatim instead of raising RunningError (contrast with the
281-
# sequence-form which validates and raises). See
282-
# ``test_dict_monitor_str_value_not_resolved``.
296+
def test_dict_monitor_str_missing_var_raises():
297+
# P15-H2 (was a documented DEFECT): an invalid variable name in a dict-form
298+
# string monitor must now be validated and raise, like the sequence form.
283299
target = _Target()
284-
r = Runner(target, monitors={'a': 'nope'}, progress_bar=False, jit=False)
285-
assert r._monitors['a'] == ('nope', None)
300+
with pytest.raises(RunningError):
301+
Runner(target, monitors={'a': 'nope'}, progress_bar=False, jit=False)
286302

287303

288304
def test_dict_monitor_nonstr_key_raises(target):
@@ -349,29 +365,24 @@ def test_find_dict_monitor_targets_type_guard(target):
349365
r._find_dict_monitor_targets(['not', 'a', 'dict'])
350366

351367

352-
def test_find_dict_monitor_targets_bare_string_branch():
353-
# NOTE: DEFECT - the ``isinstance(_mon, str)`` branch in
354-
# ``_find_dict_monitor_targets`` is dead under normal use (see
355-
# ``test_dict_monitor_str_value_not_resolved``) AND broken: it does
356-
# ``key, index = _mon[0], _mon[1]`` which takes the first *two characters*
357-
# of the string rather than treating the whole string as the variable name.
358-
# We call the helper directly to exercise this branch. With the bare string
359-
# 'Vx', key='V' (a real single-char variable) and index='x'.
368+
def test_find_dict_monitor_targets_name_tuple_branch():
369+
# P15-H2: the resolution branch fires for a ``(name_str, index)`` tuple
370+
# (the form produced by ``_format_dict_monitors`` for string monitors) and
371+
# resolves the whole name to the Variable, preserving the key and index.
360372
target = _Target()
361373
r = Runner(target, monitors=None, progress_bar=False, jit=False)
362-
out = r._find_dict_monitor_targets({'k': 'Vx'})
363-
var, index = out['V'] # NOTE: keyed by 'V' (first char), not 'k'
374+
out = r._find_dict_monitor_targets({'k': ('V', None)})
375+
var, index = out['k'] # keyed by 'k', not by the first char of 'V'
364376
assert var is target.V
365-
assert index == 'x' # NOTE: second char taken as the "index"
377+
assert index is None
366378

367379

368-
def test_find_dict_monitor_targets_bare_string_missing_var_raises():
369-
# NOTE: DEFECT - same broken branch: a string whose first character is not
370-
# an attribute of the target raises RunningError.
380+
def test_find_dict_monitor_targets_name_tuple_missing_var_raises():
381+
# P15-H2: a ``(name, index)`` tuple whose name is unknown raises RunningError.
371382
target = _Target()
372383
r = Runner(target, monitors=None, progress_bar=False, jit=False)
373384
with pytest.raises(RunningError):
374-
r._find_dict_monitor_targets({'k': 'zz'})
385+
r._find_dict_monitor_targets({'k': ('zz', None)})
375386

376387

377388
# --------------------------------------------------------------------------- #

0 commit comments

Comments
 (0)