Skip to content

Commit 59489f9

Browse files
committed
Merge master and make atomic writes umask-safe
2 parents 0703f84 + 97118ca commit 59489f9

5 files changed

Lines changed: 102 additions & 29 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1616
and stored spike-waveform subsets provide their waveform sample count.
1717
- TSV, JSON, text and `params.py` files are written atomically, so a crash during a save no longer
1818
truncates the file it was replacing, for instance `cluster_group.tsv`.
19+
- #57 a blank `dat_path` in `params.py` is now read as "no raw data file" instead of resolving to
20+
the dataset directory.
1921

2022
## [2.7.0] 2025-12-10
2123

‎phylib/io/model.py‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -322,13 +322,10 @@ def __init__(self, **kwargs):
322322
assert isinstance(self.dir_path, Path)
323323
assert self.dir_path.exists()
324324

325-
# Set dat_path.
326-
if not self.dat_path: # pragma: no cover
327-
self.dat_path = []
328-
elif not isinstance(self.dat_path, (list, tuple)):
329-
self.dat_path = [self.dat_path]
330-
assert isinstance(self.dat_path, (list, tuple))
331-
self.dat_path = [Path(p).resolve() if not Path(p).is_symlink() else p for p in self.dat_path]
325+
# Set dat_path. Blank entries mean "no raw data file", see _clean_dat_path().
326+
self.dat_path = [
327+
Path(p).resolve() if not Path(p).is_symlink() else p
328+
for p in _clean_dat_path(self.dat_path)]
332329

333330
self.dtype = getattr(self, 'dtype', np.int16)
334331
if not self.sample_rate: # pragma: no cover
@@ -1416,6 +1413,23 @@ def close(self):
14161413
_close_memmap(k, v)
14171414

14181415

1416+
def _clean_dat_path(dat_path):
1417+
"""Normalize a raw data path specification into a list of non-empty paths.
1418+
1419+
A blank `dat_path` means the dataset has no raw data file at all. Some sorters write
1420+
`dat_path = ''` in `params.py` when the recording has no file on disk, for instance
1421+
SpikeInterface with a simulated or in-memory recording. Such entries have to be dropped
1422+
rather than resolved, because `Path('')` is `Path('.')`, which would silently turn the
1423+
missing raw data file into the dataset directory itself.
1424+
1425+
"""
1426+
if not dat_path:
1427+
return []
1428+
if not isinstance(dat_path, (list, tuple)):
1429+
dat_path = [dat_path]
1430+
return [p for p in dat_path if str(p).strip()]
1431+
1432+
14191433
def _make_abs_path(p, dir_path):
14201434
p = Path(p)
14211435
if not op.isabs(p):
@@ -1438,9 +1452,8 @@ def get_template_params(params_path):
14381452
assert params['dir_path'].is_dir()
14391453
assert params['dir_path'].exists()
14401454

1441-
if isinstance(params['dat_path'], str):
1442-
params['dat_path'] = [params['dat_path']]
1443-
params['dat_path'] = [_make_abs_path(_, params['dir_path']) for _ in params['dat_path']]
1455+
params['dat_path'] = [
1456+
_make_abs_path(_, params['dir_path']) for _ in _clean_dat_path(params['dat_path'])]
14441457
return params
14451458

14461459

‎phylib/io/tests/test_model.py‎

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
# from phylib.utils import Bunch
1616
from phylib.utils.testing import captured_output
17-
from ..model import from_sparse, load_model
17+
from ..model import from_sparse, get_template_params, load_model
1818

1919
logger = logging.getLogger(__name__)
2020

@@ -135,6 +135,46 @@ def test_model_save(template_model_full):
135135
m.save_spike_clusters(m.spike_clusters)
136136

137137

138+
def _set_dat_path(params_path, value):
139+
"""Replace the `dat_path` assignment of a `params.py` file by a literal value."""
140+
lines = params_path.read_text().splitlines()
141+
lines = [
142+
'dat_path = %s' % value if line.startswith('dat_path') else line for line in lines]
143+
params_path.write_text('\n'.join(lines) + '\n')
144+
145+
146+
def test_model_blank_dat_path(template_path):
147+
# Regression test for #57: a dataset with no raw data file on disk has a blank `dat_path`
148+
# in params.py. `Path('')` is `Path('.')`, so a blank entry used to resolve to the dataset
149+
# directory instead of being dropped.
150+
_set_dat_path(template_path, "''")
151+
assert get_template_params(template_path)['dat_path'] == []
152+
153+
model = load_model(template_path)
154+
try:
155+
assert model.dat_path == []
156+
assert model.traces is None
157+
with captured_output() as (stdout, stderr):
158+
model.describe()
159+
assert stdout.getvalue().splitlines()[0].strip() == 'Data files'
160+
finally:
161+
model.close()
162+
163+
164+
def test_model_blank_dat_path_in_list(template_path):
165+
# Blank entries are dropped from a list too, and the real file is kept.
166+
_set_dat_path(template_path, "['', 'sim_binary.dat']")
167+
dat_path = get_template_params(template_path)['dat_path']
168+
assert [p.name for p in dat_path] == ['sim_binary.dat']
169+
170+
model = load_model(template_path)
171+
try:
172+
assert [p.name for p in model.dat_path] == ['sim_binary.dat']
173+
assert model.traces is not None
174+
finally:
175+
model.close()
176+
177+
138178
def test_model_reload_curated_dataset_without_templates(template_path):
139179
model = load_model(template_path)
140180
model.save_spikes_subset_waveforms(10, 16)

