Skip to content

Commit bd9291d

Browse files
Merge pull request #425 from DataLab-Platform/fix/review-2026-10-24
Atomic workspace saving and non-uniform coordinates in Web API transfers
2 parents b8b74f8 + 70170f2 commit bd9291d

12 files changed

Lines changed: 414 additions & 29 deletions

File tree

‎datalab/gui/h5io.py‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
from datalab.env import execenv
2828
from datalab.gui.panel.base import H5ImportBatch
2929
from datalab.h5 import H5Importer
30-
from datalab.h5.native import NativeH5Reader, NativeH5Writer
30+
from datalab.h5.native import NativeH5Reader, atomic_native_writer
3131
from datalab.utils.qthelpers import create_progress_bar, qt_try_loadsave_file
3232

3333
if TYPE_CHECKING:
@@ -80,14 +80,15 @@ def __progbartitle(fname: str) -> str:
8080
return _("Loading data from %s...") % osp.basename(fname)
8181

8282
def save_file(self, filename: str) -> None:
83-
"""Save all signals and images from DataLab model into a HDF5 file"""
84-
writer = NativeH5Writer(filename)
85-
try:
83+
"""Save all signals and images from DataLab model into a HDF5 file
84+
85+
The file is replaced only once completely written: if saving fails, the
86+
previous file (if any) is left untouched.
87+
"""
88+
with atomic_native_writer(filename) as writer:
8689
for panel in self.mainwindow.panels:
8790
panel.serialize_to_hdf5(writer)
8891
self.mainwindow.provenance.save(writer.h5)
89-
finally:
90-
writer.close()
9192

