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/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. 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/files.py b/src/ndi/cloud/api/files.py index fead55f5..fca513f2 100644 --- a/src/ndi/cloud/api/files.py +++ b/src/ndi/cloud/api/files.py @@ -11,6 +11,7 @@ from __future__ import annotations import logging +import re import time from pathlib import Path from typing import Annotated, Any, Literal @@ -30,6 +31,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") @@ -59,14 +87,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, @@ -103,6 +148,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. @@ -121,6 +167,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 @@ -136,12 +188,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, ) @@ -164,6 +217,8 @@ def putFileBytes( url: NonEmptyStr, data: bytes, timeout: int = 120, + *, + checksum_crc32: str = "", ) -> bool: """PUT raw bytes to a presigned S3 URL. @@ -171,6 +226,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. @@ -186,7 +243,7 @@ def putFileBytes( resp = requests.put( url, data=data, - headers={"Content-Type": "application/octet-stream"}, + headers=_upload_headers(_checked_crc32(checksum_crc32)), timeout=timeout, ) @@ -357,7 +414,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 @@ -473,6 +531,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( @@ -482,6 +543,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 # @@ -512,8 +626,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. @@ -604,7 +720,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). @@ -619,6 +736,7 @@ def getSignedURLSetAll( "datasetId": dataset_id, "documentId": document_id, "files": {}, + "fileInfo": [], "pageCount": 0, "totalCount": 0, "pages": 0, @@ -650,6 +768,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 358b9b09..38506bfd 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" @@ -759,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 @@ -771,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" @@ -805,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 @@ -829,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 @@ -892,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 @@ -924,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 @@ -962,6 +980,30 @@ 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" + 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" @@ -1426,6 +1468,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 +1680,7 @@ not_yet_ported: - name: url matlab_path: "+ndi/+cloud/+api/url.m" - matlab_last_sync_hash: "3fce6cb" + matlab_last_sync_hash: "8fa6ea5e" status: ported_differently decision_log: > MATLAB URL builder. Python uses CloudClient._build_url() @@ -1563,6 +1698,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, @@ -1573,6 +1712,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" @@ -1633,7 +1777,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. @@ -1648,7 +1792,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 @@ -1662,6 +1806,10 @@ not_yet_ported: default signer now goes through the async job path (createSignedURLSetJob + waitForSignedURLSetJob + getSignedURLSetResult). + 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/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/cloud/ndi_matlab_python_bridge.yaml b/src/ndi/cloud/ndi_matlab_python_bridge.yaml index 93a3bc0b..8b5e5ed1 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" @@ -960,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 @@ -976,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" @@ -986,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 @@ -1542,6 +1557,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/ @@ -1685,6 +1701,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/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: > 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/doc_lightsheet/ndi_matlab_python_bridge.yaml b/src/ndi/fun/doc_lightsheet/ndi_matlab_python_bridge.yaml index 2f1a76dd..8a276cc4 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: "5c9e8a8" + 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, @@ -39,22 +40,27 @@ functions: one lightsheetZarrLevel document per unique array path across all multiscales entries. Level 0 is deduped when two entries reference the same NGFF path (typical for mean + max pyramids). c8f3479 added - tileBudgetBytes (default 32 MB uncompressed; was 8 MB until the - sweet-spot measurement bumped it) and an optional chunks override, - forwarded to makePyramid so each level is re-tiled to hit the - budget. bd6a245 added materializeChunks (default false) which + tileBudgetBytes (default 8 MB uncompressed) and an optional chunks + override, forwarded to makePyramid so each level is re-tiled to hit + the budget. bd6a245 added materializeChunks (default false) which makePyramid uses to read each level from the source OME-Zarr and write chunk.bin_ files onto the level documents before database_add -- the Python port lands the same option and calls - into ndr.format.omezarr.readArray equivalent. 5c9e8a8 bumped the - default tileBudgetBytes from 8 MB to 32 MB; no Python-side change - is needed while this entry is porting_deferred -- the deferred - port will pick up the new default when it lands. Ports with the - other entries under this heading. + 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: "5c9e8a8" + matlab_last_sync_hash: "04ce18f9" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: porting_deferred decision_log: >- The worker fromOMEZarr calls; writes one lightsheetZarrPyramid @@ -63,10 +69,9 @@ functions: reduction_function on the level names the reduction ('none' for the shared raw level 0, 'mean' / 'max' / ... for reduced levels). c8f3479 drops the source zarr's chunk shape and re-chooses per - level via chooseTileShape to hit tileBudgetBytes (default 32 MB - uncompressed, sized to the measured signing+fetch+decode sweet - spot for a viewer on an ~200 MB/s link; bumped up from 8 MB after - ~121k-chunk level 0s in production). 04293e3 encodes + level via chooseTileShape to hit tileBudgetBytes (default 8 MB + uncompressed, sized for a viewer over an ~200 MB/s link fetching + ~4 tiles in parallel per pan gesture). 04293e3 encodes axes_units as a comma-separated string (e.g. 'micrometer,micrometer,micrometer') and leaves channel_names empty by default, matching the schema type change from matrix @@ -85,14 +90,54 @@ functions: self-installing private venv (sonpipe pattern; MATLAB's pyenv is never touched). Container bytes are unchanged, so this side of the port stays the same when it lands: use numcodecs.Blosc - directly. 5c9e8a8 bumped the default tileBudgetBytes from 8 MB - to 32 MB; no Python-side change is needed while this entry is - porting_deferred -- the deferred port will pick up the new - default when it lands. Ports with the other entries. + 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. + 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: "668beb13" + 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. + 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" + 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" - matlab_last_sync_hash: "5c9e8a8" + 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 @@ -109,15 +154,20 @@ functions: scalar); the equivalent Python idiom is `[ax not in "tc" for ax in axes_order.lower()]`. Ports with the other entries under this heading; the Python builder that mirrors makePyramid will call - it directly. 5c9e8a8 changed only the docstring default from - 8 MB to 32 MB (the argument default on this pure helper also - moved, but the Python port -- when it lands -- will take the - budget as a parameter and the makePyramid-side default is what - callers actually hit); no port-side change needed. + 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' @@ -132,6 +182,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 @@ -142,6 +193,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 @@ -157,6 +209,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 @@ -171,7 +224,8 @@ 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: >- MATLAB caller that pairs viewCommand + SYSTEM: builds the @@ -183,3 +237,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/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..5edc5867 100644 --- a/src/ndi/fun/ndi_matlab_python_bridge.yaml +++ b/src/ndi/fun/ndi_matlab_python_bridge.yaml @@ -1298,6 +1298,44 @@ 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" + 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" + 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/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/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/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/ndi_matlab_python_bridge.yaml b/src/ndi/gui/app/lightsheetZarr/ndi_matlab_python_bridge.yaml index 764100e6..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,8 @@ functions: # ----------------------------------------------------------------------- - name: LightsheetZarrManager matlab_path: "+ndi/+gui/+app/LightsheetZarrManager.m" - matlab_last_sync_hash: "60a79cf0" + matlab_last_sync_hash: "fb10169b" + matlab_branch: "claude/lightsheet-zarr-ndi-viewer-djp5vk" status: matlab_only decision_log: >- matlab_only: napari is Python-only, so the manager class stays @@ -52,6 +53,21 @@ 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. + + 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). # ----------------------------------------------------------------------- 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..58552a63 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 @@ -190,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 @@ -211,6 +217,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 +242,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 +300,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): @@ -351,6 +405,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: @@ -449,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() @@ -483,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, "maxWorkers", 1)) + print( + 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, + ) + 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() @@ -533,6 +645,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. @@ -755,7 +873,46 @@ 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, + fetcher=fetcher, + 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 + # 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, + ) + 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 @@ -1100,7 +1257,142 @@ 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 _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 @@ -1150,7 +1442,14 @@ def _attach_cloud_panel(viewer) -> 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 @@ -1195,6 +1494,18 @@ def _attach_cloud_panel(viewer) -> None: beta_row.addStretch(1) outer.addLayout(beta_row) + if report_collect is not None: + from ndi.gui.feedback import reportButton + + outer.addWidget( + reportButton( + box, + app="lightsheetZarr-viewer", + collect=report_collect, + title="Report a problem with the lightsheet viewer", + ) + ) + signin = QPushButton("Sign in...") signin.setToolTip( "Sign in to NDI Cloud, so tiles that are not already\n" @@ -1213,6 +1524,9 @@ def _attach_cloud_panel(viewer) -> 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/gui/app/lightsheetZarr/viewtiles.py b/src/ndi/gui/app/lightsheetZarr/viewtiles.py new file mode 100644 index 00000000..e8fc3e08 --- /dev/null +++ b/src/ndi/gui/app/lightsheetZarr/viewtiles.py @@ -0,0 +1,291 @@ +"""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", "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 +#: 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 + +#: 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]. + + 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 _why(f"layer {getattr(layer, 'name', '?')!r} is not multiscale") + try: + if len(layer.data) != len(meta_levels): + 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 + 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 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 _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"]) + 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 viewChunks(viewer, layers, meta_levels: list[dict]) -> dict | None: + """Stored chunks the current view needs: ``{(doc_id, name): doc}``. + + 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") + 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 _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[(doc_id, name)] = doc + return wanted + + +def _countReady(wanted: dict, fetcher) -> int: + ready = 0 + 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 + + +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): + """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. + """ + 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 as exc: # noqa: BLE001 + log(f"off -- no Qt timer ({exc})") + return None + + parent = None + try: + window = viewer.window + parent = getattr(window, "_qt_window", None) + except Exception: # noqa: BLE001 + pass + + 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: + 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}") + if value is not None: + last["reason"] = "" + 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() + log(f"on ({len(meta_levels)} level(s), polling every {POLL_MS} ms)") + return timer diff --git a/src/ndi/gui/feedback.py b/src/ndi/gui/feedback.py new file mode 100644 index 00000000..765c5a97 --- /dev/null +++ b/src/ndi/gui/feedback.py @@ -0,0 +1,629 @@ +"""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", + "reportButton", + "napariState", +] + +#: 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 + + +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/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..f3fd1af2 100644 --- a/src/ndi/pyramid/multiscale.py +++ b/src/ndi/pyramid/multiscale.py @@ -31,12 +31,55 @@ 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. + """ + + +# 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.""" + + __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 @@ -140,34 +183,72 @@ 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): + 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._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" @@ -186,6 +267,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): @@ -202,22 +315,118 @@ def _reopener(session): cls = type(session) return lambda: cls(path) - def _initThread(self): - self._local.session = self._reopen() - - 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 - - self._pool = ThreadPoolExecutor( - max_workers=self._workers, - thread_name_prefix="ndi-lightsheet", - initializer=self._initThread, + 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 and not self._direct: + 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, ) - return self._pool + self._threads.append(t) + t.start() + return True + + def _workerLoop(self) -> None: + # 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 + while job is None: + while (not self._heap or self._running >= self._limit) 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 + self._running += 1 + 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, [] + self._running -= 1 + self._cv.notify_all() + job.path = path + job.done.set() + for cb in callbacks: + try: + cb(path) + except Exception: # noqa: BLE001 - a bad callback is not fatal + pass + + 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 + + @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. @@ -226,18 +435,105 @@ 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 - for f in [pool.submit(lambda: None) for _ in range(self._workers)]: - f.result() + 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 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 _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 @@ -266,29 +562,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. @@ -307,10 +786,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"{max_fetch_s:.2f}s), {n_none} failures, " + f"{self._stale_released} stale reads released, " + f"{n_fallback} via a session ({self._limit} of {self._workers} workers)" ) def reset_stats(self) -> None: @@ -318,6 +800,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. @@ -390,83 +875,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,23 +931,92 @@ 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() + + +_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. Env override for the cloud-latency case.""" + """Chunks fetched at once at launch; ``NDI_LIGHTSHEET_WORKERS`` overrides. + + 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", "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, @@ -541,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 @@ -554,6 +1080,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 +1097,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/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/_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/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." } diff --git a/tests/test_cloud_api_feedback.py b/tests/test_cloud_api_feedback.py new file mode 100644 index 00000000..755f38fd --- /dev/null +++ b/tests/test_cloud_api_feedback.py @@ -0,0 +1,315 @@ +"""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) + # A hand-built client never refreshes against env credentials, so a + # 401/403 surfaces as CloudAuthError rather than a re-login attempt. + client._can_reauth = False + 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_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_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 diff --git a/tests/test_cloud_live.py b/tests/test_cloud_live.py index 445d52b0..c8ec5597 100644 --- a/tests/test_cloud_live.py +++ b/tests/test_cloud_live.py @@ -806,6 +806,125 @@ 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 _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 + + 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 + + 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 + + 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 + + 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 + + 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_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) 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 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..27e71ba3 --- /dev/null +++ b/tests/test_gui_lightsheetZarr_viewtiles.py @@ -0,0 +1,228 @@ +"""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 + 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: + 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__]) 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." ) 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 diff --git a/tests/test_pyramid_fetch_direct.py b/tests/test_pyramid_fetch_direct.py new file mode 100644 index 00000000..1b087f82 --- /dev/null +++ b/tests/test_pyramid_fetch_direct.py @@ -0,0 +1,297 @@ +"""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() == 16 + 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 + + +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) 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