‎phylib/utils/_misc.py‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,9 @@
1515
import logging
1616
import os
1717
from pathlib import Path
18+
import secrets
19+
import stat
1820
import subprocess
19-
import tempfile
2021
from textwrap import dedent
2122

2223
import numpy as np
@@ -31,15 +32,12 @@
3132
#------------------------------------------------------------------------------
3233

3334
def _copy_file_mode(src, dst):
34-
"""Give `src` the permissions `dst` has, or the permissions a new file would get."""
35+
"""Give `src` the permissions of `dst` when `dst` exists."""
3536
try:
36-
mode = os.stat(dst).st_mode & 0o777
37-
except OSError:
38-
# The destination does not exist yet: reproduce what open() would have done, since
39-
# tempfile creates its files with 0600 and saving must not make the file private.
40-
umask = os.umask(0)
41-
os.umask(umask)
42-
mode = 0o666 & ~umask
37+
mode = stat.S_IMODE(os.stat(dst).st_mode)
38+
except FileNotFoundError:
39+
# The exclusive open used for a new temporary file already applied the process umask.
40+
return
4341
os.chmod(src, mode)
4442

4543

@@ -56,11 +54,22 @@ def _atomic_open(path, mode='w', **kwargs):
5654
"""
5755
path = Path(path)
5856
ensure_dir_exists(path.parent)
57+
if not mode.startswith('w'):
58+
raise ValueError("_atomic_open() only supports write modes")
59+
exclusive_mode = 'x' + mode[1:]
5960
# NOTE: the temporary file is hidden and does not end in a data file extension, so that a
6061
# file left behind by a hard kill is not picked up by the globs used to discover datasets.
61-
fd, tmp = tempfile.mkstemp(dir=str(path.parent), prefix='.' + path.name + '.', suffix='.tmp')
62+
# Opening it with `x` both prevents a name collision and lets the OS apply the current umask,
63+
# without temporarily changing that process-global setting.
64+
while True:
65+
tmp = path.parent / ('.%s.%s.tmp' % (path.name, secrets.token_hex(16)))
66+
try:
67+
f = open(tmp, exclusive_mode, **kwargs)
68+
break
69+
except FileExistsError: # pragma: no cover - cryptographically unlikely
70+
continue
6271
try:
63-
with os.fdopen(fd, mode, **kwargs) as f:
72+
with f:
6473
yield f
6574
f.flush()
6675
os.fsync(f.fileno())

‎phylib/utils/tests/test_misc.py‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -154,18 +154,22 @@ def test_atomic_open(tempdir):
154154
# The temporary file must not be left behind.
155155
assert sorted(p.name for p in path.parent.iterdir()) == ['test.txt']
156156

157-
# A new file gets the permissions open() would have given it, not the 0600 of a
158-
# temporary file.
159-
umask = os.umask(0)
160-
os.umask(umask)
161-
assert stat.S_IMODE(path.stat().st_mode) == 0o666 & ~umask
157+
# A new file gets the permissions open() would have given it, without reading or changing
158+
# the process-global umask. Compare it with an ordinary file created in the same directory.
159+
control = path.parent / 'control.txt'
160+
control.write_text('control')
161+
assert stat.S_IMODE(path.stat().st_mode) == stat.S_IMODE(control.stat().st_mode)
162+
control.unlink()
162163

163164
# The permissions of an existing file are preserved.
164-
path.chmod(0o640)
165+
expected_mode = stat.S_IMODE(path.stat().st_mode)
166+
if os.name != 'nt':
167+
path.chmod(0o640)
168+
expected_mode = 0o640
165169
with _atomic_open(path) as f:
166170
f.write('world')
167171
assert read_text(path) == 'world'
168-
assert stat.S_IMODE(path.stat().st_mode) == 0o640
172+
assert stat.S_IMODE(path.stat().st_mode) == expected_mode
169173

170174
# A failure half-way through leaves the previous file untouched and no leftovers.
171175
with raises(_Boom):
@@ -175,6 +179,11 @@ def test_atomic_open(tempdir):
175179
assert read_text(path) == 'world'
176180
assert sorted(p.name for p in path.parent.iterdir()) == ['test.txt']
177181

182+
# Binary writes use the same atomic and cleanup path, ready for numpy saves.
183+
with _atomic_open(path, mode='wb') as f:
184+
f.write(b'bytes')
185+
assert path.read_bytes() == b'bytes'
186+
178187

179188
@mark.parametrize('writer', ['write_tsv', '_write_tsv_simple'])
180189
def test_write_tsv_atomic(tempdir, monkeypatch, writer):

0 commit comments

Comments
 (0)