9293
@staticmethod
9394
def __read_provenance(

‎datalab/h5/history.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
from datalab.config import Conf, _
1515
from datalab.gui.panel.base import H5ImportBatch
16-
from datalab.h5.native import NativeH5Reader, NativeH5Writer
16+
from datalab.h5.native import NativeH5Reader, NativeH5Writer, atomic_native_writer
1717
from datalab.history import HistorySession
1818
from datalab.objectmodel import get_uuid
1919
from datalab.utils.qthelpers import qt_try_loadsave_file, save_restore_stds
@@ -53,7 +53,7 @@ def save_to_dlhist_file(panel: HistoryPanel, filename: str | None = None) -> boo
5353
filename += ".dlhist"
5454
with qt_try_loadsave_file(panel.parentWidget(), filename, "save"):
5555
Conf.base_dir.set(filename)
56-
with NativeH5Writer(filename) as writer:
56+
with atomic_native_writer(filename) as writer:
5757
# Make the .dlhist file panel-contained: store the signal and
5858
# image panel objects (all of them) alongside the history, so
5959
# that reopening restores both the data objects and the history

‎datalab/h5/native.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,13 @@
88

99
from __future__ import annotations
1010

11+
import contextlib
1112
import importlib
13+
import os
14+
import os.path as osp
15+
import stat
16+
import tempfile
17+
from collections.abc import Generator
1218
from typing import Any, Callable
1319

1420
from guidata.io import HDF5Reader, HDF5Writer
@@ -73,6 +79,57 @@ def write(self, val: Any, group_name: str | None = None) -> None:
7379
raise
7480

7581

82+
def _set_final_permissions(tmpname: str, filename: str) -> None:
83+
"""Give the temporary file the permissions the final file must have.
84+
85+
Args:
86+
tmpname: Temporary file name (created with owner-only permissions).
87+
filename: Final file name: its permissions are kept if it exists,
88+
otherwise those of a newly created file are applied.
89+
"""
90+
if osp.exists(filename):
91+
mode = stat.S_IMODE(os.stat(filename).st_mode)
92+
else:
93+
umask = os.umask(0)
94+
os.umask(umask)
95+
mode = 0o666 & ~umask
96+
os.chmod(tmpname, mode)
97+
98+
99+
@contextlib.contextmanager
100+
def atomic_native_writer(filename: str) -> Generator[NativeH5Writer, None, None]:
101+
"""Write a native DataLab HDF5 file without losing the previous one on failure.
102+
103+
The file is written to a temporary file of the same directory, which replaces
104+
``filename`` only once it has been completely written and closed. If writing
105+
fails (serialization error, full disk, interruption...), the temporary file is
106+
removed and the previous file, if any, is left untouched.
107+
108+
Args:
109+
filename: HDF5 file name
110+
111+
Yields:
112+
Writer of the temporary file
113+
"""
114+
filename = osp.abspath(filename)
115+
fd, tmpname = tempfile.mkstemp(
116+
prefix=f".{osp.basename(filename)}.", suffix=".tmp", dir=osp.dirname(filename)
117+
)
118+
os.close(fd)
119+
try:
120+
writer = NativeH5Writer(tmpname)
121+
try:
122+
yield writer
123+
finally:
124+
writer.close()
125+
_set_final_permissions(tmpname, filename)
126+
os.replace(tmpname, filename)
127+
except BaseException:
128+
with contextlib.suppress(OSError):
129+
os.remove(tmpname)
130+
raise
131+
132+
76133
class NativeH5Reader(HDF5Reader):
77134
"""DataLab signal/image objects HDF5 guidata dataset Writer class
78135
Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
# Copyright (c) DataLab Platform Developers, BSD 3-Clause license, see LICENSE file.
2+
3+
"""
4+
Atomic HDF5 save unit tests
5+
---------------------------
6+
7+
A failed save must never destroy the previous version of a workspace (or
8+
``.dlhist``) file: the file is written to a temporary file of the same directory,
9+
which replaces the previous file only once completely written.
10+
"""
11+
12+
# guitest: show
13+
14+
from __future__ import annotations
15+
16+
import os
17+
import os.path as osp
18+
from types import SimpleNamespace
19+
20+
import h5py
21+
import numpy as np
22+
import pytest
23+
from sigima.tests.data import create_paracetamol_signal
24+
25+
from datalab.gui.h5io import H5InputOutput
26+
from datalab.h5.native import DATALAB_VERSION_NAME, atomic_native_writer
27+
from datalab.tests import datalab_test_app_context, helpers
28+
29+
30+
def _write_previous_file(filename: str) -> bytes:
31+
"""Write a previous version of a file and return its contents."""
32+
with h5py.File(filename, "w") as h5file:
33+
h5file["original"] = np.arange(5.0)
34+
with open(filename, "rb") as fdesc:
35+
return fdesc.read()
36+
37+
38+
def _read_bytes(filename: str) -> bytes:
39+
"""Return the contents of a file."""
40+
with open(filename, "rb") as fdesc:
41+
return fdesc.read()
42+
43+
44+
def _assert_no_temporary_file(dirname: str, filename: str) -> None:
45+
"""Check that no temporary file was left next to the saved file."""
46+
assert sorted(os.listdir(dirname)) == [osp.basename(filename)]
47+
48+
49+
def test_atomic_writer_replaces_file_once_written(tmp_path) -> None:
50+
"""A successful write replaces the previous file."""
51+
filename = str(tmp_path / "workspace.h5")
52+
_write_previous_file(filename)
53+
with atomic_native_writer(filename) as writer:
54+
writer.h5["partial"] = np.arange(3.0)
55+
assert not osp.samefile(writer.filename, filename)
56+
with h5py.File(filename, "r") as h5file:
57+
assert set(h5file) == {DATALAB_VERSION_NAME, "partial"}
58+
_assert_no_temporary_file(str(tmp_path), filename)
59+
60+
61+
def test_atomic_writer_creates_new_file(tmp_path) -> None:
62+
"""A new file is created with the permissions of a newly created file."""
63+
filename = str(tmp_path / "new.h5")
64+
reference = str(tmp_path / "reference")
65+
with open(reference, "wb"):
66+
pass
67+
with atomic_native_writer(filename) as writer:
68+
writer.h5["data"] = np.arange(3.0)
69+
assert os.stat(filename).st_mode == os.stat(reference).st_mode
70+
os.remove(reference)
71+
_assert_no_temporary_file(str(tmp_path), filename)
72+
73+
74+
def test_atomic_writer_keeps_previous_file_on_failure(tmp_path) -> None:
75+
"""A failed write leaves the previous file untouched and no temporary file."""
76+
filename = str(tmp_path / "workspace.h5")
77+
previous = _write_previous_file(filename)
78+
with pytest.raises(OSError, match="disk full"):
79+
with atomic_native_writer(filename) as writer:
80+
writer.h5["partial"] = np.arange(3.0)
81+
raise OSError("disk full")
82+
assert _read_bytes(filename) == previous
83+
_assert_no_temporary_file(str(tmp_path), filename)
84+
85+
86+
class _Panel:
87+
"""Panel double writing one dataset, then optionally failing."""
88+
89+
def __init__(self, name: str, fail: bool = False) -> None:
90+
self.name = name
91+
self.fail = fail
92+
93+
def serialize_to_hdf5(self, writer) -> None:
94+
"""Serialize the panel, like ``BaseDataPanel.serialize_to_hdf5``."""
95+
writer.h5[self.name] = np.arange(3.0)
96+
if self.fail:
97+
raise OSError("write error")
98+
99+
100+
class _Provenance:
101+
"""Provenance double, optionally failing."""
102+
103+
def __init__(self, fail: bool = False) -> None:
104+
self.fail = fail
105+
106+
def save(self, h5file: h5py.File) -> None:
107+
"""Save the provenance block, like ``ProvenanceService.save``."""
108+
h5file["provenance"] = np.arange(2.0)
109+
if self.fail:
110+
raise ValueError("provenance error")
111+
112+
113+
@pytest.mark.parametrize(
114+
"panels, provenance",
115+
[
116+
([_Panel("signals"), _Panel("images", fail=True)], _Provenance()),
117+
([_Panel("signals"), _Panel("images")], _Provenance(fail=True)),
118+
],
119+
ids=["panel", "provenance"],
120+
)
121+
def test_save_file_keeps_previous_workspace_on_failure(
122+
tmp_path, panels: list[_Panel], provenance: _Provenance
123+
) -> None:
124+
"""Regression: saving truncated the previous workspace file before writing,
125+
so that an error in a panel or in the provenance destroyed it."""
126+
filename = str(tmp_path / "workspace.h5")
127+
previous = _write_previous_file(filename)
128+
h5io = H5InputOutput(SimpleNamespace(panels=panels, provenance=provenance))
129+
with pytest.raises((OSError, ValueError)):
130+
h5io.save_file(filename)
131+
assert _read_bytes(filename) == previous
132+
_assert_no_temporary_file(str(tmp_path), filename)
133+
134+
135+
def test_failed_save_keeps_workspace_modified(monkeypatch) -> None:
136+
"""After a failed save, the previous file is intact and the workspace is still
137+
marked as modified."""
138+
with helpers.WorkdirRestoringTempDir() as tmpdir:
139+
filename = osp.join(tmpdir, "workspace.h5")
140+
previous = _write_previous_file(filename)
141+
with datalab_test_app_context(console=False) as win:
142+
win.signalpanel.add_object(create_paracetamol_signal())
143+
assert win.is_modified()
144+
145+
def fail(_writer) -> None:
146+
raise OSError("write error")
147+
148+
monkeypatch.setattr(win.imagepanel, "serialize_to_hdf5", fail)
149+
with pytest.raises(OSError, match="write error"):
150+
win.save_h5_workspace(filename)
151+
assert win.is_modified()
152+
assert _read_bytes(filename) == previous
153+
monkeypatch.undo()
154+
win.save_h5_workspace(filename)
155+
assert not win.is_modified()
156+
assert not [name for name in os.listdir(tmpdir) if name.endswith(".tmp")]
157+
158+
159+
if __name__ == "__main__":
160+
pytest.main([__file__, "-v"])

‎datalab/tests/scenarios/common.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -328,9 +328,9 @@ def run_image_computations(
328328
width=width_param,
329329
height=height_param,
330330
xmin=ima1.x0,
331-
xmax=ima1.x0 + ima1.width,
331+
xmax=ima1.x0 + ima1.width - ima1.dx,
332332
ymin=ima1.y0,
333-
ymax=ima1.y0 + ima1.height,
333+
ymax=ima1.y0 + ima1.height - ima1.dy,
334334
)
335335
panel.processor.run_feature("resampling", param)
336336

‎datalab/tests/webapi_extra_test.py‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,76 @@ def test_image_obj_metadata_with_numpy_origin(self) -> None:
363363
assert result.x0 == pytest.approx(1.5)
364364
assert result.dx == pytest.approx(0.1)
365365

366+
def test_image_non_uniform_coords_round_trip(self) -> None:
367+
"""Non-uniform image coordinates survive a round trip.
368+
369+
Regression: they were lost, the image being rebuilt with uniform
370+
coordinates (its pixels survived, but not their physical positions).
371+
"""
372+
obj = _make_image(shape=(3, 4))
373+
obj.xunit, obj.yunit = "mm", "s"
374+
obj.set_coords(np.array([1.0, 2.0, 4.0, 8.0]), np.array([10.0, 11.0, 15.0]))
375+
result = deserialize_object_from_npz(serialize_object_to_npz(obj))
376+
assert not result.is_uniform_coords
377+
np.testing.assert_array_equal(result.xcoords, obj.xcoords)
378+
np.testing.assert_array_equal(result.ycoords, obj.ycoords)
379+
np.testing.assert_array_equal(result.data, obj.data)
380+
assert (result.xunit, result.yunit) == ("mm", "s")
381+
382+
def test_image_uniform_coords_round_trip(self) -> None:
383+
"""Uniform images are stored without coordinate arrays."""
384+
obj = _make_image(shape=(3, 4))
385+
obj.set_uniform_coords(0.5, 2.0, -1.0, 3.0)
386+
data = serialize_object_to_npz(obj)
387+
with zipfile.ZipFile(io.BytesIO(data)) as zf:
388+
assert sorted(zf.namelist()) == ["data.npy", "metadata.json"]
389+
assert json.loads(zf.read("metadata.json"))["is_uniform_coords"] is True
390+
result = deserialize_object_from_npz(data)
391+
assert result.is_uniform_coords
392+
assert (result.dx, result.dy, result.x0, result.y0) == (0.5, 2.0, -1.0, 3.0)
393+
394+
def test_image_archive_without_coords_flag_is_uniform(self) -> None:
395+
"""Archives written by earlier versions (no ``is_uniform_coords``) are read
396+
with uniform coordinates."""
397+
buf = io.BytesIO()
398+
with zipfile.ZipFile(buf, "w") as zf:
399+
arr = io.BytesIO()
400+
np.save(arr, np.zeros((3, 4)))
401+
zf.writestr("data.npy", arr.getvalue())
402+
metadata = {"type": "image", "x0": 1.0, "y0": 2.0, "dx": 0.5, "dy": 0.25}
403+
zf.writestr("metadata.json", json.dumps(metadata))
404+
result = deserialize_object_from_npz(buf.getvalue())
405+
assert result.is_uniform_coords
406+
assert (result.dx, result.dy, result.x0, result.y0) == (0.5, 0.25, 1.0, 2.0)
407+
408+
@pytest.mark.parametrize(
409+
"xcoords, ycoords, match",
410+
[
411+
(None, np.arange(3.0), "missing xcoords.npy"),
412+
(np.arange(3.0), np.arange(3.0), "do not match image shape"),
413+
(np.arange(4.0), np.arange(4.0), "do not match image shape"),
414+
],
415+
)
416+
def test_image_invalid_non_uniform_coords_raise(
417+
self, xcoords, ycoords, match: str
418+
) -> None:
419+
"""Missing or mismatched coordinate arrays are rejected."""
420+
buf = io.BytesIO()
421+
with zipfile.ZipFile(buf, "w") as zf:
422+
for name, array in (
423+
("data.npy", np.zeros((3, 4))),
424+
("xcoords.npy", xcoords),
425+
("ycoords.npy", ycoords),
426+
):
427+
if array is not None:
428+
arr = io.BytesIO()
429+
np.save(arr, array)
430+
zf.writestr(name, arr.getvalue())
431+
metadata = {"type": "image", "is_uniform_coords": False}
432+
zf.writestr("metadata.json", json.dumps(metadata))
433+
with pytest.raises(ValueError, match=match):
434+
deserialize_object_from_npz(buf.getvalue())
435+
366436

367437
# =============================================================================
368438
# Routes integration tests
@@ -638,6 +708,29 @@ def test_get_data_missing_object(self, api_client) -> None:
638708
response = client.get("/api/v1/objects/nope/data", headers=_auth(token))
639709
assert response.status_code == 404
640710

711+
def test_non_uniform_image_round_trip_through_endpoints(self, api_client) -> None:
712+
"""An image with non-uniform coordinates keeps them when uploaded with
713+
``PUT data`` and downloaded with ``GET data``."""
714+
client, token, adapter = api_client
715+
obj = _make_image("NU", shape=(3, 4))
716+
xcoords, ycoords = np.array([1.0, 2.0, 4.0, 8.0]), np.array([10.0, 11.0, 15.0])
717+
obj.set_coords(xcoords, ycoords)
718+
response = client.put(
719+
"/api/v1/objects/NU/data",
720+
content=serialize_object_to_npz(obj),
721+
headers={**_auth(token), "Content-Type": "application/x-npz"},
722+
)
723+
assert response.status_code == 201
724+
stored = adapter.get_object("NU")
725+
assert not stored.is_uniform_coords
726+
np.testing.assert_array_equal(stored.xcoords, xcoords)
727+
response = client.get("/api/v1/objects/NU/data", headers=_auth(token))
728+
assert response.status_code == 200
729+
result = deserialize_object_from_npz(response.content)
730+
assert not result.is_uniform_coords
731+
np.testing.assert_array_equal(result.xcoords, xcoords)
732+
np.testing.assert_array_equal(result.ycoords, ycoords)
733+
641734
def test_put_creates_object(self, api_client) -> None:
642735
"""``PUT data`` creates a new object from an NPZ payload."""
643736
client, token, adapter = api_client

0 commit comments

Comments
 (0)