From 6453126cad3105cc0834268beaa08690829d34f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 20:29:26 +0000 Subject: [PATCH 01/21] lightsheet: report tiles needed for the current view; keep launch window closed Launch window: closeLaunchWindow() cleared _window, and the very next progress.note() went through _mode() -> _launchWindow(), found no window and built a fresh "starting ..." one that nothing ever closed. A _window_closed latch now stops the rebuild (same fix in genepyramid, where the bug is latent). View tile tally: the fetch counter only saw fetches as napari issued them, so it could say "12 in flight" but not how many the view needed. New viewtiles module works that out from napari's data_level, corner_pixels and dims point plus each level's chunk grid and stored chunk list (now exposed as ImagePyramidLoader.levelMeta), skipping sparse chunks. A main-thread QTimer polls it and feeds the counter, so the status bar reads "view 37/120 tiles ready | ...", the canvas overlay reads "Loading view: 37 / 120 tiles (12 in flight)" and hides when the view is complete, and stderr logs "view complete: N tile(s) in Xs". Skipped in single-level and viewport-clip modes, where the level index does not line up with the level documents. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/gui/app/genepyramid/progress.py | 11 +- src/ndi/gui/app/lightsheetZarr/progress.py | 11 +- src/ndi/gui/app/lightsheetZarr/viewer.py | 106 +++++++-- src/ndi/gui/app/lightsheetZarr/viewtiles.py | 229 ++++++++++++++++++++ src/ndi/pyramid/loader.py | 13 ++ src/ndi/pyramid/multiscale.py | 16 ++ tests/test_gui_genepyramid_progress.py | 5 + tests/test_gui_lightsheetZarr_viewtiles.py | 220 +++++++++++++++++++ 8 files changed, 593 insertions(+), 18 deletions(-) create mode 100644 src/ndi/gui/app/lightsheetZarr/viewtiles.py create mode 100644 tests/test_gui_lightsheetZarr_viewtiles.py diff --git a/src/ndi/gui/app/genepyramid/progress.py b/src/ndi/gui/app/genepyramid/progress.py index 492389b5..0e75641c 100644 --- a/src/ndi/gui/app/genepyramid/progress.py +++ b/src/ndi/gui/app/genepyramid/progress.py @@ -166,6 +166,12 @@ def observer(event, _uri, done, total): _window = None _window_failed = False +#: Set once closeLaunchWindow has run. Without it, the first progress +#: call after the close (``note("viewer running ...")`` is one) goes +#: through ``_mode()`` -> ``_launchWindow()``, finds no window, and +#: builds a FRESH one stuck on "starting ..." that nothing ever closes. +_window_closed = False + def _mode() -> str: """Which indicator to use: ``"gui"``, ``"text"`` or ``"off"``.""" @@ -221,7 +227,7 @@ def _launchWindow(): harmless where they are already the default (Qt6 deprecates both). """ global _window, _window_failed - if _window is not None or _window_failed: + if _window is not None or _window_failed or _window_closed: return _window try: from qtpy.QtCore import Qt @@ -324,8 +330,9 @@ def closeLaunchWindow() -> None: a progress window that outlives the thing it was reporting on is worse than none, because it makes a finished launch look stuck. """ - global _window + global _window, _window_closed win, _window = _window, None + _window_closed = True if win is None: return with contextlib.suppress(Exception): diff --git a/src/ndi/gui/app/lightsheetZarr/progress.py b/src/ndi/gui/app/lightsheetZarr/progress.py index c51eeb62..148732d3 100644 --- a/src/ndi/gui/app/lightsheetZarr/progress.py +++ b/src/ndi/gui/app/lightsheetZarr/progress.py @@ -188,6 +188,12 @@ def observer(event, _uri, done, total): _window = None _window_failed = False +#: Set once closeLaunchWindow has run. Without it, the first progress +#: call after the close (``note("viewer running ...")`` is one) goes +#: through ``_mode()`` -> ``_launchWindow()``, finds no window, and +#: builds a FRESH one stuck on "starting ..." that nothing ever closes. +_window_closed = False + def _mode() -> str: """Which indicator to use: ``"gui"``, ``"text"`` or ``"off"``.""" @@ -231,7 +237,7 @@ def _launchWindow(): QApplication ourselves, which napari then reuses. """ global _window, _window_failed - if _window is not None or _window_failed: + if _window is not None or _window_failed or _window_closed: return _window try: from qtpy.QtCore import Qt @@ -331,8 +337,9 @@ def closeLaunchWindow() -> None: ``processEvents`` at the end flushes whatever the close and destroy queued so the desktop actually repaints without us. """ - global _window + global _window, _window_closed win, _window = _window, None + _window_closed = True if win is None: return widget = win["widget"] diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index 35f0bd73..8d430edc 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -102,15 +102,15 @@ def __init__(self, viewer): from qtpy.QtCore import QCoreApplication, QObject, Qt, Signal class _Bridge(QObject): - update = Signal(str, int) + update = Signal(str, str) def __init__(self, outer): super().__init__() self._outer = outer self.update.connect(self._on_update, Qt.QueuedConnection) - def _on_update(self, message, in_flight): - self._outer._apply(message, in_flight) + def _on_update(self, message, overlay_text): + self._outer._apply(message, overlay_text) self._bridge = _Bridge(self) app = QCoreApplication.instance() @@ -119,7 +119,7 @@ def _on_update(self, message, in_flight): except Exception: self._bridge = None - def _apply(self, message: str, in_flight: int) -> None: + def _apply(self, message: str, overlay_text: str) -> None: """Runs on the main thread (queued-signal callback).""" v = self._viewer try: @@ -141,20 +141,23 @@ def _apply(self, message: str, in_flight: int) -> None: except Exception: pass try: - if in_flight > 0: - overlay.text = f"Loading tiles ... ({in_flight} in flight)" + if overlay_text: + overlay.text = overlay_text overlay.visible = True else: overlay.visible = False except Exception: pass - def push(self, message: str, *, in_flight: int = 0) -> None: - """Called from any thread; delivery hops to main via a signal.""" + def push(self, message: str, *, overlay: str = "") -> None: + """Called from any thread; delivery hops to main via a signal. + + OVERLAY is the canvas text; empty hides the overlay. + """ if self._bridge is None: return try: - self._bridge.update.emit(message, int(in_flight)) + self._bridge.update.emit(message, str(overlay or "")) except Exception: pass @@ -211,6 +214,12 @@ def __init__( self._last_duration: float | None = None self._last_bytes: int | None = None self._session_start = time.monotonic() + # Tiles the CURRENT VIEW needs vs has on disk, set from the + # main-thread poll in viewtiles.attachViewTally. None until the + # first poll, or when napari does not expose enough to tally. + self._view_ready: int | None = None + self._view_needed: int | None = None + self._view_since: float | None = None self._stop = threading.Event() self._heartbeat = threading.Thread( target=self._heartbeat_loop, @@ -230,6 +239,29 @@ def set_status_reporter(self, reporter) -> None: with self._lock: self._status_reporter = reporter + def set_view_tally(self, ready: int | None, needed: int | None) -> None: + """Record how many of the current view's tiles are on disk. + + Logs once when a view finishes loading, with how long it took + from the first poll that found it incomplete. + """ + with self._lock: + now = time.monotonic() + was_complete = self._view_needed is not None and self._view_ready == self._view_needed + self._view_ready, self._view_needed = ready, needed + if ready is None or needed is None: + self._view_since = None + elif ready < needed: + if self._view_since is None or was_complete: + self._view_since = now + elif self._view_since is not None: + _ls( + f"view complete: {needed} tile(s) in {now - self._view_since:.1f}s", + file=self._out, + ) + self._view_since = None + self._maybe_print(force=True) + def __call__(self, event, uri, done, total): with self._lock: now = time.monotonic() @@ -265,18 +297,37 @@ def _maybe_print(self, force: bool = False) -> None: if self._last_duration is not None: mb = (self._last_bytes or 0) / (1024.0 * 1024.0) last = f"; last fetch {self._last_duration:.2f}s, {mb:.1f} MB" + ready, needed = self._view_ready, self._view_needed + view = "" + if needed is not None: + view = f"view: {ready}/{needed} tiles ready; " _ls( - f"tiles: {self._done} loaded / {self._started} requested " + f"{view}tiles: {self._done} loaded / {self._started} requested " f"(in flight: {in_flight}){last}", file=self._out, ) if self._status_reporter is not None: - status_msg = ( - f"lightsheet: {self._done}/{self._started} tiles" + parts = [] + if needed is not None: + parts.append(f"view {ready}/{needed} tiles ready") + parts.append( + f"{self._done}/{self._started} fetched" + (f" ({in_flight} in flight)" if in_flight else "") - + (f" | last {self._last_duration:.2f}s" if self._last_duration else "") ) - self._status_reporter.push(status_msg, in_flight=in_flight) + if self._last_duration: + parts.append(f"last {self._last_duration:.2f}s") + status_msg = "lightsheet: " + " | ".join(parts) + if needed is not None and ready < needed: + overlay = f"Loading view: {ready} / {needed} tiles" + if in_flight: + overlay += f" ({in_flight} in flight)" + elif in_flight: + # View is complete (or untallied); what is still + # moving is prefetch or a neighbouring region. + overlay = f"Loading tiles ... ({in_flight} in flight)" + else: + overlay = "" + self._status_reporter.push(status_msg, overlay=overlay) def _heartbeat_loop(self) -> None: while not self._stop.wait(self._idle_interval): @@ -757,6 +808,33 @@ def openPyramid( # the dataset-shape edges called out in the README. _attach_cloud_panel(viewer) + # Tiles needed for the current view vs tiles on disk, polled on a + # main-thread timer and fed to the counter, so the status bar and + # overlay say "37 / 120" rather than only "12 in flight". Skipped + # in the modes where layer.data is not the full level ladder + # (single-level, viewport clip): there the level index napari + # reports does not line up with the level documents. + # Each env var is read with the parser its own feature uses. + from ndi.gui.app.lightsheetZarr.viewport import _env_true as _clip_env_true + from ndi.pyramid.multiscale import _env_true as _level_env_true + + if not ( + _level_env_true("NDI_LIGHTSHEET_SINGLE_LEVEL") + or _clip_env_true("NDI_LIGHTSHEET_VIEWPORT_CLIP") + ): + try: + from ndi.gui.app.lightsheetZarr.viewtiles import attachViewTally + + view_timer = attachViewTally( # noqa: F841 - parented to the window + viewer, added_layers, loader.levelMeta, fetcher, counter + ) + except Exception as exc: # noqa: BLE001 - a tally is never worth the viewer + print( + f"[lightsheet] view tile tally: could not start ({exc})", + file=sys.stderr, + flush=True, + ) + # Launch window is closed BEFORE napari.run() because napari's # event loop blocks the main thread and our Qt window can't # repaint during it -- a frozen progress bar next to napari looks diff --git a/src/ndi/gui/app/lightsheetZarr/viewtiles.py b/src/ndi/gui/app/lightsheetZarr/viewtiles.py new file mode 100644 index 00000000..506fa99f --- /dev/null +++ b/src/ndi/gui/app/lightsheetZarr/viewtiles.py @@ -0,0 +1,229 @@ +"""How many tiles does the current view need, and how many are here? + +The fetch counter in :mod:`.viewer` sees fetches only as napari asks +for them, so it can say "12 in flight" but not "12 in flight, 83 to +go". The total IS knowable: napari draws one level per layer +(``layer.data_level``) over one region of it (``layer.corner_pixels``) +at one slice of the non-displayed axes (``viewer.dims.point``). The +level documents give the chunk grid and the list of stored chunks, so +the set of chunk files covering that region -- minus the sparse ones +that resolve to fill_value with no fetch -- is the set the view needs. +A chunk is "ready" once the fetcher has it on local disk. + +Split in two so the arithmetic is testable without napari: + +* :func:`chunksInBox` -- pure: per-axis inclusive data bounds in, the + stored ``chunk.bin_#`` names covering them out. +* :func:`layerBox` / :func:`viewTally` -- read napari state and feed + the pure half. Anything napari does not expose the way we expect + returns None, and the viewer falls back to the in-flight count. + +:func:`attachViewTally` polls on a main-thread QTimer (napari layer +state is not safe to read from the fetcher threads) and hands each +change to the fetch counter, which owns the status bar and overlay. +""" + +from __future__ import annotations + +from typing import Any + +__all__ = ["chunksInBox", "layerBox", "viewTally", "attachViewTally"] + +#: A view whose box spans more chunk positions than this is not +#: tallied (it would stall the main thread on every tick). napari's 3D +#: mode draws the coarsest level, which stays far below it. +MAX_POSITIONS = 50_000 + +#: Poll interval for the main-thread timer, in milliseconds. +POLL_MS = 400 + + +def chunksInBox(meta: dict, lo: list[int], hi: list[int]) -> list[str] | None: + """Stored chunk names covering the inclusive data box [LO, HI]. + + LO and HI have one entry per axis of the level's chunk grid. Bounds + are clipped to the grid; a box past the edge just covers fewer + chunks. Returns None when the box spans more than + :data:`MAX_POSITIONS` chunk positions. + """ + chunks = meta["chunks"] + grid = meta["chunk_grid"] + stored = meta.get("stored") + n = len(grid) + if len(lo) != n or len(hi) != n: + return None + + ranges = [] + count = 1 + for a in range(n): + first = max(0, int(lo[a]) // chunks[a]) + last = min(grid[a] - 1, int(hi[a]) // chunks[a]) + if last < first: + return [] + ranges.append(range(first, last + 1)) + count *= last - first + 1 + if count > MAX_POSITIONS: + return None + + strides = [1] * n + for a in range(n - 2, -1, -1): + strides[a] = strides[a + 1] * grid[a + 1] + + names: list[str] = [] + + def walk(axis: int, linear: int) -> None: + if axis == n: + name = f"chunk.bin_{linear + 1:d}" + if stored is None or name in stored: + names.append(name) + return + for i in ranges[axis]: + walk(axis + 1, linear + i * strides[axis]) + + walk(0, 0) + return names + + +def _gridAxisMap(meta: dict, layer_ndim: int) -> tuple[list[int], int] | None: + """Map layer axes onto grid axes. + + Returns ``(layer_to_grid, c_axis)``: the grid axis for each layer + axis, and the grid's channel axis (-1 when there is none). The + layer arrays are the level arrays with the channel axis taken out, + so a ``tczyx`` grid backs a ``tzyx`` layer. Layer axes line up + with the TRAILING grid axes; any leading grid axis the layer does + not carry (a singleton ``t``) is pinned to 0 by the caller. + """ + n = len(meta["chunk_grid"]) + axes = meta.get("axes") or ("tczyx" if n == 5 else "") + c = axes.index("c") if len(axes) == n and "c" in axes else -1 + if n == layer_ndim: + # Nothing was stripped: the layer IS the grid. + return list(range(n)), -1 + spatial = [a for a in range(n) if a != c] + if len(spatial) < layer_ndim: + return None + return spatial[-layer_ndim:], c + + +def layerBox(layer, viewer, meta_levels: list[dict], channel: int = 0): + """``(level_index, lo, hi)`` for what napari is drawing of LAYER. + + LO/HI are inclusive data bounds on the level's chunk grid. None + when napari does not expose what we need. + """ + # The level index only lines up with META_LEVELS on a native + # multiscale layer over the full ladder (not single-level mode). + if not getattr(layer, "multiscale", False): + return None + try: + if len(layer.data) != len(meta_levels): + return None + level = int(layer.data_level) + meta = meta_levels[level] + corners = layer.corner_pixels + ndim = int(layer.ndim) + # Layer axes are the LAST ndim world axes. + offset = int(viewer.dims.ndim) - ndim + displayed = [int(d) - offset for d in viewer.dims.displayed] + point = list(viewer.dims.point)[-ndim:] + data_pt = list(layer.world_to_data(point)) + factors = layer.downsample_factors[level] + except Exception: # noqa: BLE001 - any napari shape surprise -> unknown + return None + + mapping = _gridAxisMap(meta, ndim) + if mapping is None: + return None + layer_to_grid, c_axis = mapping + + n = len(meta["chunk_grid"]) + lo = [0] * n + hi = [0] * n + for j in range(ndim): + g = layer_to_grid[j] + if j in displayed: + lo[g] = int(corners[0][j]) + hi[g] = int(corners[1][j]) + else: + # world_to_data maps to level-0 data; scale down to LEVEL. + idx = int(float(data_pt[j]) / float(factors[j])) + lo[g] = hi[g] = idx + if c_axis >= 0: + lo[c_axis] = hi[c_axis] = int(channel) + return level, lo, hi + + +def viewTally(viewer, layers, meta_levels: list[dict], fetcher) -> tuple[int, int] | None: + """``(ready, needed)`` stored chunks for the current view, or None. + + Counts each chunk once even when two channel layers share it. + Invisible layers draw nothing and are skipped. + """ + if not meta_levels: + return None + wanted: dict[tuple, Any] = {} + for c, layer in enumerate(layers): + if not getattr(layer, "visible", True): + continue + box = layerBox(layer, viewer, meta_levels, channel=c) + if box is None: + return None + level, lo, hi = box + names = chunksInBox(meta_levels[level], lo, hi) + if names is None: + return None + doc = meta_levels[level]["doc"] + for name in names: + wanted[(level, name)] = doc + + ready = 0 + for (_level, name), doc in wanted.items(): + try: + if fetcher.chunkPathIfCached(doc, name) is not None: + ready += 1 + except Exception: # noqa: BLE001 - unreachable scope: not ready + pass + return ready, len(wanted) + + +def attachViewTally(viewer, layers, meta_levels: list[dict], fetcher, counter): + """Poll :func:`viewTally` on a main-thread timer; report to COUNTER. + + COUNTER gets ``set_view_tally(ready, needed)`` on every change and + ``set_view_tally(None, None)`` when the view cannot be tallied. + Returns the QTimer (kept alive by its parent window), or None when + Qt or the window is not available. + """ + try: + from qtpy.QtCore import QTimer + except Exception: # noqa: BLE001 + return None + + parent = None + try: + window = viewer.window + parent = getattr(window, "_qt_window", None) + except Exception: # noqa: BLE001 + pass + + last = {"value": object()} + + def tick() -> None: + try: + value = viewTally(viewer, layers, meta_levels, fetcher) + except Exception: # noqa: BLE001 - never take the viewer down + value = None + if value == last["value"]: + return + last["value"] = value + if value is None: + counter.set_view_tally(None, None) + else: + counter.set_view_tally(*value) + + timer = QTimer(parent) + timer.setInterval(POLL_MS) + timer.timeout.connect(tick) + timer.start() + return timer diff --git a/src/ndi/pyramid/loader.py b/src/ndi/pyramid/loader.py index 6fd9b734..4cbce6a3 100644 --- a/src/ndi/pyramid/loader.py +++ b/src/ndi/pyramid/loader.py @@ -167,6 +167,19 @@ def fetcher(self): self.build() return self._fetcher + @property + def levelMeta(self) -> list[dict]: + """Per-level chunk geometry, finest first. + + Each entry has ``doc``, ``axes`` (the level's ``axes_order``, + lower case), ``chunks``, ``chunk_grid`` and ``stored`` (the set + of ``chunk.bin_#`` names the level holds, or None when the + document has no file list). The viewer uses it to count the + chunks the current view needs. + """ + self.build() + return list(getattr(self._fetcher, "_level_meta", None) or []) + @property def docs(self) -> list: """The level documents, sorted finest-first. diff --git a/src/ndi/pyramid/multiscale.py b/src/ndi/pyramid/multiscale.py index ffcde36c..a1184637 100644 --- a/src/ndi/pyramid/multiscale.py +++ b/src/ndi/pyramid/multiscale.py @@ -554,6 +554,12 @@ def levelArrays( fetcher._fallback_context = fallback_context # type: ignore[attr-defined] arrays: list[Any] = [] + # Per-level chunk geometry, finest first (same order as ARRAYS). + # The viewer reads this to work out which chunks the current view + # needs, so it can say "37 of 120 tiles" rather than only how many + # are in flight. Rides on the fetcher like the fallback context. + level_meta: list[dict] = [] + fetcher._level_meta = level_meta # type: ignore[attr-defined] for i, doc in enumerate(docs): p = doc.document_properties["lightsheetZarrLevel"] shape = tuple(int(v) for v in p["shape"]) @@ -565,6 +571,16 @@ def levelArrays( stored = _storedChunkNames(doc) _assertStoredMatchesLevel(doc, p, stored) + level_meta.append( + { + "doc": doc, + "axes": str(p.get("axes_order", "")).lower(), + "chunks": chunks, + "chunk_grid": chunk_grid, + "stored": stored, + } + ) + n_blocks = 1 for g in chunk_grid: n_blocks *= g diff --git a/tests/test_gui_genepyramid_progress.py b/tests/test_gui_genepyramid_progress.py index e19a8022..c7b72720 100644 --- a/tests/test_gui_genepyramid_progress.py +++ b/tests/test_gui_genepyramid_progress.py @@ -236,6 +236,11 @@ def test_no_window_and_no_terminal_is_off_not_a_crash(self): class TestClosingIsAlwaysSafe(unittest.TestCase): + def setUp(self): + # Closing latches _window_closed; put it back so later tests can + # still build a window. + self.addCleanup(setattr, progress, "_window_closed", progress._window_closed) + def test_closing_when_there_never_was_a_window(self): self.addCleanup(setattr, progress, "_window", progress._window) progress._window = None diff --git a/tests/test_gui_lightsheetZarr_viewtiles.py b/tests/test_gui_lightsheetZarr_viewtiles.py new file mode 100644 index 00000000..fe4873c3 --- /dev/null +++ b/tests/test_gui_lightsheetZarr_viewtiles.py @@ -0,0 +1,220 @@ +"""The viewer says how many tiles the current view needs, not only how +many are in flight; and the launch window, once closed, stays closed. + +Before this, zooming into a new region showed "12 in flight" with no way +to tell whether that was most of the view or a sliver of it. The chunk +grid and the stored-chunk list on the level documents make the total +knowable: napari draws one level over one region at one slice. +""" + +import io +import types + +import numpy as np +import pytest + +from ndi.gui.app.lightsheetZarr import progress, viewtiles +from ndi.gui.app.lightsheetZarr.viewer import _FetchCounter + + +def _meta(chunks=(1, 1, 4, 10, 10), grid=(1, 2, 3, 5, 5), stored=None, axes="tczyx"): + return { + "doc": types.SimpleNamespace(id="lvl"), + "axes": axes, + "chunks": chunks, + "chunk_grid": grid, + "stored": stored, + } + + +# ------------------------------------------------------------ chunksInBox + + +class TestChunksInBox: + def test_the_box_covers_the_chunks_it_touches(self): + m = _meta(chunks=(10, 10), grid=(5, 5), axes="yx") + names = viewtiles.chunksInBox(m, [5, 15], [12, 29]) + # rows 0..1, cols 1..2 -> linear 1-based (r*5 + c) + 1 + assert sorted(names) == sorted(["chunk.bin_2", "chunk.bin_3", "chunk.bin_7", "chunk.bin_8"]) + + def test_sparse_chunks_need_no_fetch_and_are_not_counted(self): + m = _meta(chunks=(10, 10), grid=(5, 5), axes="yx", stored={"chunk.bin_2"}) + assert viewtiles.chunksInBox(m, [0, 0], [19, 29]) == ["chunk.bin_2"] + + def test_a_box_past_the_edge_is_clipped(self): + m = _meta(chunks=(10, 10), grid=(2, 2), axes="yx") + assert len(viewtiles.chunksInBox(m, [-5, -5], [999, 999])) == 4 + + def test_a_huge_box_is_not_tallied(self, monkeypatch): + monkeypatch.setattr(viewtiles, "MAX_POSITIONS", 3) + m = _meta(chunks=(10, 10), grid=(2, 2), axes="yx") + assert viewtiles.chunksInBox(m, [0, 0], [19, 19]) is None + + def test_linear_index_matches_the_reader(self): + from ndi.pyramid.multiscale import _linear_chunk_index + + m = _meta() + names = viewtiles.chunksInBox(m, [0, 1, 8, 30, 40], [0, 1, 8, 30, 40]) + assert names == [f"chunk.bin_{_linear_chunk_index((0, 1, 2, 3, 4), m['chunk_grid'])}"] + + +# ------------------------------------------------------------ layerBox + + +def _fakeViewer(point, displayed=(1, 2), ndim=3): + dims = types.SimpleNamespace(point=point, displayed=displayed, ndim=ndim) + return types.SimpleNamespace(dims=dims) + + +def _fakeLayer(level, corners, n_levels=2, factors=None, visible=True): + factors = factors or [(1, 1, 1), (2, 2, 2)] + return types.SimpleNamespace( + multiscale=True, + data=[None] * n_levels, + data_level=level, + corner_pixels=np.array(corners), + ndim=3, + visible=visible, + downsample_factors=np.array(factors, dtype=float), + world_to_data=lambda p: list(p), + ) + + +class TestLayerBox: + def test_displayed_axes_come_from_corner_pixels_and_z_from_the_slice(self): + metas = [_meta(), _meta()] + layer = _fakeLayer(1, [[0, 3, 4], [0, 25, 31]]) + viewer = _fakeViewer(point=(10.0, 0.0, 0.0)) + level, lo, hi = viewtiles.layerBox(layer, viewer, metas, channel=1) + assert level == 1 + # t, c, z, y, x on the grid; z is world 10 / factor 2 = 5. + assert lo == [0, 1, 5, 3, 4] + assert hi == [0, 1, 5, 25, 31] + + def test_a_tzyx_layer_maps_onto_a_tczyx_grid(self): + layer = _fakeLayer(0, [[0, 0, 2, 3], [0, 0, 8, 9]], n_levels=1, factors=[(1, 1, 1, 1)]) + layer.ndim = 4 + viewer = _fakeViewer(point=(0.0, 6.0, 0.0, 0.0), displayed=(2, 3), ndim=4) + _level, lo, hi = viewtiles.layerBox(layer, viewer, [_meta()], channel=2) + assert lo == [0, 2, 6, 2, 3] + assert hi == [0, 2, 6, 8, 9] + + def test_a_single_level_layer_is_not_tallied(self): + layer = _fakeLayer(0, [[0, 0, 0], [0, 1, 1]]) + layer.multiscale = False + assert viewtiles.layerBox(layer, _fakeViewer((0, 0, 0)), [_meta()]) is None + + def test_a_ladder_that_does_not_match_the_docs_is_not_tallied(self): + layer = _fakeLayer(0, [[0, 0, 0], [0, 1, 1]], n_levels=3) + assert viewtiles.layerBox(layer, _fakeViewer((0, 0, 0)), [_meta(), _meta()]) is None + + def test_napari_without_the_attributes_is_unknown_not_a_crash(self): + layer = types.SimpleNamespace(multiscale=True, data=[None]) + assert viewtiles.layerBox(layer, _fakeViewer((0, 0, 0)), [_meta()]) is None + + +class TestViewTally: + def test_ready_counts_what_is_on_disk_and_shared_chunks_count_once(self): + # Channel chunk spans both channels -> two layers, same chunks. + m = _meta(chunks=(1, 2, 4, 10, 10), grid=(1, 1, 3, 5, 5)) + layers = [_fakeLayer(0, [[0, 0, 0], [0, 19, 9]], n_levels=1) for _ in range(2)] + viewer = _fakeViewer(point=(0.0, 0.0, 0.0)) + + class Fetcher: + def chunkPathIfCached(self, doc, name): + return "/x" if name == "chunk.bin_1" else None + + assert viewtiles.viewTally(viewer, layers, [m], Fetcher()) == (1, 2) + + def test_hidden_layers_need_nothing(self): + m = _meta(chunks=(10, 10, 10), grid=(1, 2, 2), axes="zyx") + layer = _fakeLayer(0, [[0, 0, 0], [0, 19, 19]], n_levels=1, visible=False) + + class Fetcher: + def chunkPathIfCached(self, doc, name): + return None + + assert viewtiles.viewTally(_fakeViewer((0, 0, 0)), [layer], [m], Fetcher()) == (0, 0) + + +# ------------------------------------------------------------ counter + + +class _Reporter: + def __init__(self): + self.pushed = [] + + def push(self, message, *, overlay=""): + self.pushed.append((message, overlay)) + + +class TestCounterReportsTheView: + def _counter(self): + out = io.StringIO() + rep = _Reporter() + c = _FetchCounter(out=out, idle_interval=3600, status_reporter=rep) + return c, out, rep + + def test_status_and_overlay_say_how_much_of_the_view_is_left(self): + c, _out, rep = self._counter() + try: + c("start", "u1", 0, None) + c.set_view_tally(37, 120) + message, overlay = rep.pushed[-1] + assert "view 37/120 tiles ready" in message + assert overlay == "Loading view: 37 / 120 tiles (1 in flight)" + finally: + c.stop() + + def test_a_complete_view_hides_the_overlay_and_logs_how_long_it_took(self): + c, out, rep = self._counter() + try: + c.set_view_tally(0, 4) + c.set_view_tally(4, 4) + assert rep.pushed[-1][1] == "" + assert "view complete: 4 tile(s)" in out.getvalue() + finally: + c.stop() + + def test_an_untallied_view_falls_back_to_in_flight(self): + c, _out, rep = self._counter() + try: + c("start", "u1", 0, None) + c.set_view_tally(None, None) + assert rep.pushed[-1][1] == "Loading tiles ... (1 in flight)" + finally: + c.stop() + + +# ------------------------------------------------------------ launch window + + +class TestTheLaunchWindowStaysClosed: + def test_a_note_after_closing_does_not_build_a_new_window(self, monkeypatch): + """closeLaunchWindow then note("viewer running ...") used to go + through _mode() -> _launchWindow(), find no window, and open a + fresh "starting ..." window that nothing ever closed.""" + monkeypatch.setattr(progress, "_window", None) + monkeypatch.setattr(progress, "_window_failed", False) + monkeypatch.setattr(progress, "_window_closed", False) + monkeypatch.delenv("NDI_LIGHTSHEET_PROGRESS", raising=False) + monkeypatch.delenv("NDI_LIGHTSHEET_QUIET", raising=False) + + class Widget: + def __getattr__(self, _name): + return lambda *a, **k: None + + monkeypatch.setattr( + progress, "_window", {"widget": Widget(), "app": Widget(), "step": None, "bar": None} + ) + progress.closeLaunchWindow() + progress.note("viewer running") + + assert progress._launchWindow() is None + # It short-circuited on the close latch; it never got as far as + # trying (and failing) to build a window. + assert progress._window_failed is False + + +if __name__ == "__main__": + pytest.main([__file__]) From a580ac0f33b85a27162dcc80a8242c2eec931c67 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 20:38:44 +0000 Subject: [PATCH 02/21] lightsheet: say why the view tile tally is off instead of falling back silently When the tally could not read napari's state it quietly reverted to the in-flight-only status, which looks identical to running without the feature. The poll now logs "view tile tally: on (...)" at start and "view tile tally: cannot count this view -- " whenever the reason changes (level/doc count mismatch, unreadable layer state, axis mapping, view too large); openPyramid logs when single-level or viewport-clip mode turns it off. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/gui/app/lightsheetZarr/viewer.py | 6 +++ src/ndi/gui/app/lightsheetZarr/viewtiles.py | 52 ++++++++++++++++----- tests/test_gui_lightsheetZarr_viewtiles.py | 8 ++++ 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index 8d430edc..526e83f9 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -834,6 +834,12 @@ def openPyramid( file=sys.stderr, flush=True, ) + else: + print( + "[lightsheet] view tile tally: off in single-level / viewport-clip mode", + file=sys.stderr, + flush=True, + ) # Launch window is closed BEFORE napari.run() because napari's # event loop blocks the main thread and our Qt window can't diff --git a/src/ndi/gui/app/lightsheetZarr/viewtiles.py b/src/ndi/gui/app/lightsheetZarr/viewtiles.py index 506fa99f..b20cc6db 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewtiles.py +++ b/src/ndi/gui/app/lightsheetZarr/viewtiles.py @@ -37,6 +37,18 @@ #: Poll interval for the main-thread timer, in milliseconds. POLL_MS = 400 +#: Why the last :func:`viewTally` returned None. A silent fallback to +#: the in-flight count looks exactly like a viewer without this +#: feature, so the poll logs this whenever it changes. +lastReason = "" + + +def _why(reason: str): + """Record why the view cannot be tallied; return None.""" + global lastReason + lastReason = reason + return None + def chunksInBox(meta: dict, lo: list[int], hi: list[int]) -> list[str] | None: """Stored chunk names covering the inclusive data box [LO, HI]. @@ -115,10 +127,13 @@ def layerBox(layer, viewer, meta_levels: list[dict], channel: int = 0): # The level index only lines up with META_LEVELS on a native # multiscale layer over the full ladder (not single-level mode). if not getattr(layer, "multiscale", False): - return None + return _why(f"layer {getattr(layer, 'name', '?')!r} is not multiscale") try: if len(layer.data) != len(meta_levels): - return None + return _why( + f"layer has {len(layer.data)} level(s) but the pyramid has " + f"{len(meta_levels)} level document(s)" + ) level = int(layer.data_level) meta = meta_levels[level] corners = layer.corner_pixels @@ -129,12 +144,15 @@ def layerBox(layer, viewer, meta_levels: list[dict], channel: int = 0): point = list(viewer.dims.point)[-ndim:] data_pt = list(layer.world_to_data(point)) factors = layer.downsample_factors[level] - except Exception: # noqa: BLE001 - any napari shape surprise -> unknown - return None + except Exception as exc: # noqa: BLE001 - any napari shape surprise -> unknown + return _why(f"could not read napari layer state ({type(exc).__name__}: {exc})") mapping = _gridAxisMap(meta, ndim) if mapping is None: - return None + return _why( + f"cannot map a {ndim}-D layer onto a {len(meta['chunk_grid'])}-D chunk " + f"grid (axes_order={meta.get('axes')!r})" + ) layer_to_grid, c_axis = mapping n = len(meta["chunk_grid"]) @@ -161,7 +179,7 @@ def viewTally(viewer, layers, meta_levels: list[dict], fetcher) -> tuple[int, in Invisible layers draw nothing and are skipped. """ if not meta_levels: - return None + return _why("no level chunk metadata from the loader") wanted: dict[tuple, Any] = {} for c, layer in enumerate(layers): if not getattr(layer, "visible", True): @@ -172,7 +190,7 @@ def viewTally(viewer, layers, meta_levels: list[dict], fetcher) -> tuple[int, in level, lo, hi = box names = chunksInBox(meta_levels[level], lo, hi) if names is None: - return None + return _why(f"view spans more than {MAX_POSITIONS:,} chunk positions") doc = meta_levels[level]["doc"] for name in names: wanted[(level, name)] = doc @@ -195,9 +213,15 @@ def attachViewTally(viewer, layers, meta_levels: list[dict], fetcher, counter): Returns the QTimer (kept alive by its parent window), or None when Qt or the window is not available. """ + import sys + + def log(msg: str) -> None: + print(f"[lightsheet] view tile tally: {msg}", file=sys.stderr, flush=True) + try: from qtpy.QtCore import QTimer - except Exception: # noqa: BLE001 + except Exception as exc: # noqa: BLE001 + log(f"off -- no Qt timer ({exc})") return None parent = None @@ -207,13 +231,18 @@ def attachViewTally(viewer, layers, meta_levels: list[dict], fetcher, counter): except Exception: # noqa: BLE001 pass - last = {"value": object()} + last = {"value": object(), "reason": ""} def tick() -> None: try: value = viewTally(viewer, layers, meta_levels, fetcher) - except Exception: # noqa: BLE001 - never take the viewer down - value = None + except Exception as exc: # noqa: BLE001 - never take the viewer down + value = _why(f"tally raised {type(exc).__name__}: {exc}") + if value is None and lastReason != last["reason"]: + last["reason"] = lastReason + log(f"cannot count this view -- {lastReason}") + if value is not None: + last["reason"] = "" if value == last["value"]: return last["value"] = value @@ -226,4 +255,5 @@ def tick() -> None: timer.setInterval(POLL_MS) timer.timeout.connect(tick) timer.start() + log(f"on ({len(meta_levels)} level(s), polling every {POLL_MS} ms)") return timer diff --git a/tests/test_gui_lightsheetZarr_viewtiles.py b/tests/test_gui_lightsheetZarr_viewtiles.py index fe4873c3..27e71ba3 100644 --- a/tests/test_gui_lightsheetZarr_viewtiles.py +++ b/tests/test_gui_lightsheetZarr_viewtiles.py @@ -111,6 +111,14 @@ def test_a_ladder_that_does_not_match_the_docs_is_not_tallied(self): def test_napari_without_the_attributes_is_unknown_not_a_crash(self): layer = types.SimpleNamespace(multiscale=True, data=[None]) assert viewtiles.layerBox(layer, _fakeViewer((0, 0, 0)), [_meta()]) is None + assert "could not read napari layer state" in viewtiles.lastReason + + def test_a_level_mismatch_says_so(self): + layer = _fakeLayer(0, [[0, 0, 0], [0, 1, 1]], n_levels=3) + viewtiles.layerBox(layer, _fakeViewer((0, 0, 0)), [_meta(), _meta()]) + assert viewtiles.lastReason == ( + "layer has 3 level(s) but the pyramid has 2 level document(s)" + ) class TestViewTally: From 19bbc4aa343185f33ee3dedef1d0bb9dd378d4ed Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 11 Oct 2026 21:42:33 +0000 Subject: [PATCH 03/21] lightsheet: fetch the newest view first; release reads the user zoomed past napari slices on one thread, so zooming through an intermediate level started a slice that then held up the slice for where the user stopped until every one of its tiles had arrived, and the fetch pool was FIFO on top of that. _ChunkFetcher now runs its own priority queue instead of a ThreadPoolExecutor: newest requests first, and chunks in the current view (handed over by the view-tally poll via setView) ahead of everything -- queued immediately even if napari has not asked yet. A blocking read for a chunk that has left the view is released with StaleChunkRead, so the stale slice ends and napari moves on; the fetch itself stays queued at low priority and lands in the cache for when the user zooms back. Reads are only judged against a view set AFTER they began (the poll lags napari), the coarsest level is never released (auto-contrast samples it), and an unknown view releases nothing. StaleChunkRead is raised rather than answered with fill_value so napari's dask cache never keeps a blank block; the viewer filters it out of napari's error notifications. Measured under real napari with async slicing (1 s simulated fetches, 2 workers, zoom through a 20-tile level to a 4-tile view): final view complete 11.9 s after the last zoom before, 2.7 s after. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/gui/app/lightsheetZarr/viewer.py | 37 +++ src/ndi/gui/app/lightsheetZarr/viewtiles.py | 52 ++- src/ndi/pyramid/multiscale.py | 331 +++++++++++++++----- tests/test_pyramid_fetch_priority.py | 194 ++++++++++++ 4 files changed, 518 insertions(+), 96 deletions(-) create mode 100644 tests/test_pyramid_fetch_priority.py diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index 526e83f9..3a6002cf 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -402,6 +402,37 @@ def stop(self) -> None: ) +def _quiet_stale_chunk_errors() -> None: + """Drop napari error notifications caused by StaleChunkRead. + + Idempotent; any napari that does not route slice errors through + ``notification_manager.receive_error`` is left alone (the release + then shows as a notification, which is noisy but harmless). + """ + try: + from napari.utils.notifications import notification_manager + + from ndi.pyramid.multiscale import StaleChunkRead + except Exception: # noqa: BLE001 + return + original = notification_manager.receive_error + if getattr(original, "_ndi_quiet_stale", False): + return + + def receive_error(exc_type, exc_value, tb, *args, **kwargs): + e = exc_value + seen = 0 + while e is not None and seen < 10: + if isinstance(e, StaleChunkRead): + return None + e = e.__cause__ or e.__context__ + seen += 1 + return original(exc_type, exc_value, tb, *args, **kwargs) + + receive_error._ndi_quiet_stale = True # type: ignore[attr-defined] + notification_manager.receive_error = receive_error + + def require_napari(): """Return the napari module or raise a helpful ImportError.""" try: @@ -584,6 +615,12 @@ def openPyramid( with progress.stage("opening image viewer"): viewer = napari.Viewer() + # A slice for a view the user has zoomed past is released with + # StaleChunkRead (see _ChunkFetcher.setView) so napari's one slicing + # thread can move on to the current view. napari reports a failed + # slice as an error notification; this one is deliberate. + _quiet_stale_chunk_errors() + # Now that a viewer exists, wire the counter to napari's status # bar so users see tile progress inside the viewer window rather # than having to watch the terminal. diff --git a/src/ndi/gui/app/lightsheetZarr/viewtiles.py b/src/ndi/gui/app/lightsheetZarr/viewtiles.py index b20cc6db..e8fc3e08 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewtiles.py +++ b/src/ndi/gui/app/lightsheetZarr/viewtiles.py @@ -27,7 +27,7 @@ from typing import Any -__all__ = ["chunksInBox", "layerBox", "viewTally", "attachViewTally"] +__all__ = ["chunksInBox", "layerBox", "viewChunks", "viewTally", "attachViewTally"] #: A view whose box spans more chunk positions than this is not #: tallied (it would stall the main thread on every tick). napari's 3D @@ -172,11 +172,14 @@ def layerBox(layer, viewer, meta_levels: list[dict], channel: int = 0): return level, lo, hi -def viewTally(viewer, layers, meta_levels: list[dict], fetcher) -> tuple[int, int] | None: - """``(ready, needed)`` stored chunks for the current view, or None. +def viewChunks(viewer, layers, meta_levels: list[dict]) -> dict | None: + """Stored chunks the current view needs: ``{(doc_id, name): doc}``. - Counts each chunk once even when two channel layers share it. - Invisible layers draw nothing and are skipped. + Keyed the way :class:`ndi.pyramid.multiscale._ChunkFetcher` keys its + cache, so the result can be handed to ``fetcher.setView``. Counts + each chunk once even when two channel layers share it. Invisible + layers draw nothing and are skipped. None when the view cannot be + worked out (``lastReason`` says why). """ if not meta_levels: return _why("no level chunk metadata from the loader") @@ -192,17 +195,29 @@ def viewTally(viewer, layers, meta_levels: list[dict], fetcher) -> tuple[int, in if names is None: return _why(f"view spans more than {MAX_POSITIONS:,} chunk positions") doc = meta_levels[level]["doc"] + doc_id = getattr(doc, "id", str(doc)) for name in names: - wanted[(level, name)] = doc + wanted[(doc_id, name)] = doc + return wanted + +def _countReady(wanted: dict, fetcher) -> int: ready = 0 - for (_level, name), doc in wanted.items(): + for (_doc_id, name), doc in wanted.items(): try: if fetcher.chunkPathIfCached(doc, name) is not None: ready += 1 except Exception: # noqa: BLE001 - unreachable scope: not ready pass - return ready, len(wanted) + return ready + + +def viewTally(viewer, layers, meta_levels: list[dict], fetcher) -> tuple[int, int] | None: + """``(ready, needed)`` stored chunks for the current view, or None.""" + wanted = viewChunks(viewer, layers, meta_levels) + if wanted is None: + return None + return _countReady(wanted, fetcher), len(wanted) def attachViewTally(viewer, layers, meta_levels: list[dict], fetcher, counter): @@ -231,13 +246,30 @@ def log(msg: str) -> None: except Exception: # noqa: BLE001 pass - last = {"value": object(), "reason": ""} + last = {"value": object(), "reason": "", "keys": object()} + + # The coarsest level is never released as stale: napari's + # auto-contrast samples it whatever the view is. + coarsest = meta_levels[-1]["doc"] if meta_levels else None + never_stale = [getattr(coarsest, "id", str(coarsest))] if coarsest is not None else [] + set_view = getattr(fetcher, "setView", None) def tick() -> None: try: - value = viewTally(viewer, layers, meta_levels, fetcher) + wanted = viewChunks(viewer, layers, meta_levels) + value = None if wanted is None else (_countReady(wanted, fetcher), len(wanted)) except Exception as exc: # noqa: BLE001 - never take the viewer down + wanted = None value = _why(f"tally raised {type(exc).__name__}: {exc}") + # Hand the view to the fetcher only when it CHANGES: the change + # time is what decides whether a waiting read is stale. + keys = None if wanted is None else frozenset(wanted) + if set_view is not None and keys != last["keys"]: + last["keys"] = keys + try: + set_view(wanted, never_stale_doc_ids=never_stale) + except Exception as exc: # noqa: BLE001 + log(f"could not prioritise the view ({exc})") if value is None and lastReason != last["reason"]: last["reason"] = lastReason log(f"cannot count this view -- {lastReason}") diff --git a/src/ndi/pyramid/multiscale.py b/src/ndi/pyramid/multiscale.py index a1184637..56c0ab0f 100644 --- a/src/ndi/pyramid/multiscale.py +++ b/src/ndi/pyramid/multiscale.py @@ -31,12 +31,49 @@ from __future__ import annotations +import heapq +import itertools import os import threading +import time from typing import Any from ndi.cloud.batch_signed_url import BatchScopeUnreachable + +class StaleChunkRead(Exception): + """A blocking chunk read was released because the view moved on. + + napari computes slices on ONE thread, so a slice for a view the user + has already zoomed past holds up the slice for where they stopped + until every one of its tiles has arrived. When the viewer reports + (:meth:`_ChunkFetcher.setView`) that a waiting chunk is no longer in + view, the read raises this instead of waiting. The fetch itself is + NOT cancelled: it stays queued at low priority and lands in the + local cache, so zooming back costs nothing. + + Raised, not answered with fill_value: napari's opportunistic dask + cache would keep a fill block under the chunk's task key, and the + region would stay blank after the real data arrived. + """ + + +class _Job: + """One queued or running chunk fetch, shared by everyone waiting on it.""" + + __slots__ = ("key", "doc", "filename", "started", "done", "path", "exc", "callbacks") + + def __init__(self, key, doc, filename): + self.key = key + self.doc = doc + self.filename = filename + self.started = False + self.done = threading.Event() + self.path: str | None = None + self.exc: BaseException | None = None + self.callbacks: list = [] + + # --------------------------------------------------------------------------- # depends_on / doc discovery @@ -158,16 +195,33 @@ def __init__(self, session, workers: int = 1): self._reopen = self._reopener(session) self._workers = max(1, int(workers)) self._local = threading.local() - self._pool = None self._lock = threading.Lock() self._paths: dict[tuple[str, str], str] = {} - # Async prefetch bookkeeping. When the upsample-fallback reader - # kicks off a background fetch, this maps the chunk key to the - # list of on_complete callbacks that fire when the pool worker - # finishes. Sharing the map de-duplicates: the same chunk - # requested twice enqueues one task and both callers are - # notified when it lands. - self._in_flight: dict[tuple[str, str], list] = {} + # Work queue. A heap of (tier, -seq, job): tier 0 is a chunk in + # the current view (see setView), tier 1 everything else, and + # within a tier the NEWEST request goes first -- the user is + # looking at what they asked for last, not at the levels they + # zoomed through on the way. One _Job per chunk key, shared by + # every waiter and callback, so a chunk is never fetched twice. + # A re-request pushes a fresh entry; stale heap entries for a + # started job are skipped on pop. + self._cv = threading.Condition() + self._heap: list = [] + self._jobs: dict[tuple[str, str], _Job] = {} + self._seq = itertools.count() + self._threads: list[threading.Thread] = [] + self._threads_ready = 0 + self._closed = False + # The current view's chunk keys, set from the viewer's main- + # thread poll. None means unknown: nothing is prioritised and + # no read is ever released as stale. + self._view_keys: frozenset | None = None + self._view_time = 0.0 + # Chunks of these docs are never released as stale: napari's + # auto-contrast samples the coarsest level at launch, whatever + # the view is. + self._never_stale_docs: set[str] = set() + self._stale_released = 0 # Per-fetch timing bucketed by outcome. Set by _resolve on every # call, including memo hits, so a caller can tell whether a # slow tile is "cloud is slow" vs "local cache lookup is slow" @@ -202,22 +256,95 @@ def _reopener(session): cls = type(session) return lambda: cls(path) - def _initThread(self): - self._local.session = self._reopen() + def _ensureWorkers(self) -> bool: + """Start the fetcher threads once. False when the session cannot + be reopened for another thread (then everything runs inline).""" + if self._reopen is None: + return False + with self._cv: + if self._threads or self._closed: + return not self._closed + for i in range(self._workers): + t = threading.Thread( + target=self._workerLoop, + name=f"ndi-lightsheet_{i}", + daemon=True, + ) + self._threads.append(t) + t.start() + return True - def _ensurePool(self): - if self._pool is not None or self._reopen is None: - return self._pool - with self._lock: - if self._pool is None: - from concurrent.futures import ThreadPoolExecutor + def _workerLoop(self) -> None: + try: + self._local.session = self._reopen() + finally: + with self._cv: + self._threads_ready += 1 + self._cv.notify_all() + while True: + with self._cv: + job = None + while job is None: + while not self._heap and not self._closed: + self._cv.wait() + if self._closed: + return + tier, nseq, cand = heapq.heappop(self._heap) + if cand.started: + continue + if tier == 0 and (self._view_keys is None or cand.key not in self._view_keys): + # Was in view when queued, is not any more. + heapq.heappush(self._heap, (1, nseq, cand)) + continue + cand.started = True + job = cand + self._run(job) + + def _run(self, job: _Job) -> None: + """Resolve one job on this thread and settle everyone waiting.""" + path = None + try: + path = self._resolve(job.doc, job.filename) + except BatchScopeUnreachable as exc: + # Systemic scope failure. Remember it so the next + # chunkPathIfCached / chunkPath surfaces it instead of + # letting the upsample fallback quietly return a canvas + # full of zeros. + with self._lock: + if self._unreachable_exc is None: + self._unreachable_exc = exc + job.exc = exc + except BaseException as exc: # noqa: BLE001 - handed to the waiter + job.exc = exc + if path is not None: + with self._lock: + self._paths[job.key] = path + with self._cv: + self._jobs.pop(job.key, None) + callbacks, job.callbacks = job.callbacks, [] + job.path = path + job.done.set() + for cb in callbacks: + try: + cb(path) + except Exception: # noqa: BLE001 - a bad callback is not fatal + pass - self._pool = ThreadPoolExecutor( - max_workers=self._workers, - thread_name_prefix="ndi-lightsheet", - initializer=self._initThread, - ) - return self._pool + def _enqueue(self, doc, filename, on_complete=None) -> _Job: + """Queue a fetch (or bump an already-queued one to newest).""" + key = (getattr(doc, "id", str(doc)), filename) + with self._cv: + job = self._jobs.get(key) + if job is None: + job = _Job(key, doc, filename) + self._jobs[key] = job + if on_complete is not None: + job.callbacks.append(on_complete) + if not job.started: + tier = 0 if self._view_keys is not None and key in self._view_keys else 1 + heapq.heappush(self._heap, (tier, -next(self._seq), job)) + self._cv.notify() + return job def warm(self) -> None: """Build the session handles now, off the calling thread. @@ -226,11 +353,58 @@ def warm(self) -> None: one-off session-open cost lands while the user is staring at an empty canvas rather than at the first pan. """ - pool = self._ensurePool() - if pool is None: + if not self._ensureWorkers(): + return + with self._cv: + while self._threads_ready < self._workers and not self._closed: + self._cv.wait() + + def setView(self, keys, never_stale_doc_ids=None) -> None: + """Tell the fetcher which chunks the current view needs. + + KEYS maps ``(doc_id, filename)`` to the level document (None: + the view is unknown). Chunks in view jump the queue -- and are + queued now if nobody has asked yet, so they are already on the + way while napari is still finishing an older slice. A blocking + read for a chunk that has dropped OUT of view is released with + :class:`StaleChunkRead`; its fetch stays queued at low priority. + """ + with self._cv: + if never_stale_doc_ids is not None: + self._never_stale_docs = set(never_stale_doc_ids) + if keys is None: + self._view_keys = None + self._cv.notify_all() + return + self._view_keys = frozenset(keys) + self._view_time = time.monotonic() + # Re-rank what is queued: anything now in view to tier 0. + for key in self._view_keys: + job = self._jobs.get(key) + if job is not None and not job.started: + heapq.heappush(self._heap, (0, -next(self._seq), job)) + self._cv.notify_all() + # Queue what the view needs and nobody has asked for yet. + if not self._threads and not self._ensureWorkers(): return - for f in [pool.submit(lambda: None) for _ in range(self._workers)]: - f.result() + for key, doc in keys.items(): + known = self._paths.get(key) + if known is not None and os.path.exists(known): + continue + self._enqueue(doc, key[1]) + + def _isStale(self, job: _Job, since: float) -> bool: + view = self._view_keys + return ( + view is not None + and self._view_time > since + and job.key not in view + and job.key[0] not in self._never_stale_docs + ) + + def staleReleased(self) -> int: + """How many blocking reads were released as stale so far.""" + return self._stale_released def _resolve(self, doc, filename) -> str | None: s = getattr(self._local, "session", None) or self._session @@ -310,7 +484,8 @@ def stats_summary(self) -> str: return ( f"resolves: {n_cache} cache-hits (mean {mean_cache_ms:.1f}ms), " f"{n_fetch} cloud-fetches (mean {mean_fetch_s:.2f}s, max " - f"{max_fetch_s:.2f}s), {n_none} failures" + f"{max_fetch_s:.2f}s), {n_none} failures, " + f"{self._stale_released} stale reads released" ) def reset_stats(self) -> None: @@ -390,83 +565,54 @@ def prefetchAsync(self, doc, filename, on_complete=None) -> None: The upsample-fallback reader returns coarse-upsampled data immediately for a chunk that is not on disk; this asks the - pool to go fetch the real thing so a later re-slice can - find it via :meth:`chunkPathIfCached`. On completion (or + fetcher threads to go fetch the real thing so a later re-slice + can find it via :meth:`chunkPathIfCached`. On completion (or failure), ``on_complete`` is called with the resolved path or None -- callers use that to trigger a napari refresh so the user is not stuck looking at blurred data forever. De-duplicates by memo key: the same (doc, filename) already - in flight or already cached does not enqueue a second task. + queued or already cached does not fetch a second time. Errors are absorbed; the reader will retry on the next re-slice and eventually get real data or a permanent fallback. """ key = (getattr(doc, "id", str(doc)), filename) - with self._lock: - if key in self._paths and os.path.exists(self._paths[key]): - # Already cached -- no need to enqueue. Fire the - # completion callback synchronously so the caller - # can react uniformly whether the answer was cheap - # or expensive. - if on_complete is not None: - try: - on_complete(self._paths[key]) - except Exception: # noqa: BLE001 - pass - return - if key in self._in_flight: - if on_complete is not None: - self._in_flight[key].append(on_complete) - return - self._in_flight[key] = [on_complete] if on_complete is not None else [] - - pool = self._ensurePool() - if pool is None: - # No pool means this session cannot be reopened, and - # the fetcher is single-threaded. Fall back to a - # blocking resolve on the caller's thread -- it is the - # rare directory-backed case and does not need async. - path = self._resolve(doc, filename) - self._settle_in_flight(key, path) + known = self._paths.get(key) + if known is not None and os.path.exists(known): + # Already cached -- fire the completion callback + # synchronously so the caller can react uniformly whether + # the answer was cheap or expensive. + if on_complete is not None: + try: + on_complete(known) + except Exception: # noqa: BLE001 + pass return - - def _run(): + if not self._ensureWorkers(): + # No workers means this session cannot be reopened, and + # the fetcher is single-threaded. Fall back to a blocking + # resolve on the caller's thread -- it is the rare + # directory-backed case and does not need async. try: path = self._resolve(doc, filename) - except BatchScopeUnreachable as exc: - # Systemic scope failure. Remember it so the next - # chunkPathIfCached / chunkPath surfaces it instead of - # letting the upsample fallback quietly return a - # canvas full of zeros. - with self._lock: - if self._unreachable_exc is None: - self._unreachable_exc = exc - path = None except Exception: # noqa: BLE001 path = None if path is not None: with self._lock: self._paths[key] = path - self._settle_in_flight(key, path) - - pool.submit(_run) - - def _settle_in_flight(self, key, path) -> None: - """Drain and call every pending completion callback for one key.""" - with self._lock: - callbacks = self._in_flight.pop(key, []) - for cb in callbacks: - try: - cb(path) - except Exception: # noqa: BLE001 - a bad callback is not fatal - pass + if on_complete is not None: + try: + on_complete(path) + except Exception: # noqa: BLE001 + pass + return + self._enqueue(doc, filename, on_complete=on_complete) def _fetch(self, doc, filename) -> str | None: if threading.get_ident() == self._owner: return self._resolve(doc, filename) - pool = self._ensurePool() - if pool is None: + if not self._ensureWorkers(): raise RuntimeError( f"Cannot fetch {filename!r} from thread " f"{threading.current_thread().name}: this session cannot be " @@ -475,12 +621,25 @@ def _fetch(self, doc, filename) -> str | None: f"with dask's synchronous scheduler, or pass a session that " f"exposes a path." ) - return pool.submit(self._resolve, doc, filename).result() + since = time.monotonic() + job = self._enqueue(doc, filename) + # Wait, but keep checking whether the view has moved on: a + # slice for a view the user zoomed past must not hold up the + # slice for where they stopped (napari slices on one thread). + while not job.done.wait(0.1): + if self._isStale(job, since): + with self._lock: + self._stale_released += 1 + raise StaleChunkRead(f"{filename} left the view before it arrived") + if job.exc is not None: + raise job.exc + return job.path def close(self) -> None: - if self._pool is not None: - self._pool.shutdown(wait=False) - self._pool = None + with self._cv: + self._closed = True + self._heap.clear() + self._cv.notify_all() def _default_workers() -> int: diff --git a/tests/test_pyramid_fetch_priority.py b/tests/test_pyramid_fetch_priority.py new file mode 100644 index 00000000..b83c6f1d --- /dev/null +++ b/tests/test_pyramid_fetch_priority.py @@ -0,0 +1,194 @@ +"""The chunk fetcher serves the view the user is looking at first. + +napari slices on one thread, so a slice for a level the user zoomed +through held up the slice for where they stopped until every one of its +tiles had arrived, and the fetch queue was first-in-first-out on top of +that. Now: newest requests first, chunks in the current view ahead of +everything, and a blocking read for a chunk that has left the view is +released (StaleChunkRead) while its fetch carries on in the background. +""" + +import threading +import time +import types + +import pytest + +from ndi.pyramid.multiscale import StaleChunkRead, _ChunkFetcher + +DOC = types.SimpleNamespace(id="lvl0") +COARSE = types.SimpleNamespace(id="lvl3") + + +class _Session: + """Reopenable fake session. Fetches block until released, and the + order in which they start is recorded.""" + + started: list = [] + gate = threading.Event() + + def __init__(self, path): + self.path = path + + def database_openbinarydoc(self, doc, filename): + _Session.started.append(filename) + assert _Session.gate.wait(10) + return types.SimpleNamespace(fullpathfilename=__file__) + + def database_closebinarydoc(self, fh): + pass + + +@pytest.fixture +def fetcher(): + _Session.started = [] + _Session.gate = threading.Event() + f = _ChunkFetcher(_Session("/x"), workers=1) + f.warm() + yield f + _Session.gate.set() + f.close() + + +def _occupy(f): + """Tie up the single worker so later requests queue behind it.""" + f.prefetchAsync(DOC, "busy") + for _ in range(100): + if _Session.started: + return + time.sleep(0.01) + + +def _drain(f, n): + _Session.gate.set() + for _ in range(300): + if len(_Session.started) >= n: + return + time.sleep(0.01) + + +def test_newest_request_goes_first(fetcher): + _occupy(fetcher) + for name in ("a", "b", "c"): + fetcher.prefetchAsync(DOC, name) + _drain(fetcher, 4) + assert _Session.started == ["busy", "c", "b", "a"] + + +def test_the_current_view_jumps_the_queue_and_is_queued_unasked(fetcher): + _occupy(fetcher) + fetcher.prefetchAsync(DOC, "old") + fetcher.setView({("lvl0", "v1"): DOC, ("lvl0", "v2"): DOC}) + fetcher.prefetchAsync(DOC, "newer-but-not-in-view") + _drain(fetcher, 5) + assert set(_Session.started[1:3]) == {"v1", "v2"} + assert _Session.started[3:] == ["newer-but-not-in-view", "old"] + + +def test_a_chunk_that_leaves_the_view_drops_back(fetcher): + _occupy(fetcher) + fetcher.setView({("lvl0", "was-in-view"): DOC}) + fetcher.prefetchAsync(DOC, "other") + fetcher.setView({("lvl0", "now-in-view"): DOC}) + _drain(fetcher, 4) + assert _Session.started[1] == "now-in-view" + + +def test_a_waiting_read_is_released_when_its_chunk_leaves_the_view(fetcher): + _occupy(fetcher) + out = {} + + def read(): + try: + out["path"] = fetcher.chunkPath(DOC, "zoomed-past") + except StaleChunkRead as exc: + out["stale"] = exc + + t = threading.Thread(target=read) + t.start() + time.sleep(0.15) + fetcher.setView({("lvl0", "elsewhere"): DOC}) + t.join(2) + assert not t.is_alive(), "the read must not wait for a tile nobody is looking at" + assert "stale" in out + assert fetcher.staleReleased() == 1 + + # ...and the fetch still lands, slowly, for when they zoom back. + _drain(fetcher, 3) + for _ in range(200): + if fetcher.chunkPathIfCached(DOC, "zoomed-past"): + break + time.sleep(0.01) + assert fetcher.chunkPathIfCached(DOC, "zoomed-past") is not None + + +def test_a_read_in_view_is_never_released(fetcher): + _occupy(fetcher) + fetcher.setView({("lvl0", "wanted"): DOC}) + result = {} + t = threading.Thread(target=lambda: result.update(p=fetcher.chunkPath(DOC, "wanted"))) + t.start() + time.sleep(0.2) + fetcher.setView({("lvl0", "wanted"): DOC, ("lvl0", "more"): DOC}) + _drain(fetcher, 2) + t.join(2) + assert result.get("p") == __file__ + + +def test_a_read_that_started_after_the_view_was_set_is_not_judged_by_it(fetcher): + """The poll lags napari: a read for the NEW view can start before + the poll has seen the new view. It must not be released on the old + one's say-so.""" + _occupy(fetcher) + fetcher.setView({("lvl0", "old-view"): DOC}) + result = {} + t = threading.Thread(target=lambda: result.update(p=fetcher.chunkPath(DOC, "new-view"))) + t.start() + time.sleep(0.3) + assert t.is_alive() + _drain(fetcher, 2) + t.join(2) + assert result.get("p") == __file__ + + +def test_the_coarsest_level_is_never_released(fetcher): + _occupy(fetcher) + fetcher.setView({}, never_stale_doc_ids=["lvl3"]) + result = {} + t = threading.Thread(target=lambda: result.update(p=fetcher.chunkPath(COARSE, "c"))) + t.start() + time.sleep(0.15) + fetcher.setView({("lvl0", "x"): DOC}) + time.sleep(0.3) + assert t.is_alive() + _drain(fetcher, 3) + t.join(2) + assert result.get("p") == __file__ + + +def test_an_unknown_view_releases_nothing(fetcher): + _occupy(fetcher) + fetcher.setView(None) + t = threading.Thread(target=lambda: fetcher.chunkPath(DOC, "q")) + t.start() + time.sleep(0.3) + assert t.is_alive() + _drain(fetcher, 2) + t.join(2) + + +def test_one_chunk_is_fetched_once_however_many_ask(fetcher): + _occupy(fetcher) + done = [] + threads = [ + threading.Thread(target=lambda: done.append(fetcher.chunkPath(DOC, "shared"))) + for _ in range(3) + ] + for t in threads: + t.start() + fetcher.prefetchAsync(DOC, "shared") + _drain(fetcher, 2) + for t in threads: + t.join(2) + assert _Session.started.count("shared") == 1 + assert done == [__file__] * 3 From 0c2dc8f88936bb1a1fe0e50d304117d5008b2819 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:14:02 +0000 Subject: [PATCH 04/21] Add ndi.cloud.api.feedback (submitFeedback, deleteFeedback) Port of NDI-matlab#1023 (75f937a76): wrappers for the in-app problem report endpoints from ndi-cloud-node#158. - submitFeedback(app, category, description, *, appVersion, datasetId, documentId, environment, state, log, traceback) POSTs /feedback with schemaVersion 1. Optional fields keep MATLAB's camelCase names because they are the shared payload keys; None omits a field, as MATLAB's missing / struct.empty do. Returns an APIResponse whose data is the stored report id. - deleteFeedback(feedbackID) DELETEs /feedback/{feedbackId} (204). - FeedbackCategory validator in _validators.py (bug/performance/other). - Bridge entries for both; url bumped to 75f937a76 for the two new routes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- src/ndi/cloud/api/__init__.py | 5 +- src/ndi/cloud/api/_validators.py | 9 +- src/ndi/cloud/api/feedback.py | 154 ++++++++++++++++++ .../cloud/api/ndi_matlab_python_bridge.yaml | 102 +++++++++++- 4 files changed, 265 insertions(+), 5 deletions(-) create mode 100644 src/ndi/cloud/api/feedback.py diff --git a/src/ndi/cloud/api/__init__.py b/src/ndi/cloud/api/__init__.py index 7ec3d946..ddd929ab 100644 --- a/src/ndi/cloud/api/__init__.py +++ b/src/ndi/cloud/api/__init__.py @@ -7,8 +7,9 @@ files — Presigned URL retrieval, file upload users — User creation, profile compute — Compute session management + feedback — In-app problem reports """ -from . import compute, datasets, documents, files, users +from . import compute, datasets, documents, feedback, files, users -__all__ = ["datasets", "documents", "files", "users", "compute"] +__all__ = ["datasets", "documents", "files", "users", "compute", "feedback"] diff --git a/src/ndi/cloud/api/_validators.py b/src/ndi/cloud/api/_validators.py index 01cdaf18..8a8d4ba3 100644 --- a/src/ndi/cloud/api/_validators.py +++ b/src/ndi/cloud/api/_validators.py @@ -9,6 +9,7 @@ PageSize -> (1,1) double (integer >= 1) Scope -> {iMustBeValidScope} (keyword, or dataset-id list) FilePath -> {mustBeFile} (file must exist on disk) + FeedbackCategory -> {mustBeMember(category, ["bug", "performance", "other"])} Usage:: @@ -25,7 +26,7 @@ def get_dataset(dataset_id: CloudId, *, client: CloudClient | None = None): import re from pathlib import Path -from typing import Annotated +from typing import Annotated, Literal from urllib.parse import urlparse from pydantic import AfterValidator, ConfigDict, Field @@ -82,6 +83,12 @@ def _check_file_exists(v: str) -> str: FilePath = Annotated[str, AfterValidator(_check_file_exists)] +# -- {mustBeMember}: feedback report category -------------------------------- +#: The categories POST /feedback accepts (ndi-cloud-node FEEDBACK_CATEGORIES). +FEEDBACK_CATEGORIES = ("bug", "performance", "other") + +FeedbackCategory = Literal["bug", "performance", "other"] + # -- Shared validate_call config --------------------------------------------- VALIDATE_CONFIG = ConfigDict(arbitrary_types_allowed=True) diff --git a/src/ndi/cloud/api/feedback.py b/src/ndi/cloud/api/feedback.py new file mode 100644 index 00000000..28f4d425 --- /dev/null +++ b/src/ndi/cloud/api/feedback.py @@ -0,0 +1,154 @@ +""" +ndi.cloud.api.feedback - In-app problem reports. + +All functions accept an optional ``client`` keyword argument. When omitted, +a client is created automatically from environment variables. + +The request body is a contract shared with NDI-matlab and the server +(ndi-cloud-node#158): field names, and which fields are left out when not +supplied, follow ``ndi.cloud.api.feedback.submitFeedback`` exactly. That is +why the optional arguments keep their camelCase payload names here rather +than the snake_case the sibling modules use for arguments. + +MATLAB equivalents: +ndi/+cloud/+api/+feedback/*.m, + +implementation/+feedback/*.m +""" + +from __future__ import annotations + +from typing import Annotated, Any + +from pydantic import SkipValidation, validate_call + +from ..client import APIResponse, CloudClient, _auto_client +from ._validators import VALIDATE_CONFIG, CloudId, FeedbackCategory, NonEmptyStr + +_Client = Annotated[CloudClient | None, SkipValidation()] + +#: The report schema this client writes. The server refuses any other. +FEEDBACK_SCHEMA_VERSION = 1 + + +@_auto_client +@validate_call(config=VALIDATE_CONFIG) +def submitFeedback( + app: NonEmptyStr, + category: FeedbackCategory, + description: NonEmptyStr, + *, + appVersion: str | None = None, + datasetId: str | None = None, + documentId: str | None = None, + environment: dict[str, Any] | None = None, + state: dict[str, Any] | None = None, + log: str | None = None, + traceback: str | None = None, + client: _Client = None, +) -> APIResponse: + """POST /feedback -- Submit an in-app problem report to NDI Cloud. + + The reporter is the logged-in user. The server redacts anything that + looks like a bearer token or a signed S3 URL credential before storing + the report, but callers should still scrub logs before sending. + + Args: + app: Name of the client sending the report + (e.g. ``"lightsheetZarr-viewer"``). + category: One of ``"bug"``, ``"performance"`` or ``"other"``. + description: The user's free-text description of the problem. + appVersion: Version of the client (e.g. ``"NDI-python 2.0"``). + datasetId: Cloud dataset id the report concerns. + documentId: Document id the report concerns. + environment: Dict describing the client environment (os, versions, ...). + state: Dict with client state useful for diagnosis. + log: Recent log lines. + traceback: Error stack, if any. + client: Authenticated cloud client. + + An optional field left at ``None`` is omitted from the request, as + MATLAB omits a ``missing`` string or an empty struct. An empty string + or an empty dict is sent, as MATLAB sends ``""`` and ``struct()``. + + Server limits: 1 MB per request; description 20,000 characters, log + 500,000, traceback 50,000; environment 20 kB and state 200 kB as JSON; + 10 reports per user per hour. Exceeding a size cap raises + :class:`~ndi.cloud.exceptions.CloudAPIError` with ``status_code`` 413; + the hourly limit, 429. + + Returns: + :class:`~ndi.cloud.client.APIResponse` whose ``data`` is the id of + the stored report (use it with :func:`deleteFeedback`), so that + ``b, feedbackID, status, url = submitFeedback(...)`` mirrors + MATLAB's ``[b, feedbackID, apiResponse, apiURL]``. + """ + body: dict[str, Any] = { + "schemaVersion": FEEDBACK_SCHEMA_VERSION, + "app": app, + "category": category, + "description": description, + } + optional = { + "appVersion": appVersion, + "datasetId": datasetId, + "documentId": documentId, + "log": log, + "traceback": traceback, + "environment": environment, + "state": state, + } + body.update({name: value for name, value in optional.items() if value is not None}) + return _submit(body, client=client) + + +def _submit(body: dict[str, Any], *, client: CloudClient) -> APIResponse: + """POST BODY to /feedback as-is and return the stored report's id. + + MATLAB equivalent: ``ndi.cloud.api.implementation.feedback.SubmitFeedback``, + which likewise sends whatever struct it is given, so tests can reach + the server's own validation past the wrapper's. + """ + result = client.post("/feedback", json=body) + data = result.data if hasattr(result, "data") else result + feedback_id = data.get("id", "") if isinstance(data, dict) else "" + return APIResponse( + feedback_id, + success=True, + status_code=getattr(result, "status_code", 201), + url=getattr(result, "url", ""), + headers=getattr(result, "headers", None), + reason=getattr(result, "reason", ""), + elapsed=getattr(result, "elapsed", None), + http_response=getattr(result, "http_response", None), + ) + + +@_auto_client +@validate_call(config=VALIDATE_CONFIG) +def deleteFeedback(feedbackID: CloudId, *, client: _Client = None) -> APIResponse: + """DELETE /feedback/{feedbackId} -- Withdraw one of your own reports. + + Only the user who submitted a report can delete it. A report that does + not exist, or that belongs to another user, raises + :class:`~ndi.cloud.exceptions.CloudNotFoundError` (HTTP 404); an id + that is not a well-formed ObjectId raises + :class:`~ndi.cloud.exceptions.CloudAPIError` with ``status_code`` 400. + + Args: + feedbackID: The id returned by :func:`submitFeedback`. + client: Authenticated cloud client. + + Returns: + :class:`~ndi.cloud.client.APIResponse` (HTTP 204) whose ``data`` is + MATLAB's confirmation message, ``"Feedback deleted successfully."``. + """ + result = client.delete("/feedback/{feedbackId}", feedbackId=feedbackID) + return APIResponse( + "Feedback deleted successfully.", + success=True, + status_code=getattr(result, "status_code", 204), + url=getattr(result, "url", ""), + headers=getattr(result, "headers", None), + reason=getattr(result, "reason", ""), + elapsed=getattr(result, "elapsed", None), + http_response=getattr(result, "http_response", None), + ) diff --git a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml index 0f991cea..367b1ea0 100644 --- a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml @@ -1,7 +1,8 @@ # ndi_matlab_python_bridge.yaml — src/ndi/cloud/api/ # The Primary Contract for the ndi.cloud.api namespace. # -# Covers: datasets.py, documents.py, files.py, users.py, compute.py, _validators.py +# Covers: datasets.py, documents.py, files.py, users.py, compute.py, feedback.py, +# _validators.py project_metadata: bridge_version: "1.1" @@ -1426,6 +1427,99 @@ functions: misconfigured CI from silently deleting a real registered license). + # --- feedback.py --- + - name: submitFeedback + matlab_path: "+ndi/+cloud/+api/+feedback/submitFeedback.m" + matlab_last_sync_hash: "75f937a76" + python_path: "ndi/cloud/api/feedback.py" + input_arguments: + - name: app + type_matlab: "string {mustBeNonzeroLengthText}" + type_python: "NonEmptyStr" + - name: category + type_matlab: "string {mustBeMember(category, [\"bug\", \"performance\", \"other\"])}" + type_python: "FeedbackCategory" + - name: description + type_matlab: "string {mustBeNonzeroLengthText}" + type_python: "NonEmptyStr" + - name: appVersion + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: datasetId + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: documentId + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: environment + type_matlab: "struct {mustBeScalarOrEmpty} = struct.empty" + type_python: "dict[str, Any] | None" + default: "None" + - name: state + type_matlab: "struct {mustBeScalarOrEmpty} = struct.empty" + type_python: "dict[str, Any] | None" + default: "None" + - name: log + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: traceback + type_matlab: "string = missing" + type_python: "str | None" + default: "None" + - name: client + type_python: "_Client" + default: "None" + output_arguments: + - name: result + type_python: "APIResponse" + decision_log: > + Ported from NDI-matlab#1023 (75f937a76). POST /feedback with body + {schemaVersion: 1, app, category, description} plus each optional + field the caller supplied; the contract is ndi-cloud-node#158. + The optional arguments keep MATLAB's camelCase names (rather than the + snake_case the sibling modules use for arguments) because each one is + also the payload key, and the payload is shared with MATLAB and the + server. They are keyword-only, as MATLAB's are name-value options. + None plays the role of MATLAB's missing / struct.empty: the field is + omitted. "" and {} are sent, as MATLAB sends "" and struct(). + Returns an APIResponse whose data is the stored report's id, so + `b, feedbackID, status, url = submitFeedback(...)` unpacks like + MATLAB's [b, feedbackID, apiResponse, apiURL]. Where MATLAB returns + b = false on a non-201, Python raises (CloudAPIError with + status_code 400 / 413 / 429, CloudAuthError on 401 / 403), as every + other ndi.cloud.api function does. The implementation class + SubmitFeedback (sends any struct as-is) is the private helper + feedback._submit, used by the tests to reach server-side validation. + Tests: tests/test_cloud_api_feedback.py (offline) and + tests/test_cloud_feedback_live.py (port of feedbackTest.m). + + - name: deleteFeedback + matlab_path: "+ndi/+cloud/+api/+feedback/deleteFeedback.m" + matlab_last_sync_hash: "75f937a76" + python_path: "ndi/cloud/api/feedback.py" + input_arguments: + - name: feedbackID + type_matlab: "string" + type_python: "CloudId" + - name: client + type_python: "_Client" + default: "None" + output_arguments: + - name: result + type_python: "APIResponse" + decision_log: > + Ported from NDI-matlab#1023 (75f937a76). DELETE /feedback/{feedbackId}; + the server answers 204. data is MATLAB's confirmation message + "Feedback deleted successfully.". A report that does not exist or + belongs to another user raises CloudNotFoundError (MATLAB: b = false, + 404); a malformed id raises CloudAPIError with status_code 400. + The id is not checked for ObjectId shape client-side, matching MATLAB, + so the server's 400 stays reachable. + # --- compute.py --- - name: startSession matlab_path: "+ndi/+cloud/+api/+compute/startSession.m" @@ -1545,7 +1639,7 @@ not_yet_ported: - name: url matlab_path: "+ndi/+cloud/+api/url.m" - matlab_last_sync_hash: "3fce6cb" + matlab_last_sync_hash: "75f937a76" status: ported_differently decision_log: > MATLAB URL builder. Python uses CloudClient._build_url() @@ -1563,6 +1657,10 @@ not_yet_ported: bulk-upload status in api/files.py, /advance is what compute.advanceSession already posts to, and the signed-URL-set gap is recorded on batchSignedUrlLookup and downloadGenericFiles. + 2026-10-11: examined against 75f937a76 (was 3fce6cb). NDI-matlab#1023 + added submit_feedback (POST /feedback) and delete_feedback + (DELETE /feedback/{feedbackId}); both are interpolated at their call + sites in api/feedback.py, so there is no route table to update here. 2026-09-22: examined against 3fce6cb (was 6fdec7e7). NDI-matlab#1004 (6bf12fc0) added the list_dataset_files route (/datasets/{datasetId}/files) and made it keyset-paginated. As above, From ad7bd64084f9ae3854095b7e6db5a9b1011afcde Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:14:03 +0000 Subject: [PATCH 05/21] Test ndi.cloud.api.feedback offline and live Port of NDI-matlab feedbackTest.m (75f937a76). - tests/test_cloud_api_feedback.py: real CloudClient over a stub transport; pins the exact payload, optional-field omission, URL and method, MATLAB-style unpacking, client-side validation, and the 400/401/403/404/413/429 error mapping. POST is not retried. - tests/test_cloud_feedback_live.py: the live cases, skipped without credentials or where the endpoint is not deployed (404 on a malformed id); every submitted report is deleted, with a teardown backstop. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- tests/test_cloud_api_feedback.py | 312 ++++++++++++++++++++++++++++++ tests/test_cloud_feedback_live.py | 266 +++++++++++++++++++++++++ 2 files changed, 578 insertions(+) create mode 100644 tests/test_cloud_api_feedback.py create mode 100644 tests/test_cloud_feedback_live.py diff --git a/tests/test_cloud_api_feedback.py b/tests/test_cloud_api_feedback.py new file mode 100644 index 00000000..f80730a0 --- /dev/null +++ b/tests/test_cloud_api_feedback.py @@ -0,0 +1,312 @@ +"""Unit tests for ndi.cloud.api.feedback -- no network required. + +Offline half of the port of NDI-matlab +``tests/+ndi/+unittest/+cloud/feedbackTest.m`` (75f937a76); the live half +is ``tests/test_cloud_feedback_live.py``. + +The request body is a contract shared with NDI-matlab and the server +(ndi-cloud-node#158), so these tests pin the exact payload: which keys are +always sent, which are left out when not supplied, and that none is +renamed. The client is a real :class:`CloudClient` over a stub transport, +so the URL, method and error mapping under test are the ones a caller +actually gets. +""" + +from __future__ import annotations + +import json +import types + +import pydantic +import pytest + +from ndi.cloud.api import feedback +from ndi.cloud.api.feedback import deleteFeedback, submitFeedback +from ndi.cloud.client import CloudClient +from ndi.cloud.config import CloudConfig +from ndi.cloud.exceptions import CloudAPIError, CloudAuthError, CloudNotFoundError + +API_URL = "https://api.example.test/v1" +APP = "NDI-python-unittest" +FEEDBACK_ID = "65f0c0ffee0000000000abcd" + + +def response(status=200, body=None, reason=""): + """Something shaped like a ``requests.Response``.""" + text = json.dumps(body) if body is not None else "" + + def _json(): + if body is None: + raise ValueError("no json") + return body + + return types.SimpleNamespace( + status_code=status, + reason=reason, + json=_json, + text=text, + content=text.encode(), + headers={}, + elapsed=None, + ) + + +class Transport: + """A stub ``requests.Session`` that records requests and replies RESP.""" + + def __init__(self, resp): + self.resp = resp + self.calls: list[dict] = [] + + def request(self, method, url, **kwargs): + self.calls.append({"method": method, "url": url, **kwargs}) + return self.resp + + +def make_client(resp, token="test-token"): + client = CloudClient.__new__(CloudClient) + client.config = CloudConfig(api_url=API_URL, token=token) + client._session = Transport(resp) + return client + + +def created(feedback_id=FEEDBACK_ID): + return make_client(response(201, {"id": feedback_id}, "Created")) + + +# --------------------------------------------------------------------------- +# submitFeedback +# --------------------------------------------------------------------------- + + +class TestSubmitFeedbackPayload: + def test_minimal_report_sends_only_the_required_fields(self): + client = created() + submitFeedback(APP, "other", "minimal report", client=client) + + (call,) = client._session.calls + assert call["json"] == { + "schemaVersion": 1, + "app": APP, + "category": "other", + "description": "minimal report", + } + + def test_post_to_feedback_with_bearer_token(self): + client = created() + submitFeedback(APP, "bug", "x", client=client) + + (call,) = client._session.calls + assert call["method"] == "POST" + assert call["url"] == f"{API_URL}/feedback" + assert call["headers"]["Authorization"] == "Bearer test-token" + + def test_full_report_sends_every_optional_field_under_its_matlab_name(self): + environment = {"os": "linux", "python": "3.12", "launchedFrom": "pytest"} + state = {"level": 3, "fetchStats": {"ok": 10, "failed": 2}} + client = created() + submitFeedback( + APP, + "bug", + "full report", + appVersion="NDI-python unittest", + datasetId="000000000000000000000000", + documentId="unittest-document", + environment=environment, + state=state, + log="line 1\nline 2", + traceback="no real traceback", + client=client, + ) + + (call,) = client._session.calls + assert call["json"] == { + "schemaVersion": 1, + "app": APP, + "category": "bug", + "description": "full report", + "appVersion": "NDI-python unittest", + "datasetId": "000000000000000000000000", + "documentId": "unittest-document", + "environment": environment, + "state": state, + "log": "line 1\nline 2", + "traceback": "no real traceback", + } + + @pytest.mark.parametrize( + "field, value", + [ + ("appVersion", "1.0"), + ("datasetId", "abc"), + ("documentId", "def"), + ("log", "a log"), + ("traceback", "a traceback"), + ("environment", {"os": "linux"}), + ("state", {"k": 1}), + ], + ) + def test_each_optional_field_is_sent_alone(self, field, value): + client = created() + submitFeedback(APP, "performance", "d", client=client, **{field: value}) + + body = client._session.calls[0]["json"] + assert body[field] == value + assert set(body) == {"schemaVersion", "app", "category", "description", field} + + def test_empty_values_are_sent_not_dropped(self): + # MATLAB omits only a missing string or struct.empty; "" and + # struct() go out as they are. None is Python's "missing". + client = created() + submitFeedback(APP, "bug", "d", log="", environment={}, client=client) + + body = client._session.calls[0]["json"] + assert body["log"] == "" + assert body["environment"] == {} + + @pytest.mark.parametrize("category", ["bug", "performance", "other"]) + def test_every_category_is_accepted(self, category): + client = created() + submitFeedback(APP, category, "d", client=client) + assert client._session.calls[0]["json"]["category"] == category + + +class TestSubmitFeedbackResult: + def test_returns_the_stored_report_id(self): + result = submitFeedback(APP, "bug", "d", client=created("abc123")) + assert result.data == "abc123" + assert result.success is True + assert result.status_code == 201 + assert result.url == f"{API_URL}/feedback" + + def test_unpacks_like_matlab(self): + b, feedbackID, status, url = submitFeedback(APP, "bug", "d", client=created("abc123")) + assert (b, feedbackID, status, url) == (True, "abc123", 201, f"{API_URL}/feedback") + + +class TestSubmitFeedbackValidation: + """Client-side argument validation: nothing reaches the network.""" + + def test_unknown_category_is_rejected(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "feature", "never sent", client=client) + assert client._session.calls == [] + + def test_empty_description_is_rejected(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "bug", "", client=client) + assert client._session.calls == [] + + def test_empty_app_is_rejected(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback("", "bug", "d", client=client) + assert client._session.calls == [] + + def test_environment_must_be_a_mapping(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "bug", "d", environment=["not", "a", "dict"], client=client) + assert client._session.calls == [] + + def test_optional_fields_are_keyword_only(self): + client = created() + with pytest.raises(pydantic.ValidationError): + submitFeedback(APP, "bug", "d", "1.0", client=client) + assert client._session.calls == [] + + +class TestSubmitFeedbackErrors: + @pytest.mark.parametrize( + "status, error", + [ + (400, "category must be one of bug, performance, other"), + (413, "log exceeds 500000 characters"), + (429, "Too many feedback reports; the limit is 10 per hour"), + (500, "Failed to store feedback"), + ], + ) + def test_server_rejection_raises_with_status(self, status, error): + client = make_client(response(status, {"error": error})) + with pytest.raises(CloudAPIError) as excinfo: + submitFeedback(APP, "bug", "d", client=client) + assert excinfo.value.status_code == status + assert excinfo.value.response_body == {"error": error} + + @pytest.mark.parametrize("status", [401, 403]) + def test_unauthenticated_raises_auth_error(self, status): + client = make_client(response(status, {"error": "Unauthorized"})) + with pytest.raises(CloudAuthError): + submitFeedback(APP, "bug", "d", client=client) + + def test_post_is_not_retried(self): + # A repeated POST could store the report twice. + client = make_client(response(503, {"error": "unavailable"})) + with pytest.raises(CloudAPIError): + submitFeedback(APP, "bug", "d", client=client) + assert len(client._session.calls) == 1 + + +class TestSubmitRaw: + """feedback._submit mirrors the SubmitFeedback implementation class.""" + + def test_sends_the_body_as_is(self): + body = {"schemaVersion": 99, "app": APP, "category": "feature"} + client = created() + feedback._submit(body, client=client) + assert client._session.calls[0]["json"] == body + + +# --------------------------------------------------------------------------- +# deleteFeedback +# --------------------------------------------------------------------------- + + +class TestDeleteFeedback: + def test_delete_url_and_method(self): + client = make_client(response(204, None, "No Content")) + deleteFeedback(FEEDBACK_ID, client=client) + + (call,) = client._session.calls + assert call["method"] == "DELETE" + assert call["url"] == f"{API_URL}/feedback/{FEEDBACK_ID}" + assert call["headers"]["Authorization"] == "Bearer test-token" + + def test_success_unpacks_like_matlab(self): + client = make_client(response(204, None, "No Content")) + b, answer, status, url = deleteFeedback(FEEDBACK_ID, client=client) + assert b is True + assert answer == "Feedback deleted successfully." + assert status == 204 + assert url == f"{API_URL}/feedback/{FEEDBACK_ID}" + + def test_id_is_url_encoded(self): + client = make_client(response(204, None)) + deleteFeedback("a/b", client=client) + assert client._session.calls[0]["url"] == f"{API_URL}/feedback/a%2Fb" + + def test_missing_report_raises_not_found(self): + client = make_client(response(404, {"error": "Feedback not found"})) + with pytest.raises(CloudNotFoundError): + deleteFeedback("000000000000000000000000", client=client) + + def test_malformed_id_raises_400(self): + client = make_client(response(400, {"error": "Invalid feedback id"})) + with pytest.raises(CloudAPIError) as excinfo: + deleteFeedback("not-a-valid-id", client=client) + assert excinfo.value.status_code == 400 + + def test_empty_id_is_rejected_without_a_request(self): + client = make_client(response(204, None)) + with pytest.raises(pydantic.ValidationError): + deleteFeedback("", client=client) + assert client._session.calls == [] + + +def test_registered_in_package(): + import ndi.cloud.api as api + + assert "feedback" in api.__all__ + assert api.feedback.submitFeedback is submitFeedback diff --git a/tests/test_cloud_feedback_live.py b/tests/test_cloud_feedback_live.py new file mode 100644 index 00000000..1e16bc86 --- /dev/null +++ b/tests/test_cloud_feedback_live.py @@ -0,0 +1,266 @@ +""" +Live tests for the in-app problem report endpoints. + +Ported from NDI-matlab tests/+ndi/+unittest/+cloud/feedbackTest.m +(75f937a76, NDI-matlab#1023). Exercises ndi.cloud.api.feedback against +the real API (ndi-cloud-node#158): + + POST /feedback + DELETE /feedback/{feedbackId} + +Every report a test submits is deleted again -- by the test itself, or by +the ``submitted`` fixture's teardown if the test fails part-way -- so the +suite does not leave reports in the admin triage queue. Deleted reports do +not count against the server's limit of 10 reports per user per hour. + +Where MATLAB checks ``b == false`` and a status code, Python checks the +exception the call raises and its ``status_code``, since every +ndi.cloud.api function raises on a non-2xx response. + +The client-side validation tests of feedbackTest.m +(testWrapperRejectsUnknownCategory / testWrapperRejectsEmptyDescription) +need no network and live in tests/test_cloud_api_feedback.py. + +Environment variables consumed: + NDI_CLOUD_USERNAME / NDI_CLOUD_PASSWORD -- skip if absent (standard + live-test pattern, matches test_cloud_live.py). + CLOUD_API_ENVIRONMENT -- 'prod' (default) or 'dev'. The module skips + in an environment that does not have the feedback endpoint yet. +""" + +from __future__ import annotations + +import os +import platform +import sys + +import pytest + +_has_creds = bool(os.environ.get("NDI_CLOUD_USERNAME") and os.environ.get("NDI_CLOUD_PASSWORD")) +pytestmark = pytest.mark.skipif(not _has_creds, reason="NDI cloud credentials not set") + +APP_NAME = "NDI-python-unittest" +#: Valid 24-hex id that will not match any stored report. +NONEXISTENT_ID = "000000000000000000000000" + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + +@pytest.fixture(scope="module") +def cloud_config(): + """Authenticate once for the entire module and return config.""" + from ndi.cloud.auth import login + from ndi.cloud.config import CloudConfig + + config = CloudConfig.from_env() + config = login(config=config) + assert config.is_authenticated, "Login failed -- no token received" + return config + + +@pytest.fixture(scope="module") +def client(cloud_config): + """Return an authenticated CloudClient.""" + from ndi.cloud.client import CloudClient + + return CloudClient(cloud_config) + + +@pytest.fixture(scope="module", autouse=True) +def endpoint_deployed(client): + """Skip (rather than fail) where the feedback endpoint is not deployed. + + The cloud suite runs against both prod and dev. Once deployed, a + malformed id is answered 400 by the route; before that, the API's + catch-all answers 404. Mirrors feedbackTest.checkEndpointDeployed. + """ + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudAPIError, CloudNotFoundError + + try: + deleteFeedback("not-a-valid-id", client=client) + except CloudNotFoundError: + pytest.skip("The /feedback endpoint is not deployed in this API environment") + except CloudAPIError: + pass + + +@pytest.fixture() +def submitted(client): + """Collect ids of submitted reports; delete any still present at teardown. + + Mirrors feedbackTest.registerCleanup. A 404 here just means the test + already deleted the report itself. + """ + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudError + + ids: list[str] = [] + yield ids + for feedback_id in ids: + try: + deleteFeedback(feedback_id, client=client) + except CloudError: + pass + + +def _submit_raw(client, body): + """POST BODY as-is, bypassing the wrapper's own validation. + + MATLAB reaches the server's validation through the implementation class + ndi.cloud.api.implementation.feedback.SubmitFeedback; feedback._submit + is its counterpart. + """ + from ndi.cloud.api.feedback import _submit + + return _submit(body, client=client) + + +def _expect_rejected(client, submitted, body, status): + """Submit BODY raw and require the server to refuse it with STATUS.""" + from ndi.cloud.exceptions import CloudAPIError + + try: + result = _submit_raw(client, body) + except CloudAPIError as exc: + assert exc.status_code == status, exc + return + submitted.append(result.data) + pytest.fail(f"server accepted an invalid report (id {result.data}); expected HTTP {status}") + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +class TestFeedbackLive: + def test_submit_and_delete(self, client, submitted): + """Submit a minimal report, delete it, and confirm a second delete is 404.""" + from ndi.cloud.api.feedback import deleteFeedback, submitFeedback + from ndi.cloud.exceptions import CloudNotFoundError + + b, feedback_id, status, url = submitFeedback( + APP_NAME, "other", "feedbackTest: minimal report (safe to ignore)", client=client + ) + assert b is True, url + assert status == 201, url + assert isinstance(feedback_id, str) and feedback_id, url + submitted.append(feedback_id) + + b_del, answer, status_del, url_del = deleteFeedback(feedback_id, client=client) + assert b_del is True, url_del + assert status_del == 204, url_del + assert answer == "Feedback deleted successfully." + + with pytest.raises(CloudNotFoundError): + deleteFeedback(feedback_id, client=client) + + def test_submit_full_report(self, client, submitted): + """Every optional field, including a log carrying credentials to redact. + + The reporter cannot read reports back, so redaction itself is + checked in the server's own tests. + """ + from ndi.cloud.api.feedback import deleteFeedback, submitFeedback + + environment = { + "os": platform.platform(), + "python": sys.version.split()[0], + "launchedFrom": "pytest", + } + state = {"level": 3, "fetchStats": {"ok": 10, "failed": 2}} + fake_token = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJ1bml0dGVzdCJ9.c2lnbmF0dXJlX3VuaXR0ZXN0" + log_text = ( + "[lightsheet] GET https://bucket.s3.amazonaws.com/k?X-Amz-Signature=deadbeef01\n" + f"Authorization: Bearer {fake_token}" + ) + + result = submitFeedback( + APP_NAME, + "bug", + "feedbackTest: full report (safe to ignore)", + appVersion="NDI-python unittest", + datasetId=NONEXISTENT_ID, + documentId="unittest-document", + environment=environment, + state=state, + log=log_text, + traceback="feedbackTest: no real traceback", + client=client, + ) + assert result.success, result.url + assert result.status_code == 201, result.url + submitted.append(result.data) + + assert deleteFeedback(result.data, client=client).success + + def test_invalid_category_is_rejected(self, client, submitted): + body = { + "schemaVersion": 1, + "app": APP_NAME, + "category": "feature", + "description": "feedbackTest: invalid category", + } + _expect_rejected(client, submitted, body, 400) + + def test_missing_description_is_rejected(self, client, submitted): + body = {"schemaVersion": 1, "app": APP_NAME, "category": "bug"} + _expect_rejected(client, submitted, body, 400) + + def test_unsupported_schema_version_is_rejected(self, client, submitted): + body = { + "schemaVersion": 99, + "app": APP_NAME, + "category": "bug", + "description": "feedbackTest: bad schemaVersion", + } + _expect_rejected(client, submitted, body, 400) + + def test_oversize_log_is_rejected(self, client, submitted): + """A log over the 500,000-character field cap but under the 1 MB request cap.""" + from ndi.cloud.api.feedback import submitFeedback + from ndi.cloud.exceptions import CloudAPIError + + try: + result = submitFeedback( + APP_NAME, + "performance", + "feedbackTest: oversize log", + log="x" * 600_000, + client=client, + ) + except CloudAPIError as exc: + assert exc.status_code == 413, exc + return + submitted.append(result.data) + pytest.fail("server accepted a 600,000-character log; expected HTTP 413") + + def test_unauthenticated_submit_is_rejected(self, cloud_config): + """No token: refused. The API answers 403 throughout; 401 is accepted too.""" + from ndi.cloud.api.feedback import submitFeedback + from ndi.cloud.client import CloudClient + from ndi.cloud.config import CloudConfig + from ndi.cloud.exceptions import CloudAuthError + + anonymous = CloudClient(CloudConfig(api_url=cloud_config.api_url, token="")) + with pytest.raises(CloudAuthError): + submitFeedback(APP_NAME, "bug", "feedbackTest: no token", client=anonymous) + + def test_delete_nonexistent_report(self, client): + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudNotFoundError + + with pytest.raises(CloudNotFoundError): + deleteFeedback(NONEXISTENT_ID, client=client) + + def test_delete_malformed_id(self, client): + from ndi.cloud.api.feedback import deleteFeedback + from ndi.cloud.exceptions import CloudAPIError + + with pytest.raises(CloudAPIError) as excinfo: + deleteFeedback("not-a-valid-id", client=client) + assert excinfo.value.status_code == 400 From 03e5f268f645da0e0ffc1c853b3ea8a896a66ba7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:33:01 +0000 Subject: [PATCH 06/21] Bridge checks: verify MATLAB counterparts that live on an NDI-matlab branch NDI-python main records MATLAB files (the lightsheet OME-Zarr functions, LightsheetZarrManager, resolveOrganizationId, and six ndi_common document files) that exist only on VH-Lab/NDI-matlab claude/lightsheet-zarr-ndi-viewer-djp5vk, which has not merged. CI checks out NDI-matlab main, so the bridge job has been red on main since 2026-09-26, reporting them as stale paths. Add a matlab_branch field (and MATLAB_BRANCH_PENDING for ndi_common) that names the branch, and check the claim rather than skip it: - the branch must exist and the file be on it (ndi_common: byte-identical); - drift is measured against the branch tip instead of HEAD; - once the branch merges into NDI-matlab main, the marker must be removed. Helpers in tests/_matlab_branch.py, tested against a throwaway git repo in tests/test_matlab_branch_pending.py. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- tests/_matlab_branch.py | 71 +++++++++++++++++ tests/test_matlab_branch_pending.py | 99 ++++++++++++++++++++++++ tests/test_matlab_bridge_completeness.py | 97 ++++++++++++++++++++++- tests/test_matlab_bridge_hashes.py | 39 +++++++--- tests/test_ndi_common_matlab_sync.py | 62 ++++++++++++++- 5 files changed, 355 insertions(+), 13 deletions(-) create mode 100644 tests/_matlab_branch.py create mode 100644 tests/test_matlab_branch_pending.py diff --git a/tests/_matlab_branch.py b/tests/_matlab_branch.py new file mode 100644 index 00000000..6ff1c30e --- /dev/null +++ b/tests/_matlab_branch.py @@ -0,0 +1,71 @@ +"""Reading a named NDI-matlab branch, for counterparts not yet on its main. + +Some NDI-python work lands before its MATLAB half: the lightsheet OME-Zarr +documents and functions, and ``resolveOrganizationId``, were written as a +pair with VH-Lab/NDI-matlab ``claude/lightsheet-zarr-ndi-viewer-djp5vk`` and +merged here while that branch was still open. The bridge checks compare +against the checked-out NDI-matlab tree (its ``main`` unless a branch of the +same name exists), so those records looked stale -- a MATLAB file "that does +not exist" -- when the truth is "exists, not merged yet". + +A bridge entry says so with ``matlab_branch: ``, and an +``ndi_common`` file with an entry in ``MATLAB_BRANCH_PENDING``. The checks +then verify the claim against that branch instead of skipping it: the file +must be there, drift is measured against the branch tip, and once the branch +is merged the marker must go (see section 8 of +docs/developer_notes/ndi_matlab_python_bridge.yaml). + +These helpers read the branch through git, so they need the NDI-matlab +checkout's remote branches -- which ``actions/checkout`` provides with +``fetch-depth: 0``, and the bridge job already asks for that. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Any + + +def pending_branch(entry: dict[str, Any]) -> str: + """The entry's ``matlab_branch``, or ``""`` when it names none.""" + value = entry.get("matlab_branch") + return value.strip() if isinstance(value, str) else "" + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess: + return subprocess.run(["git", "-C", str(repo), *args], capture_output=True, text=False) + + +def branch_ref(repo: Path, branch: str) -> str | None: + """The commit at the tip of BRANCH in REPO, or None if it is not there. + + Tries the remote-tracking ref first, since that is what a CI checkout + holds, then a local branch of that name. + """ + for ref in (f"refs/remotes/origin/{branch}", f"refs/heads/{branch}"): + result = _git(repo, "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}") + if result.returncode == 0: + return result.stdout.decode().strip() + return None + + +def read_on_branch(repo: Path, commit: str, repo_path: str) -> bytes | None: + """The bytes of REPO_PATH (relative to the repo root) at COMMIT, or None.""" + result = _git(repo, "show", f"{commit}:{repo_path}") + return result.stdout if result.returncode == 0 else None + + +#: NDI-matlab's default branch. "Merged" means merged here, not into whatever +#: happens to be checked out: CI checks out a same-named NDI-matlab branch +#: when one exists, and on the pending branch itself HEAD contains its own +#: tip by definition. +MAIN_BRANCH = "main" + + +def is_merged(repo: Path, commit: str) -> bool: + """True if COMMIT is already part of NDI-matlab main.""" + main = branch_ref(repo, MAIN_BRANCH) + if main is None: + return False + return _git(repo, "merge-base", "--is-ancestor", commit, main).returncode == 0 diff --git a/tests/test_matlab_branch_pending.py b/tests/test_matlab_branch_pending.py new file mode 100644 index 00000000..b0d3a2d6 --- /dev/null +++ b/tests/test_matlab_branch_pending.py @@ -0,0 +1,99 @@ +"""The ``matlab_branch`` helpers answer correctly, against a real git repo. + +MATLAB counterpart: none; this tests the bridge checks themselves. + +``matlab_branch`` lets a bridge entry (and ``MATLAB_BRANCH_PENDING`` an +``ndi_common`` file) say "the MATLAB side is on this NDI-matlab branch, not +yet on main" -- section 8 of docs/developer_notes/ndi_matlab_python_bridge.yaml. +The checks exempt such a record from "file must exist in the checked-out +tree" and verify it against the branch instead, so everything rests on +:mod:`tests._matlab_branch` telling the truth. A helper that always said +"found" would let any typo through; one that never said "merged" would let +the marker outlive its branch. + +The repo is built in a temporary directory, so this needs no NDI-matlab +checkout and runs in every job. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from tests._matlab_branch import branch_ref, is_merged, pending_branch, read_on_branch + +FILE = "src/ndi/+ndi/+fun/+doc/+lightsheet/view.m" + + +def git(repo: Path, *args: str) -> str: + return subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True, check=True + ).stdout.strip() + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + """main with one commit; ``feature`` one commit ahead, adding FILE.""" + git(tmp_path, "init", "-q", "-b", "main") + git(tmp_path, "config", "user.email", "test@example.invalid") + git(tmp_path, "config", "user.name", "test") + (tmp_path / "README").write_text("main\n") + git(tmp_path, "add", "README") + git(tmp_path, "commit", "-q", "-m", "main") + git(tmp_path, "checkout", "-q", "-b", "feature") + (tmp_path / FILE).parent.mkdir(parents=True) + (tmp_path / FILE).write_text("function view()\n") + git(tmp_path, "add", FILE) + git(tmp_path, "commit", "-q", "-m", "feature") + git(tmp_path, "checkout", "-q", "main") + return tmp_path + + +def test_a_branch_resolves_to_its_tip(repo): + assert branch_ref(repo, "feature") == git(repo, "rev-parse", "feature") + + +def test_a_missing_branch_resolves_to_nothing(repo): + assert branch_ref(repo, "no-such-branch") is None + + +def test_a_file_on_the_branch_is_read_from_it(repo): + tip = branch_ref(repo, "feature") + assert read_on_branch(repo, tip, FILE) == b"function view()\n" + assert not (repo / FILE).exists(), "the file is on the branch, not the checkout" + + +def test_a_file_not_on_the_branch_is_reported_absent(repo): + tip = branch_ref(repo, "feature") + assert read_on_branch(repo, tip, FILE.replace("view", "veiw")) is None + + +def test_an_unmerged_branch_is_not_merged(repo): + assert not is_merged(repo, branch_ref(repo, "feature")) + + +def test_checking_out_the_branch_does_not_make_it_merged(repo): + """CI checks out a same-named NDI-matlab branch when one exists. HEAD + then contains the branch tip, which must not read as "merged".""" + git(repo, "checkout", "-q", "feature") + assert not is_merged(repo, branch_ref(repo, "feature")) + + +def test_a_branch_merged_into_main_is_merged(repo): + git(repo, "merge", "-q", "--no-ff", "-m", "merge", "feature") + assert is_merged(repo, branch_ref(repo, "feature")) + + +@pytest.mark.parametrize( + "entry, expected", + [ + ({"matlab_branch": "feature"}, "feature"), + ({"matlab_branch": " feature "}, "feature"), + ({}, ""), + ({"matlab_branch": None}, ""), + ], +) +def test_pending_branch(entry, expected): + assert pending_branch(entry) == expected diff --git a/tests/test_matlab_bridge_completeness.py b/tests/test_matlab_bridge_completeness.py index 6a8af92a..ac72410b 100644 --- a/tests/test_matlab_bridge_completeness.py +++ b/tests/test_matlab_bridge_completeness.py @@ -50,6 +50,8 @@ import pytest import yaml +from tests._matlab_branch import branch_ref, is_merged, pending_branch, read_on_branch + #: Set by CI once NDI-matlab is checked out. See the module docstring. STRICT_ENV_VAR = "NDI_BRIDGE_CHECK_STRICT" @@ -364,15 +366,24 @@ def test_every_recorded_matlab_path_points_at_a_real_file(self, package: Bridged root = require_matlab_root() index = read_bridge_index(package) prefix = f"{package.matlab_dir}/" + # A path recorded with matlab_branch is checked against that branch + # by TestBranchPendingEntries instead -- its absence from the + # checked-out tree is the claim, not a defect. + pending = {path for _, path, _ in branch_pending_entries(package)} stale = sorted( - path for path in index.paths if path.startswith(prefix) and not (root / path).exists() + path + for path in index.paths + if path.startswith(prefix) and path not in pending and not (root / path).exists() ) assert not stale, ( f"{BRIDGE_FILENAME} entries under {package.python_dir} name MATLAB " "files that do not exist:\n " + "\n ".join(stale) + '\n\nPoint each at its new path, or set matlab_path: "N/A" and say ' - "in the decision_log that MATLAB removed it." + "in the decision_log that MATLAB removed it. If the file is on an " + "NDI-matlab branch that has not merged yet, add matlab_branch: " + " to the entry instead (section 8 of " + "docs/developer_notes/ndi_matlab_python_bridge.yaml)." ) @pytest.mark.parametrize("package", PACKAGES, ids=lambda p: p.id) @@ -396,6 +407,74 @@ def test_every_excluded_layer_carries_a_reason(self, package: BridgedPackage): assert len(reason.split()) >= 10, f"exclusion {prefix!r} needs a real reason" +def branch_pending_entries(package: BridgedPackage) -> list[tuple[str, str, dict[str, Any]]]: + """``(source, matlab_path, entry)`` for every entry carrying ``matlab_branch``. + + ``matlab_path`` is normalized; it is ``""`` when the entry names no file, + which :class:`TestBranchPendingEntries` reports. + """ + found = [] + for source in sorted((REPO_ROOT / package.python_dir).rglob(BRIDGE_FILENAME)): + data = yaml.safe_load(source.read_text(encoding="utf-8")) + for entry in _entries_with_a_matlab_branch(data): + path = normalize_matlab_path(entry.get("matlab_path") or "") + found.append((str(source.relative_to(REPO_ROOT)), path, entry)) + return found + + +class TestBranchPendingEntries: + """``matlab_branch`` is a claim, and the claim is checked. + + An entry may record a MATLAB file that is on a named NDI-matlab branch + but not yet on the tree this job checked out (section 8 of + docs/developer_notes/ndi_matlab_python_bridge.yaml). The stale-path + guard above exempts it, so this has to hold it to account instead, or + ``matlab_branch`` would be a way to silence that guard for good: + + * the branch must exist and the file must be on it -- a typo in either + is the stale path the exemption would otherwise hide; + * once the branch has merged into NDI-matlab main, the marker must go, + so an entry does not keep pointing at a branch after its file has + reached main (and the branch, typically, has been deleted). + """ + + @pytest.mark.parametrize("package", PACKAGES, ids=lambda p: p.id) + def test_every_branch_pending_file_is_on_its_branch(self, package: BridgedPackage): + root = require_matlab_root() + repo = root.parent.parent + offenders, unresolved = [], [] + for source, path, entry in branch_pending_entries(package): + name = entry.get("name", "") + branch = pending_branch(entry) + if not path: + offenders.append(f"{source}: {name} -- matlab_branch needs a real matlab_path") + continue + tip = branch_ref(repo, branch) + if tip is None: + unresolved.append(f"{source}: {name} -- branch {branch!r} not found") + continue + if read_on_branch(repo, tip, f"src/ndi/{path}") is None: + offenders.append(f"{source}: {name} -- {path} is not on {branch!r}") + elif is_merged(repo, tip): + offenders.append( + f"{source}: {name} -- {branch!r} has merged into NDI-matlab main; " + "remove matlab_branch" + ) + if unresolved and not os.environ.get(STRICT_ENV_VAR, "").strip(): + pytest.skip( + "this NDI-matlab checkout lacks branches the bridge names " + "(fetch them, or clone with full history): " + "; ".join(unresolved) + ) + problems = offenders + unresolved + assert not problems, ( + "bridge entries recorded against an NDI-matlab branch do not hold up:\n " + + "\n ".join(problems) + + "\n\nA branch that is not found was usually merged and deleted: check the " + "file is on NDI-matlab main and remove matlab_branch from the entry. See " + "section 8 of docs/developer_notes/ndi_matlab_python_bridge.yaml." + ) + + class TestTheDeferralsSayWhy: """``not_yet_ported`` and ``not_applicable`` are decisions, not labels. @@ -606,6 +685,20 @@ def _entries_with_a_matlab_path(node: Any) -> list[dict[str, Any]]: return found +def _entries_with_a_matlab_branch(node: Any) -> list[dict[str, Any]]: + """Every mapping in the tree that carries a ``matlab_branch:`` key.""" + found: list[dict[str, Any]] = [] + if isinstance(node, dict): + if isinstance(node.get("matlab_branch"), str): + found.append(node) + for value in node.values(): + found.extend(_entries_with_a_matlab_branch(value)) + elif isinstance(node, list): + for item in node: + found.extend(_entries_with_a_matlab_branch(item)) + return found + + def _entries_with_status(node: Any) -> list[dict[str, Any]]: """Every mapping in the tree that carries a ``status:`` key.""" found: list[dict[str, Any]] = [] diff --git a/tests/test_matlab_bridge_hashes.py b/tests/test_matlab_bridge_hashes.py index 7588036f..6427c034 100644 --- a/tests/test_matlab_bridge_hashes.py +++ b/tests/test_matlab_bridge_hashes.py @@ -81,6 +81,7 @@ import pytest import yaml +from tests._matlab_branch import branch_ref, pending_branch from tests.test_matlab_bridge_completeness import ( BRIDGE_FILENAME, REPO_ROOT, @@ -240,11 +241,28 @@ def drift_key(entry: dict[str, Any]) -> tuple[str, str]: return (matlab_file(entry), entry.get("name", "")) -def has_drifted(repo: Path, entry: dict[str, Any]) -> bool: +def drift_target(repo: Path, entry: dict[str, Any]) -> str | None: + """The NDI-matlab revision this entry's drift is measured against. + + ``HEAD`` when the file is in the checked-out tree. An entry whose file is + only on a not-yet-merged branch (``matlab_branch``, section 8 of + docs/developer_notes/ndi_matlab_python_bridge.yaml) is measured against + that branch's tip, so recording one does not exempt it from drift. None + when there is nothing to measure against -- a file on neither, which the + completeness guard reports. + """ + if (repo / "src" / "ndi" / matlab_file(entry)).exists(): + return "HEAD" + branch = pending_branch(entry) + return branch_ref(repo, branch) if branch else None + + +def has_drifted(repo: Path, entry: dict[str, Any], target: str = "HEAD") -> bool: """Have commits touching this entry's MATLAB file landed since its hash? Path-filtered on purpose: an unrelated commit elsewhere in NDI-matlab is - not this entry's problem and must not turn CI red. + not this entry's problem and must not turn CI red. TARGET is the + revision to measure up to; see :func:`drift_target`. """ full = f"src/ndi/{matlab_file(entry)}" result = subprocess.run( @@ -253,7 +271,7 @@ def has_drifted(repo: Path, entry: dict[str, Any]) -> bool: "-C", str(repo), "log", - f"{entry['matlab_last_sync_hash']}..HEAD", + f"{entry['matlab_last_sync_hash']}..{target}", "--oneline", "--", full, @@ -300,13 +318,14 @@ def test_no_entry_has_drifted(self): key = drift_key(entry) if key in allowed: continue - full = repo / "src" / "ndi" / key[0] - if not full.exists(): + target = drift_target(repo, entry) + if target is None: continue - if has_drifted(repo, entry): + if has_drifted(repo, entry, target): + where = "" if target == "HEAD" else f" on {pending_branch(entry)!r}" offenders.append( f"{display_path(source)}: {key[1]} ({key[0]}) -- MATLAB has " - f"moved since {entry['matlab_last_sync_hash']}" + f"moved{where} since {entry['matlab_last_sync_hash']}" ) assert not offenders, ( "these bridge entries have drifted -- NDI-matlab changed the file after " @@ -314,6 +333,7 @@ def test_no_entry_has_drifted(self): "known to match:\n " + "\n ".join(offenders) + "\n\n" "Read the diff:\n" " git -C ../NDI-matlab diff ..HEAD -- src/ndi/\n" + "(for an entry with matlab_branch, diff up to origin/ instead)\n" "then port the behavioural change and bump the hash, or -- if nothing on " "the Python side must follow -- just bump it. No decision_log is required " "for a regular port. See section 7 of " @@ -361,9 +381,10 @@ def test_the_allowlist_has_no_stale_entries(self): if not isinstance(entry.get("matlab_last_sync_hash"), str): live_nohash.add(key) continue - if not (repo / "src" / "ndi" / key[0]).exists(): + target = drift_target(repo, entry) + if target is None: continue - if has_drifted(repo, entry): + if has_drifted(repo, entry, target): live_drifted.add(key) fixed = sorted( [f"drifted: {n} ({p})" for p, n in drifted_allowed - live_drifted] diff --git a/tests/test_ndi_common_matlab_sync.py b/tests/test_ndi_common_matlab_sync.py index 779a4e96..32a5590d 100644 --- a/tests/test_ndi_common_matlab_sync.py +++ b/tests/test_ndi_common_matlab_sync.py @@ -36,6 +36,8 @@ import pytest +from tests._matlab_branch import branch_ref, is_merged, read_on_branch + NDI_COMMON_REL = Path("src/ndi/ndi_common") #: Set by CI once NDI-matlab is checked out. See the module docstring. @@ -125,6 +127,25 @@ def _require_matlab_repo() -> Path: ) +# Present in Python and on a named NDI-matlab branch that has not merged into +# the checked-out tree yet. Mapped to that branch. Not an exclusion: each file +# must match its copy on the branch byte-for-byte, and the entry must go once +# the branch merges (test_branch_pending_files_match_their_branch). Section 8 +# of docs/developer_notes/ndi_matlab_python_bridge.yaml. +_LIGHTSHEET_BRANCH = "claude/lightsheet-zarr-ndi-viewer-djp5vk" +MATLAB_BRANCH_PENDING: dict[str, str] = { + # The lightsheet OME-Zarr document types, landed here with the napari + # viewer (src/ndi/gui/app/lightsheetZarr) and authored as a pair with + # the MATLAB writer on this branch. + "database_documents/data/lightsheetZarrLevel.json": _LIGHTSHEET_BRANCH, + "database_documents/data/lightsheetZarrLevel.md": _LIGHTSHEET_BRANCH, + "database_documents/data/lightsheetZarrPyramid.json": _LIGHTSHEET_BRANCH, + "database_documents/data/lightsheetZarrPyramid.md": _LIGHTSHEET_BRANCH, + "schema_documents/data/lightsheetZarrLevel_schema.json": _LIGHTSHEET_BRANCH, + "schema_documents/data/lightsheetZarrPyramid_schema.json": _LIGHTSHEET_BRANCH, +} + + # Files present on both sides that intentionally do not match byte-for-byte. # Each entry pairs the relative path with a one-line reason so the exclusion # is not silent. When one of these converges, the test asks the caller to @@ -183,11 +204,48 @@ def test_no_undeclared_python_only_files(python_common, matlab_common): py = _list_files(python_common) ml = _list_files(matlab_common) extra = py - ml - unexpected = extra - PYTHON_ONLY_EXPECTED + unexpected = extra - PYTHON_ONLY_EXPECTED - MATLAB_BRANCH_PENDING.keys() assert not unexpected, ( "The following files exist in NDI-python/ndi_common but not in " "NDI-matlab. Either remove them, or add them to PYTHON_ONLY_EXPECTED " - "with a reason:\n " + "\n ".join(sorted(unexpected)) + "with a reason (or, if they are on an NDI-matlab branch that has not " + "merged yet, to MATLAB_BRANCH_PENDING):\n " + "\n ".join(sorted(unexpected)) + ) + + +def test_branch_pending_files_match_their_branch(python_common, matlab_common): + """Every MATLAB_BRANCH_PENDING file is on its branch, byte-for-byte. + + The map exempts these files from the Python-only check above, so this is + what keeps it honest: a file that is not on the branch, or differs from + it, is an undeclared divergence; and a branch that has merged into + NDI-matlab main means the entry is no longer needed. + """ + repo = matlab_common.parent.parent.parent + offenders, unresolved = [], [] + for rel, branch in sorted(MATLAB_BRANCH_PENDING.items()): + tip = branch_ref(repo, branch) + if tip is None: + unresolved.append(f"{rel}: branch {branch!r} not found") + continue + theirs = read_on_branch(repo, tip, f"{NDI_COMMON_REL.as_posix()}/{rel}") + if theirs is None: + offenders.append(f"{rel}: not on {branch!r}") + elif theirs != (python_common / rel).read_bytes(): + offenders.append(f"{rel}: differs from its copy on {branch!r}") + elif is_merged(repo, tip): + offenders.append(f"{rel}: {branch!r} has merged; remove it from MATLAB_BRANCH_PENDING") + if unresolved and not os.environ.get(STRICT_ENV_VAR, "").strip(): + pytest.skip( + "this NDI-matlab checkout lacks branches MATLAB_BRANCH_PENDING names " + "(fetch them, or clone with full history): " + "; ".join(unresolved) + ) + problems = offenders + unresolved + assert not problems, ( + "MATLAB_BRANCH_PENDING entries do not hold up:\n " + + "\n ".join(problems) + + "\n\nA branch that is not found was usually merged and deleted: check the " + "file is on NDI-matlab main and remove its MATLAB_BRANCH_PENDING entry." ) From 26f859a562b961ed9b1c09680fa306469afe5f10 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:33:01 +0000 Subject: [PATCH 07/21] Bridge: record branch-pending MATLAB files; clear decodeJwt and lightsheet drift - matlab_branch on resolveOrganizationId, LightsheetZarrManager and the eight +ndi/+fun/+doc/+lightsheet entries. - fromOMEZarr / makePyramid / chooseTileShape: examined against 5c9e8a83 on that branch (default tile budget 8 MB -> 32 MB). No Python counterpart yet, nothing to follow; noted in each decision_log. - decodeJwt: examined against 4a571dce (NDI-matlab#1024, Java base64 decoder replaced by matlab.net.base64decode). Python uses the stdlib; nothing to follow. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- src/ndi/cloud/ndi_matlab_python_bridge.yaml | 10 ++++- .../ndi_matlab_python_bridge.yaml | 38 +++++++++++++++++-- .../ndi_matlab_python_bridge.yaml | 1 + 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/ndi/cloud/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/ndi_matlab_python_bridge.yaml index 80046115..77f35ad2 100644 --- a/src/ndi/cloud/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/ndi_matlab_python_bridge.yaml @@ -272,7 +272,7 @@ functions: - name: decodeJwt matlab_path: "+ndi/+cloud/+internal/decodeJwt.m" - matlab_last_sync_hash: "7ab96514" + matlab_last_sync_hash: "4a571dce" python_path: "ndi/cloud/auth.py" input_arguments: - name: token @@ -284,6 +284,13 @@ functions: decision_log: > MATLAB places this in +internal; Python places it in auth.py since it is tightly coupled with authentication logic. Name exact match. + 2026-10-12: examined against 4a571dce (was 7ab96514), NDI-matlab#1024. + MATLAB swapped javax.xml.bind.DatatypeConverter.parseBase64Binary, + which recent MATLAB releases no longer ship, for + matlab.net.base64decode. Same base64url-to-base64 translation and + padding before it, same bytes out. Nothing to follow: Python decodes + with the standard library's base64 module and never had the Java + dependency. - name: getTokenExpiration matlab_path: "+ndi/+cloud/+internal/getTokenExpiration.m" @@ -1610,6 +1617,7 @@ not_yet_ported: - name: resolveOrganizationId matlab_path: "+ndi/+cloud/+internal/resolveOrganizationId.m" matlab_last_sync_hash: "f847d7c4" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: > MATLAB helper introduced with NDI-matlab#979: given diff --git a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml index b30f2eaf..b6545fc4 100644 --- a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml @@ -31,7 +31,8 @@ functions: - name: fromOMEZarr matlab_path: "+ndi/+fun/+doc/+lightsheet/fromOMEZarr.m" - matlab_last_sync_hash: "71d7387" + matlab_last_sync_hash: "5c9e8a83" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- MATLAB probes an OME-Zarr store with ndr.format.omezarr.listPyramids, @@ -47,10 +48,19 @@ functions: database_add -- the Python port lands the same option and calls into ndr.format.omezarr.readArray equivalent. Ports with the other entries under this heading. + 2026-10-12: examined against 5c9e8a83 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 71d7387). It raises + the default tile budget from 8*2^20 to 32*2^20 bytes + (signing/fetch/decode measurements on a whole-brain volume) and + updates the doc comments to match. Nothing on the Python side to + follow: this function has no Python counterpart yet, and the + Python viewer only reads the levels MATLAB wrote. The Python port + must use the 32 MB default when it lands. - name: makePyramid matlab_path: "+ndi/+fun/+doc/+lightsheet/makePyramid.m" - matlab_last_sync_hash: "8134a20" + matlab_last_sync_hash: "5c9e8a83" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- The worker fromOMEZarr calls; writes one lightsheetZarrPyramid @@ -81,10 +91,19 @@ functions: is never touched). Container bytes are unchanged, so this side of the port stays the same when it lands: use numcodecs.Blosc directly. Ports with the other entries. + 2026-10-12: examined against 5c9e8a83 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 8134a20). It raises + the default tile budget from 8*2^20 to 32*2^20 bytes + (signing/fetch/decode measurements on a whole-brain volume) and + updates the doc comments to match. Nothing on the Python side to + follow: this function has no Python counterpart yet, and the + Python viewer only reads the levels MATLAB wrote. The Python port + must use the 32 MB default when it lands. - name: chooseTileShape matlab_path: "+ndi/+fun/+doc/+lightsheet/chooseTileShape.m" - matlab_last_sync_hash: "1d11302" + matlab_last_sync_hash: "5c9e8a83" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Pure function that picks an OME-Zarr chunk shape landing near a @@ -102,10 +121,19 @@ functions: in axes_order.lower()]`. Ports with the other entries under this heading; the Python builder that mirrors makePyramid will call it directly. + 2026-10-12: examined against 5c9e8a83 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 1d11302). It raises + the default tile budget from 8*2^20 to 32*2^20 bytes + (signing/fetch/decode measurements on a whole-brain volume) and + updates the doc comments to match. Nothing on the Python side to + follow: this function has no Python counterpart yet, and the + Python viewer only reads the levels MATLAB wrote. The Python port + must use the 32 MB default when it lands. - name: makeSourceFile matlab_path: "+ndi/+fun/+doc/+lightsheet/makeSourceFile.m" matlab_last_sync_hash: "1af66bb" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- MATLAB creates the fileReference document the lightsheet documents' @@ -120,6 +148,7 @@ functions: - name: chooseLevel matlab_path: "+ndi/+fun/+doc/+lightsheet/chooseLevel.m" matlab_last_sync_hash: "5ab84f3" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Given a level table and a target per-axis voxel size, returns the @@ -130,6 +159,7 @@ functions: - name: levelTable matlab_path: "+ndi/+fun/+doc/+lightsheet/levelTable.m" matlab_last_sync_hash: "1af66bb" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Joins a lightsheetZarrPyramid and its lightsheetZarrLevel children @@ -145,6 +175,7 @@ functions: - name: viewCommand matlab_path: "+ndi/+fun/+doc/+lightsheet/viewCommand.m" matlab_last_sync_hash: "5ab84f3" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- Pure command builder that ndi.gui.app.LightsheetZarrManager calls @@ -160,6 +191,7 @@ functions: - name: view matlab_path: "+ndi/+fun/+doc/+lightsheet/view.m" matlab_last_sync_hash: "a7ba07a" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- MATLAB caller that pairs viewCommand + SYSTEM: builds the diff --git a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml index 764100e6..849e37ca 100644 --- a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml +++ b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml @@ -36,6 +36,7 @@ functions: - name: LightsheetZarrManager matlab_path: "+ndi/+gui/+app/LightsheetZarrManager.m" matlab_last_sync_hash: "60a79cf0" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: matlab_only decision_log: >- matlab_only: napari is Python-only, so the manager class stays From 87750993b08dac4e65a0b6006852219bf84f8440 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 00:33:01 +0000 Subject: [PATCH 08/21] Document matlab_branch in the bridge reference and porting guide Section 8 of docs/developer_notes/ndi_matlab_python_bridge.yaml defines when an entry may name an NDI-matlab branch and how the checks hold it to account; the field table and the porting guide point there. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01B22A1gLVKa1E3uQm3tv4t8 --- docs/developer_notes/PYTHON_PORTING_GUIDE.md | 2 + .../ndi_matlab_python_bridge.yaml | 57 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/docs/developer_notes/PYTHON_PORTING_GUIDE.md b/docs/developer_notes/PYTHON_PORTING_GUIDE.md index 2b36e99b..01ea8e8b 100644 --- a/docs/developer_notes/PYTHON_PORTING_GUIDE.md +++ b/docs/developer_notes/PYTHON_PORTING_GUIDE.md @@ -31,6 +31,8 @@ To port or update a function, agents must follow these steps: **It must be a commit, never a blob.** `git hash-object ` returns the hash *of the file's contents*, which looks equally plausible in the YAML and is useless here: the freshness check works by walking history (`git log ..HEAD -- `), and a blob is not a point in history to walk from. 35 entries carried blob hashes before this was written down, because "the hash of the MATLAB file" reads naturally as either one. A test now rejects anything that is not a commit. Either kind of commit is acceptable: the file'''s own last-touching commit (what the command above gives), or a repo-wide commit such as NDI-matlab `HEAD` when you examined a batch of files together. Both are points in history, so both let the check ask "has this file moved since?" + + If the MATLAB file is on an NDI-matlab branch that has not merged into main yet, record a commit on that branch and add `matlab_branch: ` to the entry. The checks then verify the entry against that branch and ask for the field to be removed once it merges. Section 8 of `docs/developer_notes/ndi_matlab_python_bridge.yaml` defines when this applies; do not use it for anything else. 4. **Implement:** Write the Python code to satisfy the `input_arguments` and `output_arguments` defined in the YAML. If you are NOT porting it, the entry needs a `status` and a `decision_log` instead. The five permitted values, what each claims, and how to choose between them are defined in section 6 of `docs/developer_notes/ndi_matlab_python_bridge.yaml` — that is the normative list, and this guide deliberately does not repeat it. diff --git a/docs/developer_notes/ndi_matlab_python_bridge.yaml b/docs/developer_notes/ndi_matlab_python_bridge.yaml index 943d790c..82aa8d65 100644 --- a/docs/developer_notes/ndi_matlab_python_bridge.yaml +++ b/docs/developer_notes/ndi_matlab_python_bridge.yaml @@ -105,6 +105,9 @@ project_metadata: # | commit, or a repo-wide commit for a batch sync. Both are # | points in history. NEVER a blob hash (git hash-object): # | enforced by tests/test_matlab_bridge_hashes.py. +# matlab_branch | OPTIONAL. The NDI-matlab branch the MATLAB file lives on +# | when it has not merged into NDI-matlab main yet. Only +# | for that case, and removed once it merges. Section 8. # status | One of the five values in section 6 below, or ABSENT. # | Absent + a python_path is the normal case and means # | ported. Enforced by tests/test_matlab_bridge_status.py. @@ -335,3 +338,57 @@ project_metadata: # drifts, so a fix must delist in the same PR. Start with the 18 whose MATLAB # change touched a signature or an arguments block; that is where a stale hash # is most likely hiding a real defect. + +# ============================================================================= +# 8. MATLAB ON A BRANCH: a counterpart that has not reached NDI-matlab main +# ============================================================================= +# Some work lands here before its MATLAB half reaches NDI-matlab main. The +# lightsheet OME-Zarr feature is the case that forced this: the napari viewer, +# its two document types and the bridge entries for the MATLAB writer merged +# into NDI-python while VH-Lab/NDI-matlab claude/lightsheet-zarr-ndi-viewer-djp5vk +# was still open. CI checks out NDI-matlab main (or a branch named like this +# repo's), so every such entry read as a stale path -- "names a MATLAB file +# that does not exist" -- and the bridge job stayed red on main. Neither +# existing escape fits: `matlab_path: "N/A"` / `retired` say the MATLAB file is +# gone or never was, which is false, and would have to be undone by hand the +# day the branch merges. +# +# SO THE ENTRY SAYS WHERE THE FILE IS: +# +# - name: makePyramid +# matlab_path: "+ndi/+fun/+doc/+lightsheet/makePyramid.m" +# matlab_last_sync_hash: "5c9e8a83" # a commit ON that branch +# matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" +# status: porting_deferred +# +# Everything else about the entry -- status, decision_log, python_path -- means +# what it always does. An ndi_common file in the same position goes in +# MATLAB_BRANCH_PENDING in tests/test_ndi_common_matlab_sync.py, mapped to its +# branch. +# +# IT IS A CLAIM, AND IT IS CHECKED -- otherwise it would be a way to switch the +# stale-path guard off for good. Read from the branch with git, so it needs +# NDI-matlab's remote branches (fetch-depth: 0, which the bridge job has): +# * the branch must exist and the file must be on it +# (TestBranchPendingEntries in tests/test_matlab_bridge_completeness.py); +# an ndi_common file must also match its copy on the branch byte-for-byte +# (test_branch_pending_files_match_their_branch); +# * DRIFT STILL APPLIES (section 7), measured against the branch tip instead +# of HEAD: a commit on the branch touching the file after the recorded +# hash fails CI exactly as it would on main; +# * ONCE THE BRANCH MERGES INTO NDI-matlab main, the marker must go: the +# check fails, asking for matlab_branch (or the MATLAB_BRANCH_PENDING line) +# to be removed. A branch that has vanished fails the same way -- it was +# usually merged and deleted. Removing the marker is then all it takes: +# the file is on main and the ordinary checks take over. +# +# "Merged" means merged into NDI-matlab main, not into whatever is checked +# out. When CI checks out the pending branch itself (an NDI-python branch of +# the same name), the file is simply in the tree and is checked as usual. +# +# DO NOT USE IT FOR A FILE THAT IS ON MAIN, or to record an entry against a +# branch because main has moved and the drift is inconvenient. It answers one +# question -- "where is this MATLAB file, if not on main?" -- and nothing else. +# +# The checks are in tests/_matlab_branch.py and are themselves tested, against +# a throwaway git repo, in tests/test_matlab_branch_pending.py. From e89db4d8e4e89d8c96e53aa20e6d368a47c1db54 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 01:01:13 +0000 Subject: [PATCH 09/21] lightsheet: "Report a problem..." button in the NDI Cloud panel A beta viewer's problems mostly go unreported: a user launched from MATLAB has no terminal to copy a log from, and writing an email with versions and context is too much. The NDI Cloud panel now has a "Report a problem..." button under the BETA badge. The dialog asks what kind of problem and what the user saw, shows exactly what will be sent, and sends it through ndi.cloud.api.feedback.submitFeedback; Save to file and Copy cover a user who is not signed in or offline. New shared module ndi.gui.feedback (the gene-pyramid viewer can reuse it): * LogBuffer / installLogBuffer: tees stderr and the logging root into a ring of the last 5,000 lines from launch on. * scrub: removes signed-URL credentials (X-Amz-Signature/Credential/ Security-Token), bearer tokens, JWTs and secret-named values -- viewer logs do carry signed URLs. * buildReport: the submitFeedback kwargs, scrubbed, with versions and NDI_* flags as environment, cut to the server's size limits (log keeps its END). * sendReport / saveReport / describeSendError and the Qt dialog (send runs off the UI thread; one report per dialog; errors say what to do). The viewer's collector attaches the pyramid and level geometry, the view tally, each layer's level and region, fetch counts, loader stats and the log, each probe guarded so a report about a broken viewer still builds. _FetchCounter now resolves sys.stderr at print time so its lines reach the log buffer. Checked in real napari: the button docks, the dialog's report carries the view tally, layer level, document id and log, and a signed-URL signature in the log is scrubbed. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/gui/app/lightsheetZarr/viewer.py | 144 +++++- src/ndi/gui/feedback.py | 543 +++++++++++++++++++++++ tests/test_gui_feedback.py | 261 +++++++++++ 3 files changed, 945 insertions(+), 3 deletions(-) create mode 100644 src/ndi/gui/feedback.py create mode 100644 tests/test_gui_feedback.py diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index 3a6002cf..0a88991d 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -193,12 +193,15 @@ class _FetchCounter: def __init__( self, - out=sys.stderr, + out=None, interval: float = 0.5, idle_interval: float = 5.0, status_reporter=None, ): self._lock = threading.Lock() + # None: resolve sys.stderr at print time (see _ls), so a log tee + # installed after import -- the feedback report's log buffer -- + # still sees the counter's lines. self._out = out self._interval = interval self._idle_interval = idle_interval @@ -531,6 +534,16 @@ def openPyramid( # picks GUI on macOS/Windows and where a display is set. from . import progress + # Keep the last few thousand log lines in memory from here on, so a + # "Report a problem" from the NDI Cloud panel can attach them -- a + # viewer launched from MATLAB has no terminal to copy them from. + try: + from ndi.gui.feedback import installLogBuffer + + installLogBuffer() + except Exception: # noqa: BLE001 - reporting is never worth the launch + pass + progress.note("opening lightsheet pyramid ...") napari = require_napari() @@ -843,7 +856,12 @@ def openPyramid( # login control. The lightsheet viewer carries a "beta" badge # under the wordmark because this surface is still rough on # the dataset-shape edges called out in the README. - _attach_cloud_panel(viewer) + _attach_cloud_panel( + viewer, + report_collect=_reportCollector( + viewer, session, pyramid_doc, loader, counter, added_layers + ), + ) # Tiles needed for the current view vs tiles on disk, polled on a # main-thread timer and fed to the counter, so the status bar and @@ -1221,7 +1239,102 @@ def _unhide(): _CLOUD_PANELS: dict[int, Any] = {} -def _attach_cloud_panel(viewer) -> None: +def _reportCollector(viewer, session, pyramid_doc, loader, counter, layers): + """A zero-argument callable returning ``buildReport`` extras. + + Everything a "the view is stuck / slow / wrong" report needs that + the user cannot be expected to type: which pyramid, which level and + region napari is drawing, how far the view's tiles have got, what + the fetcher has done, and the recent log. Each probe is guarded on + its own: a report about a broken viewer must not fail to build + because the viewer is broken. + """ + + def collect() -> dict: + state: dict[str, Any] = {} + + def probe(key, fn): + try: + state[key] = fn() + except Exception as exc: # noqa: BLE001 + state[key] = f"" + + probe( + "pyramid", + lambda: { + "id": ( + str(pyramid_doc.id()) + if callable(getattr(pyramid_doc, "id", None)) + else str(getattr(pyramid_doc, "id", "")) + ), + "levels": [ + { + k: d.document_properties["lightsheetZarrLevel"].get(k) + for k in ("shape", "chunks", "chunk_grid", "dtype", "codec", "axes_order") + } + for d in loader.docs + ], + }, + ) + probe( + "view", + lambda: { + "ready": counter._view_ready, + "needed": counter._view_needed, + "dims_point": [float(x) for x in viewer.dims.point], + "ndisplay": int(viewer.dims.ndisplay), + "camera_zoom": float(viewer.camera.zoom), + "camera_center": [float(x) for x in viewer.camera.center], + }, + ) + probe( + "layers", + lambda: [ + { + "name": layer.name, + "visible": bool(layer.visible), + "data_level": int(getattr(layer, "data_level", -1)), + "corner_pixels": [[int(v) for v in row] for row in layer.corner_pixels], + "loaded": bool(getattr(layer, "loaded", True)), + } + for layer in layers + ], + ) + probe( + "fetches", + lambda: { + "requested": counter._started, + "loaded": counter._done, + "in_flight": counter._started - counter._done, + "last_fetch_s": counter._last_duration, + }, + ) + probe("loader", loader.stats) + + extra: dict[str, Any] = {"state": state} + try: + extra["datasetId"] = _cloud_dataset_id(session) or None + except Exception: # noqa: BLE001 + pass + try: + doc_id = ( + pyramid_doc.id() if callable(getattr(pyramid_doc, "id", None)) else pyramid_doc.id + ) + extra["documentId"] = str(doc_id) + except Exception: # noqa: BLE001 + pass + try: + from ndi.gui.feedback import installLogBuffer + + extra["log"] = installLogBuffer().text() + except Exception: # noqa: BLE001 + pass + return {k: v for k, v in extra.items() if v is not None} + + return collect + + +def _attach_cloud_panel(viewer, report_collect=None) -> None: """Dock an NDI Cloud sign-in panel with a BETA badge. Reuses the gene-pyramid viewer's addCloudPanel contract -- same @@ -1316,6 +1429,31 @@ def _attach_cloud_panel(viewer) -> None: beta_row.addStretch(1) outer.addLayout(beta_row) + if report_collect is not None: + report = QPushButton("Report a problem...") + report.setToolTip( + "Tell us what went wrong. The viewer attaches its own\n" + "diagnostics (versions, what it was loading, recent log\n" + "lines) with sign-in tokens and signed links removed,\n" + "and you can see exactly what will be sent." + ) + dialogs: list = [] # keep the non-modal dialog alive + + def _report(): + from ndi.gui.feedback import openFeedbackDialog + + dialogs[:] = [ + openFeedbackDialog( + box, + app="lightsheetZarr-viewer", + collect=report_collect, + title="Report a problem with the lightsheet viewer", + ) + ] + + report.clicked.connect(_report) + outer.addWidget(report) + signin = QPushButton("Sign in...") signin.setToolTip( "Sign in to NDI Cloud, so tiles that are not already\n" diff --git a/src/ndi/gui/feedback.py b/src/ndi/gui/feedback.py new file mode 100644 index 00000000..d6f58d4e --- /dev/null +++ b/src/ndi/gui/feedback.py @@ -0,0 +1,543 @@ +"""Report a problem from inside a viewer. + +A beta viewer's problems mostly go unreported: asking a user to find a +log, copy versions and write an email is too much. This module turns +that into one dialog -- the user says what they were doing, the viewer +attaches what it knows, and the report goes to NDI Cloud through +:func:`ndi.cloud.api.feedback.submitFeedback` (the user is already +signed in for the tiles). Save-to-file and Copy cover a user who is +not signed in, or offline. + +Pieces, each usable on its own: + +* :class:`LogBuffer` -- keeps the last lines written to stderr (and to + the ``logging`` root) so a report can carry them without the user + ever having seen a terminal. +* :func:`scrub` -- strips credentials before anything leaves the + machine. Viewer logs DO carry them: signed S3 URLs embed a signature + and session token, and auth lines can carry a bearer token. +* :func:`buildReport` -- the ``submitFeedback`` keyword arguments, + scrubbed and cut to the server's size limits. +* :func:`sendReport` / :func:`saveReport` -- deliver it. +* :class:`FeedbackDialog` -- the Qt dialog (imported lazily). + +The payload field names are a contract with NDI-matlab and the server +(Waltham-Data-Science/ndi-cloud-node#158). +""" + +from __future__ import annotations + +import collections +import json +import logging +import os +import platform +import re +import sys +import threading +from collections.abc import Callable +from typing import Any + +__all__ = [ + "LogBuffer", + "installLogBuffer", + "scrub", + "buildReport", + "sendReport", + "saveReport", + "describeSendError", + "FeedbackDialog", + "openFeedbackDialog", +] + +#: Server limits (characters / JSON bytes). Reports are cut to fit, not +#: refused: a truncated log is worth more than no report. +MAX_DESCRIPTION = 20_000 +MAX_LOG = 500_000 +MAX_TRACEBACK = 50_000 +MAX_ENVIRONMENT_JSON = 20_000 +MAX_STATE_JSON = 200_000 + +#: Lines the log buffer keeps. Generous: a stuck-view report wants the +#: minutes before the user gave up, and the log is cut to MAX_LOG anyway. +LOG_LINES = 5_000 + +_REDACTED = "[REDACTED]" + +# Order matters: whole signed-URL query strings first, then the bits +# that can appear on their own. +_SCRUB_PATTERNS: list[tuple[re.Pattern, str]] = [ + # AWS signed-URL credentials (query params, any case). + ( + re.compile( + r"(?i)\b(X-Amz-(?:Signature|Credential|Security-Token)|Signature|" + r"AWSAccessKeyId|x-amz-security-token)=[^&\s\"'<>]+" + ), + rf"\1={_REDACTED}", + ), + # Authorization headers / bearer tokens. + (re.compile(r"(?i)\b(Bearer)\s+[A-Za-z0-9._~+/=-]+"), rf"\1 {_REDACTED}"), + # JWTs anywhere (header.payload.signature, base64url). + (re.compile(r"\beyJ[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]+"), _REDACTED), + # key=value / "key": "value" for anything named like a secret. + ( + re.compile( + r"(?i)([\"']?\b[\w-]*(?:token|password|passwd|secret|api[_-]?key)[\w-]*[\"']?" + r"\s*[:=]\s*[\"']?)([^\s\"',&}]+)" + ), + rf"\1{_REDACTED}", + ), +] + + +def scrub(text: str) -> str: + """Remove credentials from TEXT. Safe on any string.""" + if not text: + return text + for pattern, repl in _SCRUB_PATTERNS: + text = pattern.sub(repl, text) + return text + + +def _scrubValue(value: Any) -> Any: + """Scrub every string inside a JSON-like value; drop secret-named keys.""" + if isinstance(value, str): + return scrub(value) + if isinstance(value, dict): + out = {} + for k, v in value.items(): + key = str(k) + if re.search(r"(?i)token|password|passwd|secret|api[_-]?key", key): + out[key] = _REDACTED + else: + out[key] = _scrubValue(v) + return out + if isinstance(value, (list, tuple)): + return [_scrubValue(v) for v in value] + if value is None or isinstance(value, (bool, int, float)): + return value + return scrub(str(value)) + + +# ------------------------------------------------------------------ log buffer + + +class LogBuffer: + """The last :data:`LOG_LINES` lines written to stderr and to logging. + + Installed as a tee in front of ``sys.stderr``: everything still + reaches the real stream, and a copy of each complete line lands in + a ring buffer. A ``\\r``-redrawn progress line keeps only its last + state. Thread-safe; fetcher threads print too. + """ + + def __init__(self, max_lines: int = LOG_LINES): + self._lines: collections.deque[str] = collections.deque(maxlen=max_lines) + self._partial = "" + self._lock = threading.Lock() + + def feed(self, text: str) -> None: + if not text: + return + with self._lock: + data = self._partial + text + parts = data.split("\n") + self._partial = parts.pop() + for line in parts: + line = line.rsplit("\r", 1)[-1] + if line.strip(): + self._lines.append(line) + # A long run of \r redraws never ends in \n; keep only the + # current state so it cannot grow without bound. + self._partial = self._partial.rsplit("\r", 1)[-1][-10_000:] + + def text(self) -> str: + with self._lock: + lines = list(self._lines) + if self._partial.strip(): + lines.append(self._partial) + return "\n".join(lines) + + +class _Tee: + """sys.stderr stand-in: write through, and copy into a LogBuffer.""" + + def __init__(self, stream, buffer: LogBuffer): + self._stream = stream + self._buffer = buffer + + def write(self, text): + try: + self._buffer.feed(text) + except Exception: # noqa: BLE001 - the log must never break printing + pass + return self._stream.write(text) + + def flush(self): + return self._stream.flush() + + def __getattr__(self, name): + return getattr(self._stream, name) + + +class _BufferHandler(logging.Handler): + def __init__(self, buffer: LogBuffer): + super().__init__(level=logging.INFO) + self._buffer = buffer + self.setFormatter(logging.Formatter("%(asctime)s %(levelname)s %(name)s: %(message)s")) + + def emit(self, record): + try: + self._buffer.feed(self.format(record) + "\n") + except Exception: # noqa: BLE001 + pass + + +_installed: LogBuffer | None = None + + +def installLogBuffer() -> LogBuffer: + """Start buffering stderr and logging for reports. Idempotent. + + Logging records reach the buffer through their own handler, since + a logging StreamHandler holds the stderr it was built with and would + bypass the tee. A record that ALSO prints to stderr after the tee is + installed can appear twice; harmless in a diagnostic log. + """ + global _installed + if _installed is not None: + return _installed + buf = LogBuffer() + if not isinstance(sys.stderr, _Tee): + sys.stderr = _Tee(sys.stderr, buf) + logging.getLogger().addHandler(_BufferHandler(buf)) + _installed = buf + return buf + + +# ------------------------------------------------------------------ report + + +def _versions() -> dict[str, Any]: + env: dict[str, Any] = { + "os": f"{platform.system()} {platform.release()}", + "platform": platform.platform(), + "machine": platform.machine(), + "python": sys.version.split()[0], + } + try: + import ndi + + env["ndi"] = str(getattr(ndi, "__version__", "?")) + except Exception: # noqa: BLE001 + pass + for mod in ("napari", "numpy", "dask", "qtpy"): + m = sys.modules.get(mod) + if m is not None: + env[mod] = str(getattr(m, "__version__", "?")) + try: + import qtpy + + env["qtApi"] = str(getattr(qtpy, "API_NAME", "?")) + env["qt"] = str(getattr(qtpy, "QT_VERSION", "?")) + except Exception: # noqa: BLE001 + pass + return env + + +def _envFlags(prefixes: tuple[str, ...]) -> dict[str, str]: + return {k: v for k, v in sorted(os.environ.items()) if k.startswith(prefixes)} + + +def _capJson(value: dict, limit: int, what: str) -> dict: + """VALUE if its JSON fits LIMIT bytes, else a note saying it was cut.""" + try: + raw = json.dumps(value, default=str) + except Exception as exc: # noqa: BLE001 + return {"_note": f"{what} could not be serialised ({exc})"} + if len(raw.encode("utf-8")) <= limit: + return json.loads(raw) + return { + "_note": f"{what} was {len(raw):,} bytes, over the {limit:,} limit; truncated", + "_head": raw[: limit // 2], + } + + +def _tail(text: str | None, limit: int) -> str | None: + if text is None or len(text) <= limit: + return text + marker = f"[... {len(text) - limit:,} earlier characters cut ...]\n" + return marker + text[-(limit - len(marker)) :] + + +def buildReport( + app: str, + category: str, + description: str, + *, + appVersion: str | None = None, + datasetId: str | None = None, + documentId: str | None = None, + state: dict | None = None, + log: str | None = None, + traceback: str | None = None, + envPrefixes: tuple[str, ...] = ("NDI_",), +) -> dict[str, Any]: + """Keyword arguments for ``submitFeedback``, scrubbed and size-capped. + + ``environment`` is filled in here (OS, Python, NDI and GUI library + versions, and the ``NDI_*`` environment flags -- with any secret- + named ones redacted). Everything the user did not type is scrubbed + of credentials; the description is the user's own words and is + sent as typed (cut to the limit). + """ + if appVersion is None: + appVersion = f"NDI-python {_versions().get('ndi', '?')}" + environment = _versions() + environment["flags"] = _envFlags(envPrefixes) + report: dict[str, Any] = { + "app": app, + "category": category, + "description": (description or "")[:MAX_DESCRIPTION], + "appVersion": appVersion, + "environment": _capJson(_scrubValue(environment), MAX_ENVIRONMENT_JSON, "environment"), + } + if datasetId: + report["datasetId"] = str(datasetId) + if documentId: + report["documentId"] = str(documentId) + if state is not None: + report["state"] = _capJson(_scrubValue(state), MAX_STATE_JSON, "state") + if log: + report["log"] = _tail(scrub(log), MAX_LOG) + if traceback: + report["traceback"] = _tail(scrub(traceback), MAX_TRACEBACK) + return report + + +def sendReport(report: dict[str, Any], *, client=None) -> str: + """Submit REPORT; return the stored report's id. Raises on failure.""" + from ndi.cloud.api.feedback import submitFeedback + + kwargs = dict(report) + if client is not None: + kwargs["client"] = client + b, feedback_id, _status, _url = submitFeedback(**kwargs) + return str(feedback_id) + + +def describeSendError(exc: BaseException) -> str: + """One sentence a user can act on, for a failed send.""" + try: + from ndi.cloud.exceptions import CloudAPIError, CloudAuthError + except Exception: # noqa: BLE001 + CloudAPIError = CloudAuthError = () # type: ignore[assignment] + if CloudAuthError and isinstance(exc, CloudAuthError): + return ( + "Not signed in to NDI Cloud. Sign in, then send again -- or save the report to a file." + ) + status = getattr(exc, "status_code", None) + if CloudAPIError and isinstance(exc, CloudAPIError) and status is not None: + if status == 429: + return "Too many reports this hour. Save it to a file and send it later." + if status == 413: + return "The report is too large for the server. Save it to a file instead." + if status == 404: + return "This NDI Cloud server does not accept reports yet. Save it to a file instead." + return f"NDI Cloud refused the report (HTTP {status}). Save it to a file instead." + return f"Could not send the report ({type(exc).__name__}: {exc}). Save it to a file instead." + + +def saveReport(report: dict[str, Any], path: str) -> None: + """Write REPORT as JSON to PATH (schema as sent to the server).""" + from ndi.cloud.api.feedback import FEEDBACK_SCHEMA_VERSION + + body = {"schemaVersion": FEEDBACK_SCHEMA_VERSION, **report} + with open(path, "w", encoding="utf-8") as f: + json.dump(body, f, indent=2, default=str) + + +# ------------------------------------------------------------------ dialog + + +def FeedbackDialog( # noqa: N802 - a class factory; Qt is imported lazily + parent=None, + *, + app: str, + collect: Callable[[], dict[str, Any]], + title: str = "Report a problem", + send: Callable[[dict[str, Any]], str] = sendReport, +): + """Build the report dialog. COLLECT returns the non-text fields. + + COLLECT is called when the dialog opens (and again on Send/Save so + the attached state is current) and returns ``buildReport`` keyword + arguments other than app/category/description: ``state``, ``log``, + ``datasetId``, ``documentId``, ``traceback``, ... + """ + from qtpy.QtCore import Qt, QTimer + from qtpy.QtWidgets import ( + QApplication, + QComboBox, + QDialog, + QFileDialog, + QHBoxLayout, + QLabel, + QPlainTextEdit, + QPushButton, + QToolButton, + QVBoxLayout, + ) + + class _Dialog(QDialog): + CATEGORIES = [("Something is broken", "bug"), ("Slow", "performance"), ("Other", "other")] + + def __init__(self): + super().__init__(parent) + self.setWindowTitle(title) + self.resize(560, 480) + self.feedbackID: str | None = None + self._future = None + + lay = QVBoxLayout(self) + intro = QLabel( + "What were you doing, and what did you see? The viewer attaches " + "its own diagnostics (versions, what it was loading, recent log " + "lines) with sign-in tokens and signed links removed." + ) + intro.setWordWrap(True) + lay.addWidget(intro) + + row = QHBoxLayout() + row.addWidget(QLabel("Kind of problem:")) + self.category = QComboBox() + for label, value in self.CATEGORIES: + self.category.addItem(label, value) + row.addWidget(self.category, 1) + lay.addLayout(row) + + self.description = QPlainTextEdit() + self.description.setPlaceholderText( + "e.g. I zoomed into the cortex at Z 1921 and the view stayed blurry " + "for over a minute." + ) + lay.addWidget(self.description, 2) + + self.toggle = QToolButton() + self.toggle.setText("Show what will be sent") + self.toggle.setCheckable(True) + self.toggle.setToolButtonStyle(Qt.ToolButtonTextBesideIcon) + self.toggle.setArrowType(Qt.RightArrow) + lay.addWidget(self.toggle, 0, Qt.AlignLeft) + self.preview = QPlainTextEdit() + self.preview.setReadOnly(True) + self.preview.hide() + lay.addWidget(self.preview, 3) + self.toggle.toggled.connect(self._togglePreview) + + self.status = QLabel("") + self.status.setWordWrap(True) + self.status.setTextInteractionFlags(Qt.TextSelectableByMouse) + lay.addWidget(self.status) + + buttons = QHBoxLayout() + self.saveButton = QPushButton("Save to file...") + self.copyButton = QPushButton("Copy") + buttons.addWidget(self.saveButton) + buttons.addWidget(self.copyButton) + buttons.addStretch(1) + self.cancelButton = QPushButton("Close") + self.sendButton = QPushButton("Send") + self.sendButton.setDefault(True) + buttons.addWidget(self.cancelButton) + buttons.addWidget(self.sendButton) + lay.addLayout(buttons) + + self.description.textChanged.connect(self._sync) + self.sendButton.clicked.connect(self._send) + self.saveButton.clicked.connect(self._save) + self.copyButton.clicked.connect(self._copy) + self.cancelButton.clicked.connect(self.reject) + + self._poll = QTimer(self) + self._poll.setInterval(100) + self._poll.timeout.connect(self._checkSend) + self._sync() + + # -- report + def report(self) -> dict[str, Any]: + try: + extra = dict(collect() or {}) + except Exception as exc: # noqa: BLE001 - a broken probe is itself a finding + extra = {"state": {"_collectError": f"{type(exc).__name__}: {exc}"}} + return buildReport( + app, + self.category.currentData(), + self.description.toPlainText().strip() or "(no description)", + **extra, + ) + + def _togglePreview(self, on): + self.toggle.setArrowType(Qt.DownArrow if on else Qt.RightArrow) + self.toggle.setText("Hide what will be sent" if on else "Show what will be sent") + if on: + self.preview.setPlainText(json.dumps(self.report(), indent=2, default=str)) + self.preview.setVisible(on) + + def _sync(self): + has_text = bool(self.description.toPlainText().strip()) + busy = self._future is not None + self.sendButton.setEnabled(has_text and not busy and self.feedbackID is None) + + # -- deliver + def _send(self): + from concurrent.futures import ThreadPoolExecutor + + rep = self.report() + self.status.setText("Sending ...") + pool = ThreadPoolExecutor(max_workers=1, thread_name_prefix="ndi-feedback") + self._future = pool.submit(send, rep) + pool.shutdown(wait=False) + self._sync() + self._poll.start() + + def _checkSend(self): + fut = self._future + if fut is None or not fut.done(): + return + self._poll.stop() + self._future = None + exc = fut.exception() + if exc is None: + self.feedbackID = fut.result() + self.status.setText(f"Sent -- thank you. Report id: {self.feedbackID}") + self.cancelButton.setText("Done") + else: + self.status.setText(describeSendError(exc)) + self._sync() + + def _save(self): + path, _ = QFileDialog.getSaveFileName( + self, "Save problem report", "ndi-problem-report.json", "JSON (*.json)" + ) + if not path: + return + try: + saveReport(self.report(), path) + self.status.setText(f"Saved to {path}") + except Exception as exc: # noqa: BLE001 + self.status.setText(f"Could not save ({exc})") + + def _copy(self): + QApplication.clipboard().setText(json.dumps(self.report(), indent=2, default=str)) + self.status.setText("Report copied to the clipboard.") + + return _Dialog() + + +def openFeedbackDialog(parent=None, **kwargs): + """Build and show a :func:`FeedbackDialog` (non-modal); return it.""" + dlg = FeedbackDialog(parent, **kwargs) + dlg.show() + dlg.raise_() + return dlg diff --git a/tests/test_gui_feedback.py b/tests/test_gui_feedback.py new file mode 100644 index 00000000..cce77185 --- /dev/null +++ b/tests/test_gui_feedback.py @@ -0,0 +1,261 @@ +"""A user can report a problem from inside a viewer in one step. + +The report carries what the viewer knows (versions, state, recent log) +without carrying credentials -- viewer logs do contain signed S3 URLs +and can contain bearer tokens -- and fits the server's size limits. +""" + +import io +import json +import os +import sys + +import pytest + +from ndi.gui import feedback + +# ------------------------------------------------------------ scrub + + +class TestScrub: + def test_signed_url_credentials_are_removed(self): + url = ( + "https://bucket.s3.amazonaws.com/chunk.bin_7?X-Amz-Algorithm=AWS4-HMAC-SHA256" + "&X-Amz-Credential=ASIAXYZ%2F20261012&X-Amz-Security-Token=IQoJb3JpZ2lu" + "&X-Amz-Signature=deadbeef1234&X-Amz-Expires=3600" + ) + out = feedback.scrub(url) + for secret in ("ASIAXYZ", "IQoJb3JpZ2lu", "deadbeef1234"): + assert secret not in out + # The non-secret parts stay, so the log is still readable. + assert "chunk.bin_7" in out and "X-Amz-Expires=3600" in out + + def test_bearer_tokens_and_jwts_are_removed(self): + jwt = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NSJ9.c2lnbmF0dXJl" + out = feedback.scrub(f"Authorization: Bearer abc.def-123 and later {jwt}") + assert "abc.def-123" not in out and jwt not in out + + def test_secret_named_values_are_removed(self): + out = feedback.scrub('password=hunter2 "api_key": "k-999" NDI_CLOUD_TOKEN=xyz') + for secret in ("hunter2", "k-999", "xyz"): + assert secret not in out + + def test_ordinary_log_lines_survive(self): + line = "12:00:01.123 [lightsheet] tiles: 56 loaded / 112 requested (in flight: 8)" + assert feedback.scrub(line) == line + + +# ------------------------------------------------------------ log buffer + + +class TestLogBuffer: + def test_lines_are_kept_and_redraws_keep_their_last_state(self): + buf = feedback.LogBuffer(max_lines=10) + buf.feed("first line\n[lightsheet] 10%\r[lightsheet] 50%\r[lightsheet] 100%\nthird") + assert buf.text().splitlines() == ["first line", "[lightsheet] 100%", "third"] + + def test_only_the_newest_lines_are_kept(self): + buf = feedback.LogBuffer(max_lines=3) + buf.feed("".join(f"line {i}\n" for i in range(10))) + assert buf.text().splitlines() == ["line 7", "line 8", "line 9"] + + def test_the_tee_still_writes_through(self): + real = io.StringIO() + buf = feedback.LogBuffer() + tee = feedback._Tee(real, buf) + print("hello", file=tee) + assert real.getvalue() == "hello\n" + assert buf.text() == "hello" + + +# ------------------------------------------------------------ report + + +class TestBuildReport: + def test_payload_fields_match_the_api(self): + rep = feedback.buildReport( + "lightsheetZarr-viewer", + "performance", + "slow zoom", + datasetId="ds1", + documentId="doc1", + state={"view": {"ready": 56, "needed": 112}}, + log="a\nb", + ) + assert rep["app"] == "lightsheetZarr-viewer" + assert rep["category"] == "performance" + assert rep["description"] == "slow zoom" + assert rep["datasetId"] == "ds1" and rep["documentId"] == "doc1" + assert rep["state"] == {"view": {"ready": 56, "needed": 112}} + assert rep["log"] == "a\nb" + assert rep["appVersion"].startswith("NDI-python") + assert "python" in rep["environment"] + # Every key is one submitFeedback accepts. + import inspect + + from ndi.cloud.api.feedback import submitFeedback + + params = inspect.signature(submitFeedback).parameters + assert set(rep) <= set(params) + + def test_unset_optional_fields_are_left_out(self): + rep = feedback.buildReport("app", "bug", "d") + for key in ("datasetId", "documentId", "state", "log", "traceback"): + assert key not in rep + + def test_secrets_in_state_env_and_log_are_scrubbed(self, monkeypatch): + monkeypatch.setenv("NDI_CLOUD_PASSWORD", "hunter2") + monkeypatch.setenv("NDI_LIGHTSHEET_DEBUG", "1") + rep = feedback.buildReport( + "app", + "bug", + "d", + state={"url": "https://x/y?X-Amz-Signature=abc123", "token": "t0k"}, + log="Bearer sekrit", + ) + blob = json.dumps(rep) + for secret in ("hunter2", "abc123", "t0k", "sekrit"): + assert secret not in blob + assert rep["environment"]["flags"]["NDI_LIGHTSHEET_DEBUG"] == "1" + + def test_a_huge_log_is_cut_to_the_server_limit_keeping_the_end(self): + log = "".join(f"line {i}\n" for i in range(200_000)) + rep = feedback.buildReport("app", "bug", "d", log=log) + assert len(rep["log"]) <= feedback.MAX_LOG + assert rep["log"].rstrip().endswith("line 199999") + assert "earlier characters cut" in rep["log"] + + def test_an_oversized_state_is_replaced_by_a_note(self): + rep = feedback.buildReport("app", "bug", "d", state={"big": "x" * 300_000}) + assert "_note" in rep["state"] + assert len(json.dumps(rep["state"])) < feedback.MAX_STATE_JSON + 1000 + + +class TestSendAndSave: + def test_send_returns_the_stored_id(self, monkeypatch): + seen = {} + + def fake_submit(**kwargs): + seen.update(kwargs) + return (True, "6710aa", 201, "https://api/v1/feedback") + + import ndi.cloud.api.feedback as api + + monkeypatch.setattr(api, "submitFeedback", fake_submit) + rep = feedback.buildReport("app", "bug", "d") + assert feedback.sendReport(rep) == "6710aa" + assert seen["app"] == "app" + + def test_save_writes_the_report_with_its_schema_version(self, tmp_path): + path = tmp_path / "r.json" + feedback.saveReport(feedback.buildReport("app", "other", "d"), str(path)) + body = json.loads(path.read_text()) + assert body["schemaVersion"] == 1 and body["description"] == "d" + + @pytest.mark.parametrize( + "status, phrase", + [(429, "Too many"), (413, "too large"), (404, "does not accept"), (500, "HTTP 500")], + ) + def test_errors_read_as_something_a_user_can_act_on(self, status, phrase): + from ndi.cloud.exceptions import CloudAPIError + + assert phrase in feedback.describeSendError(CloudAPIError("x", status_code=status)) + + def test_not_signed_in_says_so(self): + from ndi.cloud.exceptions import CloudAuthError + + assert "Not signed in" in feedback.describeSendError(CloudAuthError("no")) + + +# ------------------------------------------------------------ dialog + + +@pytest.fixture +def qapp(): + os.environ.setdefault("QT_QPA_PLATFORM", "offscreen") + try: + from qtpy.QtWidgets import QApplication + except Exception: + pytest.skip("no Qt binding") + return QApplication.instance() or QApplication([]) + + +def _wait(qapp, cond, timeout=5.0): + import time + + end = time.monotonic() + timeout + while not cond() and time.monotonic() < end: + qapp.processEvents() + time.sleep(0.02) + return cond() + + +class TestDialog: + def test_send_is_off_until_the_user_says_something(self, qapp): + dlg = feedback.FeedbackDialog(app="a", collect=dict, send=lambda r: "id") + assert not dlg.sendButton.isEnabled() + dlg.description.setPlainText("it froze") + assert dlg.sendButton.isEnabled() + + def test_sending_reports_the_id_and_carries_the_collected_state(self, qapp): + sent = [] + dlg = feedback.FeedbackDialog( + app="lightsheetZarr-viewer", + collect=lambda: {"state": {"view": {"ready": 1, "needed": 2}}, "documentId": "p1"}, + send=lambda rep: sent.append(rep) or "6710aa", + ) + dlg.category.setCurrentIndex(1) # Slow + dlg.description.setPlainText("zoom is slow") + dlg.sendButton.click() + assert _wait(qapp, lambda: dlg.feedbackID == "6710aa") + assert "6710aa" in dlg.status.text() + assert not dlg.sendButton.isEnabled(), "one report per dialog" + (rep,) = sent + assert rep["category"] == "performance" + assert rep["state"]["view"] == {"ready": 1, "needed": 2} + assert rep["documentId"] == "p1" + + def test_a_failed_send_says_what_to_do_and_can_be_retried(self, qapp): + from ndi.cloud.exceptions import CloudAuthError + + def boom(rep): + raise CloudAuthError("no token") + + dlg = feedback.FeedbackDialog(app="a", collect=dict, send=boom) + dlg.description.setPlainText("x") + dlg.sendButton.click() + assert _wait(qapp, lambda: "Not signed in" in dlg.status.text()) + assert dlg.sendButton.isEnabled() + + def test_the_preview_shows_exactly_the_report(self, qapp): + dlg = feedback.FeedbackDialog( + app="a", collect=lambda: {"log": "Bearer sekrit"}, send=lambda r: "id" + ) + dlg.description.setPlainText("x") + dlg.toggle.setChecked(True) + shown = json.loads(dlg.preview.toPlainText()) + assert shown["app"] == "a" and "sekrit" not in shown["log"] + + def test_a_broken_collector_still_produces_a_report(self, qapp): + def broken(): + raise RuntimeError("viewer is wedged") + + dlg = feedback.FeedbackDialog(app="a", collect=broken, send=lambda r: "id") + dlg.description.setPlainText("x") + assert "viewer is wedged" in json.dumps(dlg.report()) + + +def test_installing_the_log_buffer_tees_stderr(monkeypatch): + monkeypatch.setattr(feedback, "_installed", None) + real = io.StringIO() + monkeypatch.setattr(sys, "stderr", real) + buf = feedback.installLogBuffer() + print("[lightsheet] hello", file=sys.stderr) + assert "[lightsheet] hello" in buf.text() + assert real.getvalue() == "[lightsheet] hello\n" + assert feedback.installLogBuffer() is buf + import logging + + for h in list(logging.getLogger().handlers): + if isinstance(h, feedback._BufferHandler): + logging.getLogger().removeHandler(h) From 1bb5cca3ff901378b931d29ecca12a279344506a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 12:25:28 +0000 Subject: [PATCH 10/21] genepyramid: "Report a problem..." button, shared with the lightsheet viewer The gene expression viewer gets the same report button as the lightsheet viewer. On a cloud session it sits on the NDI Cloud panel, where the reader signs in (which sending needs); a local session has no cloud panel, so it gets a small "Feedback" dock with just the button -- Save to file and Copy work without signing in. The button itself moves into ndi.gui.feedback.reportButton so both viewers share one definition (text, tooltip, dialog); the lightsheet panel now uses it. ndi.gui.feedback.napariState gives every viewer the same picture of the canvas (dims, camera, each layer's type, visibility, level and region); the gene viewer's collector adds the pyramid id, density mode, the gene selection size, the cloud dataset id and the recent log, and openPyramid starts the log buffer at launch. addCloudPanel and addAllPanels take an optional report_collect; without it nothing changes. Checked in real napari: one button in the cloud and in the local layout, and the report carries app, document id, layers and section. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/gui/app/genepyramid/controls.py | 50 +++++++++++++- src/ndi/gui/app/genepyramid/viewer.py | 55 +++++++++++++++ src/ndi/gui/app/lightsheetZarr/viewer.py | 31 +++------ src/ndi/gui/feedback.py | 86 ++++++++++++++++++++++++ tests/test_gui_genepyramid_controls.py | 57 ++++++++++++++++ 5 files changed, 255 insertions(+), 24 deletions(-) diff --git a/src/ndi/gui/app/genepyramid/controls.py b/src/ndi/gui/app/genepyramid/controls.py index 6b2049ae..af27da93 100644 --- a/src/ndi/gui/app/genepyramid/controls.py +++ b/src/ndi/gui/app/genepyramid/controls.py @@ -32,6 +32,7 @@ "addCellTypePanel", "addAllPanels", "addCloudPanel", + "addReportPanel", "cloudSignInDialog", "buildCloudSignInDialog", "cloudProfileChoices", @@ -1827,7 +1828,7 @@ def cloudLogoLabel(parent=None): return label -def addCloudPanel(viewer) -> Any: +def addCloudPanel(viewer, report_collect=None) -> Any: """A clock on the cloud token, and a button that renews it. A VIEWER OUTLIVES ITS TOKEN. Someone reading a section works for @@ -1844,6 +1845,10 @@ def addCloudPanel(viewer) -> Any: and the time left. The credentials live behind the button, in a dialog, rather than as three permanently docked fields for something a reader does once a day at most. + + REPORT_COLLECT, when given, adds the shared "Report a problem..." + button (:func:`ndi.gui.feedback.reportButton`): this is where a reader + is signed in, which is what sending a report needs. """ from qtpy.QtCore import QTimer from qtpy.QtWidgets import QHBoxLayout, QLabel, QPushButton, QVBoxLayout, QWidget @@ -1876,6 +1881,8 @@ def addCloudPanel(viewer) -> Any: result.setWordWrap(True) result.hide() outer.addWidget(result) + if report_collect is not None: + outer.addWidget(_reportButton(box, report_collect)) outer.addStretch() def _tick(): @@ -1915,6 +1922,35 @@ def _open(): return box +def _reportButton(parent, report_collect): + from ...feedback import reportButton + + return reportButton( + parent, + app="genepyramid-viewer", + collect=report_collect, + title="Report a problem with the gene expression viewer", + ) + + +def addReportPanel(viewer, report_collect) -> Any: + """Just the "Report a problem..." button, for a session with no cloud + panel to carry it. + + A purely local section has no token and so no NDI Cloud panel, but its + reader can still hit a problem. Sending needs a sign-in the dialog + will ask for; Save to file and Copy work without one. + """ + from qtpy.QtWidgets import QVBoxLayout, QWidget + + box = QWidget() + outer = QVBoxLayout(box) + outer.addWidget(_reportButton(box, report_collect)) + outer.addStretch() + _dockScrollable(viewer, box, name="Feedback") + return box + + def cloudSignInDialog(parent=None): """Show the sign-in dialog and sign in. Returns ``(signed_in, note)``. @@ -2561,6 +2597,7 @@ def addAllPanels( shapes_layer=None, labelings=None, gene_layers=None, + report_collect=None, ): """Every panel the session supports, skipping what it has no data for. @@ -2616,7 +2653,16 @@ def _build(name, fn): # at a convenient moment instead of discovering it through a tile # that will not load. if cloudSessionLooksLikely(): - made["cloud"] = _build("cloud", lambda: addCloudPanel(viewer)) + if report_collect is None: + made["cloud"] = _build("cloud", lambda: addCloudPanel(viewer)) + else: + made["cloud"] = _build( + "cloud", lambda: addCloudPanel(viewer, report_collect=report_collect) + ) + elif report_collect is not None: + # No cloud panel to carry the report button; a local reader can + # still hit a problem, so it gets a panel of its own. + made["report"] = _build("report", lambda: addReportPanel(viewer, report_collect)) made["display"] = _build( "display", lambda: addDisplayPanel(viewer, session, pyr_doc, image_layer, density) ) diff --git a/src/ndi/gui/app/genepyramid/viewer.py b/src/ndi/gui/app/genepyramid/viewer.py index 1b56a4fc..f5e37f61 100644 --- a/src/ndi/gui/app/genepyramid/viewer.py +++ b/src/ndi/gui/app/genepyramid/viewer.py @@ -35,6 +35,50 @@ def require_napari(): return napari +def _reportCollector(viewer, session, pyr_doc, gene_rows, density): + """A zero-argument callable returning ``buildReport`` extras. + + The canvas and every layer (via :func:`ndi.gui.feedback.napariState`), + which section and gene selection is open, and the recent log -- what + a report needs that a reader cannot be expected to type. + """ + + def _docId(doc): + if doc is None: + return None + ident = getattr(doc, "id", None) + return str(ident() if callable(ident) else ident) + + def collect() -> dict: + from ...feedback import installLogBuffer, napariState + + state = napariState(viewer) + try: + state["section"] = { + "pyramid": _docId(pyr_doc), + "density": bool(density), + "gene_rows": None if gene_rows is None else len(list(gene_rows)), + } + except Exception as exc: # noqa: BLE001 + state["section"] = f"" + extra: dict[str, Any] = {"state": state, "documentId": _docId(pyr_doc)} + try: + checker = getattr(session, "is_in_cloud", None) + if callable(checker): + in_cloud, cloud_id = checker() + if in_cloud and cloud_id: + extra["datasetId"] = str(cloud_id) + except Exception: # noqa: BLE001 + pass + try: + extra["log"] = installLogBuffer().text() + except Exception: # noqa: BLE001 + pass + return {k: v for k, v in extra.items() if v is not None} + + return collect + + def openPyramid( session, pyr_doc, @@ -104,6 +148,16 @@ def openPyramid( """ from .progress import closeLaunchWindow, note, stage + # Keep the recent log in memory from launch on, so "Report a + # problem..." can attach it -- a viewer opened from MATLAB has no + # terminal to copy it from. + try: + from ...feedback import installLogBuffer + + installLogBuffer() + except Exception: # noqa: BLE001 - reporting is never worth the launch + pass + with stage("starting napari"): napari = require_napari() @@ -198,6 +252,7 @@ def openPyramid( shapes_layer=shapes, labelings=labelings, gene_layers=gene_layers, + report_collect=_reportCollector(viewer, session, pyr_doc, gene_rows, density), ) # The launch window has reported everything it can: the viewer is diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index 0a88991d..8903ebf0 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -1430,29 +1430,16 @@ def _attach_cloud_panel(viewer, report_collect=None) -> None: outer.addLayout(beta_row) if report_collect is not None: - report = QPushButton("Report a problem...") - report.setToolTip( - "Tell us what went wrong. The viewer attaches its own\n" - "diagnostics (versions, what it was loading, recent log\n" - "lines) with sign-in tokens and signed links removed,\n" - "and you can see exactly what will be sent." + from ndi.gui.feedback import reportButton + + outer.addWidget( + reportButton( + box, + app="lightsheetZarr-viewer", + collect=report_collect, + title="Report a problem with the lightsheet viewer", + ) ) - dialogs: list = [] # keep the non-modal dialog alive - - def _report(): - from ndi.gui.feedback import openFeedbackDialog - - dialogs[:] = [ - openFeedbackDialog( - box, - app="lightsheetZarr-viewer", - collect=report_collect, - title="Report a problem with the lightsheet viewer", - ) - ] - - report.clicked.connect(_report) - outer.addWidget(report) signin = QPushButton("Sign in...") signin.setToolTip( diff --git a/src/ndi/gui/feedback.py b/src/ndi/gui/feedback.py index d6f58d4e..765c5a97 100644 --- a/src/ndi/gui/feedback.py +++ b/src/ndi/gui/feedback.py @@ -48,6 +48,8 @@ "describeSendError", "FeedbackDialog", "openFeedbackDialog", + "reportButton", + "napariState", ] #: Server limits (characters / JSON bytes). Reports are cut to fit, not @@ -541,3 +543,87 @@ def openFeedbackDialog(parent=None, **kwargs): dlg.show() dlg.raise_() return dlg + + +def reportButton(parent, *, app: str, collect: Callable[[], dict[str, Any]], title: str): + """A "Report a problem..." QPushButton that opens the dialog. + + One definition for every viewer, so the wording, the tooltip and the + dialog stay the same wherever the button appears. The open dialog is + kept on the button (it is non-modal and would otherwise be collected). + """ + from qtpy.QtWidgets import QPushButton + + button = QPushButton("Report a problem...", parent) + button.setToolTip( + "Tell us what went wrong. The viewer attaches its own\n" + "diagnostics (versions, what it was showing, recent log\n" + "lines) with sign-in tokens and signed links removed,\n" + "and you can see exactly what will be sent." + ) + + def _open(): + button._ndi_dialog = openFeedbackDialog(parent, app=app, collect=collect, title=title) + + button.clicked.connect(_open) + return button + + +def napariState(viewer) -> dict[str, Any]: + """What a napari viewer is showing: camera, dims and every layer. + + Viewer-agnostic, so each viewer's collector starts from the same + picture of the canvas and adds its own specifics. Each part is + guarded: a report about a broken viewer must still build. + """ + state: dict[str, Any] = {} + + def probe(key, fn): + try: + state[key] = fn() + except Exception as exc: # noqa: BLE001 + state[key] = f"" + + probe( + "canvas", + lambda: { + "dims_point": [float(x) for x in viewer.dims.point], + "ndisplay": int(viewer.dims.ndisplay), + "camera_zoom": float(viewer.camera.zoom), + "camera_center": [float(x) for x in viewer.camera.center], + }, + ) + + def _layers(): + out = [] + for layer in viewer.layers: + entry: dict[str, Any] = { + "name": str(layer.name), + "type": type(layer).__name__, + "visible": bool(layer.visible), + } + if getattr(layer, "multiscale", False): + entry["n_levels"] = len(layer.data) + entry["data_level"] = int(layer.data_level) + entry["corner_pixels"] = [[int(v) for v in row] for row in layer.corner_pixels] + try: + entry["loaded"] = bool(layer.loaded) + except Exception: # noqa: BLE001 + pass + try: + entry["n_items"] = ( + len(layer.data) + if type(layer).__name__ + in ( + "Points", + "Shapes", + ) + else entry.get("n_items") + ) + except Exception: # noqa: BLE001 + pass + out.append({k: v for k, v in entry.items() if v is not None}) + return out + + probe("layers", _layers) + return state diff --git a/tests/test_gui_genepyramid_controls.py b/tests/test_gui_genepyramid_controls.py index 6f1e5c4b..b048f9b1 100644 --- a/tests/test_gui_genepyramid_controls.py +++ b/tests/test_gui_genepyramid_controls.py @@ -522,3 +522,60 @@ def test_a_failing_appearance_panel_still_leaves_a_tour(monkeypatch, capsys): _args, kwargs = tour.calls[0] assert kwargs.get("appearance") is None assert "gene appearance" in capsys.readouterr().err + + +def _stubPanels(monkeypatch): + monkeypatch.setattr(controls, "_importQtWidgets", lambda: None) + for name in ( + "addDisplayPanel", + "addCellTypePanel", + "addGenePanel", + "addRotationPanel", + "addGeneAppearancePanel", + "addGeneTourPanel", + ): + monkeypatch.setattr(controls, name, _Recorder()) + + +def test_a_cloud_session_gets_the_report_button_on_the_cloud_panel(monkeypatch): + """Reporting a problem needs a sign-in, and the cloud panel is where + the reader signs in -- so that is where the button goes.""" + _stubPanels(monkeypatch) + cloud, report = _Recorder(), _Recorder() + monkeypatch.setattr(controls, "addCloudPanel", cloud) + monkeypatch.setattr(controls, "addReportPanel", report) + monkeypatch.setenv("NDI_CLOUD_TOKEN", "anything") + collect = dict + + made = controls.addAllPanels( + object(), object(), object(), object(), True, report_collect=collect + ) + assert cloud.calls[0][1] == {"report_collect": collect} + assert not report.calls + assert "report" not in made + + +def test_a_local_session_still_gets_a_way_to_report(monkeypatch): + """No token, no cloud panel -- but a local reader can still hit a + problem, and Save to file / Copy work without signing in.""" + _stubPanels(monkeypatch) + cloud, report = _Recorder(), _Recorder() + monkeypatch.setattr(controls, "addCloudPanel", cloud) + monkeypatch.setattr(controls, "addReportPanel", report) + monkeypatch.delenv("NDI_CLOUD_TOKEN", raising=False) + + made = controls.addAllPanels(object(), object(), object(), object(), True, report_collect=dict) + assert not cloud.calls + assert report.calls and made["report"] is not None + + +def test_without_a_collector_nothing_changes(monkeypatch): + _stubPanels(monkeypatch) + cloud, report = _Recorder(), _Recorder() + monkeypatch.setattr(controls, "addCloudPanel", cloud) + monkeypatch.setattr(controls, "addReportPanel", report) + monkeypatch.setenv("NDI_CLOUD_TOKEN", "anything") + + controls.addAllPanels(object(), object(), object(), object(), True) + assert cloud.calls[0][1] == {} + assert not report.calls From 710c09db5917ccadae5e6d46f818135e8682e9f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 12:34:35 +0000 Subject: [PATCH 11/21] bridge: record NDI-matlab 2f9b0eaf (viewer launched with debug log) NDI-matlab 2f9b0eaf routes both MATLAB launches of the napari viewer (ndi.fun.doc.lightsheet.view and LightsheetZarrManager's View dialog) through the new ndi.fun.doc.lightsheet.launchViewer, which sets NDI_LIGHTSHEET_DEBUG=1 and keeps the viewer's output in lightsheet_YYYYMMDDHHMMSS.txt in NDI's temp folder. * view and LightsheetZarrManager: bumped to 2f9b0eaf with a dated note; nothing on the Python side follows (it is the launched process and already honours NDI_LIGHTSHEET_DEBUG). * launchViewer, viewerLogFile, viewerShellCommand: new matlab_only entries -- Python calls the viewer directly, with its output on the caller's own stderr and in the in-viewer log buffer. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- .../ndi_matlab_python_bridge.yaml | 46 ++++++++++++++++++- .../ndi_matlab_python_bridge.yaml | 11 ++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml index b6545fc4..e729e711 100644 --- a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml @@ -190,7 +190,7 @@ functions: thin. - name: view matlab_path: "+ndi/+fun/+doc/+lightsheet/view.m" - matlab_last_sync_hash: "a7ba07a" + matlab_last_sync_hash: "2f9b0eaf" matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- @@ -203,3 +203,47 @@ functions: stays interactive. No Python-side counterpart is needed because the Python side IS the launcher; this is the MATLAB-side entry point. Ports with the other entries. + 2026-10-12: examined against 2f9b0eaf on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was a7ba07a). A viewer + launch now goes through launchViewer (NDI_LIGHTSHEET_DEBUG=1, output + appended to lightsheet_YYYYMMDDHHMMSS.txt in NDI's temp folder), and + view gains 'debug' and 'logFile' options; --list mode is unchanged. + Nothing on the Python side to follow: the Python side is the + launched process, and it already honours NDI_LIGHTSHEET_DEBUG. + + - name: launchViewer + matlab_path: "+ndi/+fun/+doc/+lightsheet/launchViewer.m" + matlab_last_sync_hash: "2f9b0eaf" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" + status: matlab_only + decision_log: >- + MATLAB-side launch of the Python viewer: runs a viewCommand line + through SYSTEM with NDI_LIGHTSHEET_DEBUG=1 and stdout/stderr + appended to lightsheet_YYYYMMDDHHMMSS.txt in + ndi.common.PathConstants.TempFolder, so a viewer started from + MATLAB (no terminal) leaves a log. No Python counterpart: from + Python the viewer is called directly (napariViewLightsheet or + ndi.gui.app.lightsheetZarr.viewer.openPyramid) with its output on + the caller's own stderr, and the in-viewer log buffer + (ndi.gui.feedback) already keeps it for problem reports. + + - name: viewerLogFile + matlab_path: "+ndi/+fun/+doc/+lightsheet/viewerLogFile.m" + matlab_last_sync_hash: "2f9b0eaf" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" + status: matlab_only + decision_log: >- + Pure helper of launchViewer: the timestamped log path + lightsheet_YYYYMMDDHHMMSS.txt under NDI's temp folder. Not + needed in Python for the same reason as launchViewer -- nothing in + Python launches the viewer as a detached child that needs a log. + + - name: viewerShellCommand + matlab_path: "+ndi/+fun/+doc/+lightsheet/viewerShellCommand.m" + matlab_last_sync_hash: "2f9b0eaf" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" + status: matlab_only + decision_log: >- + Pure helper of launchViewer: the POSIX / cmd.exe line that sets + NDI_LIGHTSHEET_DEBUG, appends output to the log and detaches. Not + needed in Python for the same reason as launchViewer. diff --git a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml index 849e37ca..0ebfcfd1 100644 --- a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml +++ b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml @@ -35,7 +35,7 @@ functions: # ----------------------------------------------------------------------- - name: LightsheetZarrManager matlab_path: "+ndi/+gui/+app/LightsheetZarrManager.m" - matlab_last_sync_hash: "60a79cf0" + matlab_last_sync_hash: "2f9b0eaf" matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: matlab_only decision_log: >- @@ -53,6 +53,15 @@ functions: lightsheetZarrLevel documents rather than nonexistent parent fields. Still matlab_only; no Python counterpart to update. + 2026-10-12: examined against 2f9b0eaf (was 60a79cf0). This range + brings in the 32 MB tile-budget and main merges plus 2f9b0eaf, where + the View dialog launches through ndi.fun.doc.lightsheet.launchViewer + (NDI_LIGHTSHEET_DEBUG=1, output kept in + lightsheet_YYYYMMDDHHMMSS.txt in NDI's temp folder, path shown in + the status line) and the private detach helper is removed. Still + matlab_only; the launched Python viewer already honours + NDI_LIGHTSHEET_DEBUG. + # ----------------------------------------------------------------------- # CLI (napariViewLightsheet). # ----------------------------------------------------------------------- From 83608aa606589dd357eacf46aacc70091edae1c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 13:31:44 +0000 Subject: [PATCH 12/21] bridge: record NDI-matlab 04ce18f9 (fromSPIM, reduceBlocks, makePyramid levelSources) * makePyramid: bumped to 04ce18f9 with a dated note -- levelSources and channelNames added for fromSPIM; default OME-Zarr behaviour unchanged. * fromSPIM, reduceBlocks: new porting_deferred entries. The Python side is a viewer today; both port after the MATLAB version is validated, on top of a makePyramid port. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- .../ndi_matlab_python_bridge.yaml | 34 ++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml index e729e711..f31ac5e5 100644 --- a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml @@ -59,7 +59,7 @@ functions: - name: makePyramid matlab_path: "+ndi/+fun/+doc/+lightsheet/makePyramid.m" - matlab_last_sync_hash: "5c9e8a83" + matlab_last_sync_hash: "04ce18f9" matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- @@ -99,6 +99,38 @@ functions: follow: this function has no Python counterpart yet, and the Python viewer only reads the levels MATLAB wrote. The Python port must use the 32 MB default when it lands. + 2026-10-12: examined against 04ce18f9 on + claude/lightsheet-zarr-ndi-viewer-djp5vk (was 5c9e8a83). Adds + levelSources (per level path: 'omezarr' -- the unchanged default -- + 'smartspim' slices, or 'reduce' from an earlier level's stored + chunks) and channelNames, for fromSPIM. Default behaviour is + unchanged; the Python port must carry levelSources when it lands. + + - name: fromSPIM + matlab_path: "+ndi/+fun/+doc/+lightsheet/fromSPIM.m" + matlab_last_sync_hash: "04ce18f9" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" + status: porting_deferred + decision_log: >- + Builds a lightsheetZarrPyramid straight from a SmartSPIM + acquisition's stitched All_Channels slices (no spim2zarr step): + level 0 from ndr.format.smartspim, mean and max ladders built by + block-reducing each level just written, through makePyramid's + levelSources. Deferred with the rest of the ingest side (the + Python package is a viewer today); planned to port after the + MATLAB version is validated, on top of a makePyramid port, using + ndr-python's smartspim All_Channels reader. + + - name: reduceBlocks + matlab_path: "+ndi/+fun/+doc/+lightsheet/reduceBlocks.m" + matlab_last_sync_hash: "04ce18f9" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" + status: porting_deferred + decision_log: >- + Vectorized per-axis block mean/max with + ndr.format.omezarr.reduce's semantics (trailing partial block + included), used by fromSPIM to build reduced levels. Ports with + fromSPIM; in numpy it is a reshape + mean/max per axis. - name: chooseTileShape matlab_path: "+ndi/+fun/+doc/+lightsheet/chooseTileShape.m" From 19a5575750f26ec307403557e0826187135d5a5a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 13:35:55 +0000 Subject: [PATCH 13/21] bridge: mark signedUrlDiskCache and fetchManifest as branch-pending CI's "MATLAB/Python bridge completeness" checks the bridge against NDI-matlab main, where +ndi/+cloud/+download/+internal/signedUrlDiskCache.m and +ndi/+cloud/+sync/+internal/fetchManifest.m do not exist yet: both live on claude/lightsheet-zarr-ndi-viewer-djp5vk (VH-Lab/NDI-matlab#979). Add matlab_branch to the two entries, as the bridge reference prescribes for files on an unmerged branch. Passes against NDI-matlab main and against the branch. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/cloud/ndi_matlab_python_bridge.yaml | 1 + src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml | 1 + 2 files changed, 2 insertions(+) diff --git a/src/ndi/cloud/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/ndi_matlab_python_bridge.yaml index d832e5d7..c2a5f6b4 100644 --- a/src/ndi/cloud/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/ndi_matlab_python_bridge.yaml @@ -1549,6 +1549,7 @@ not_yet_ported: matlab_path: "+ndi/+cloud/+download/+internal/signedUrlDiskCache.m" python_path: "ndi/cloud/signed_url_disk_cache.py" matlab_last_sync_hash: "988ec74" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: ported decision_log: > PORTED for VH-Lab/NDI-matlab#1010 follow-on / Waltham-Data-Science/ diff --git a/src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml index 6e49d170..9bddecaa 100644 --- a/src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml @@ -712,6 +712,7 @@ not_yet_ported: - name: fetchManifest matlab_path: "+ndi/+cloud/+sync/+internal/fetchManifest.m" matlab_last_sync_hash: "a84130a7" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: ported_differently python_path: "ndi/cloud/filehandler.py" decision_log: > From eb6eb52c3451ab76e0dbf3a4daa96eddadc16401 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 13:45:53 +0000 Subject: [PATCH 14/21] bridge: fromSPIM examined against NDI-matlab 668beb13 668beb13 preallocates fromSPIM's per-ladder arrays (no behaviour change) alongside a TestFromSPIM expectation fix; nothing to port. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml index f31ac5e5..8a276cc4 100644 --- a/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml @@ -108,7 +108,7 @@ functions: - name: fromSPIM matlab_path: "+ndi/+fun/+doc/+lightsheet/fromSPIM.m" - matlab_last_sync_hash: "04ce18f9" + matlab_last_sync_hash: "668beb13" matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- @@ -120,6 +120,8 @@ functions: Python package is a viewer today); planned to port after the MATLAB version is validated, on top of a makePyramid port, using ndr-python's smartspim All_Channels reader. + 2026-10-12: examined against 668beb13 (was 04ce18f9): preallocation of + the ladder arrays only; no behaviour change, nothing to port. - name: reduceBlocks matlab_path: "+ndi/+fun/+doc/+lightsheet/reduceBlocks.m" From a12f14f02d4411ae4a4e838660ebcabe71d5361f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 15:19:07 +0000 Subject: [PATCH 15/21] Point lightsheet cloud fixture at the rebuilt dataset The previous fixture dataset (6ab84b54...) lost a file blob in S3 (NoSuchKey), failing the production pyramid integration test. Rebuilt it with the Build lightsheet blob fixture workflow (run 38265299422). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- tests/fixtures/lightsheet_cloud_fixture.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/fixtures/lightsheet_cloud_fixture.json b/tests/fixtures/lightsheet_cloud_fixture.json index 9225f353..1d1bb812 100644 --- a/tests/fixtures/lightsheet_cloud_fixture.json +++ b/tests/fixtures/lightsheet_cloud_fixture.json @@ -1,8 +1,8 @@ { - "cloud_dataset_id": "6ab84b549852a120dbcb22bc", + "cloud_dataset_id": "6accf3a0044c77d93a6ebdb7", "user": "1", "environment": "prod", - "created_at": "2026-09-26T22:47:00Z", - "workflow_run": "https://github.com/Waltham-Data-Science/NDI-python/actions/runs/36277182766", + "created_at": "2026-10-12T14:50:35Z", + "workflow_run": "https://github.com/Waltham-Data-Science/NDI-python/actions/runs/38265299422", "notes": "Small synthetic lightsheet blob uploaded by ndi.test.cloud.lightsheet_blob_cloud_roundtrip (MATLAB), 12 documents / 15 files / 1.92 MB. Regenerate with the Build lightsheet blob fixture workflow. tests/test_pyramid_loader_integration.py reads this JSON by default when NDI_LIGHTSHEET_TEST_FIXTURE_ID is not set." } From dbdedf68f8031ae7407ce8875342b74bf63f2095 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 23:29:06 +0000 Subject: [PATCH 16/21] Add file CRC-32 support for NDI Cloud: lookupFiles, checksummed uploads (ndi-cloud-node#162) Python half of VH-Lab/NDI-matlab#1025. - ndi.fun.file.crc32 / crc32ToBase64: a file's CRC-32 in the cloud's encoding (base64 of the 4-byte big-endian value), via zlib in chunks. - getFileUploadURL(..., checksum_crc32=) sends checksumCRC32; putFiles / putFileBytes(..., checksum_crc32=) send the x-amz-checksum-crc32 header that such a URL signs. Malformed values are refused. - ndi.cloud.api.files.lookupFiles: POST /files/lookup, split at 5000 uids. - uploadSingleFile sends the checksum by default (checksum=True). - getSignedURLSetAll merges the pages' fileInfo. - Bridge: lookupFiles, crc32, crc32ToBase64 recorded with matlab_branch claude/file-crc32-lookup; getFileUploadURL, putFiles, uploadSingleFile, listFiles(All), getSignedURLSet(All) and url synced to NDI-matlab 8fa6ea5e. - Tests: offline tests/test_cloud_api_files_crc32.py; live TestFileChecksums in tests/test_cloud_live.py (skips where the server predates the route). Two test_phase2_gaps uploadSingleFile tests pass checksum=False, since their file does not exist and the default now reads it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SwsqFA9Zs1HGm6YM4g2XRV --- CHANGELOG.md | 15 + src/ndi/cloud/api/files.py | 133 ++++++++- .../cloud/api/ndi_matlab_python_bridge.yaml | 67 ++++- src/ndi/cloud/ndi_matlab_python_bridge.yaml | 10 +- src/ndi/cloud/upload.py | 14 +- src/ndi/fun/file.py | 59 +++- src/ndi/fun/ndi_matlab_python_bridge.yaml | 40 +++ tests/test_cloud_api_files_crc32.py | 272 ++++++++++++++++++ tests/test_cloud_live.py | 139 +++++++++ tests/test_phase2_gaps.py | 9 +- 10 files changed, 739 insertions(+), 19 deletions(-) create mode 100644 tests/test_cloud_api_files_crc32.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f734caf..5c1af198 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,21 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## [Unreleased] +### Added + +- **File CRC-32 checksums for NDI Cloud** (ndi-cloud-node#162, mirroring + NDI-matlab#1025), so an importer can resume an interrupted upload by + re-uploading only the files whose cloud copy does not match. + `ndi.fun.file.crc32(path)` gives a file's CRC-32 in the cloud's encoding + (base64 of the 4-byte big-endian value). `getFileUploadURL` and `putFiles` + / `putFileBytes` take `checksum_crc32=`, so S3 rejects a corrupted upload + and the cloud records the value; `ndi.cloud.upload.uploadSingleFile` sends + it by default (`checksum=False` turns it off). New + `ndi.cloud.api.files.lookupFiles(dataset_id, uids)` returns `uploaded`, + `size` and `crc32` for given uids plus the `missing` ones, splitting lists + over the server's 5000-uid limit. `getSignedURLSetAll` merges the pages' + `fileInfo`. `listFiles` and `getFileDetails` already pass `crc32` through. + ### Fixed - **`ndi.probe.timeseries.readtimeseries()` names its first argument diff --git a/src/ndi/cloud/api/files.py b/src/ndi/cloud/api/files.py index 2e628dab..669e86ef 100644 --- a/src/ndi/cloud/api/files.py +++ b/src/ndi/cloud/api/files.py @@ -10,6 +10,7 @@ from __future__ import annotations +import re import time from pathlib import Path from typing import Annotated, Any, Literal @@ -27,6 +28,33 @@ _Client = Annotated[CloudClient | None, SkipValidation()] +# A CRC-32 as NDI Cloud and S3 encode it: base64 of the 4-byte big-endian +# value, so six base64 characters and '=='. See ndi.fun.file.crc32. +_CRC32_BASE64 = re.compile(r"^[A-Za-z0-9+/]{6}==$") + +# POST /files/lookup accepts at most this many uids per call; lookupFiles +# splits longer lists. +LOOKUP_MAX_UIDS = 5000 + + +def _checked_crc32(checksum_crc32: str) -> str: + if checksum_crc32 and not _CRC32_BASE64.match(checksum_crc32): + raise ValueError( + "checksum_crc32 must be the base64 of a 4-byte CRC-32 (e.g. 'NhCmhg=='), " + f"got {checksum_crc32!r}" + ) + return checksum_crc32 + + +def _upload_headers(checksum_crc32: str) -> dict[str, str]: + headers = {"Content-Type": "application/octet-stream"} + if checksum_crc32: + # The upload URL signs this header when it was requested with a + # checksum (NDI Cloud #162), so it must be sent, with the same value. + headers["x-amz-checksum-crc32"] = checksum_crc32 + return headers + + # Terminal job states reported by the bulk-upload service. _TERMINAL_BULK_STATES = ("complete", "failed") _ACTIVE_BULK_STATES = ("queued", "extracting") @@ -56,14 +84,31 @@ def getFileUploadURL( dataset_id: CloudId, file_uid: NonEmptyStr, *, + checksum_crc32: str = "", client: _Client = None, ) -> str: """GET /datasets/{organizationId}/{datasetId}/files/{file_uid} Returns a presigned S3 URL for uploading. + + Args: + org_id: The organization id. + dataset_id: The cloud dataset id. + file_uid: The uid the file will have. + checksum_crc32: Optional. The file's CRC-32 as base64 of its 4-byte + big-endian value (:func:`ndi.fun.file.crc32`). When given, the URL + signs the ``x-amz-checksum-crc32`` header, so the upload must send + it: pass the same value to :func:`putFiles`. S3 then rejects + content that does not match, and the cloud records the value as + the file's ``crc32``. + client: Authenticated cloud client (auto-created if omitted). + + MATLAB equivalent: +cloud/+api/+files/getFileUploadURL.m """ + params = {"checksumCRC32": _checked_crc32(checksum_crc32)} if checksum_crc32 else None result = client.get( "/datasets/{organizationId}/{datasetId}/files/{file_uid}", + params=params, organizationId=org_id, datasetId=dataset_id, file_uid=file_uid, @@ -100,6 +145,7 @@ def putFiles( job_id: str = "", wait_for_completion: bool = False, completion_timeout: float = 60.0, + checksum_crc32: str = "", ) -> bool: """PUT a local file to a presigned S3 URL. @@ -118,6 +164,12 @@ def putFiles( the signed PUT returning 200 already means done. completion_timeout: Overall wait-for-completion deadline, in seconds. Default 60. + checksum_crc32: The file's CRC-32 (:func:`ndi.fun.file.crc32`). + Required when the URL came from + ``getFileUploadURL(..., checksum_crc32=crc)``: that URL signs the + ``x-amz-checksum-crc32`` header, so it is sent with the same + value, and S3 rejects content that does not match. Default ``""`` + (no header). Returns: True on success (and, when ``wait_for_completion`` is True, the @@ -133,12 +185,13 @@ def putFiles( from ..exceptions import CloudUploadError assert_safe_transfer_url(url, what="upload URL") + headers = _upload_headers(_checked_crc32(checksum_crc32)) file_path = Path(file_path) with open(file_path, "rb") as fh: resp = requests.put( url, data=fh, - headers={"Content-Type": "application/octet-stream"}, + headers=headers, timeout=timeout, ) @@ -161,6 +214,8 @@ def putFileBytes( url: NonEmptyStr, data: bytes, timeout: int = 120, + *, + checksum_crc32: str = "", ) -> bool: """PUT raw bytes to a presigned S3 URL. @@ -168,6 +223,8 @@ def putFileBytes( url: Presigned URL. data: Bytes to upload. timeout: Request timeout in seconds. + checksum_crc32: The bytes' CRC-32 (:func:`ndi.fun.file.crc32ToBase64` + of ``zlib.crc32(data)``); see :func:`putFiles`. Returns: True on success. @@ -183,7 +240,7 @@ def putFileBytes( resp = requests.put( url, data=data, - headers={"Content-Type": "application/octet-stream"}, + headers=_upload_headers(_checked_crc32(checksum_crc32)), timeout=timeout, ) @@ -343,7 +400,8 @@ def listFiles( The page envelope with fields ``datasetId``, ``limit``, ``count``, ``cursor``, ``hasMore``, ``totalNumber``, and ``files`` -- a list of file summaries, each with ``uid``, ``uploaded``, ``sourceDatasetId``, - and ``size``. Follow ``cursor`` by passing it back as *after* while + ``size`` and, when one was recorded at upload, ``crc32`` (base64 + big-endian CRC-32; compare with :func:`ndi.fun.file.crc32`). Follow ``cursor`` by passing it back as *after* while ``hasMore`` is true. MATLAB equivalent: +cloud/+api/+files/listFiles.m @@ -459,6 +517,9 @@ def getFileDetails( ) -> dict[str, Any]: """Get detail info (including download URL) for a file. + The record carries ``crc32`` (base64 big-endian CRC-32 of the content) + when one was recorded at upload. + MATLAB equivalent: +cloud/+api/+files/getFileDetails.m """ return client.get( @@ -468,6 +529,59 @@ def getFileDetails( ) +@_auto_client +@validate_call(config=VALIDATE_CONFIG) +def lookupFiles( + dataset_id: CloudId, + file_uids: list[str], + *, + client: _Client = None, +) -> dict[str, Any]: + """Look up the upload state, size and CRC-32 of files by uid. + + ``POST /datasets/{datasetId}/files/lookup``. Tells, for each uid, whether + the dataset has the file, whether it finished uploading, its size, and + the CRC-32 recorded at upload, without listing the whole dataset or + signing any URLs. Use it to decide which files of a document or import + batch still need uploading, e.g. to resume an interrupted upload. + + The server takes at most :data:`LOOKUP_MAX_UIDS` (5000) uids per call; + longer lists are split into several calls and the results merged. + Duplicate uids are looked up once. + + Args: + dataset_id: The cloud dataset id. + file_uids: File uids to look up. + client: Authenticated cloud client (auto-created if omitted). + + Returns: + ``{"files": [...], "missing": [...]}``. ``files`` has one dict per uid + the dataset has a record for, with ``uid``, ``uploaded``, ``size`` and + ``crc32`` (base64 big-endian CRC-32, or ``None`` when none was + recorded -- compare ``size`` instead). ``missing`` lists the uids with + no record. + + A cloud copy matches a local file when it is uploaded and its ``crc32`` + equals :func:`ndi.fun.file.crc32` of the local file. + + MATLAB equivalent: +cloud/+api/+files/lookupFiles.m + """ + uids = list(dict.fromkeys(u for u in file_uids if u)) + result: dict[str, Any] = {"files": [], "missing": []} + for start in range(0, len(uids), LOOKUP_MAX_UIDS): + batch = uids[start : start + LOOKUP_MAX_UIDS] + page = client.post( + "/datasets/{datasetId}/files/lookup", + json={"uids": batch}, + datasetId=dataset_id, + ) + files = page.get("files", []) if hasattr(page, "get") else [] + missing = page.get("missing", []) if hasattr(page, "get") else [] + result["files"].extend(files or []) + result["missing"].extend(missing or []) + return result + + # --------------------------------------------------------------------------- # Signed-URL-set family # @@ -498,8 +612,10 @@ def getSignedURLSet( """GET one page of a document's uid -> signed URL map. Returns a dict with fields ``datasetId``, ``documentId``, ``totalCount``, - ``pageCount``, optionally ``nextCursor``, ``expiresAt``, and ``files`` - (a dict mapping file uid to pre-signed download URL, in uid-sorted order). + ``pageCount``, optionally ``nextCursor``, ``expiresAt``, ``files`` + (a dict mapping file uid to pre-signed download URL, in uid-sorted order) + and ``fileInfo`` (a list of ``{uid, uploaded, size, crc32}`` for the same + files; ``crc32`` is ``None`` when none was recorded). Args: dataset_id: The cloud dataset id. @@ -590,7 +706,8 @@ def getSignedURLSetAll( Returns: Merged dict with ``datasetId``, ``documentId``, ``files`` - (uid -> URL), ``pageCount`` (total uids across pages), ``pages`` + (uid -> URL), ``fileInfo`` (``{uid, uploaded, size, crc32}`` dicts + across pages), ``pageCount`` (total uids across pages), ``pages`` (number of HTTP calls), ``totalCount`` (server-reported total), and ``expiresAt`` (from the last page carrying it). @@ -605,6 +722,7 @@ def getSignedURLSetAll( "datasetId": dataset_id, "documentId": document_id, "files": {}, + "fileInfo": [], "pageCount": 0, "totalCount": 0, "pages": 0, @@ -624,6 +742,9 @@ def getSignedURLSetAll( if isinstance(files, dict): merged["files"].update(files) merged["pageCount"] += len(files) + file_info = page.get("fileInfo") if hasattr(page, "get") else None + if isinstance(file_info, list): + merged["fileInfo"].extend(file_info) total = page.get("totalCount") if hasattr(page, "get") else None if isinstance(total, int): merged["totalCount"] = total diff --git a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml index 367b1ea0..4b587bd3 100644 --- a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml @@ -760,7 +760,7 @@ functions: # --- files.py --- - name: getFileUploadURL matlab_path: "+ndi/+cloud/+api/+files/getFileUploadURL.m" - matlab_last_sync_hash: "9b75c0fe" + matlab_last_sync_hash: "8fa6ea5e" python_path: "ndi/cloud/api/files.py" input_arguments: - name: org_id @@ -772,13 +772,22 @@ functions: - name: file_uid type_matlab: "char" type_python: "NonEmptyStr" + - name: checksum_crc32 + type_matlab: "string" + type_python: "str" + default: "''" - name: client type_python: "_Client" default: "None" output_arguments: - name: url type_python: "str" - decision_log: "Exact match." + decision_log: > + Exact match. 2026-10-12: synced to NDI-matlab 8fa6ea5e + (claude/file-crc32-lookup, NDI-matlab#1025): the 'checksumCRC32' + option becomes checksum_crc32, sent as the checksumCRC32 query + parameter (NDI Cloud ndi-cloud-node#162). The returned URL then signs + the x-amz-checksum-crc32 header, which putFiles must send. - name: getBulkUploadURL matlab_path: "+ndi/+cloud/+api/+documents/getBulkUploadURL.m" @@ -806,7 +815,7 @@ functions: - name: putFiles matlab_path: "+ndi/+cloud/+api/+files/putFiles.m" - matlab_last_sync_hash: "2566fe4d" + matlab_last_sync_hash: "8fa6ea5e" python_path: "ndi/cloud/api/files.py" input_arguments: - name: url @@ -830,10 +839,18 @@ functions: type_matlab: "double" type_python: "float" default: "60.0" + - name: checksum_crc32 + type_matlab: "string" + type_python: "str" + default: "''" output_arguments: - name: success type_python: "bool" decision_log: > + 2026-10-12: synced to NDI-matlab 8fa6ea5e (claude/file-crc32-lookup, + NDI-matlab#1025): 'checksumCRC32' becomes checksum_crc32 and is sent + as the x-amz-checksum-crc32 header, which a checksummed upload URL + signs (ndi-cloud-node#162). Validated as a base64 4-byte CRC-32. Synchronized with MATLAB main as of 2026-05-11 (matlab HEAD 2566fe4d). Added job_id and wait_for_completion options to mirror the new MATLAB bulk-upload "fire-and-wait" pattern: after a @@ -893,7 +910,7 @@ functions: - name: listFiles matlab_path: "+ndi/+cloud/+api/+files/listFiles.m" - matlab_last_sync_hash: "3fce6cb" + matlab_last_sync_hash: "8fa6ea5e" python_path: "ndi/cloud/api/files.py" input_arguments: - name: dataset_id @@ -925,7 +942,7 @@ functions: - name: listFilesAll matlab_path: "+ndi/+cloud/+api/+files/listFilesAll.m" - matlab_last_sync_hash: "3fce6cb" + matlab_last_sync_hash: "8fa6ea5e" python_path: "ndi/cloud/api/files.py" input_arguments: - name: dataset_id @@ -963,6 +980,31 @@ functions: MATLAB `retries` knob is omitted: transient GET retries are handled centrally by CloudClient, not per call site. + - name: lookupFiles + matlab_path: "+ndi/+cloud/+api/+files/lookupFiles.m" + matlab_last_sync_hash: "8fa6ea5e" + matlab_branch: "claude/file-crc32-lookup" + python_path: "ndi/cloud/api/files.py" + input_arguments: + - name: dataset_id + type_matlab: "string" + type_python: "CloudId" + - name: file_uids + type_matlab: "string array" + type_python: "list[str]" + - name: client + type_python: "_Client" + default: "None" + output_arguments: + - name: result + type_python: "dict[str, Any]" + decision_log: > + Exact match. POST /datasets/{datasetId}/files/lookup (ndi-cloud-node#162), + split into calls of at most LOOKUP_MAX_UIDS (5000) uids and merged, as + MATLAB does. Returns {"files": [{uid, uploaded, size, crc32}], + "missing": [uid, ...]}. Where MATLAB normalizes a missing crc32 to '' + (a struct field cannot be null), Python keeps the server's None. + - name: getFileDetails matlab_path: "+ndi/+cloud/+api/+files/getFileDetails.m" matlab_last_sync_hash: "eb577fbd" @@ -1639,7 +1681,7 @@ not_yet_ported: - name: url matlab_path: "+ndi/+cloud/+api/url.m" - matlab_last_sync_hash: "75f937a76" + matlab_last_sync_hash: "8fa6ea5e" status: ported_differently decision_log: > MATLAB URL builder. Python uses CloudClient._build_url() @@ -1671,6 +1713,11 @@ not_yet_ported: get_file_tier_job -> GET /file-tier-jobs/{jobId}); both are inlined at the call sites in api/files.py by setFileTier / getFileTierJob, so this entry has nothing to add. + 2026-10-12: examined against 8fa6ea5e (was 75f937a76), NDI-matlab#1025: + one row added to url()'s route table, lookup_dataset_files -> + /datasets/{datasetId}/files/lookup. Nothing to port here; the route + string lives in ndi.cloud.api.files.lookupFiles, as every other route + does in Python. - name: documentCount matlab_path: "+ndi/+cloud/+api/+documents/documentCount.m" @@ -1720,7 +1767,7 @@ not_yet_ported: - name: getSignedURLSet matlab_path: "+ndi/+cloud/+api/+files/getSignedURLSet.m" python_path: "ndi/cloud/api/files.py" - matlab_last_sync_hash: "5804cd80" + matlab_last_sync_hash: "8fa6ea5e" status: ported decision_log: > Ported by NDI-python#262 as the API foundation of batch_signed_url. @@ -1735,7 +1782,7 @@ not_yet_ported: - name: getSignedURLSetAll matlab_path: "+ndi/+cloud/+api/+files/getSignedURLSetAll.m" python_path: "ndi/cloud/api/files.py" - matlab_last_sync_hash: "5804cd80" + matlab_last_sync_hash: "8fa6ea5e" status: ported decision_log: > Ported by NDI-python#262. Walks getSignedURLSet cursors and merges @@ -1746,6 +1793,10 @@ not_yet_ported: partial merged dict on the exception. The async job path (createSignedURLSetJob + waitForSignedURLSetJob) is still deferred because batch_signed_url does not use it. + 2026-10-12: synced to NDI-matlab 8fa6ea5e (NDI-matlab#1025): pages' + fileInfo lists ({uid, uploaded, size, crc32}, ndi-cloud-node#162) are + concatenated into merged["fileInfo"]. getSignedURLSet passes fileInfo + through untouched. - name: getSignedURLSetJob matlab_path: "+ndi/+cloud/+api/+files/getSignedURLSetJob.m" diff --git a/src/ndi/cloud/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/ndi_matlab_python_bridge.yaml index 77f35ad2..47731d6b 100644 --- a/src/ndi/cloud/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/ndi_matlab_python_bridge.yaml @@ -967,7 +967,7 @@ functions: - name: uploadSingleFile matlab_path: "+ndi/+cloud/uploadSingleFile.m" - matlab_last_sync_hash: "d9213ccb7" + matlab_last_sync_hash: "8fa6ea5e" python_path: "ndi/cloud/upload.py" input_arguments: - name: dataset_id @@ -983,6 +983,10 @@ functions: type_matlab: "logical" type_python: "bool" default: "False" + - name: checksum + type_matlab: "logical" + type_python: "bool" + default: "True" - name: client type_python: "CloudClient | None" default: "None" @@ -993,6 +997,10 @@ functions: type_python: "str" decision_log: > Exact match. + 2026-10-12: synced to NDI-matlab 8fa6ea5e (claude/file-crc32-lookup, + NDI-matlab#1025): 'checksum' (default true) computes the file's CRC-32 + and sends it with the single-file upload URL request and the PUT + (ndi-cloud-node#162). 2026-09-08: examined against d9213ccb7 (was 7026d493). Two commits. 39ac4cabe forwards useCurl through the bulk branch and does not port. d9213ccb7 adds the bulk-upload status API and a waitForCompletion option on diff --git a/src/ndi/cloud/upload.py b/src/ndi/cloud/upload.py index fad86a54..9c173831 100644 --- a/src/ndi/cloud/upload.py +++ b/src/ndi/cloud/upload.py @@ -369,6 +369,7 @@ def uploadSingleFile( file_path: str, *, use_bulk_upload: bool = False, + checksum: bool = True, client: CloudClient | None = None, ) -> tuple[bool, str]: """Upload a single file to the NDI cloud service. @@ -381,6 +382,11 @@ def uploadSingleFile( file_path: Local path of the file to upload. use_bulk_upload: If True, zip the file and use the bulk upload mechanism. Defaults to False. + checksum: If True (default), compute the file's CRC-32 + (:func:`ndi.fun.file.crc32`) and send it with the single-file + upload, so S3 rejects a corrupted upload and NDI Cloud records + the file's ``crc32``. Ignored for bulk uploads, whose zip entries + carry their own CRC-32. client: Authenticated cloud client (auto-created if omitted). Returns: @@ -408,13 +414,19 @@ def uploadSingleFile( if zip_path.exists(): zip_path.unlink() else: + checksum_crc32 = "" + if checksum: + from ..fun.file import crc32 + + checksum_crc32 = crc32(file_path) url = files_api.getFileUploadURL( client.config.org_id, dataset_id, file_uid, + checksum_crc32=checksum_crc32, client=client, ) - files_api.putFiles(url, file_path) + files_api.putFiles(url, file_path, checksum_crc32=checksum_crc32) return True, "" except Exception as exc: diff --git a/src/ndi/fun/file.py b/src/ndi/fun/file.py index eb081d65..724bc2fd 100644 --- a/src/ndi/fun/file.py +++ b/src/ndi/fun/file.py @@ -1,12 +1,15 @@ """ ndi.fun.file - File utility functions. -MATLAB equivalents: +ndi/+fun/+file/MD5.m, dateCreated.m, dateUpdated.m +MATLAB equivalents: +ndi/+fun/+file/MD5.m, crc32.m, crc32ToBase64.m, + dateCreated.m, dateUpdated.m """ from __future__ import annotations +import base64 import hashlib +import zlib from datetime import datetime, timezone from pathlib import Path from typing import Any @@ -36,6 +39,60 @@ def MD5(file_path: str) -> str: return h.hexdigest() +def crc32ToBase64(value: int) -> str: + """Encode a numeric CRC-32 as base64 of its 4-byte big-endian value. + + MATLAB equivalent: ndi.fun.file.crc32ToBase64 + + NDI Cloud and S3 represent a CRC-32 this way (``x-amz-checksum-crc32``). + + Args: + value: The CRC-32, an integer in ``[0, 0xFFFFFFFF]`` (e.g. from + ``zlib.crc32``). + + Returns: + 8-character base64 string, e.g. ``'NhCmhg=='`` for ``0x3610A686``. + + Raises: + ValueError: If *value* is not a 32-bit unsigned integer. + """ + if isinstance(value, bool) or not isinstance(value, int) or not 0 <= value <= 0xFFFFFFFF: + raise ValueError(f"CRC-32 must be an integer in [0, 0xFFFFFFFF], got {value!r}") + return base64.b64encode(value.to_bytes(4, "big")).decode("ascii") + + +def crc32(file_path: str, chunk_size: int = 8 * 1024 * 1024) -> str: + """CRC-32 of a file, base64-encoded the way NDI Cloud and S3 store it. + + MATLAB equivalent: ndi.fun.file.crc32 + + The CRC-32 (IEEE, as ``zlib.crc32``) of the file's bytes, returned as the + base64 of its 4-byte big-endian value. That is the value + :func:`ndi.cloud.api.files.getFileUploadURL` sends as ``checksum_crc32`` + and the value NDI Cloud returns as a file's ``crc32``. The file is read in + chunks. + + Args: + file_path: Path to the file. + chunk_size: Bytes read per chunk. Default 8 MiB. + + Returns: + 8-character base64 string, e.g. ``'NhCmhg=='`` for a file containing + ``b'hello'``. + + Raises: + FileNotFoundError: If the file does not exist. + """ + p = Path(file_path) + if not p.is_file(): + raise FileNotFoundError(f"File not found: {file_path}") + value = 0 + with open(p, "rb") as f: + for chunk in iter(lambda: f.read(chunk_size), b""): + value = zlib.crc32(chunk, value) + return crc32ToBase64(value) + + def dateCreated(file_path: str) -> datetime | None: """Get the creation date of a file. diff --git a/src/ndi/fun/ndi_matlab_python_bridge.yaml b/src/ndi/fun/ndi_matlab_python_bridge.yaml index 540ee534..68eb096c 100644 --- a/src/ndi/fun/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/ndi_matlab_python_bridge.yaml @@ -1298,6 +1298,46 @@ functions: the 32-character lowercase digest and both refuse a missing file. Nothing to port. + - name: crc32 + type: function + matlab_path: "+ndi/+fun/+file/crc32.m" + matlab_last_sync_hash: "8fa6ea5e" + matlab_branch: "claude/file-crc32-lookup" + python_path: "ndi/fun/file.py" + input_arguments: + - name: file_path + type_matlab: "string (fileName)" + type_python: "str" + - name: chunk_size + type_matlab: "double (chunkSize)" + type_python: "int" + default: "8388608" + output_arguments: + - name: checksum + type_python: "str" + decision_log: > + Exact match. CRC-32 (IEEE) of the file's bytes as base64 of the 4-byte + big-endian value, the encoding NDI Cloud and S3 use + (ndi-cloud-node#162). MATLAB uses java.util.zip.CRC32, Python + zlib.crc32; both read in chunks and give the same value. + + - name: crc32ToBase64 + type: function + matlab_path: "+ndi/+fun/+file/crc32ToBase64.m" + matlab_last_sync_hash: "8fa6ea5e" + matlab_branch: "claude/file-crc32-lookup" + python_path: "ndi/fun/file.py" + input_arguments: + - name: value + type_matlab: "double" + type_python: "int" + output_arguments: + - name: checksum + type_python: "str" + decision_log: > + Exact match. Base64 of the 4-byte big-endian value; rejects values + outside [0, 0xFFFFFFFF]. + - name: dateCreated type: function matlab_path: "+ndi/+fun/+file/dateCreated.m" diff --git a/tests/test_cloud_api_files_crc32.py b/tests/test_cloud_api_files_crc32.py new file mode 100644 index 00000000..111497cf --- /dev/null +++ b/tests/test_cloud_api_files_crc32.py @@ -0,0 +1,272 @@ +"""Unit tests for file CRC-32 support (NDI Cloud ndi-cloud-node#162) -- no network. + +MATLAB counterpart: ``tests/+ndi/+unittest/FileCrc32OfflineTest.m`` +(VH-Lab/NDI-matlab#1025). + +* :func:`ndi.fun.file.crc32` gives the cloud's encoding: base64 of the + 4-byte big-endian CRC-32. Expected values are zlib reference values. +* ``getFileUploadURL(..., checksum_crc32=...)`` sends ``checksumCRC32``. +* ``putFiles`` / ``putFileBytes`` send the signed ``x-amz-checksum-crc32`` + header, and refuse a malformed value. +* ``lookupFiles`` posts ``{"uids": [...]}``, de-duplicates, splits at the + server's 5000-uid limit and merges ``files`` / ``missing``. +* ``getSignedURLSetAll`` concatenates the pages' ``fileInfo``. +* ``uploadSingleFile`` sends the checksum by default. + +The live round trip is ``TestFileLifecycle`` in ``tests/test_cloud_live.py``. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from ndi.cloud.api import files as files_api +from ndi.fun.file import crc32, crc32ToBase64 + +DATASET = "507f1f77bcf86cd799439011" + + +# --------------------------------------------------------------------------- +# Encoding +# --------------------------------------------------------------------------- + + +class TestCrc32Encoding: + def test_big_endian_base64(self): + assert crc32ToBase64(0x3610A686) == "NhCmhg==" + assert crc32ToBase64(0) == "AAAAAA==" + assert crc32ToBase64(0xFFFFFFFF) == "/////w==" + + @pytest.mark.parametrize("bad", [-1, 2**32, 1.5, True, "NhCmhg=="]) + def test_rejects_values_outside_32_bits(self, bad): + with pytest.raises(ValueError): + crc32ToBase64(bad) + + def test_file_crc_of_known_text(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(b"hello") + assert crc32(str(p)) == "NhCmhg==" + + def test_empty_file(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(b"") + assert crc32(str(p)) == "AAAAAA==" + + def test_chunk_size_does_not_matter(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(b"The quick brown fox jumps over the lazy dog") + assert crc32(str(p)) == "QU+jOQ==" + assert crc32(str(p), chunk_size=1) == "QU+jOQ==" + assert crc32(str(p), chunk_size=7) == "QU+jOQ==" + + def test_many_chunks_and_high_bytes(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(bytes(range(256)) * 1000) + assert crc32(str(p), chunk_size=4096) == "/HCvGg==" + + def test_missing_file(self, tmp_path): + with pytest.raises(FileNotFoundError): + crc32(str(tmp_path / "nope")) + + +# --------------------------------------------------------------------------- +# Upload URL and PUT +# --------------------------------------------------------------------------- + + +class TestChecksummedUpload: + def test_upload_url_sends_checksum_query(self): + client = MagicMock() + client.get.return_value = {"url": "https://s3/x"} + + url = files_api.getFileUploadURL( + "org1", DATASET, "uid1", checksum_crc32="QU+jOQ==", client=client + ) + + assert url == "https://s3/x" + assert client.get.call_args.kwargs["params"] == {"checksumCRC32": "QU+jOQ=="} + + def test_upload_url_without_checksum_sends_no_query(self): + client = MagicMock() + client.get.return_value = {"url": "https://s3/x"} + + files_api.getFileUploadURL("org1", DATASET, "uid1", client=client) + + assert client.get.call_args.kwargs["params"] is None + + def test_upload_url_refuses_a_malformed_checksum(self): + with pytest.raises(ValueError): + files_api.getFileUploadURL( + "org1", DATASET, "uid1", checksum_crc32="abc", client=MagicMock() + ) + + def test_put_sends_the_checksum_header(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(b"hello") + resp = MagicMock(status_code=200) + with patch("requests.put", return_value=resp) as put: + assert files_api.putFiles( + "https://bucket.s3.amazonaws.com/k", str(p), checksum_crc32="NhCmhg==" + ) + headers = put.call_args.kwargs["headers"] + assert headers["x-amz-checksum-crc32"] == "NhCmhg==" + assert headers["Content-Type"] == "application/octet-stream" + + def test_put_without_checksum_sends_no_header(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(b"hello") + with patch("requests.put", return_value=MagicMock(status_code=200)) as put: + files_api.putFiles("https://bucket.s3.amazonaws.com/k", str(p)) + assert "x-amz-checksum-crc32" not in put.call_args.kwargs["headers"] + + def test_put_refuses_a_malformed_checksum(self, tmp_path): + p = tmp_path / "f.bin" + p.write_bytes(b"hello") + with patch("requests.put") as put, pytest.raises(ValueError): + files_api.putFiles( + "https://bucket.s3.amazonaws.com/k", str(p), checksum_crc32="not a crc" + ) + put.assert_not_called() + + def test_put_bytes_sends_the_checksum_header(self): + with patch("requests.put", return_value=MagicMock(status_code=200)) as put: + files_api.putFileBytes( + "https://bucket.s3.amazonaws.com/k", b"hello", checksum_crc32="NhCmhg==" + ) + assert put.call_args.kwargs["headers"]["x-amz-checksum-crc32"] == "NhCmhg==" + + +# --------------------------------------------------------------------------- +# lookupFiles +# --------------------------------------------------------------------------- + + +class _LookupServer: + """A fake POST /files/lookup over an in-memory {uid: record} map.""" + + def __init__(self, records): + self.records = records + self.calls = [] + + def post(self, endpoint, json=None, data=None, **path_params): + self.calls.append({"endpoint": endpoint, "json": json, "path": path_params}) + uids = json["uids"] + assert len(uids) <= files_api.LOOKUP_MAX_UIDS, "client must split long lists" + return { + "files": [self.records[u] for u in uids if u in self.records], + "missing": [u for u in uids if u not in self.records], + } + + +class TestLookupFiles: + def test_found_and_missing(self): + server = _LookupServer( + { + "u1": {"uid": "u1", "uploaded": True, "size": 5, "crc32": "NhCmhg=="}, + "u2": {"uid": "u2", "uploaded": False, "size": 0, "crc32": None}, + } + ) + + result = files_api.lookupFiles(DATASET, ["u1", "u2", "u3", "u1"], client=server) + + assert len(server.calls) == 1 + call = server.calls[0] + assert call["endpoint"] == "/datasets/{datasetId}/files/lookup" + assert call["path"] == {"datasetId": DATASET} + assert call["json"] == {"uids": ["u1", "u2", "u3"]} + assert [f["uid"] for f in result["files"]] == ["u1", "u2"] + assert result["files"][0]["crc32"] == "NhCmhg==" + assert result["files"][1]["crc32"] is None + assert result["missing"] == ["u3"] + + def test_splits_lists_over_the_server_limit(self): + server = _LookupServer({"u0": {"uid": "u0", "uploaded": True, "size": 1, "crc32": None}}) + uids = [f"u{i}" for i in range(files_api.LOOKUP_MAX_UIDS + 3)] + + result = files_api.lookupFiles(DATASET, uids, client=server) + + assert [len(c["json"]["uids"]) for c in server.calls] == [files_api.LOOKUP_MAX_UIDS, 3] + assert [f["uid"] for f in result["files"]] == ["u0"] + assert len(result["missing"]) == len(uids) - 1 + + def test_no_uids_makes_no_call(self): + server = _LookupServer({}) + assert files_api.lookupFiles(DATASET, [], client=server) == {"files": [], "missing": []} + assert server.calls == [] + + +# --------------------------------------------------------------------------- +# signed-url-set fileInfo +# --------------------------------------------------------------------------- + + +class TestSignedURLSetFileInfo: + def test_all_concatenates_file_info_across_pages(self): + pages = [ + { + "files": {"4192a3c0dd1b4e00_3fe8a1b2c3d4e5f6": "https://s3/a"}, + "fileInfo": [ + { + "uid": "4192a3c0dd1b4e00_3fe8a1b2c3d4e5f6", + "uploaded": True, + "size": 5, + "crc32": "NhCmhg==", + } + ], + "nextCursor": "c1", + }, + { + "files": {"b": "https://s3/b"}, + "fileInfo": [{"uid": "b", "uploaded": True, "size": 7, "crc32": None}], + }, + ] + client = MagicMock() + client.get.side_effect = pages + + merged = files_api.getSignedURLSetAll(DATASET, "doc1", client=client) + + assert [f["uid"] for f in merged["fileInfo"]] == [ + "4192a3c0dd1b4e00_3fe8a1b2c3d4e5f6", + "b", + ] + assert merged["fileInfo"][0]["crc32"] == "NhCmhg==" + + def test_all_without_file_info_gives_an_empty_list(self): + client = MagicMock() + client.get.return_value = {"files": {"a": "https://s3/a"}} + merged = files_api.getSignedURLSetAll(DATASET, "doc1", client=client) + assert merged["fileInfo"] == [] + + +# --------------------------------------------------------------------------- +# uploadSingleFile +# --------------------------------------------------------------------------- + + +class TestUploadSingleFileChecksum: + def _run(self, tmp_path, **kwargs): + from ndi.cloud.upload import uploadSingleFile + + p = tmp_path / "f.bin" + p.write_bytes(b"hello") + client = MagicMock() + client.config.org_id = "org1" + with ( + patch.object(files_api, "getFileUploadURL", return_value="https://s3/x") as get_url, + patch.object(files_api, "putFiles", return_value=True) as put, + ): + ok, err = uploadSingleFile(DATASET, "uid1", str(p), client=client, **kwargs) + assert ok, err + return get_url, put + + def test_sends_the_checksum_by_default(self, tmp_path): + get_url, put = self._run(tmp_path) + assert get_url.call_args.kwargs["checksum_crc32"] == "NhCmhg==" + assert put.call_args.kwargs["checksum_crc32"] == "NhCmhg==" + + def test_checksum_can_be_turned_off(self, tmp_path): + get_url, put = self._run(tmp_path, checksum=False) + assert get_url.call_args.kwargs["checksum_crc32"] == "" + assert put.call_args.kwargs["checksum_crc32"] == "" diff --git a/tests/test_cloud_live.py b/tests/test_cloud_live.py index 0a16f1d3..c7b80848 100644 --- a/tests/test_cloud_live.py +++ b/tests/test_cloud_live.py @@ -796,6 +796,145 @@ def test_file_details_has_download_url(self, client, fresh_dataset, cloud_config assert details.get("downloadUrl") +# =========================================================================== +# TestFileChecksums -- matches MATLAB FilesTest checksum / lookupFiles tests +# (NDI Cloud ndi-cloud-node#162, NDI-matlab#1025) +# =========================================================================== + + +def _skip_unless_server_has_file_lookup(client, dataset_id): + """Skip when this environment predates POST /files/lookup (#162). + + The route and the crc32 recording ship together; a 404 on a dataset that + exists means the server has neither yet. + """ + from ndi.cloud.api.files import lookupFiles + from ndi.cloud.exceptions import CloudNotFoundError + + try: + lookupFiles(dataset_id, ["probe_uid"], client=client) + except CloudNotFoundError: + pytest.skip("This NDI Cloud environment predates POST /files/lookup (#162)") + + +def _wait_for_lookup(client, dataset_id, uid, missing_uid=None): + """Poll lookupFiles until *uid* is uploaded (the S3 event is async).""" + from ndi.cloud.api.files import lookupFiles + + uids = [uid] + ([missing_uid] if missing_uid else []) + result = {} + for wait in (3, 5, 10, 15): + time.sleep(wait) + result = lookupFiles(dataset_id, uids, client=client) + found = [f for f in result["files"] if f.get("uid") == uid] + if found and found[0].get("uploaded"): + break + return result + + +class TestFileChecksums: + CONTENT = b"Checksum test file for NDI Cloud #162." + + def test_upload_with_checksum_round_trip(self, client, cloud_config, fresh_dataset, tmp_path): + """A checksum sent with the upload is returned as crc32 everywhere.""" + from ndi.cloud.api.files import ( + getFileDetails, + getFileUploadURL, + listFiles, + putFiles, + ) + from ndi.fun.file import crc32 + + _skip_unless_server_has_file_lookup(client, fresh_dataset) + path = tmp_path / "payload.bin" + path.write_bytes(self.CONTENT) + local_crc = crc32(str(path)) + file_uid = "pytest-crc32-roundtrip" + missing_uid = "pytest-crc32-never-uploaded" + + url = getFileUploadURL( + cloud_config.org_id, fresh_dataset, file_uid, checksum_crc32=local_crc, client=client + ) + assert putFiles(url, str(path), checksum_crc32=local_crc) + + result = _wait_for_lookup(client, fresh_dataset, file_uid, missing_uid) + assert [f["uid"] for f in result["files"]] == [file_uid] + rec = result["files"][0] + assert rec["uploaded"] is True + assert rec["size"] == len(self.CONTENT) + assert rec["crc32"] == local_crc + assert result["missing"] == [missing_uid] + + details = getFileDetails(fresh_dataset, file_uid, client=client) + assert details.get("crc32") == local_crc + + rows = list(listFiles(fresh_dataset, client=client).get("files", [])) + row = [f for f in rows if f.get("uid") == file_uid] + assert row and row[0].get("crc32") == local_crc + + def test_upload_with_wrong_checksum_is_rejected( + self, client, cloud_config, fresh_dataset, tmp_path + ): + """S3 refuses content whose CRC-32 does not match the signed checksum.""" + from ndi.cloud.api.files import getFileUploadURL, putFiles + from ndi.cloud.exceptions import CloudUploadError + from ndi.fun.file import crc32, crc32ToBase64 + + _skip_unless_server_has_file_lookup(client, fresh_dataset) + path = tmp_path / "payload.bin" + path.write_bytes(self.CONTENT) + wrong = crc32ToBase64(0) + assert crc32(str(path)) != wrong + + url = getFileUploadURL( + cloud_config.org_id, + fresh_dataset, + "pytest-crc32-wrong", + checksum_crc32=wrong, + client=client, + ) + with pytest.raises(CloudUploadError): + putFiles(url, str(path), checksum_crc32=wrong) + + def test_upload_single_file_records_checksum(self, client, fresh_dataset, tmp_path): + """ndi.cloud.upload.uploadSingleFile sends the checksum by default.""" + from ndi.cloud.upload import uploadSingleFile + from ndi.fun.file import crc32 + + _skip_unless_server_has_file_lookup(client, fresh_dataset) + path = tmp_path / "payload.bin" + path.write_bytes(self.CONTENT) + file_uid = "pytest-crc32-single" + + ok, err = uploadSingleFile(fresh_dataset, file_uid, str(path), client=client) + assert ok, err + + result = _wait_for_lookup(client, fresh_dataset, file_uid) + assert result["files"] and result["files"][0]["crc32"] == crc32(str(path)) + + def test_lookup_files_rejects_an_invalid_body(self, client, fresh_dataset): + """The server refuses more than 5000 uids, or a body without uids.""" + from ndi.cloud.exceptions import CloudAPIError + + _skip_unless_server_has_file_lookup(client, fresh_dataset) + for body in ({"uids": [f"u{i}" for i in range(5001)]}, {"notUids": 1}): + with pytest.raises(CloudAPIError) as exc_info: + client.post( + "/datasets/{datasetId}/files/lookup", json=body, datasetId=fresh_dataset + ) + assert exc_info.value.status_code == 400 + + def test_lookup_files_splits_long_lists(self, client, fresh_dataset): + """More than one server call's worth of uids, none uploaded.""" + from ndi.cloud.api.files import LOOKUP_MAX_UIDS, lookupFiles + + _skip_unless_server_has_file_lookup(client, fresh_dataset) + uids = [f"nouid{i}" for i in range(LOOKUP_MAX_UIDS + 3)] + result = lookupFiles(fresh_dataset, uids, client=client) + assert result["files"] == [] + assert sorted(result["missing"]) == sorted(uids) + + # =========================================================================== # TestNDIQuery -- matches MATLAB testNdiQuery # =========================================================================== diff --git a/tests/test_phase2_gaps.py b/tests/test_phase2_gaps.py index d200ec11..eab60d5a 100644 --- a/tests/test_phase2_gaps.py +++ b/tests/test_phase2_gaps.py @@ -440,8 +440,11 @@ def test_direct_upload_success(self): with patch.dict("sys.modules", {"ndi.cloud.api.files": mock_files}): with patch("ndi.cloud.api.files", mock_files): + # The file is never read here (the upload API is mocked), so + # skip the CRC-32 that uploadSingleFile computes by default; + # tests/test_cloud_api_files_crc32.py covers that path. success, err = uploadSingleFile( - "ds-123", "file-uid-1", "/tmp/test.dat", client=client + "ds-123", "file-uid-1", "/tmp/test.dat", checksum=False, client=client ) assert success is True @@ -457,8 +460,10 @@ def test_upload_failure(self): with patch.dict("sys.modules", {"ndi.cloud.api.files": mock_files}): with patch("ndi.cloud.api.files", mock_files): + # As above: the file does not exist, so skip the default CRC-32 + # and let the mocked network error be what fails. success, err = uploadSingleFile( - "ds-123", "file-uid-1", "/tmp/test.dat", client=client + "ds-123", "file-uid-1", "/tmp/test.dat", checksum=False, client=client ) assert success is False From e7ca3b021094efa45853f6a4feb6d772ff879d51 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 23:54:52 +0000 Subject: [PATCH 17/21] Fail, not skip, live checksum tests on a server without #162 A skip hid whether the tests ran at all; an environment without the lookup route or fileInfo now fails these tests. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SwsqFA9Zs1HGm6YM4g2XRV --- tests/test_cloud_live.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/tests/test_cloud_live.py b/tests/test_cloud_live.py index c7b80848..c98089ff 100644 --- a/tests/test_cloud_live.py +++ b/tests/test_cloud_live.py @@ -802,21 +802,6 @@ def test_file_details_has_download_url(self, client, fresh_dataset, cloud_config # =========================================================================== -def _skip_unless_server_has_file_lookup(client, dataset_id): - """Skip when this environment predates POST /files/lookup (#162). - - The route and the crc32 recording ship together; a 404 on a dataset that - exists means the server has neither yet. - """ - from ndi.cloud.api.files import lookupFiles - from ndi.cloud.exceptions import CloudNotFoundError - - try: - lookupFiles(dataset_id, ["probe_uid"], client=client) - except CloudNotFoundError: - pytest.skip("This NDI Cloud environment predates POST /files/lookup (#162)") - - def _wait_for_lookup(client, dataset_id, uid, missing_uid=None): """Poll lookupFiles until *uid* is uploaded (the S3 event is async).""" from ndi.cloud.api.files import lookupFiles @@ -845,7 +830,6 @@ def test_upload_with_checksum_round_trip(self, client, cloud_config, fresh_datas ) from ndi.fun.file import crc32 - _skip_unless_server_has_file_lookup(client, fresh_dataset) path = tmp_path / "payload.bin" path.write_bytes(self.CONTENT) local_crc = crc32(str(path)) @@ -880,7 +864,6 @@ def test_upload_with_wrong_checksum_is_rejected( from ndi.cloud.exceptions import CloudUploadError from ndi.fun.file import crc32, crc32ToBase64 - _skip_unless_server_has_file_lookup(client, fresh_dataset) path = tmp_path / "payload.bin" path.write_bytes(self.CONTENT) wrong = crc32ToBase64(0) @@ -901,7 +884,6 @@ def test_upload_single_file_records_checksum(self, client, fresh_dataset, tmp_pa from ndi.cloud.upload import uploadSingleFile from ndi.fun.file import crc32 - _skip_unless_server_has_file_lookup(client, fresh_dataset) path = tmp_path / "payload.bin" path.write_bytes(self.CONTENT) file_uid = "pytest-crc32-single" @@ -916,7 +898,6 @@ def test_lookup_files_rejects_an_invalid_body(self, client, fresh_dataset): """The server refuses more than 5000 uids, or a body without uids.""" from ndi.cloud.exceptions import CloudAPIError - _skip_unless_server_has_file_lookup(client, fresh_dataset) for body in ({"uids": [f"u{i}" for i in range(5001)]}, {"notUids": 1}): with pytest.raises(CloudAPIError) as exc_info: client.post( @@ -928,7 +909,6 @@ def test_lookup_files_splits_long_lists(self, client, fresh_dataset): """More than one server call's worth of uids, none uploaded.""" from ndi.cloud.api.files import LOOKUP_MAX_UIDS, lookupFiles - _skip_unless_server_has_file_lookup(client, fresh_dataset) uids = [f"nouid{i}" for i in range(LOOKUP_MAX_UIDS + 3)] result = lookupFiles(fresh_dataset, uids, client=client) assert result["files"] == [] From d469c5b5e97f365e58221b018d5eccec07cb430b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 12:57:27 +0000 Subject: [PATCH 18/21] Drop matlab_branch markers now that NDI-matlab#1025 has merged lookupFiles, crc32 and crc32ToBase64 are on NDI-matlab main, so the ordinary bridge checks apply. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01SwsqFA9Zs1HGm6YM4g2XRV --- src/ndi/cloud/api/ndi_matlab_python_bridge.yaml | 1 - src/ndi/fun/ndi_matlab_python_bridge.yaml | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml index 4b587bd3..8b6131b6 100644 --- a/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml +++ b/src/ndi/cloud/api/ndi_matlab_python_bridge.yaml @@ -983,7 +983,6 @@ functions: - name: lookupFiles matlab_path: "+ndi/+cloud/+api/+files/lookupFiles.m" matlab_last_sync_hash: "8fa6ea5e" - matlab_branch: "claude/file-crc32-lookup" python_path: "ndi/cloud/api/files.py" input_arguments: - name: dataset_id diff --git a/src/ndi/fun/ndi_matlab_python_bridge.yaml b/src/ndi/fun/ndi_matlab_python_bridge.yaml index 68eb096c..5edc5867 100644 --- a/src/ndi/fun/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/ndi_matlab_python_bridge.yaml @@ -1302,7 +1302,6 @@ functions: type: function matlab_path: "+ndi/+fun/+file/crc32.m" matlab_last_sync_hash: "8fa6ea5e" - matlab_branch: "claude/file-crc32-lookup" python_path: "ndi/fun/file.py" input_arguments: - name: file_path @@ -1325,7 +1324,6 @@ functions: type: function matlab_path: "+ndi/+fun/+file/crc32ToBase64.m" matlab_last_sync_hash: "8fa6ea5e" - matlab_branch: "claude/file-crc32-lookup" python_path: "ndi/fun/file.py" input_arguments: - name: value From fee001d158f5b54411a9b5ee2f301a7df94d3a75 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 20:04:55 +0000 Subject: [PATCH 19/21] Bridge: LightsheetZarrManager examined against NDI-matlab fb10169b Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- .../gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml index 0ebfcfd1..5b63506e 100644 --- a/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml +++ b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml @@ -35,7 +35,7 @@ functions: # ----------------------------------------------------------------------- - name: LightsheetZarrManager matlab_path: "+ndi/+gui/+app/LightsheetZarrManager.m" - matlab_last_sync_hash: "2f9b0eaf" + matlab_last_sync_hash: "fb10169b" matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: matlab_only decision_log: >- @@ -62,6 +62,12 @@ functions: matlab_only; the launched Python viewer already honours NDI_LIGHTSHEET_DEBUG. + 2026-10-13: examined against fb10169b (was 2f9b0eaf). The range adds + the main merge (CRC32 file support) and fb10169b, where the View + dialog's command preview shows the exact logged shell line + (viewerShellCommand with the log named when the dialog opens) and + Launch reuses that log file. Still matlab_only. + # ----------------------------------------------------------------------- # CLI (napariViewLightsheet). # ----------------------------------------------------------------------- From 4c941d420f33e98de34fda23d6b4c59e8f1632c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 20:42:15 +0000 Subject: [PATCH 20/21] Lightsheet fetcher: resolve chunks without a session, 32 workers, wide dask pool A tile takes seconds on the cloud, almost all of it waiting on the network, but only ~CPU-count tiles were ever in flight, and every fetcher worker opened a whole NDI session at launch just so it could call database_openbinarydoc on its own thread. Resolving a chunk needs no database. The level document is in memory, its series manifest names each member's uid, and DID's cached_path_for_uid (documented as safe from any thread) finds a member on disk with no SQL. A member not on disk is now fetched through download_file_from_cloud with the same context DID's open_doc passes (manifest location as source, seriesName and member uid in the context), into DID's file cache under its uid, so DID and later lookups find it exactly where open_doc would have put it. The session path stays as the fallback (manifest not on disk yet, no ndic:// location), and a worker opens a session only on first need. NDI_LIGHTSHEET_DIRECT=0 turns the session-free path off. - Default worker count 8 -> 32 (NDI_LIGHTSHEET_WORKERS still overrides). - configureDaskPool: napari reads through dask's threaded scheduler, whose pool defaults to one thread per CPU; the viewer now sizes it to the worker count so napari can keep every worker busy. - fetch_cloud_file records per-thread stage timing (signing, batch or per-file, transfer, bytes); under NDI_LIGHTSHEET_DEBUG each slow resolve logs "[lightsheet] fetched chunk.bin_N in Xs: sign ..s, transfer ..s, .. MB, .. MB/s", so the log says where a tile's time goes. - stats_summary reports how many resolves went via a session. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/cloud/filehandler.py | 35 ++ src/ndi/gui/app/lightsheetZarr/viewer.py | 23 +- src/ndi/pyramid/multiscale.py | 396 ++++++++++++++++-- src/ndi/pyramid/ndi_matlab_python_bridge.yaml | 12 + tests/test_pyramid_fetch_direct.py | 245 +++++++++++ 5 files changed, 674 insertions(+), 37 deletions(-) create mode 100644 tests/test_pyramid_fetch_direct.py diff --git a/src/ndi/cloud/filehandler.py b/src/ndi/cloud/filehandler.py index 95817b18..f6a3380d 100644 --- a/src/ndi/cloud/filehandler.py +++ b/src/ndi/cloud/filehandler.py @@ -395,6 +395,22 @@ def watchFetches(observer): _fetch_observer = previous +#: Per-thread timing of the last :func:`fetch_cloud_file` on that thread. +_last_fetch = threading.local() + + +def lastFetchTiming() -> dict | None: + """Timing of this thread's most recent :func:`fetch_cloud_file`, or None. + + ``{"sign_s", "batch", "transfer_s", "bytes", "ok"}``: seconds spent + getting a signed URL (and whether it came from the batch cache), seconds + spent downloading, and bytes written. Per thread, so a fetcher worker + reads back the fetch it just made and no other. Used by the lightsheet + viewer's debug log to say where a slow tile's time went. + """ + return getattr(_last_fetch, "timing", None) + + def fetch_cloud_file( ndic_uri: str, target_path: str | Path, @@ -440,6 +456,10 @@ def fetch_cloud_file( if client is None: client = get_or_create_cloud_client() + import time as _time + + _last_fetch.timing = None + t_sign = _time.monotonic() download_url = "" url_came_from_batch = False if ndi_document_id: @@ -465,6 +485,8 @@ def fetch_cloud_file( raise CloudError(f"No downloadUrl in file details for {ndic_uri}. Response: {details}") + sign_s = _time.monotonic() - t_sign + # Stream download to temp file, then atomic rename target = Path(target_path) target.parent.mkdir(parents=True, exist_ok=True) @@ -480,6 +502,7 @@ def fetch_cloud_file( call_kwargs: dict = {"timeout": 300} if error_out is not None: call_kwargs["error_out"] = error_out + t_get = _time.monotonic() if observer is None: success = getFile(download_url, tmp_path, **call_kwargs) else: @@ -494,6 +517,18 @@ def fetch_cloud_file( finally: observer("done", ndic_uri, 0, None) + try: + nbytes = tmp_path.stat().st_size if success else 0 + except OSError: + nbytes = 0 + _last_fetch.timing = { + "sign_s": sign_s, + "batch": url_came_from_batch, + "transfer_s": _time.monotonic() - t_get, + "bytes": nbytes, + "ok": bool(success), + } + if success: tmp_path.rename(target) logger.debug("Cloud file cached: %s", target) diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index 8903ebf0..a337e79d 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -578,9 +578,26 @@ def openPyramid( specs = loader.specs fetcher = loader.fetcher - # Warm the worker pool now, off the main thread. The one-off - # session-open cost then overlaps with napari.Viewer()'s Qt startup - # rather than serialising against the first fetch. + # napari reads through dask, whose thread pool defaults to one thread + # per CPU -- sized for computation, but a chunk read is seconds of + # waiting on the network. Widen it to the fetcher's worker count so + # napari can keep every worker busy. + try: + from ndi.pyramid.multiscale import configureDaskPool + + n_pool = configureDaskPool(getattr(fetcher, "_workers", 1)) + print( + f"[lightsheet] fetch pool: {getattr(fetcher, '_workers', 1)} workers, " + f"dask pool {n_pool} threads", + file=sys.stderr, + flush=True, + ) + except Exception as exc: # noqa: BLE001 - the default pool still works + print(f"[lightsheet] could not widen the dask pool ({exc})", file=sys.stderr) + + # Start the worker threads now, off the main thread, so the first + # fetch does not pay for it. Workers open no session up front: chunks + # resolve session-free, and a worker opens one only if it must. with progress.stage("connecting to image store"): try: fetcher.warm() diff --git a/src/ndi/pyramid/multiscale.py b/src/ndi/pyramid/multiscale.py index 56c0ab0f..62bdf609 100644 --- a/src/ndi/pyramid/multiscale.py +++ b/src/ndi/pyramid/multiscale.py @@ -58,6 +58,12 @@ class StaleChunkRead(Exception): """ +# Returned by _ChunkFetcher._resolveDirect when the session-free path cannot +# answer (no manifest on disk yet, no cloud location, not a series member) and +# the caller should go through the session's database_openbinarydoc instead. +_FALLBACK = object() + + class _Job: """One queued or running chunk fetch, shared by everyone waiting on it.""" @@ -177,16 +183,31 @@ class _ChunkFetcher: resolving is fetching. Eager resolution of every chunk of every level then downloads the whole pyramid before napari draws a pixel. - This class is the middle ground: dedicated fetcher threads each own - their own re-opened session. Delayed tasks call ``chunkPath(doc, name)`` + This class is the middle ground: dedicated fetcher threads resolve + chunks off the caller's thread. Delayed tasks call ``chunkPath(doc, name)`` from whichever thread they run on; the fetcher dispatches through its pool and returns the resolved local path. Resolved paths are memoised per (doc_id, filename) so a re-render never reaches the fetch threads again -- a cached hit is one ``os.path.exists`` from any thread. - ``workers`` DEFAULTS TO 1: one session opened once, exactly as if a - separate process held it. Cloud fetches are latency-bound, so raising - it buys overlap; it also costs another whole session per worker. + SESSION-FREE MEMBER RESOLUTION. A chunk is a file series member, and + resolving one needs no database at all: the level document is already + in memory, its manifest names each member's uid, and DID's + ``cached_path_for_uid`` finds a member on disk with no SQL (it is + documented as safe from any thread). A member that is not on disk is + fetched through NDI's cloud handler -- the same + ``download_file_from_cloud`` DID itself would call, with the same + context -- and lands in DID's file cache under its uid, exactly where + ``open_doc`` would have put it. So workers are plain threads, and a + worker only opens a session of its own (lazily, on first need) when a + chunk has to go the long way: the manifest is not on disk yet, or the + document records no cloud location. ``NDI_LIGHTSHEET_DIRECT=0`` turns + the session-free path off. + + ``workers`` defaults to 1 here; the viewer passes + :func:`_default_workers`. Cloud fetches are latency-bound, so many + workers buy overlap, and with session-free resolution an extra worker + costs one idle thread. """ def __init__(self, session, workers: int = 1): @@ -240,6 +261,38 @@ def __init__(self, session, workers: int = 1): # See Waltham-Data-Science/NDI-python#322 and the strict-mode # rationale in ndi.cloud.batch_signed_url.BatchScopeUnreachable. self._unreachable_exc: BatchScopeUnreachable | None = None + # Session-free member resolution (see the class docstring). The + # file directory is read once, here, on the owner thread; the + # per-document and per-series caches hold only in-memory data + # and manifest uid lists. + self._direct = os.environ.get("NDI_LIGHTSHEET_DIRECT", "1").strip().lower() not in ( + "0", + "false", + "off", + ) + self._file_dir = self._fileDir(session) + self._did_docs: dict[str, Any] = {} + self._series: dict[tuple[str, str], tuple] = {} + self._direct_hits = 0 + self._direct_fetches = 0 + self._fallbacks = 0 + + @staticmethod + def _fileDir(session) -> str | None: + """DID's per-database file directory, or None if not reachable. + + A session exposes its database directly; a dataset holds one + through its inner session. + """ + for holder in (session, getattr(session, "_session", None)): + database = getattr(holder, "database", None) if holder is not None else None + try: + path = database.binary_path if database is not None else None + except Exception: # noqa: BLE001 - not DID-backed + path = None + if path: + return str(path) + return None @staticmethod def _reopener(session): @@ -259,7 +312,7 @@ def _reopener(session): def _ensureWorkers(self) -> bool: """Start the fetcher threads once. False when the session cannot be reopened for another thread (then everything runs inline).""" - if self._reopen is None: + if self._reopen is None and not self._direct: return False with self._cv: if self._threads or self._closed: @@ -275,12 +328,11 @@ def _ensureWorkers(self) -> bool: return True def _workerLoop(self) -> None: - try: - self._local.session = self._reopen() - finally: - with self._cv: - self._threads_ready += 1 - self._cv.notify_all() + # No session here: most chunks resolve without one, and a worker + # that needs one opens it on first use (_threadSession). + with self._cv: + self._threads_ready += 1 + self._cv.notify_all() while True: with self._cv: job = None @@ -406,12 +458,52 @@ def staleReleased(self) -> int: """How many blocking reads were released as stale so far.""" return self._stale_released + def _threadSession(self): + """The session this thread may use, opening one on first need. + + The owner thread uses the caller's session. A worker reopens its + own the first time a chunk cannot be resolved session-free. A + session that cannot be reopened (no path) is used as-is, as before: + that is the directory-less case the inline fallback serves. + """ + if threading.get_ident() == self._owner or self._reopen is None: + return self._session + s = getattr(self._local, "session", None) + if s is None: + s = self._reopen() + self._local.session = s + return s + def _resolve(self, doc, filename) -> str | None: - s = getattr(self._local, "session", None) or self._session import time as _time t0 = _time.monotonic() + if self._direct: + try: + direct = self._resolveDirect(doc, filename) + except BatchScopeUnreachable: + with self._stats_lock: + self._resolve_none += 1 + raise + except Exception as exc: # noqa: BLE001 - same contract as below + with self._stats_lock: + self._resolve_none += 1 + if os.environ.get("NDI_LIGHTSHEET_DEBUG"): + import sys + + print( + f"[lightsheet] chunk fetch failed for {filename}: " + f"{type(exc).__name__}: {exc}", + file=sys.stderr, + ) + return None + if direct is not _FALLBACK: + self._recordResolve(filename, _time.monotonic() - t0, direct) + return direct + with self._stats_lock: + self._fallbacks += 1 try: + s = self._threadSession() fh = s.database_openbinarydoc(doc, filename) except BatchScopeUnreachable: # A systemic failure of the batch signed-URL path is not a @@ -440,29 +532,212 @@ def _resolve(self, doc, filename) -> str | None: return None try: path = getattr(fh, "fullpathfilename", None) - dt = _time.monotonic() - t0 - # database_openbinarydoc has two happy paths: a local-cache - # hit (near instant, microseconds; no cloud roundtrip) and - # a cloud fetch (tens of ms to seconds). Split the buckets - # so a summary later can say how many of each we saw and - # what each one costs. - # - # 50 ms is comfortably above local disk latency and well - # under any credible cloud RTT; picking a threshold rather - # than routing on internal state keeps the classifier - # independent of DID's evolving fast paths. - with self._stats_lock: - if dt < 0.05: - self._resolve_times_cache.append(dt) - else: - self._resolve_times_fetch.append(dt) - return str(path) if path else None + path = str(path) if path else None + self._recordResolve(filename, _time.monotonic() - t0, path) + return path finally: try: s.database_closebinarydoc(fh) except Exception: pass + def _recordResolve(self, filename, dt: float, path) -> None: + """Bucket one resolve's time; under debug, say what a fetch cost. + + database_openbinarydoc and the session-free path both have two + happy paths: a local hit (microseconds) and a cloud fetch (tens of + ms to seconds). 50 ms is comfortably above local disk latency and + well under any credible cloud round trip; a threshold keeps the + classifier independent of either path's internals. + """ + with self._stats_lock: + if dt < 0.05: + self._resolve_times_cache.append(dt) + else: + self._resolve_times_fetch.append(dt) + if dt >= 0.05 and os.environ.get("NDI_LIGHTSHEET_DEBUG"): + import sys + + from ndi.cloud.filehandler import lastFetchTiming + + t = lastFetchTiming() + if t: + size = t.get("bytes", 0) or 0 + xfer = t.get("transfer_s", 0.0) or 0.0 + rate = f", {size / xfer / 1e6:.1f} MB/s" if xfer > 0 and size else "" + detail = ( + f"sign {t.get('sign_s', 0.0):.2f}s " + f"({'batch' if t.get('batch') else 'per-file'}), " + f"transfer {xfer:.2f}s, {size / 1e6:.2f} MB{rate}" + ) + else: + detail = "no cloud timing recorded" + print( + f"[lightsheet] fetched {filename} in {dt:.2f}s: {detail} " + f"(thread {threading.current_thread().name})", + file=sys.stderr, + flush=True, + ) + + # ------------------------------------------------------------ direct path + + def _resolveDirect(self, doc, filename): + """Resolve a series member with no database, or return _FALLBACK. + + Local first, through DID's ``cached_path_for_uid`` (global file + cache, then this database's file directory -- the order + ``open_doc`` uses). A member that is not on disk is fetched through + the cloud handler into DID's file cache. Returns the local path, + None for a member the manifest records as absent (sparse; what + ``open_doc`` reports as NoSuchMember), or _FALLBACK when this path + cannot answer and the session must. + """ + from did.file import cached_path_for_uid + + found = self._seriesFor(doc, filename) + if found is None: + return _FALLBACK + stem, index, uids, manifest_location, doc_id = found + if index > len(uids) or not uids[index - 1]: + return None + member_uid = str(uids[index - 1]) + roots = [self._file_dir] if self._file_dir else [] + local = cached_path_for_uid(member_uid, additional_roots=roots) + if local: + with self._stats_lock: + self._direct_hits += 1 + return str(local) + if not manifest_location: + return _FALLBACK + path = self._fetchMember(doc_id, filename, stem, member_uid, manifest_location) + with self._stats_lock: + self._direct_fetches += 1 + return path + + def _seriesFor(self, doc, filename): + """(stem, index, member uids, manifest ndic location, doc id), or None. + + Reads only the document in memory and the manifest on disk. The + manifest's uid list is read once per (document, series) and kept; + a manifest that is not on disk yet is not remembered as missing, + so once the session path has fetched it the next call finds it. + """ + try: + from did.document import Document as DIDDocument + from did.file import cached_path_for_uid, read_series_manifest + except ImportError: + return None + doc_id = getattr(doc, "id", None) + if not doc_id: + return None + did_doc = self._did_docs.get(doc_id) + if did_doc is None: + props = getattr(doc, "document_properties", None) + if not isinstance(props, dict): + return None + did_doc = DIDDocument(props) + self._did_docs[doc_id] = did_doc + stem, index = did_doc.series_member_of(filename) + if not stem or index is None or index < 1: + return None + entry = self._series.get((doc_id, stem)) + if entry is None: + roots = [self._file_dir] if self._file_dir else [] + manifest_path = None + for uid in did_doc.file_uids(stem): + manifest_path = cached_path_for_uid(uid, additional_roots=roots) + if manifest_path: + break + if not manifest_path: + return None + try: + uids = [ + str(u) if u else "" for u in read_series_manifest(manifest_path).get("uids", []) + ] + except Exception: # noqa: BLE001 - let the session path report it + return None + entry = (uids, _ndicLocation(did_doc.document_properties, stem)) + self._series[(doc_id, stem)] = entry + uids, location = entry + return stem, index, uids, location, doc_id + + def _fetchMember(self, doc_id, filename, stem, member_uid, manifest_location) -> str: + """Fetch one member through the cloud handler into DID's file cache. + + The same call DID's ``open_doc`` makes -- the manifest's location + as the source and the member in the context, so the handler fetches + the member's own object through the batch signed-URL cache -- into + a unique scratch file in DID's temp folder, then moved into the + global file cache under the member's uid. A cache that cannot take + it (unwritable, or a uid of the wrong width) leaves the file in + this database's file directory instead; either way + ``cached_path_for_uid`` finds it next time, in this process or the + next, and so does DID. + """ + import tempfile + + from did.common import PathConstants + + from ndi.cloud.filehandler import download_file_from_cloud + + temp_dir = PathConstants().temppath + os.makedirs(temp_dir, exist_ok=True) + fd, scratch = tempfile.mkstemp(dir=temp_dir, prefix=f"{member_uid}.", suffix=".part") + os.close(fd) + os.remove(scratch) + try: + download_file_from_cloud( + scratch, + manifest_location, + { + "documentId": doc_id, + "filename": filename, + "seriesName": stem, + "uid": member_uid, + "mode": "open", + }, + ) + if not os.path.isfile(scratch): + raise FileNotFoundError( + f"the cloud handler produced no file for {filename} (uid {member_uid})" + ) + return self._keepFetched(scratch, member_uid) + except BaseException: + try: + os.remove(scratch) + except OSError: + pass + raise + + def _keepFetched(self, scratch: str, member_uid: str) -> str: + """Move a fetched member to where cached_path_for_uid will find it.""" + import struct + + try: + from did.common import get_cache + + cache = get_cache() + if len(member_uid) == cache.file_name_characters: + if cache.is_file(member_uid): + existing = cache.full_path(member_uid) + if os.path.isfile(existing): + # Another fetch of the same uid won the race; same bytes. + os.remove(scratch) + return existing + cache.remove_file(member_uid) + cache.add_file(scratch, member_uid) + if os.path.exists(scratch): + os.remove(scratch) + return cache.full_path(member_uid) + except (OSError, ValueError, struct.error): + pass + if self._file_dir: + os.makedirs(self._file_dir, exist_ok=True) + dest = os.path.join(self._file_dir, member_uid) + os.replace(scratch, dest) + return dest + return scratch + def stats_summary(self) -> str: """One-line summary of resolve timings for the last session. @@ -481,11 +756,13 @@ def stats_summary(self) -> str: mean_cache_ms = 1000.0 * (sum(self._resolve_times_cache) / n_cache if n_cache else 0.0) mean_fetch_s = sum(self._resolve_times_fetch) / n_fetch if n_fetch else 0.0 max_fetch_s = max(self._resolve_times_fetch) if n_fetch else 0.0 + n_fallback = self._fallbacks return ( f"resolves: {n_cache} cache-hits (mean {mean_cache_ms:.1f}ms), " f"{n_fetch} cloud-fetches (mean {mean_fetch_s:.2f}s, max " f"{max_fetch_s:.2f}s), {n_none} failures, " - f"{self._stale_released} stale reads released" + f"{self._stale_released} stale reads released, " + f"{n_fallback} via a session ({self._workers} workers)" ) def reset_stats(self) -> None: @@ -493,6 +770,9 @@ def reset_stats(self) -> None: self._resolve_times_cache.clear() self._resolve_times_fetch.clear() self._resolve_none = 0 + self._direct_hits = 0 + self._direct_fetches = 0 + self._fallbacks = 0 def chunkPath(self, doc, filename) -> str | None: """Resolve one chunk file to a local path, fetching it if remote. @@ -642,15 +922,63 @@ def close(self) -> None: self._cv.notify_all() +_DEFAULT_WORKERS = 32 + + def _default_workers() -> int: - """Chunk-fetcher worker count. Env override for the cloud-latency case.""" + """Chunk-fetcher worker count; ``NDI_LIGHTSHEET_WORKERS`` overrides. + + 32 by default. A cloud tile is seconds of waiting on the network and + almost no CPU, and with session-free resolution a worker is just a + thread, so a wide pool is cheap and buys overlap. + """ try: - n = int(os.environ.get("NDI_LIGHTSHEET_WORKERS", "8")) + n = int(os.environ.get("NDI_LIGHTSHEET_WORKERS", str(_DEFAULT_WORKERS))) except ValueError: - return 8 + return _DEFAULT_WORKERS return max(1, n) +def configureDaskPool(workers: int) -> int: + """Size dask's thread pool for I/O-bound chunk reads; returns its size. + + napari computes slices through dask's threaded scheduler, whose pool + defaults to one thread per CPU -- right for computation, wrong here: + each chunk read mostly waits on the network, so the pool, not the + fetcher, caps how many tiles are in flight. This sets the pool to at + least ``workers`` threads (never fewer than the CPU count), process + wide, so napari's reads can keep every fetcher worker busy. + """ + from concurrent.futures import ThreadPoolExecutor + + import dask + + n = max(os.cpu_count() or 4, int(workers)) + dask.config.set(pool=ThreadPoolExecutor(n, thread_name_prefix="ndi-dask")) + return n + + +def _ndicLocation(props: dict, name: str) -> str: + """The ``ndic://`` location recorded for file ``name``, or "".""" + from ndi.cloud.filehandler import NDIC_SCHEME + + files = props.get("files") if isinstance(props, dict) else None + if not isinstance(files, dict): + return "" + file_info = files.get("file_info") + infos = file_info if isinstance(file_info, list) else [file_info] if file_info else [] + for info in infos: + if not isinstance(info, dict) or str(info.get("name", "")).lower() != name.lower(): + continue + locations = info.get("locations") + locs = locations if isinstance(locations, list) else [locations] if locations else [] + for loc in locs: + where = loc.get("location", "") if isinstance(loc, dict) else "" + if str(where).startswith(NDIC_SCHEME): + return str(where) + return "" + + def levelArrays( session: Any, pyramid_doc: Any, diff --git a/src/ndi/pyramid/ndi_matlab_python_bridge.yaml b/src/ndi/pyramid/ndi_matlab_python_bridge.yaml index 98c57a0e..438fa364 100644 --- a/src/ndi/pyramid/ndi_matlab_python_bridge.yaml +++ b/src/ndi/pyramid/ndi_matlab_python_bridge.yaml @@ -100,6 +100,18 @@ functions: One layer per channel. Kept here (rather than viewer.py) so a headless caller can build the ladders without a display. + - name: configureDaskPool + matlab_path: null + python_path: "ndi/pyramid/multiscale.py" + output_arguments: + - name: n + type_python: int + decision_log: >- + Python-only in this namespace. Widens dask's threaded-scheduler pool + (default: one thread per CPU) to the chunk fetcher's worker count, so + napari's I/O-bound chunk reads can keep every worker busy. MATLAB has + no dask; nothing to mirror. + - name: prefetchCoarsestLevel matlab_path: null python_path: "ndi/pyramid/multiscale.py" diff --git a/tests/test_pyramid_fetch_direct.py b/tests/test_pyramid_fetch_direct.py new file mode 100644 index 00000000..bc278f09 --- /dev/null +++ b/tests/test_pyramid_fetch_direct.py @@ -0,0 +1,245 @@ +"""Session-free chunk resolution in ndi.pyramid.multiscale._ChunkFetcher. + +A pyramid chunk is a file series member. Resolving one needs no database: +the document is in memory, its manifest names each member's uid, and DID's +cached_path_for_uid finds a member on disk with no SQL. So fetcher workers +are plain threads, and a worker opens a session of its own only when a chunk +must go the long way. These tests pin that down: + +* a member on disk resolves from a worker thread without any session being + reopened; +* a member not on disk is fetched through the cloud handler with DID's + context (seriesName, member uid) and lands where cached_path_for_uid + finds it; +* an absent (sparse) slot resolves to None, as open_doc's NoSuchMember; +* NDI_LIGHTSHEET_DIRECT=0 goes back through the session; +* configureDaskPool widens dask's pool to the worker count; +* fetch_cloud_file records per-stage timing for the debug log. +""" + +from __future__ import annotations + +import copy +import os +import threading + +import pytest + +from ndi.pyramid import multiscale +from ndi.pyramid.multiscale import _ChunkFetcher + +SERIES = "chunkdata.bin" +COUNT = 4 + + +def _content(i: int) -> bytes: + return bytes([(i * 37 + k) % 256 for k in range(64)]) + + +@pytest.fixture +def series_session(tmp_path): + """A local dataset holding one demoNDISeries document with COUNT members.""" + from did.document import Document as DIDDocument + + from ndi.dataset import ndi_dataset_dir + from ndi.document import ndi_document + + members = tmp_path / "members" + members.mkdir() + paths = [] + for i in range(COUNT): + p = members / f"chunk_{i + 1}.bin" + p.write_bytes(_content(i)) + paths.append(str(p)) + + local = ndi_dataset_dir("direct_ds", str(tmp_path / "ds")) + seed = ndi_document("demoNDISeries", **{"base.name": "direct_series", "demoNDISeries.value": 1}) + did_doc = DIDDocument(seed.document_properties) + did_doc.add_file_series(SERIES, paths) + doc = ndi_document(did_doc.document_properties) + local.database_add(doc) + stored = local.database_search( + __import__("ndi.query", fromlist=["ndi_query"]).ndi_query("base.id") == doc.id + )[0] + return local, stored + + +def _no_reopen(fetcher): + """Make any session reopen fail loudly: the direct path must not need one.""" + + def boom(): + raise AssertionError("a worker reopened a session") + + fetcher._reopen = boom + + +def _resolve_on_worker(fetcher, doc, filename): + out = {} + + def run(): + try: + out["path"] = fetcher.chunkPath(doc, filename) + except BaseException as exc: # noqa: BLE001 + out["exc"] = exc + + t = threading.Thread(target=run) + t.start() + t.join(30) + assert not t.is_alive(), "chunkPath did not return" + if "exc" in out: + raise out["exc"] + return out["path"] + + +def test_member_on_disk_resolves_without_a_session(series_session): + session, doc = series_session + fetcher = _ChunkFetcher(session, workers=4) + _no_reopen(fetcher) + try: + for i in range(COUNT): + path = _resolve_on_worker(fetcher, doc, f"{SERIES}_{i + 1}") + assert path is not None + with open(path, "rb") as f: + assert f.read() == _content(i), f"member {i + 1} resolved to the wrong file" + assert fetcher._fallbacks == 0 + finally: + fetcher.close() + + +def test_member_not_on_disk_is_fetched_through_the_cloud_handler( + series_session, monkeypatch, tmp_path +): + from did.file import cached_path_for_uid, read_series_manifest + + import ndi.cloud.filehandler as fh + + session, doc = series_session + fetcher = _ChunkFetcher(session, workers=2) + _no_reopen(fetcher) + + # The manifest's uid list, and the member we will make remote. + props = copy.deepcopy(doc.document_properties) + manifest_uid = None + for info in props["files"]["file_info"]: + if info["name"] == SERIES: + locs = info["locations"] + locs = locs if isinstance(locs, list) else [locs] + manifest_uid = locs[0]["uid"] + # Record a cloud copy of the manifest, as a downloaded dataset does. + locs.append( + { + "location": f"ndic://dsX/{manifest_uid}", + "location_type": "ndicloud", + "uid": manifest_uid, + "ingest": 0, + "delete_original": 0, + } + ) + info["locations"] = locs + assert manifest_uid + manifest_path = cached_path_for_uid(manifest_uid, additional_roots=[fetcher._file_dir]) + uids = read_series_manifest(manifest_path)["uids"] + member_uid = uids[1] + on_disk = cached_path_for_uid(member_uid, additional_roots=[fetcher._file_dir]) + os.remove(on_disk) + + calls = [] + + def fake_download(dest, source, context=None): + calls.append((source, dict(context or {}))) + with open(dest, "wb") as f: + f.write(b"fetched-" + _content(1)) + + monkeypatch.setattr(fh, "download_file_from_cloud", fake_download) + + # Keep the fetched file out of the user's global DID cache. + import did.common + + def no_cache(): + raise OSError("no cache in tests") + + monkeypatch.setattr(did.common, "get_cache", no_cache) + + remote_doc = type(doc)(props) + try: + path = _resolve_on_worker(fetcher, remote_doc, f"{SERIES}_2") + with open(path, "rb") as f: + assert f.read() == b"fetched-" + _content(1) + assert len(calls) == 1 + source, context = calls[0] + assert source == f"ndic://dsX/{manifest_uid}" + assert context["uid"] == member_uid + assert context["seriesName"] == SERIES + assert context["documentId"] == doc.id + assert context["mode"] == "open" + # Where DID will find it next time, with no database. + assert cached_path_for_uid(member_uid, additional_roots=[fetcher._file_dir]) == path + assert fetcher._fallbacks == 0 + finally: + fetcher.close() + + +def test_sparse_slot_is_none(series_session): + session, doc = series_session + fetcher = _ChunkFetcher(session, workers=1) + _no_reopen(fetcher) + try: + assert _resolve_on_worker(fetcher, doc, f"{SERIES}_{COUNT + 3}") is None + finally: + fetcher.close() + + +def test_direct_path_can_be_turned_off(series_session, monkeypatch): + monkeypatch.setenv("NDI_LIGHTSHEET_DIRECT", "0") + session, doc = series_session + fetcher = _ChunkFetcher(session, workers=1) + try: + # Owner thread: goes through the session's database_openbinarydoc. + path = fetcher.chunkPath(doc, f"{SERIES}_3") + with open(path, "rb") as f: + assert f.read() == _content(2) + assert fetcher._direct is False + finally: + fetcher.close() + + +def test_default_workers_is_wide(monkeypatch): + monkeypatch.delenv("NDI_LIGHTSHEET_WORKERS", raising=False) + assert multiscale._default_workers() == 32 + monkeypatch.setenv("NDI_LIGHTSHEET_WORKERS", "5") + assert multiscale._default_workers() == 5 + + +def test_configure_dask_pool_matches_workers(): + import dask + + previous = dask.config.get("pool", None) + try: + n = multiscale.configureDaskPool(48) + assert n >= 48 + assert dask.config.get("pool")._max_workers == n + finally: + dask.config.set(pool=previous) + + +def test_fetch_cloud_file_records_stage_timing(monkeypatch, tmp_path): + import ndi.cloud.api.files as files_api + import ndi.cloud.filehandler as fh + + monkeypatch.setattr( + files_api, "getFileDetails", lambda ds, uid, client=None: {"downloadUrl": "https://x"} + ) + + def fake_get(url, dest, **kw): + with open(dest, "wb") as f: + f.write(b"x" * 1000) + return True + + monkeypatch.setattr(files_api, "getFile", fake_get) + assert fh.fetch_cloud_file("ndic://ds/uid1", tmp_path / "out", client=object()) + t = fh.lastFetchTiming() + assert t is not None + assert t["bytes"] == 1000 + assert t["ok"] is True + assert t["batch"] is False + assert t["sign_s"] >= 0 and t["transfer_s"] >= 0 From 81be0285e0d0826116fd1735a8a8d421cc3e2234 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 13 Oct 2026 23:12:10 +0000 Subject: [PATCH 21/21] Lightsheet viewer: "Tiles in flight" drop-down, live; default 16 32 tiles at once on a ~100 Mb/s line took ~30 s per tile against ~5 s with 8 on a faster one: past the line's capacity every tile in flight shares the bandwidth, so each arrives later, the ones in view included. The right number depends on the connection, so the reader picks it. - _ChunkFetcher starts max_workers threads (MAX_WORKERS = 64 from levelArrays) and lets at most `workers` fetch at once. setWorkers(n) changes that live (clamped to 1..maxWorkers): threads past the limit wait, running fetches finish undisturbed. Workers are plain threads since the session-free change, so 64 idle ones cost nothing. - Default limit 16 (NDI_LIGHTSHEET_WORKERS still overrides). - The NDI Cloud panel gets a "Tiles in flight" drop-down (8/16/32/64, plus the launch value if different) wired to setWorkers. dask's pool is sized to maxWorkers so any choice can actually be used. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wj6cuvAwgAaYN5dNM1o6L --- src/ndi/gui/app/lightsheetZarr/viewer.py | 61 ++++++++++++++++++++++-- src/ndi/pyramid/multiscale.py | 59 +++++++++++++++++++---- tests/test_pyramid_fetch_direct.py | 54 ++++++++++++++++++++- 3 files changed, 158 insertions(+), 16 deletions(-) diff --git a/src/ndi/gui/app/lightsheetZarr/viewer.py b/src/ndi/gui/app/lightsheetZarr/viewer.py index a337e79d..58552a63 100644 --- a/src/ndi/gui/app/lightsheetZarr/viewer.py +++ b/src/ndi/gui/app/lightsheetZarr/viewer.py @@ -585,10 +585,10 @@ def openPyramid( try: from ndi.pyramid.multiscale import configureDaskPool - n_pool = configureDaskPool(getattr(fetcher, "_workers", 1)) + n_pool = configureDaskPool(getattr(fetcher, "maxWorkers", 1)) print( - f"[lightsheet] fetch pool: {getattr(fetcher, '_workers', 1)} workers, " - f"dask pool {n_pool} threads", + f"[lightsheet] fetch pool: {getattr(fetcher, 'workerLimit', 1)} tiles in " + f"flight (up to {getattr(fetcher, 'maxWorkers', 1)}), dask pool {n_pool} threads", file=sys.stderr, flush=True, ) @@ -875,6 +875,7 @@ def openPyramid( # the dataset-shape edges called out in the README. _attach_cloud_panel( viewer, + fetcher=fetcher, report_collect=_reportCollector( viewer, session, pyramid_doc, loader, counter, added_layers ), @@ -1351,7 +1352,47 @@ def probe(key, fn): return collect -def _attach_cloud_panel(viewer, report_collect=None) -> None: +def _tilesInFlightRow(fetcher, QHBoxLayout, QLabel, QComboBox): + """A "Tiles in flight" drop-down that changes the fetch limit live. + + More tiles at once hides per-request latency, until the line is full: + past that, every tile in flight shares the same bandwidth and each one + arrives later, including the ones in view. The right number depends on + the connection, so the reader picks it. + """ + from ndi.pyramid.multiscale import WORKER_CHOICES + + row = QHBoxLayout() + label = QLabel("Tiles in flight") + combo = QComboBox() + choices = [n for n in WORKER_CHOICES if n <= fetcher.maxWorkers] + current = fetcher.workerLimit + if current not in choices: + choices = sorted(set(choices) | {current}) + for n in choices: + combo.addItem(str(n), n) + combo.setCurrentIndex(choices.index(current)) + tip = ( + "How many tiles download at once. More hides network latency;\n" + "too many on a slow line just share the bandwidth, so every\n" + "tile -- including the ones in view -- arrives later. Try 8 on\n" + "~100 Mb/s, 32-64 on a fast connection. Takes effect now." + ) + label.setToolTip(tip) + combo.setToolTip(tip) + + def _changed(_index): + n = fetcher.setWorkers(int(combo.currentData())) + print(f"[lightsheet] tiles in flight: {n}", file=sys.stderr, flush=True) + + combo.currentIndexChanged.connect(_changed) + row.addWidget(label) + row.addWidget(combo) + row.addStretch(1) + return row + + +def _attach_cloud_panel(viewer, report_collect=None, fetcher=None) -> None: """Dock an NDI Cloud sign-in panel with a BETA badge. Reuses the gene-pyramid viewer's addCloudPanel contract -- same @@ -1401,7 +1442,14 @@ def _attach_cloud_panel(viewer, report_collect=None) -> None: try: from qtpy.QtCore import Qt, QTimer - from qtpy.QtWidgets import QHBoxLayout, QLabel, QPushButton, QVBoxLayout, QWidget + from qtpy.QtWidgets import ( + QComboBox, + QHBoxLayout, + QLabel, + QPushButton, + QVBoxLayout, + QWidget, + ) except ImportError: return @@ -1476,6 +1524,9 @@ def _attach_cloud_panel(viewer, report_collect=None) -> None: result.setWordWrap(True) result.hide() outer.addWidget(result) + + if fetcher is not None and hasattr(fetcher, "setWorkers"): + outer.addLayout(_tilesInFlightRow(fetcher, QHBoxLayout, QLabel, QComboBox)) outer.addStretch() def _tick(): diff --git a/src/ndi/pyramid/multiscale.py b/src/ndi/pyramid/multiscale.py index 62bdf609..f3fd1af2 100644 --- a/src/ndi/pyramid/multiscale.py +++ b/src/ndi/pyramid/multiscale.py @@ -210,11 +210,17 @@ class _ChunkFetcher: costs one idle thread. """ - def __init__(self, session, workers: int = 1): + def __init__(self, session, workers: int = 1, max_workers: int | None = None): self._session = session self._owner = threading.get_ident() self._reopen = self._reopener(session) - self._workers = max(1, int(workers)) + # max_workers threads are started; at most `workers` of them fetch + # at once. The limit can change while the viewer runs (setWorkers): + # threads past it just wait, so lowering it never interrupts a + # fetch and raising it takes effect on the next job. + self._limit = max(1, int(workers)) + self._workers = max(self._limit, int(max_workers or 0)) + self._running = 0 self._local = threading.local() self._lock = threading.Lock() self._paths: dict[tuple[str, str], str] = {} @@ -337,7 +343,7 @@ def _workerLoop(self) -> None: with self._cv: job = None while job is None: - while not self._heap and not self._closed: + while (not self._heap or self._running >= self._limit) and not self._closed: self._cv.wait() if self._closed: return @@ -349,6 +355,7 @@ def _workerLoop(self) -> None: heapq.heappush(self._heap, (1, nseq, cand)) continue cand.started = True + self._running += 1 job = cand self._run(job) @@ -374,6 +381,8 @@ def _run(self, job: _Job) -> None: with self._cv: self._jobs.pop(job.key, None) callbacks, job.callbacks = job.callbacks, [] + self._running -= 1 + self._cv.notify_all() job.path = path job.done.set() for cb in callbacks: @@ -398,6 +407,27 @@ def _enqueue(self, doc, filename, on_complete=None) -> _Job: self._cv.notify() return job + @property + def workerLimit(self) -> int: + """How many chunks may be fetched at once right now.""" + return self._limit + + @property + def maxWorkers(self) -> int: + """The most :meth:`setWorkers` will allow: the threads started.""" + return self._workers + + def setWorkers(self, n: int) -> int: + """Change how many chunks are fetched at once; returns the new limit. + + Clamped to 1..:attr:`maxWorkers`. Takes effect immediately for + queued work; fetches already running finish undisturbed. + """ + with self._cv: + self._limit = max(1, min(int(n), self._workers)) + self._cv.notify_all() + return self._limit + def warm(self) -> None: """Build the session handles now, off the calling thread. @@ -762,7 +792,7 @@ def stats_summary(self) -> str: f"{n_fetch} cloud-fetches (mean {mean_fetch_s:.2f}s, max " f"{max_fetch_s:.2f}s), {n_none} failures, " f"{self._stale_released} stale reads released, " - f"{n_fallback} via a session ({self._workers} workers)" + f"{n_fallback} via a session ({self._limit} of {self._workers} workers)" ) def reset_stats(self) -> None: @@ -922,15 +952,23 @@ def close(self) -> None: self._cv.notify_all() -_DEFAULT_WORKERS = 32 +_DEFAULT_WORKERS = 16 + +#: Threads started per fetcher, so the viewer can raise the limit live. +MAX_WORKERS = 64 + +#: The choices the viewer offers for tiles in flight. +WORKER_CHOICES = (8, 16, 32, 64) def _default_workers() -> int: - """Chunk-fetcher worker count; ``NDI_LIGHTSHEET_WORKERS`` overrides. + """Chunks fetched at once at launch; ``NDI_LIGHTSHEET_WORKERS`` overrides. - 32 by default. A cloud tile is seconds of waiting on the network and - almost no CPU, and with session-free resolution a worker is just a - thread, so a wide pool is cheap and buys overlap. + 16 by default. A cloud tile is seconds of waiting on the network and + almost no CPU, so overlap pays -- until the line is full: on a + 100 Mb/s connection 32 tiles of a few MB each just share the same + bandwidth and every one arrives later. The viewer can change it live + (:meth:`_ChunkFetcher.setWorkers`). """ try: n = int(os.environ.get("NDI_LIGHTSHEET_WORKERS", str(_DEFAULT_WORKERS))) @@ -1028,7 +1066,8 @@ def levelArrays( _ = channel # single-channel narrowing lands with the magicgui panels - fetcher = _ChunkFetcher(session, workers=workers if workers is not None else _default_workers()) + limit = workers if workers is not None else _default_workers() + fetcher = _ChunkFetcher(session, workers=limit, max_workers=max(limit, MAX_WORKERS)) # Upsample-fallback context: build once from the coarsest level's # document so every fine-level delayed task can look up covering diff --git a/tests/test_pyramid_fetch_direct.py b/tests/test_pyramid_fetch_direct.py index bc278f09..1b087f82 100644 --- a/tests/test_pyramid_fetch_direct.py +++ b/tests/test_pyramid_fetch_direct.py @@ -205,7 +205,7 @@ def test_direct_path_can_be_turned_off(series_session, monkeypatch): def test_default_workers_is_wide(monkeypatch): monkeypatch.delenv("NDI_LIGHTSHEET_WORKERS", raising=False) - assert multiscale._default_workers() == 32 + assert multiscale._default_workers() == 16 monkeypatch.setenv("NDI_LIGHTSHEET_WORKERS", "5") assert multiscale._default_workers() == 5 @@ -243,3 +243,55 @@ def fake_get(url, dest, **kw): assert t["ok"] is True assert t["batch"] is False assert t["sign_s"] >= 0 and t["transfer_s"] >= 0 + + +def test_worker_limit_caps_concurrency_and_changes_live(): + """setWorkers caps how many chunks fetch at once, and can be changed live.""" + import time + + class _Session: # no path, no database: nothing to reopen + pass + + fetcher = _ChunkFetcher(_Session(), workers=2, max_workers=4) + gate = threading.Event() + lock = threading.Lock() + state = {"now": 0, "peak": 0} + + def slow_resolve(doc, filename): + with lock: + state["now"] += 1 + state["peak"] = max(state["peak"], state["now"]) + gate.wait(10) + with lock: + state["now"] -= 1 + return f"/tmp/{filename}" + + fetcher._resolve = slow_resolve + done = [] + try: + for i in range(8): + fetcher.prefetchAsync(f"doc{i}", f"c_{i}", on_complete=done.append) + time.sleep(0.3) + assert state["peak"] == 2, f"limit 2 but {state['peak']} ran at once" + + assert fetcher.setWorkers(99) == 4 # clamped to the threads started + time.sleep(0.3) + assert state["peak"] == 4 + + assert fetcher.setWorkers(0) == 1 + assert fetcher.workerLimit == 1 + gate.set() + deadline = time.time() + 10 + while len(done) < 8 and time.time() < deadline: + time.sleep(0.05) + assert len(done) == 8 + finally: + gate.set() + fetcher.close() + + +def test_level_arrays_start_max_threads_with_default_limit(monkeypatch): + monkeypatch.delenv("NDI_LIGHTSHEET_WORKERS", raising=False) + assert multiscale._default_workers() == 16 + assert set(multiscale.WORKER_CHOICES) >= {8, 16, 32, 64} + assert multiscale.MAX_WORKERS >= max(multiscale.WORKER_CHOICES)