Repository navigation
Lightsheet cloud fixture + pyramid loader integration test - #320
Open
stevevanhooser wants to merge 55 commits into
Open
stevevanhooser wants to merge 55 commits into
stevevanhooser wants to merge 55 commits into
Conversation
Run 2 of build-lightsheet-fixture succeeded at the roundtrip and printed: CLOUD_DATASET_ID=6ab84b549852a120dbcb22bc but crashed on the next line with: Unrecognized field name "remoteDatasetName". The info struct that ndi.test.cloud.lightsheet_blob_cloud_roundtrip returns spells the field ``cloudDatasetName``, not ``remoteDatasetName`` -- I made the wrong name up when I wrote the workflow. Because the crash happened BEFORE the fopen to GITHUB_OUTPUT, neither job output landed and the step summary + lightsheet-fixture-id artifact steps never ran. The cloud upload itself is fine (the fixture is live under test user 1, dataset id 6ab84b549852a120dbcb22bc); this fixes the workflow so the next dispatch surfaces the id through every channel (log, outputs, summary, artifact) instead of only in the raw log. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Fixture: 6ab84b549852a120dbcb22bc (test user 1, prod), uploaded by
the MATLAB roundtrip via build-lightsheet-fixture.yml. Its ids
live in tests/fixtures/lightsheet_cloud_fixture.json so the test
finds them and a regeneration is one edit in one place.
Test file: tests/test_pyramid_loader_integration.py
* Skips automatically when NDI_CLOUD_USERNAME / NDI_CLOUD_PASSWORD
are not set (same gate as test_cloud_live.py), so a laptop
without secrets stays quiet.
* Module-scoped download: pulls the fixture ONCE with sync_files=false
and hands the path to every test. sync_files=false is deliberate
-- we want to exercise the on-demand cloud-fetch path the viewer
uses, not a fully-hydrated local copy.
* Five checks:
- loader.numLevels + loader.docs enumerate finest-first
- loader.specs returns one 3D array per channel
- the coarsest level computes and is not all-zero (guards the
silent-fill_value failure the DID series switch once hid)
- every level in the ladder computes; per-level timings are
printed for the "home vs. office vs. CI" comparison
- loader.stats() reports a non-empty fetcher summary so we know
real cloud fetches ran
Registration:
* new "cloud" pytest marker in pyproject.toml
* test-cloud-api.yml now includes test_pyramid_loader_integration.py
in the same account-and-env matrix as the rest of test_cloud_*.py
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
test-cloud-api.yml was schedule + dispatch only, so a PR that adds or edits the pyramid loader had no PR-time signal it was still reaching the cloud correctly. The new integration test in this PR (tests/test_pyramid_loader_integration.py) needs to run against the real fixture (6ab84b549852a120dbcb22bc) before merging, so we know the loader hasn't regressed while the diff was in flight. Add a path-filtered pull_request trigger covering: the pyramid package (src/ndi/pyramid/**), the fixture registry, the integration test file, the existing test_cloud_*.py suite, and this workflow file. Anything else on a PR does not fire the cloud tests, so unrelated PRs still cost nothing in cloud API calls. Runs under the same matrix (User 1/2 x prod/dev) as the schedule. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
First PR run on run 36281586795 failed with: ModuleNotFoundError: No module named 'dask' on four of the five pyramid loader integration tests (the fifth one only touched loader.docs / .numLevels which do not build any dask array, so it passed). The cloud CI installs with `ndi_install.py --dev --no-validate`, which does not pull dask -- dask lives under the napari extra (``[napari]``) alongside napari itself and its Qt / OpenGL dependencies. Installing the whole napari extra here is overkill: we do not need Qt on a headless cloud runner, only the dask array machinery the loader builds its multiscale ladder with. Add a one-line ``pip install 'dask[array]>=2023.1'`` to the cloud CI's install step so the loader can compute in that environment without dragging in Qt / OpenGL. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
The pyramid loader in ndi.pyramid.ImagePyramidLoader is napari-independent -- that was the point of splitting it out of gui/app/lightsheetZarr. Callers use it from matplotlib, Jupyter, headless batch analyses, and (once the sibling namespace lands) a MATLAB comparison harness. Every one of those callers needs dask[array], because that is what the loader builds its lazy multiscale arrays with. Keeping dask under the napari extra was defensible when the loader lived inside the napari viewer module; it is not now. A headless pip install of ndi should be able to run `from ndi.pyramid.loader import ImagePyramidLoader` and get to work without discovering, deep inside the traceback, that a viewer-side extra was needed. * Move `dask[array]>=2023.1` into project.dependencies alongside numpy / scipy / networkx. * Leave it pinned in the napari extra too, so an existing lock file that resolved dask via napari does not suddenly declare a version conflict. * Drop the workaround from test-cloud-api.yml -- dask now installs via the base package, so the cloud CI job needs no extra step. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
The fixture (currently 6ab84b549852a120dbcb22bc, see lightsheet_cloud_fixture.json) lives on one account + env pair: user 1, prod. The cloud CI matrix runs every account x env combination, so User 1 dev + User 2 (any env) tried to download that id and crashed with HTTP 404 + CloudNotFoundError. Turn the 404 into a skip: catch CloudNotFoundError in the downloaded_dataset fixture and pytest.skip with a message that names the current NDI_CLOUD_USERNAME + CLOUD_API_ENVIRONMENT and points at NDI_LIGHTSHEET_TEST_FIXTURE_ID for redirection. All five tests in the module inherit the skip through the fixture chain, so the three unrelated cloud CI cells no longer fail the build. Behaviour on User 1 prod is unchanged -- the fixture resolves, the download runs, all five tests execute. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
One transient TLS or connection error at start of run used to mark a scope failed and drop every uid in it to per-member getFileDetails -- O(N) API calls on a series that should have cost one. For a 156k-member lightsheet series that hung downloadDataset for hours on a residential network. _default_signer now retries getSignedURLSetAll with exponential backoff (1s / 4s / 16s, 4 total attempts) before surfacing the exception as a scope failure. Injected fake signers keep their old semantics -- tests are unaffected. See #322 and VH-Lab/NDI-matlab#1010 for the parallel MATLAB fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
A hung getSignedURLSetAll call today emits no output for minutes: the CloudClient retries transient errors up to MAX_ATTEMPTS silently, each retry after a 120s socket timeout, and the whole walk goes silent between HTTP calls too. On a residential connection where the batch endpoint stalls this looks indistinguishable from a genuine deadlock. Two log lines break that silence: * getSignedURLSetAll: "fetching page N ..." and "page N returned M uids in T.TTs" bracket every HTTP call. Silence for more than a page-time now names which page and which document is stuck. * CloudClient._request: each retry after a transient error or 5xx logs the exception name and the retry delay. The underlying retry loop is no longer invisible. Diagnostic only -- no behavior changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
_fetch_manifest asks for ONE known uid (the series manifest file). Going through fetch_cloud_file with the document id set makes the batch cache try to answer, and to answer it fetches the WHOLE document's uid->URL map through getSignedURLSetAll. A lightsheet OME-Zarr level with 121k chunks means walking 243 pages at 25s each -- 100+ minutes to fetch one manifest. Pass ndi_document_id="" so the batch is skipped and fetch_cloud_file drops straight to getFileDetails: one API call for the one uid we want. Downloading a real lightsheet dataset with SyncFiles=false is unblocked by this; before this change the download hung for hours reconstructing manifests. The batch is still the right choice for chunk fetches at viewer time, where hundreds of uids in the same document are requested back-to-back and the O(1) batch amortizes across them. The viewer-time scale problem is separate and needs the async signed-URL-set job family (porting_deferred in the bridge). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…through it Adds the four MATLAB entry points that stabilized on NDI-matlab main at 0a2cdec (VH-Lab/NDI-matlab#1009): createSignedURLSetJob, getSignedURLSetJob, waitForSignedURLSetJob and getSignedURLSetResult. Rewires batch_signed_url._default_signer from the paged getSignedURLSetAll walk (~25 s per 500-uid page, so 100+ min for a 121k-member document) to the async job path that builds the whole uid -> URL map server-side and returns it in one gzipped blob. Retry semantics (NDI-python#322) are preserved around the whole three-step exchange. Bridge YAML flipped for all four functions (porting_deferred -> ported) with matlab_last_sync_hash bumped to 0a2cdec, and batchSignedUrlLookup's decision_log updated to record the signer swap. Filed as #206. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SK8sCN6BWj7VYzt2pieTpd
…id fallback Silent fallback to per-uid getFileDetails hides real bugs in the async signed-URL-set-job path behind slow-but-working chunk fetches. A napari viewport with 100k+ chunks per document is unusable at one API call per chunk (thousands per zoom), and the underlying async-job failure -- endpoint 404, schema drift, timeout, missing resultUrl -- never surfaces. _default_signer now raises BatchScopeUnreachable when every retry attempt of create/wait/read has failed. _fetch_scope propagates it up through BatchSignedUrlLookup.lookup and out to callers (fetch_cloud_file, viewer chunk fetches). The partial-map case (batch returned a map that didn't name a specific uid) stays as a legitimate per-uid fallback -- that's a data-drift path, not a broken endpoint. Injected test signers that raise other exceptions still fall back per the old behavior, so existing failure-mode tests keep working. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Without this, every logger.info() in ndi.cloud.* is silently dropped: async signed-URL-set-job submit/wait/read progress, batch retry decisions, per-page walk timings all vanish. A viewer stuck inside _default_signer looks identical to a healthy one -- no way to tell whether the async job is running server-side or dead on arrival. Set up stderr logging at INFO when the root logger has no handlers so an embedding host (notebook, downstream CLI) that has already configured logging keeps control. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
The retry log printed only the exception type name; a bare 'RuntimeError' tells us the async job path failed but not why. Widen to include the (dataset, document, file_series) scope and the exception message, so a running viewer surfaces the actual reason each attempt failed rather than four identical opaque lines. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…lookups Strict-mode BatchScopeUnreachable was raising all the way up, but did not update _failed_scopes -- so every next uid in the viewport triggered another full 4-attempt retry cycle against the broken endpoint. A viewer whose async job path is dead spent 20 s per failed cycle in an infinite loop instead of surfacing the actual cause once and staying quiet. _fetch_scope now records the failure with unreachable=True and the cause string before re-raising. lookup() consults the cache and, when it finds an unreachable entry within failure_ttl_seconds, re-raises a fresh BatchScopeUnreachable naming the original cause and elapsed time. Also widened the retry log to include scope and exception message instead of just the exception type name, so 'RuntimeError' becomes something legible like 'RuntimeError: signed-URL-set job ... failed'. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…er-chunk catch _resolve caught EVERY exception from database_openbinarydoc and returned None, which _read_chunk_from_fetcher then turned into a fill_value block. That catch is right for a per-chunk transient blip (one chunk drops out, canvas keeps rendering) but wrong for a systemic scope failure: every chunk in the scope raises identically, every block becomes fill_value, and the whole canvas silently reads as zeros while the fetcher summary line reports "N failures" that no test asserts on. It is the exact silent-fetch-failure pattern the integration test's non-zero assertion was written to catch (see test_the_coarsest_level_computes_and_is_not_all_zero), now firing on the tiny fixture too. Let BatchScopeUnreachable propagate past the per-chunk catch so a broken batch surfaces through .compute() as an actual error rather than an all-zero result. Ordinary exceptions still degrade to fill_value. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…ync prefetch The upsample fallback never calls chunkPath synchronously -- it uses chunkPathIfCached (memoization only, no fetch) plus prefetchAsync (fire-and-forget). So BatchScopeUnreachable raised inside _resolve fires in a background pool thread and is caught by prefetchAsync's generic 'except Exception' handler, which turns it into path=None. The main thread then goes down the "coarse not cached either" branch and returns a zero_block for every chunk, producing a canvas of silent zeros -- the very pattern the loader's integration test was designed to catch, now firing on the tiny fixture. _ChunkFetcher now tracks systemic scope failures on the instance (_unreachable_exc). When prefetchAsync's background _run catches BatchScopeUnreachable it stores the exception rather than dropping it, and chunkPath / chunkPathIfCached re-raise on subsequent calls. The very first chunk still races: its prefetch may not have failed before the fallback returns zeros -- but any subsequent chunk in the same compute surfaces the exception, which propagates through dask to .compute(). In practice that is the second chunk, which means the test fails loudly instead of silently. Ordinary per-chunk transient failures still degrade to fill_value (the existing 'except Exception' branch below the new one keeps that behavior). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…gible _default_signer and waitForSignedURLSetJob were completely silent during a successful long-running job. A viewer waiting minutes on a 121k-member scope looked identical to a hung viewer, and the only signal was a raw TCP connection to the API host visible in netstat. Add INFO log lines at each step: * _default_signer: submit -> jobId, terminal state + elapsed, result blob delivered with uid count. * waitForSignedURLSetJob: every poll logs the state and signedCount / totalCount, so a long wait shows visible progress instead of silence. Diagnostic only -- no behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…ive token expiry Observed today: a signed-URL-set-job poll loop that had to run for ~1 hour (server signs ~30 URLs/sec, and a 121k-member scope means tens of minutes) died at ~9 min with HTTP 403 "Token is invalid or expired". The bearer token's TTL is shorter than the wall-clock time some legitimate long operations need, and the client kept sending the stale token forever until the outer wait timed out at 60 minutes, having produced nothing useful. CloudClient._request now catches a 401/403 once per request, calls self._reauthenticate() (a fresh CloudConfig.from_env() + authenticate() that mutates config.token in place), updates the Authorization header, and retries. If the reauth itself fails, or if the retried request still comes back 401/403, the original error surfaces as before -- a second 401 after a successful reauth is a real credential problem, not something worth looping over. Also updates the "a 401 is not retried" test to acknowledge the new one-shot reauth semantics, and adds four tests pinning: happy-path reauth-and-retry, 403 same as 401, no loop after a persistent 401 past reauth, and the retry using the fresh token in its Authorization header. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Port of VH-Lab/NDI-matlab commit 27661a3 (issue #322). A working scientist reopening the same lightsheet-scale dataset over a day should pay the ~85 min async signed-URL-set-job cost once, not every open. New signed_url_disk_cache module writes each scope (dataset_id, document_id, series_name) to ~/.ndi/signed-url-cache/<ds>/<doc>[_<escaped_series>].json.gz as gzipped JSON, TTL-checked against the server's own filesExpireAt with a 30 min safety buffer, atomically renamed on write. MATLAB and Python write byte-for-byte identical files: schemaVersion, path convention, filename escaping (percent-encode outside [A-Za-z0-9._-], SHA-1 hash above 96 chars), gzip framing, JSON keys, env var (NDI_SIGNED_URL_CACHE_DIR / NDI_SIGNED_URL_CACHE_SAFETY_SECONDS) all match. A cache dir written by MATLAB reads clean from Python. BatchSignedUrlLookup gains disk_cache=False; get_default() flips it on -- production path. Existing test-suite instances stay off by default. On S3 403 during the actual getFile the scope is forgotten from disk so a rotated token doesn't cascade to N per-uid 403s; getFile takes an opt-in error_out sink so the status code drives the hook. Session-scoped conftest fixture isolates the cache dir so no test can pollute a developer's ~/.ndi/. Bridge YAML: adds signedUrlDiskCache entry and bumps batchSignedUrlLookup's matlab_last_sync_hash to 27661a3.
This was referenced Sep 28, 2026
The signed-URL disk cache commit (18ec747) did not run black over the files it touched, so CI's lint check flagged four files as needing reformatting. This applies black without altering any logic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Three unrelated failures on head 18ec747, all fixed here: 1. cloud.client: gate mid-request reauth on _can_reauth flag test_bad_auth_raises hand-built a CloudClient(CloudConfig(token=...)) with a deliberately invalid token and expected CloudAuthError. On 401 the reauth path added in d7b14b7 called CloudConfig.from_env() and authenticate() against the ambient CI env credentials, silently swapping the hard-coded bad token for a valid one, retrying, and succeeding -- exactly the "used a different token than the caller asked for" behavior the test was supposed to catch. CloudClient now defaults _can_reauth = False. from_env() (and any future login helper) sets it True after authenticate(). A test that stands up a hand-built client to verify auth failure is used as passed. Long-running clients built by from_env() still get the mid-run token refresh needed to survive TTL expiry. Adds test_a_hand_built_client_does_not_reauth_against_env as a regression pin, and updates make_client(*, can_reauth=True) so existing retry tests continue to behave as though the client had been obtained via from_env(). The client fixture in test_cloud_live.py wraps CloudClient(cloud_config) with _can_reauth = True since its config comes from an env-driven login(). 2. tests/test_cloud_filehandler: reset ambient CloudClient around TestGetOrCreateCloudClient test_env_vars_present passed in isolation and in the No-Cloud suite but failed in the live Cloud API suites: an earlier live test populated _ambient_cloud_client via a real cloud fetch, and the "should call from_env once" assertion then saw zero calls (the cached client was returned instead). An autouse fixture resets both slots before and after each test in the class. 3. sync bridge: record ndi.cloud.sync.internal.fetchManifest Added on the MATLAB side in commit 5541a00; the Python analog (ndi.cloud.filehandler._fetch_manifest, commit 5ab9c04) is ported_differently -- inline with its caller rather than a standalone module. Records the port so test_matlab_bridge_completeness stops flagging it as unrecorded. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Fixes the MATLAB/Python bridge completeness job on Waltham-Data-Science/ NDI-python#320 head 8eb7927. - src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml: fetchManifest 0ae6bfad → a84130a7. 0ae6bfad was the blob hash from a raw file fetch, not a commit; a84130a7 is the commit that introduced the standalone helper. No Python change: the port was always inline in ndi.cloud.filehandler._fetch_manifest. - src/ndi/cloud/ndi_matlab_python_bridge.yaml: updateFileInfoForRemoteFiles 9467a0a7 → a84130a7. Same commit extracted the local fetchManifest helper out of this file into its own module. Nothing to port here: the extraction is a pure refactor and Python calls its own inline helper. - src/ndi/gui/component/ndi_matlab_python_bridge.yaml: ndi_gui_component_ProgressBarWindow b367283b → 2b23dd30. Commit 2b23dd30 dropped the (1,:) size constraint from the addBar Timeout argument-block declaration (a MATLAB argument-validation adjustment). No behavioural change; note added to the decision_log. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Contributor
Author
|
CI note — What is failing on
Why it is not this PR's diff
Standing down on this failure for this PR No fix exists to port yet — the trigger is server-side and specific to User 1 prod. Not re-running: the failure is deterministic on this matrix cell, not a flake, and a re-run would reproduce identically. Everything the disk-cache/CI-fix push ( Generated by Claude Code |
Was: one 0.5 s retry per scope when the first signed-URL-set response did not name a requested uid. Now: a bounded schedule of 1 s / 3 s / 9 s. #320's Cloud API (User 1, prod) run shows the signed-URL-set index lagging further than 0.5 s after ``waitForAllBulkUploads`` returns -- the wait covers the bulk-upload extraction subsystem reporting done, but not the seam between that and the signed-URL-set subsystem catching up. One retry isn't enough to bridge it, so the round-trip test's manifest reconstruction sees a partial map for the demoNDISeries members, the strict-mode ingest_locations rebuild produces no entries, and the local add rejects the document (main sees the same server-side race as ``uid_misses > 0`` further along in the same test). Three waves cover the observed tail, still bounded (each wave costs one scope refetch, not one per uid), and never fire in the common case where the first map is already complete. API: - New ``partial_map_retry_delays`` kwarg on BatchSignedUrlLookup (tuple of waits). Defaults to DEFAULT_PARTIAL_MAP_RETRY_DELAYS = (1.0, 3.0, 9.0). - ``partial_map_retry_seconds`` kept as a deprecated scalar (None means "use partial_map_retry_delays"; any real number replaces the schedule with a one-element tuple containing that wait, so ``partial_map_retry_seconds=0`` still fires ONE retry with no wait, matching the pre-schedule contract every existing test pins). - ``_retried_scopes`` set becomes ``_scope_retry_attempts`` dict of attempts spent per scope; the loop walks the schedule until a fresh map names the uid or the schedule is exhausted. Two new unit tests pin the multi-wave behaviour (``test_a_multi_wave_schedule_retries_until_a_hit_or_exhaustion`` and ``test_a_schedule_that_never_settles_bounds_retries_to_len``); the round-trip test's cap on ``partial_map_retries`` relaxes from 1 to ``len(DEFAULT_PARTIAL_MAP_RETRY_DELAYS)``. MATLAB mirror lands in VH-Lab/NDI-matlab commit 02880062d on the same branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
MATLAB's batchSignedUrlLookup.m moved to 02880062 (partial-map retry schedule went from a single 0.5 s wait to a bounded 1 s / 3 s / 9 s schedule). Python's counterpart on this branch shipped the mirror change in the previous commit (4dff8bf), so this hash bump records a same-commit port, not a drift. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
stevevanhooser
pushed a commit
to VH-Lab/NDI-matlab
that referenced
this pull request
Sep 28, 2026
When DID reads a series member, it fetches the manifest first via the same download handler with seriesName="". That call previously went through ``batchSignedUrlLookup`` under the whole-document scope, and for a lightsheet-scale pyramid document (15k+ files) the batch endpoint spent 60-90 s signing URLs the caller had no use for -- just to answer the ONE manifest URL. The subsequent seriesName="chunk.bin" call then fired its own scope, and the two answers overlapped almost completely. Now: when seriesName is empty, the batch is skipped and the fetch takes the direct ``getFileDetails`` path (O(1)), matching what ``ndi.cloud.sync.internal.fetchManifest`` already does for the internal manifest fetch. When DID follows up with a member fetch (seriesName="chunk.bin"), the batch fires there and amortizes across every member exactly as it did before. Python mirror lands in Waltham-Data-Science/NDI-python on the same branch (filehandler.download_file_from_cloud gained the same guard). See #1010 and Waltham-Data-Science/NDI-python#320. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
When DID reads a series member, it fetches the manifest first via the same download handler with seriesName="". That call previously threaded documentId through to fetch_cloud_file, which used the whole-document batch scope, and for a lightsheet-scale pyramid document (15k+ files) the batch endpoint spent 60-90 s signing URLs the caller had no use for -- just to answer the ONE manifest URL. The subsequent seriesName="chunk.bin" call then fired its own scope, and the two answers overlapped almost completely. Now: when seriesName is empty, download_file_from_cloud clears ndi_document_id and the fetch takes the direct getFileDetails path (O(1)), matching what _fetch_manifest already does for the internal manifest fetch. When DID follows up with a member fetch (seriesName="chunk.bin"), the batch fires there and amortizes across every member exactly as it did before. test_ordinary_file_still_carries_document_scope is repurposed as test_a_single_file_fetch_bypasses_batch, pinning the new invariant. MATLAB mirror lands in VH-Lab/NDI-matlab commit 20391306 on the same branch (didsqlite.download_file_from_cloud gained the same guard). Real-world impact: on User 1 prod, a level swap that previously fired a redundant 76 s whole-doc signed-URL-set job now skips it entirely, and only the chunks-series scope needed for the actual member fetches runs -- 76 s of dark screen goes away. See #320. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Kick off the async signed-URL-set job for every level's chunk.bin series as soon as the viewer knows the pyramid's level documents, in a single background thread. The first zoom into a new level no longer waits 20-80 s for the batch endpoint to sign that scope -- the URLs are already cached from work done behind the initial level 0 render. - BatchSignedUrlLookup.prefetch_scope: seed one scope without asking about a specific uid. Skips the partial-map retry on purpose (that's a lookup-time concern; a later lookup drives it if needed). - BatchSignedUrlLookup.start_prefetch: daemon thread that iterates scopes and calls prefetch_scope for each, logging per-scope timing, skipping empty document ids, and continuing past BatchScopeUnreachable so one bad level does not stop the rest. - ImagePyramidLoader.startSignedUrlPrefetch: enumerate the pyramid's level docs, build (cloud_dataset_id, doc.id, "chunk.bin") scopes for every level that has a chunk.bin series, and hand them to the module-level lookup. Off with NDI_LIGHTSHEET_PREFETCH_SIGNED_URLS=0. - lightsheetZarr.viewer.openPyramid: discover the cloud dataset id via session.is_in_cloud() and fire the prefetch after the loader builds, so the jobs run while napari is still opening. Sequential rather than parallel across scopes on purpose: 5 levels at ~1 min each hide inside the initial-render wall time, and a parallel refactor would need to unwind the shared lock that serialises signer calls today. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
stevevanhooser
pushed a commit
to VH-Lab/NDI-matlab
that referenced
this pull request
Sep 28, 2026
Was: the test built a spatial shape via ``pickShapeForTarget(N, [16 16 16])`` that would contain ~N chunks if the fixture wrote at [16 16 16], but its call to ``ndi.fun.doc.lightsheet.fromOMEZarr`` did not forward a ``chunks`` argument. ``makePyramid`` therefore fell through to ``chooseTileShape``'s default ~8 MB per-tile byte budget, which for uint16 lands at roughly [161 161 161] per chunk. Concrete outcome: requested N actual members ------------ -------------- 5 1 100 1 500 1 2000 8 10000 27 50000 64 100000 125 Every row degenerated into a "~125-member series" test, and the scaling axis this class exists to sweep was silently collapsed to a constant. The rows still exercised the bug tracked in #1010 (createSignedURLSetJob rejects a manifest that getFileDetails accepts), but the N label carried no information about scope size -- it only labelled cloud budget spent on a fixture that reproduces the small-N case seven times over. Fix: forward ``'chunks', chunkShape`` to fromOMEZarr so makePyramid uses the chunk shape the fixture wrote (which is the same shape pickShapeForTarget was already sizing the level to). Also add a regression-guard ``warning()`` that fires when the actual member count is <25% of the target for N>=100, so if this ever silently regresses again the run log makes the failure loud rather than legible only by squinting at the [scaleprobe] rows. Companion to Waltham-Data-Science/NDI-python#320 and ndi-cloud-node#151. Once the server-side signing behaviour stabilises, the huge-N rows will actually produce huge-N scopes and the performance-regime evidence they were added to capture becomes meaningful. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Live log at viewer launch showed the eager signed-URL prefetch firing a fresh createSignedURLSetJob for a 121,440-URL scope every launch, even after a previous run had populated the on-disk cache. The tell was the missing "signed-URL disk cache: served scope (...) signer not called" line and the immediate "submitting createSignedURLSetJob" right after "warming N scope(s) in background". Root cause: only ``lookup`` (the per-uid path exercised on tile reads) was checking ``_try_disk_cache``. ``prefetch_scope`` -- the entry point ``start_prefetch`` calls in the background thread at viewer launch -- checked only the in-memory cache and fell straight through to ``_fetch_scope``. So a fresh process (empty in-memory cache) never consulted the disk, defeating the whole point of the persistent cache for the prefetch path. Mirror the ``lookup`` shape: between the in-memory check and the fetch, probe ``_try_disk_cache`` when ``self._disk_cache`` is on. A disk hit populates the in-memory cache (``_try_disk_cache`` already does that) and returns True with the scope warmed, no server round trip. ``signed_url_disk_cache.load`` filters URLs that would expire inside the 30 min safety buffer, so a served scope is safe to hand to downstream reads. A miss (no cache file, expired, or corrupt) falls through to the existing ``_fetch_scope`` path unchanged. With this in place the first launch after a URL-set expires still pays the createSignedURLSetJob cost; every subsequent launch inside that TTL warms from disk in <1s and skips the signer entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…result Explains why ~/.ndi/signed-url-cache/<dataset>/ stayed empty after a full successful signing on a fresh machine (#320). The disk cache's save() correctly refuses to persist a payload that carries neither ``filesExpireAt`` nor ``expiresAt`` -- it will not invent a TTL. The default signer was handing it exactly such a payload: the result blob from getSignedURLSetResult only carries {jobId, datasetId, documentId, generatedAt, fileCount, files}, and the URL-expiration fields live one hop earlier, on the job STATUS returned by waitForSignedURLSetJob (see its docstring, which lists filesExpireAt/expiresAt as job-status fields, not result-blob fields). The signer had ``status`` in hand but only returned ``answer`` from getSignedURLSetResult, so those fields were dropped and every save was a silent no-op. From the user's viewpoint: complete signing successful, cache directory empty, next launch re-signs from scratch. Fix: after fetching the result blob, copy filesExpireAt and expiresAt from ``status`` into ``answer`` when present. Only string values are carried, and we don't overwrite a field already on the answer -- if the server ever starts including them in the blob too, that wins. Non-string / missing values fall through to save's own refuse-to- persist path, unchanged. With this in place the first launch on a fresh machine still pays the createSignedURLSetJob cost but now leaves a valid cache entry behind; every subsequent launch inside the URL TTL (minus the 30 min safety buffer) warms from disk via prefetch_scope's disk-cache probe added in dc91fd3 and skips the signer entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…ble payload Follow-up to 40ec43d. The un-cacheable-payload branch of save() was returning silently, which is exactly how the previous signer bug (dropping filesExpireAt/expiresAt on the way from job status to result blob) stayed invisible: complete signings, empty cache directory, next launch re-signs from scratch, no log line naming the scope or the cause. WARN when a scope is refused for having neither filesExpireAt nor expiresAt; that's a caller bug (a signer must carry an expiry it saw upstream through to the payload it hands save) rather than an environmental issue, and the noise turns "the cache mysteriously never fills" into "here is the exact scope and the exact reason". Keep the I/O-error branches (missing dir, permission, disk full, json.dumps failure) silent -- those are legitimate best-effort skips. Rationale for warn-not-raise: the surrounding read has already succeeded and we don't want to break it just because the cache side of things bounced. WARN is loud in a log the user is already watching and quiet in production logging setups that filter WARNING out. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Mirrors VH-Lab/NDI-matlab@5c9e8a83a on the fromOMEZarr and makePyramid entries. Python port is still porting_deferred for these; this change updates the decision_log so the deferred port lands on the current MATLAB default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Follow-up to 298fabd. The bridge-hash guard (test_matlab_bridge_hashes.py::TestNothingDriftsFromMatlab) noticed that fromOMEZarr, makePyramid and chooseTileShape have moved in NDI-matlab since their last recorded sync (71d7387 / 8134a20 / 1d11302) and the yaml did not say which commit the port now matches. That is the error it is there to catch, and the fix is to bump the hash on each affected entry. VH-Lab/NDI-matlab@5c9e8a8 is the commit that just moved the three files for the tileBudgetBytes default change; the Python port stays porting_deferred so no code follows -- just the hash. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Follow-up to eb2387a. The hash-justify guard (test_matlab_bridge_hashes.py::TestAHashChangeIsJustified) requires that when a matlab_last_sync_hash is bumped without a Python-side change, the entry's decision_log names the new commit and says why it is a no-op on the port side. eb2387a bumped the three lightsheet entries to 5c9e8a8 but either rewrote the decision_log without naming the SHA (fromOMEZarr, makePyramid) or did not touch it at all (chooseTileShape). Add one sentence to each decision_log naming 5c9e8a8 and saying the port is still porting_deferred so the new default lands when the port lands. Satisfies the guard without widening the entries. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Napari defaults the dimension sliders and the cursor-position readout to "pixels" when nothing sets viewer.dims.units. For a multiscale lightsheet pyramid where every level carries voxel_size in micrometres, that means the slider range CHANGES as napari auto-swaps multiscale LOD -- the user zooms in, napari switches to a finer level with a larger voxel count, and the slider's "N / M" indicator jumps even though the world position is unchanged. User reports this reads as the viewer briefly rescaling, then settling. Pull axes_order (e.g. "tczyx") and voxel_size_units (e.g. "micrometer") from the finest level document's lightsheetZarrLevel props and set viewer.dims.axis_labels + viewer.dims.units once, after add_image. The axis labels (t, c, z, y, x) populate the slider titles; units attach the micrometre label and switch cursor readout to world coordinates. Non-spatial axes (t, c) get a blank unit since napari treats "" as unitless. Older napari releases may not expose dims.units; falls back to just setting axis_labels on that failure path and logs the specific exception so a user who sees inconsistent slider behaviour knows to look at napari's version. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
The old default was to open the coarsest level as a plain (non-multiscale) layer with a magicgui dock widget swapping layer.data on zoom. That path dates from a napari 0.5 bug where the multiscale slicer never marked a cloud-backed layer loaded=True; on the lightsheet data in production it is now the thing creating the sympthoms the user reports -- a one-minute "no visible loading" pause on zoom-in while the single-layer swap re-slices level 0's full array before the picture updates. Flip the default: pass napari a true multiscale ladder so it picks the level the current zoom actually needs. Fine levels still carry the upsample-fallback reader, so a zoom-in paints coarse-upsampled pixels immediately and refines as fine chunks arrive. The env knob flips to NDI_LIGHTSHEET_SINGLE_LEVEL=1 as the escape hatch if the old napari bug resurfaces on a dataset. Make the single-level-only controls (_attach_level_selector, viewport.attach_viewport_clip) no-op when the layers report layer.multiscale=True -- otherwise they would corrupt napari's MultiScaleData wrapper by swapping layer.data out from under it. Add a visible loading overlay: _NapariStatusReporter now writes to viewer.text_overlay in addition to viewer.status, so fetches in flight show up as a yellow "Loading tiles ... (N in flight)" over the canvas rather than only in the status bar footer. The previous zoom report said the status bar was easy to miss; the overlay is big and lives on the picture itself. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
c129852 flipped the lightsheet viewer default to a true multiscale ladder, so a layer spec's "data" is now a list of per-level arrays rather than a single 3D array at the coarsest level. Two integration tests assumed the single-level shape: - test_the_loader_returns_one_spec_per_channel: accept data as either a list (take the first level) or a bare 3D array. - test_the_coarsest_level_computes_and_is_not_all_zero: pick the last level from the list, or the bare array if single-level mode is on. Keeps compatibility with NDI_LIGHTSHEET_SINGLE_LEVEL=1 (the escape hatch) so the tests pass under either mode. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Current napari rejects plain strings on viewer.dims.units with a pydantic is_instance_of error -- the field now wants pint.Unit instances. a814f7c set strings and fell back to labels-only on every launch; build pint.Unit values instead so the dim sliders actually read micrometers. Non-spatial axes (c, t) get pint.Unit("") (dimensionless) to match their lack of a world-space unit on the pyramid document. Also expand the multiscale default comment: a user report of a ~2-minute first-paint wait on the Maddie dataset (napari's slicer sitting on "no tiles requested yet" while vispy fired destroyed- dispatcher warnings) is still the expected worst case before the first interaction; the view does come alive and subsequent zooms are responsive, but the escape hatch (NDI_LIGHTSHEET_SINGLE_LEVEL=1) stays documented. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Multiscale default (c129852) did not survive production: on the Maddie dataset first paint sat for ~2 minutes with "no tiles requested yet by napari" while vispy fired destroyed-dispatcher warnings, and once the view did come alive a plain zoom-in did NOT trigger a level swap -- the user had to zoom out and back in to get finer data to render. The single-level + Resolution-picker flow does swap on a plain zoom via the zoom-level-swap listener and paints promptly, so that is the default again. NDI_LIGHTSHEET_MULTISCALE=1 opts into the multiscale path for anyone testing whether napari's slicer has been fixed on a given data shape. The level-selector/viewport-clip no-op guards and the loading overlay stay in: both are correct in either mode and the dims.units pint fix is independent of the default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
User A/B tested single-level vs multiscale and prefers multiscale: the transition between levels is smoother because napari handles the swap internally, while single-level mutates layer.data and shows a short black frame on every change. Both modes have been observed to hit the same napari slicer wedge on the Maddie dataset (vispy "QBasicTimer destroyed dispatcher" spam, no tile requests ever land), so single-level is not actually safer -- just janker when it does paint. Flip the default back to multiscale. NDI_LIGHTSHEET_SINGLE_LEVEL=1 keeps the earlier single-level + Resolution-picker flow as the opt-in. NDI_LIGHTSHEET_ASYNC=0 is the escape hatch when the async slicer wedges: it blocks add_image on the first sync-sliced paint and bypasses the broken async path. Both knobs are now documented in the comment on the single_level decision. _NapariStatusReporter was calling QTimer.singleShot(0, _apply) from whichever thread the fetch counter happened to run on (the heartbeat is a daemon thread, fetch handlers run on async workers). QTimer requires an event dispatcher on the current thread; without one it did nothing useful and emitted "QBasicTimer::start: current thread's event dispatcher has already been destroyed" into every log line. Switch to a Qt signal/slot: _Bridge is a QObject moved to the main thread with a signal that _apply listens to via a queued connection, so emit() from any worker crosses safely to the main-thread apply. Removes the vispy spam and should keep the overlay reliable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
On macOS, napari+vispy sometimes leaves the slicer wedged: launch paints the coarsest level (served from the prefetch cache) and the fetch counter sits on "no tiles requested yet by napari" while vispy spams "QBasicTimer::start: current thread's event dispatcher has already been destroyed." A user zooming in has to zoom-out-and- back-in several times before napari actually asks the fetcher for finer tiles -- unacceptable UX for a viewer meant to beat Imaris on a local machine. Add _attach_camera_nudge: on every viewer.camera.events.zoom / center / angles event, start a 120 ms debounce timer that fires layer.refresh() on every added layer. layer.refresh() forces napari to compile a new slice request on the main thread, which does reliably fire the slicer -- so a single zoom gesture becomes enough to trigger a level swap instead of three. The 120 ms debounce coalesces a pinch/wheel-zoom's many micro- events into one refresh at rest, matching how the viewport-clip listener elsewhere in this viewer handles the same event storm. A module-level _NUDGE_TIMERS dict keeps the QTimer and its slot connection alive for the viewer's lifetime; without the strong ref Python GC would cut the timer loose the moment the function returned. NDI_LIGHTSHEET_NUDGE=0 is the escape hatch (prints one line and installs no listeners) in case the extra refresh storm interacts badly with another dataset. This is a shim, not a fix: the real bug is in vispy/napari's slicer dispatcher on this macOS stack. A proper fix needs an upstream napari issue (next on the list). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
User-facing lever for the macOS napari slicer wedge. The camera-nudge alone does not reliably unstick napari on this stack, but the user found a manual action that does: switching the level dropdown away-and-back a few times. That is a visibility toggle under the covers -- the layer tears down its vispy node, napari re-initializes the slicer when it comes back, and the next slice compute fires reliably. Dock a Refresh View button that performs that same sequence: set layer.visible=False for every visible layer, let Qt process the hide-event (50 ms QTimer.singleShot), then set visible=True and call layer.refresh(). Users click it when regions still show coarse data after a zoom-in, instead of fighting the Resolution dropdown. NDI_LIGHTSHEET_REFRESH_BUTTON=0 hides the button for scripted viewers. Silent no-op if magicgui isn't installed. Timing of each refresh is logged to stderr so a user watching the terminal can see which clicks did work. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Mirror the gene-pyramid viewer's cloud panel so a reader of a cloud-backed lightsheet pyramid can see token time-left and renew without quitting -- the viewer outlives its token on anything but a few-minute browse, and up to now a reader on an expired token would see "missing tiles" rather than a login prompt. Build the panel locally rather than calling addCloudPanel from the gene-pyramid module directly, because the lightsheet viewer needs a small BETA badge immediately under the wordmark (the first-paint / zoom-refinement rough edges called out in the README should not be a surprise). Reuses the gene module's cloudSignInDialog, cloudSessionLooksLikely, and cloudLogoLabel so profile list, token clock, and auth plumbing are identical across the two viewers. Only shown when the environment carries NDI_CLOUD_TOKEN, same rule the gene viewer uses -- a local-only pyramid has no reason for a login control, and a login panel on a local window implies the picture is waiting on something it isn't. NDI_LIGHTSHEET_CLOUD_PANEL=0 opts it out for scripted viewers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
v1 is the control: lazy multiscale dask pyramid with the same shape ratios as production. Confirmed to RUN CORRECTLY on vanhoosr's machine (~1,600 block reads, no vispy QBasicTimer warnings), so the slicer wedge is caused by something our launcher does on top of this baseline, not by napari/vispy on this data shape. v2 adds the first candidate: a daemon heartbeat threading.Thread plus a ThreadPoolExecutor with a few submitted jobs started BEFORE napari.Viewer(). This mirrors _FetchCounter._heartbeat + fetcher.warm() + loader.startPrefetch() in src/ndi/gui/app/lightsheetZarr/viewer.py. If just spawning Python threads before Viewer() construction wedges the slicer, this file reproduces it; if not, v3 will add the next piece (refresh-hint registration / _NapariStatusReporter / etc.). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…loud panel
The visibility-toggle in the Refresh View button turned out to be
insufficient on the production dataset: layer.visible off/on alone
does not fire a new slice compute, so a region stuck at the coarse
level stayed stuck. The user's manual workaround -- zoom out, then
zoom back in with the mouse -- always works, because that is a real
camera event.
Change the button to do exactly that:
* phase 1: viewer.camera.zoom *= 0.8 (immediate)
* phase 2 (100 ms): viewer.camera.zoom = original_zoom
* phase 3 (150 ms): visibility off/on + layer.refresh() as a
belt-and-suspenders fallback in case napari coalesces the two
camera events out.
The flash on screen is ~150 ms and barely perceptible; what the user
sees is the view jumping back to a real level 0 render.
Separately, show the NDI Cloud panel (wordmark + BETA badge + Sign
in) on every lightsheet session, not only sessions that already have
a cloud token. The wordmark identifies the viewer and the user
asked for it to be visible always; on a local-only session the
clock copy reads "Not signed in. Sign in to access NDI Cloud
pyramids." instead of a stale token clock.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
v3 adds the next launcher candidate after v2: a QObject that lives on the Qt main thread (via moveToThread(app.thread())) and exposes a Signal that a background thread emits through Qt.QueuedConnection. This mirrors _NapariStatusReporter._Bridge in the production launcher and is the most Qt-adjacent thing we do before napari.Viewer(). Also shrinks the fake pyramid 8x per axis (chunk count drops ~512x). The reporter in the earlier wait reported ~5-minute launch waits for v1 and v2; nearly all of that was dask.from_delayed construction and napari's walk of the resulting graphs, O(1M) delayed objects per level before. v3's launch should be seconds, with each move taking seconds rather than minutes. The slicer-wedge bug does not depend on absolute pyramid size -- shape ratio and lazy multiscale do -- so the shrink does not change what we are testing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…idge
v3 reproduced the wedge (zero block reads, slicer never dispatched).
v3 added three things on top of v2:
(a) smaller pyramid shape + cheap np.full chunk reader
(b) QApplication explicitly pre-created before napari.Viewer()
(c) QObject cross-thread bridge with Qt.QueuedConnection emits
from a background thread
v4 isolates (a): v2's thread setup unchanged, v3's leaner pyramid.
No QApplication pre-create, no Qt bridge. If v4 still produces
"block read" lines on zoom, the smaller pyramid is NOT the trigger
and the wedge is driven by (b) and/or (c). If v4 reproduces the
wedge, the shape ratios in the shrunken pyramid matter and the Qt
bridge is off the hook; next bisect would be v1-ish but with v3's
shape.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
v4 (lean pyramid + threads only) worked -- the lean pyramid is cleared. The wedge in v3 is in the QApplication pre-create and/or the Qt bridge. v5 isolates the QApplication pre-create: v4 unchanged, plus one line of `app = QApplication.instance() or QApplication(sys.argv)` BEFORE napari.Viewer(). No QObject, no Signal, no QueuedConnection, no background emitter. If v5 wedges, the pre-created QApplication (vs the napari-built QNapariApplication class napari.Viewer() otherwise constructs) is the trigger. If v5 does not wedge, v6 adds the Qt bridge on top and that one has to be it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Contributor
Author
|
CI update — same cross-cell flake, now on Three of four cloud matrix cells on
This PR's diff on Re-ran the failed job once. Generated by Claude Code |
… trigger) Bisect so far: v1 baseline -> works (~1,600 block reads) v2 +threads -> works (~1,500 before force-quit) v3 +shrink +app +bridge -> WEDGES v4 shrink only -> works (shape cleared) v5 shrink + app -> works (QApplication pre-create cleared) Only piece left is the cross-thread Qt bridge: a QObject with a Signal, moveToThread(app.thread()), Qt.QueuedConnection, and a background daemon thread emitting through it. v6 adds that on top of v5 and should wedge, confirming the bridge as the fix target in the production launcher (_NapariStatusReporter._Bridge). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Chasing a "level shows coarse pixels while Auto picker reports level 0
and no error ever appears" symptom. Root cause in
readChunkWithFallback Path 1:
try:
result = _read_chunk_from_fetcher(fetcher, fine_doc, ...)
_bump("hit_fine")
return result
except Exception: # noqa: BLE001 - a bad decode is fallback time
pass # ← fell through to coarse-upsample. No log, no counter.
A fine chunk on disk whose decode raises (truncated download, codec
mismatch, corrupt blob) is silently replaced with coarse-upsample
pixels. The user sees the wrong resolution forever, and there is no
hint anywhere in the log that a decode even failed. That was the
"not shippable even as a beta" blocker.
Three changes, all additive:
1. New stat key `fine_decode_failed` so the stats summary and the
end-of-session loader.stats() dump report how often this fired.
2. On every fine-decode failure, print a one-line warning to stderr
with the chunk filename and the exception type + message. This
is the signal the user asked for -- a silent fallback becomes a
loud fallback, so a corrupt-on-disk situation is immediately
visible.
3. Lazy daemon thread (started on first readChunkWithFallback call,
only when NDI_LIGHTSHEET_DEBUG / NDI_LIGHTSHEET_UPSAMPLE_DEBUG
is on) that dumps the fallback stats every ~5 s when something
changed. A session that stays blurry can now be diagnosed
live rather than only after the viewer closes.
Behaviour-preserving: Path 1 still falls through to upsample on a
decode failure (a black block would be worse UX than blurry); the
only difference is you can see it happen now.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
…ints Two small diagnostics prompted by the last run's log: (1) The 5-second "no tiles requested yet by napari; waiting for first slice ..." heartbeat was telling the user napari had not asked for a slice -- for 70+ seconds -- while 200+ fallback-reader calls were in fact completing. The underlying _FetchCounter counts cloud HTTPS fetches via watchFetches, which is permanently zero on a local-disk pyramid. Check totalReaderActivity() from the upsample fallback before claiming the slicer is dead; when it is non-zero print "local-disk slicer active: N fallback-reader call(s)" instead, with a note that the cloud fetch counter staying at zero is normal on a local pyramid. The user will no longer be told the slicer is dead when it is working. (2) Added a `refresh_hints_fired` counter to the fallback stats dict, incremented on every RefreshHint._fire. Combined with the live heartbeat (NDI_LIGHTSHEET_DEBUG=1), this exposes whether the swap- coarse-to-fine prod is actually running when prefetches complete. If `hit_fine` keeps climbing but `refresh_hints_fired` stays low, the on-complete callback is not landing in RefreshHint; if both climb but the view still looks coarse, napari is ignoring our refresh and the fix moves to the napari / vispy side (synthesising a QWheelEvent to the canvas, or forcing viewer.camera.events.zoom through its native path). Also exposed `_bump` under the public name `bump` from the fallback module so RefreshHint can increment the shared counter without reaching into a private name. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Last run showed refresh_hints_fired=0 for the entire session despite
~250 fine chunks landing on disk and the slicer running against 500+
blocks. Napari was only re-slicing because the user was generating
camera events by hand -- the automatic nudge our prefetch-complete
callback was supposed to send was never arriving.
To find the broken link, split the single counter into four and log
one install line:
refresh_hints_null reader called with slot[0] still None
(hint not yet installed, or install
was a no-op because _fallback_context
did not exist)
refresh_hints_called RefreshHint.__call__ invoked from the
prefetch's completion callback
refresh_hints_scheduled QTimer.singleShot accepted the fire
refresh_hints_fired RefreshHint._fire actually ran on the
Qt main thread
Plus one log line in loader.registerRefreshHint saying whether the
hint got installed into the slot or the ctx was None (which would
silently skip the install).
Interpretation on the next run:
refresh_hints_null > 0 at the start -> add_image is slicing before
the hint is registered;
the earliest prefetches
have on_complete=None and
can never nudge.
refresh_hints_null high the whole run -> registerRefreshHint ran
but the slot write did not
reach the ctx the reader
sees (two different ctx
dicts somewhere).
called=0, scheduled=0, fired=0 -> prefetches complete but
_settle_in_flight is not
dispatching the callback;
bug in the fetcher.
called>0 but fired=0 -> QTimer.singleShot does not
dispatch from the worker
thread -- Qt event-loop
issue, maybe tied to the
same destroyed-dispatcher
warning we have been
seeing from vispy.
The counters all ride in the same _STATS dict the live heartbeat
already dumps, so the next NDI_LIGHTSHEET_DEBUG=1 run reports them
every 5 seconds.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Last run surfaced the root cause of the "stays coarse" symptom: refresh_hints_null=1 only the one pre-registration slice refresh_hints_called=122 RefreshHint.__call__ invoked refresh_hints_scheduled=122 QTimer.singleShot accepted refresh_hints_fired=0 _fire NEVER ran QTimer.singleShot(ms, callable) posts the timer to the event dispatcher of the thread that called it. The fetcher's background pool threads do not have a Qt event loop, so the timer sits there and never fires -- exactly the pattern the vispy warnings "QBasicTimer:: start: current thread's event dispatcher has already been destroyed" describe. Napari never got the nudge to re-slice, so new fine chunks on disk only became visible when the user manually zoomed. Rewrite RefreshHint as a QObject that lives on the Qt main thread and expose the completion callback as a Signal wired with Qt.QueuedConnection. Emitting from any thread marshals a queued event to the main-thread event loop; _onRequest runs on main and schedules the debounced _fire via QTimer (safe now that we are on the right thread). Same pattern as _NapariStatusReporter._Bridge in viewer.py. Also keep a bare-Python no-op shim for headless (no-Qt) imports so tests and non-GUI callers can still import the class. With this fix, refresh_hints_fired should climb along with hit_fine, and napari should repaint without the user zooming. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Previous run proved the queued-connection fix works
(refresh_hints_fired=205) but surfaced two more problems:
(1) The debounce was not debouncing. Each prefetch completion
emitted the signal, each emit queued a 250 ms single-shot
timer, so a burst of 205 completions produced 205 timers and
205 fires -- hundreds of refresh() calls in a 10 s window.
(2) Even with 205 refresh() + events.set_data() calls, hit_fine
stayed 0 and upsampled stopped climbing: napari 0.9.1 async
slicer ignores the public refresh/set_data API in this build.
The Refresh View button's programmatic
viewer.camera.zoom *= 0.8 also failed to re-slice. The only
thing proven to work on this user's machine is a real mouse
wheel over the canvas.
Fixes:
(1) Replace the per-emit QTimer.singleShot with ONE persistent
QTimer owned by the hint. Each _onRequest restarts the
timer, so a burst collapses into one _fire ~250 ms after the
last fetch completion in the burst.
(2) In _fire, after the inert refresh()+set_data() pair, post a
synthetic QWheelEvent pair (wheel-up then wheel-down) to the
napari qt canvas widget. The two notches cancel so the user
sees no zoom flicker, but Qt delivers two camera events to
napari's view machinery through the same code path a real
mouse wheel takes -- the only path we have evidence re-slices
on this build. Canvas widget is resolved through
viewer.window._qt_viewer.canvas.native (and a couple of
fallbacks) with a best-effort try/except so a napari version
bump that renames the attribute doesn't crash the viewer.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
… data)
Live test on the user's Maddie lightsheet pyramid showed the fallback
actively blocking the swap-to-fine refresh:
with fallback ON (previous default):
first paint -> upsampled coarse (Path 2) -> napari marks slice
"loaded". Prefetches land. Our refresh hint fires. Napari re-slices,
Path 1 returns real fine data (hit_fine climbs). But napari does
NOT re-render -- the on-screen pixels stay coarse until the user
zooms in and out by hand to generate real mouse-wheel camera
events. The user described this as "the low-res version replaces
the correct draw".
with fallback OFF (NDI_LIGHTSHEET_UPSAMPLE_FALLBACK=0):
first paint -> black for missing chunks. As prefetches land and
napari re-slices, the view refreshes to show real fine data
without needing the user to zoom. Correct behaviour.
Flip the default. Keep the knob (NDI_LIGHTSHEET_UPSAMPLE_FALLBACK=1
now opts IN) so cloud users with multi-minute fetch stretches can
still get the "blurry but present" first paint if they prefer it to
black -- that was the original motivation and it may still be the
right trade on very slow cloud connections. But locally and on fast
cloud links, OFF is the correct default: "black briefly then correct
fine pixels" beats "coarse pixels forever that never refine".
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Follow-up to 9c5134f which flipped env_on() default to False. Two tests still asserted the old ON-by-default behaviour and failed on 3.12 (and would have failed on 3.10 and 3.11 once they finished running): * test_pyramid_upsample_fallback.TestEnvGate.test_absent_env_is_on -> renamed to test_absent_env_is_off and inverted the assertion. * test_pyramid_loader.TestStats.test_fallback_line_only_appears_when_the_env_is_on -> inverted the two branches: unset env now means no 'fallback' key in loader.stats(), and NDI_LIGHTSHEET_UPSAMPLE_FALLBACK=1 is the opt-in that makes it appear. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tests/test_pyramid_loader_integration.pyagainst the permanent lightsheet fixture (6ab84b549852a120dbcb22bc, test user 1, prod) that the MATLAB roundtrip uploaded viabuild-lightsheet-fixture.yml. Skips cleanly without cloud creds.tests/fixtures/lightsheet_cloud_fixture.jsonso a regeneration is one edit in one place; the env varNDI_LIGHTSHEET_TEST_FIXTURE_IDoverrides for a one-shot against a fresh fixture.info.remoteDatasetName→info.cloudDatasetNamein the fixture-build workflow (the last dispatch printedCLOUD_DATASET_IDcorrectly, then crashed on that field name, so outputs / summary / artifact never populated).test-cloud-api.ymlgains a path-filteredpull_requesttrigger so the loader integration test runs on PRs that touch the pyramid code without burning cloud calls on unrelated PRs.Commits
5310aaf— CI: useinfo.cloudDatasetName(roundtrip's real field name)ba67878— Wire the pyramid loader integration test to the cloud fixtureb320cf1— CI: run Test Cloud API on pyramid-loader PRsTest plan
tests/test_pyramid_loader_integration.py— 5 tests pass, per-level compute times +loader.stats()printed for baseline profiling.test_pyramid_loader_integration.py sssssskips locally without creds (already confirmed).build-lightsheet-fixture.ymlafter merge → confirm the step summary +lightsheet-fixture-idartifact now populate (the field-name fix reaches them via GITHUB_OUTPUT).🤖 Generated with Claude Code
https://claude.ai/code/session_01N67xH9BejMzZp4GNAq8m7w
Generated by Claude Code