Retire the manifest cloud-reference workaround (#300)

src/ndi/cloud/filehandler.py

@@ -274,33 +274,6 @@ def get_or_create_cloud_client() -> CloudClient:
     return CloudClient.from_env()
 
 
-def _ndic_location_record(cloud_dataset_id: str, file_uid: str) -> dict:
-    """One ``locations`` entry pointing at ``file_uid`` in the cloud.
-
-    The same record :meth:`ndi.document.ndi_document.add_file` builds for an
-    ``ndic://`` location, spelled out here because this function works on a
-    properties DICT rather than a document -- as updateFileInfoForRemoteFiles
-    does, and as the whole download pipeline does, so that both rewrites
-    mutate the same objects in place. ``test_the_record_matches_add_files``
-    pins the two together so this cannot drift.
-
-    The uid inside the URI is the file's ORIGINAL uid, the one the cloud
-    knows it by. The record gets a fresh uid of its own, exactly as add_file
-    mints one: DID passes only the location STRING to the handler, and a
-    series member's uid comes from the context.
-    """
-    from ..ido import ndi_ido
-
-    return {
-        "delete_original": 0,
-        "uid": ndi_ido().id,
-        "location": f"{NDIC_SCHEME}{cloud_dataset_id}/{file_uid}",
-        "parameters": "",
-        "location_type": "ndicloud",
-        "ingest": 0,
-    }
-
-
 def reconstructSeriesIngestLocations(
     doc_props: dict,
     file_directory: str,
@@ -443,28 +416,6 @@ def _manifest_uid(file_info: list, series_name: str) -> str:
     return ""
 
 
-def _declared_series_names(doc_props: dict) -> set:
-    """Lowercased names of the file series this document's class declares.
-
-    Asked of did.document rather than read out of files.file_series here:
-    which names are series is DID's question, the declaration is a property
-    of the document CLASS, and the matching is case-insensitive in a way
-    worth having in one place. Document() wraps the dict without copying it,
-    so this costs nothing and sees exactly what we are about to edit.
-
-    A document whose class declares no series -- almost all of them -- gives
-    an empty set, and nothing below changes.
-    """
-    try:
-        from did.document import Document
-    except ImportError:  # pragma: no cover - did is a hard dependency
-        return set()
-    try:
-        return {str(name).lower() for name in Document(doc_props).series_names()}
-    except Exception:  # noqa: BLE001 - a malformed document is not ours to fix
-        return set()
-
-
 def updateFileInfoForLocalFiles(
     doc_props: dict,
     file_directory: str,
@@ -478,40 +429,21 @@ def updateFileInfoForLocalFiles(
     the location with the local file path ``{file_directory}/{uid}``
     and sets ``delete_original=1``, ``ingest=1``.
 
-    SERIES, when ``cloud_dataset_id`` is given. Two things happen that do
-    not for an ordinary file:
-
-    A series MANIFEST keeps the cloud reference it came from, as a second
-    location beside the local copy. Its members are still on the cloud --
-    left there deliberately, so that opening a dataset does not drag down a
-    28,000-member series -- and a member has no location of its own, so DID
-    resolves one by handing the MANIFEST's location to the file handler with
-    the member's uid in the context. That location is the only thing telling
-    the handler where to look; with the local path alone the handler is
-    given something that does not start with ``ndic://`` and every member of
-    a downloaded series is unreadable. See NDI-matlab#966.
-
-    And the series' ``ingest_locations`` are rebuilt from the downloaded
-    manifest, which is what lets the document be added at all. See
-    :func:`reconstructSeriesIngestLocations`.
-
-    MATLAB carries files.series_info across a ``reset_file_info`` it makes
-    here (NDI-matlab#945). There is nothing to carry here: this edits the
-    properties dict where it stands and never resets, so the per-series
-    record survives on its own.
+    When ``cloud_dataset_id`` is given and the document declares any file
+    series with n_present > 0, the series' ``ingest_locations`` are rebuilt
+    from the downloaded manifest -- one entry per present member,
+    ``ingest=0`` -- which is what lets the document be added at all. See
+    :func:`reconstructSeriesIngestLocations` and NDI-matlab#958.
 
     Args:
         doc_props: ndi_document properties dict (as from JSON).
         file_directory: Directory where local files are stored.
-        cloud_dataset_id: Cloud dataset id. Without it, series get neither
-            the manifest's second location nor rebuilt ingest_locations,
-            and a document with a populated series will be refused by DID
-            on the following add.
+        cloud_dataset_id: Cloud dataset id. Without it, series get no
+            rebuilt ingest_locations, and a document with a populated
+            series will be refused by DID on the following add.
     """
     import os
 
-    series_names = _declared_series_names(doc_props)
-
     files = doc_props.get("files")
     if not files or not isinstance(files, dict):
         return
@@ -546,7 +478,6 @@ def updateFileInfoForLocalFiles(
         else:
             continue
 
-        extra_locations: list[dict] = []
         for loc in loc_list:
             if not isinstance(loc, dict):
                 continue
@@ -559,34 +490,15 @@ def updateFileInfoForLocalFiles(
                 loc["location_type"] = "file"
                 loc["delete_original"] = 1
                 loc["ingest"] = 1
-                # Manifests only. Every other file is already here, and a
-                # second location on each would be rows to no purpose.
-                if (
-                    cloud_dataset_id
-                    and str(fi.get("name", "")).lower() in series_names
-                    and not any(
-                        isinstance(other, dict)
-                        and str(other.get("location", "")).startswith(NDIC_SCHEME)
-                        for other in loc_list
-                    )
-                ):
-                    extra_locations.append(_ndic_location_record(cloud_dataset_id, uid))
             else:
                 logger.warning(
                     "Local file does not exist for uid %s at %s",
                     uid,
                     file_location,
                 )
 
-        loc_list.extend(extra_locations)
-
-        # A manifest that gained its cloud reference now has two locations,
-        # so it cannot go back as a bare dict -- doing so would drop the
-        # very location the members are resolved through.
-        if loc_was_dict and len(loc_list) == 1:
+        if loc_was_dict:
             fi["locations"] = loc_list[0]
-        elif loc_was_dict:
-            fi["locations"] = loc_list
 
     if was_dict:
         files["file_info"] = fi_list[0]

src/ndi/cloud/ndi_matlab_python_bridge.yaml

@@ -999,7 +999,7 @@ functions:
   # --- filehandler.py functions ---
   - name: updateFileInfoForRemoteFiles
     matlab_path: "+ndi/+cloud/+sync/+internal/updateFileInfoForRemoteFiles.m"
-    matlab_last_sync_hash: "fe64a9f5"
+    matlab_last_sync_hash: "aea0c72c"
     python_path: "ndi/cloud/filehandler.py"
     input_arguments:
       - name: doc_props
@@ -1019,25 +1019,26 @@ functions:
       and the per-series record survives on its own.
       SERIES ARE PORTED NOW (NDI-python#215; this entry used to say they
       were not). cloudDatasetId is the third argument on both sides, and
-      with it a downloaded series gets the two things it needs to be
-      readable: the manifest keeps the ndic:// reference it came from as a
-      second location beside the local copy (504c5bcf4, NDI-matlab#966),
-      so DID can hand that location to the file handler with a member's uid
-      in the context; and files.series_info(:).ingest_locations are rebuilt
-      from the downloaded manifest by reconstructSeriesIngestLocations
-      (cd3c11673, NDI-matlab#958), without which DID refuses the document
-      outright. The representation itself is DID's -- files.series_info,
-      file_series and n_present are read through did.document, not
-      reimplemented here, per AGENTS.md section 4.
-      Python builds the manifest's second location record directly rather
-      than through ndi.document.add_file, because this works on a properties
-      DICT: the whole download pipeline mutates dicts in place, as
-      updateFileInfoForRemoteFiles does, and converting to a document would
-      deep-copy and lose the mutation. The record shape is pinned to
-      add_file's by test_cloud_series_reconstruction.py.
+      with it files.series_info(:).ingest_locations are rebuilt from the
+      downloaded manifest by reconstructSeriesIngestLocations (cd3c11673,
+      NDI-matlab#958) -- without which DID refuses the document outright.
+      The representation itself is DID's -- files.series_info, file_series
+      and n_present are read through did.document, not reimplemented here,
+      per AGENTS.md section 4.
+      The second ``ndic://`` location the manifest used to keep beside the
+      local copy (504c5bcf4, NDI-matlab#966) was retired in NDI-matlab#986
+      once DID-matlab#191/#201 taught the read path to reach a local-file
+      manifest directly. Ported here alongside NDI-matlab#989.
+      NDI-matlab#988 (1a6ec2fc, 414e8572, a4dfb076) additionally taught
+      updateFileInfoForRemoteFiles.m to rebuild series ingest_locations
+      by fetching the manifest bytes through the DID customFileHandler
+      contract, so a SyncFiles=false round trip carrying a series document
+      is accepted on add. NOT YET PORTED HERE; hash bumped to aea0c72c to
+      acknowledge the drift. Filed against Waltham-Data-Science/NDI-python#300
+      as a follow-up.
   - name: updateFileInfoForLocalFiles
     matlab_path: "+ndi/+cloud/+sync/+internal/updateFileInfoForLocalFiles.m"
-    matlab_last_sync_hash: "504c5bcf4"
+    matlab_last_sync_hash: "2ad46c0f"
     python_path: "ndi/cloud/filehandler.py"
     input_arguments:
       - name: doc_props
@@ -1058,15 +1059,13 @@ functions:
       rebuilds, Python edits the properties dict where it stands. So
       MATLAB's #945 fix -- carrying files.series_info across the reset that
       would otherwise drop it -- has nothing to fix here; there is no reset.
-      NOT PORTED, and larger than this entry: MATLAB's cloudDatasetId
-      argument (045c0eef4, 504c5bcf4) reconstructs a downloaded series'
-      ingest_locations and keeps the manifest's ndic:// reference beside the
-      local copy, so a member of a 28,000-member series left on the cloud
-      can still be resolved. Python has no file-series support in the cloud
-      path at all -- nothing here reads files.series_info, file_series or
-      n_present -- so there is no partial fix to make; the port of series
-      themselves has to come first. Recorded here rather than left implied
-      by a bumped hash.
+      cloudDatasetId (045c0eef4) drives the ingest_locations rebuild via
+      reconstructSeriesIngestLocations so DID-matlab#185's MembersNotLocatable
+      guard is satisfied on the following add_docs. The second ``ndic://``
+      location the manifest used to keep beside the local copy (504c5bcf4,
+      NDI-matlab#966) was retired in NDI-matlab#986 (PR #989, 2ad46c0f) once
+      DID-matlab#191/#201 taught the read path to reach a local-file manifest
+      directly. Ported here alongside that removal.
 
   - name: parse_ndic_uri
     matlab_path: "N/A"

src/ndi/cloud/orchestration.py

@@ -143,9 +143,9 @@ def downloadDataset(
             print(f'  Files downloaded: {report["downloaded"]}, failed: {report["failed"]}')
         for dj in doc_jsons:
             # cloud_dataset_id is threaded through so a downloaded series
-            # keeps its manifest's ndic:// reference and gets its
-            # ingest_locations rebuilt -- without which DID refuses the
-            # document outright. Mirrors NDI-matlab#958.
+            # gets its ingest_locations rebuilt from the downloaded manifest
+            # -- without which DID refuses the document outright
+            # (DID-matlab#185). Mirrors NDI-matlab#958.
             updateFileInfoForLocalFiles(dj, str(staging), cloud_dataset_id)
 
     # Convert to ndi_document objects and create ndi_dataset with them.

src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml

@@ -661,9 +661,14 @@ functions:
       optional third argument and every test called
       updateFileInfoForLocalFiles directly, so dropping it at the call site
       broke nothing any test could see while a downloaded series silently lost
-      its manifest reference. Now pinned by
+      its ingest_locations reconstruction. Now pinned by
       TestTheCloudDatasetIdReachesTheRewrite in
       tests/test_cloud_download_ordering.py.
+      (3) The second ``ndic://`` location the manifest used to keep beside
+      the local copy (NDI-matlab#966) was retired in NDI-matlab#986 (PR
+      #989, 2ad46c0f) once DID-matlab#191/#201 taught the read path to
+      reach a local-file manifest directly. Ported here alongside that
+      removal.
 
 # =============================================================================
 # Not-yet-ported MATLAB functions

src/ndi/gui/ndi_matlab_python_bridge.yaml

@@ -838,7 +838,7 @@ ported_late:
 
   - name: GEFManager
     matlab_path: "+ndi/+gui/+app/GEFManager.m"
-    matlab_last_sync_hash: "06f31a4e5"
+    matlab_last_sync_hash: "59c96e57"
     status: porting_deferred
     decision_log: >
       MATLAB App Designer window that lists, views, adds and removes
@@ -875,3 +875,9 @@ ported_late:
       dialog with it. Nothing to port: the Python side draws no dialog at
       all. View builds a command line and runs napariViewGEF, which is
       already its own process and has never blocked anything.
+
+      NDI-matlab 59c96e57 (VH-Lab/NDI-matlab#984) raises the open GEF
+      Manager for a session instead of building a second window when the
+      navigator's Apps menu opens it again. MATLAB App Designer window
+      uniqueness only; nothing to port on this side, which draws no such
+      window in the first place. Hash bumped to acknowledge the drift.

tests/test_cloud_download_ordering.py

@@ -246,14 +246,16 @@ class TestTheCloudDatasetIdReachesTheRewrite:
 
     MATLAB does it inside downloadNdiDocuments, where the documentUpdateFcn
     is built; here the rewrite lives in orchestration.downloadDataset. Either
-    way the id is what lets a downloaded series keep its manifest's ndic://
-    reference and get its ingest_locations rebuilt -- without which DID
-    refuses the document.
+    way the id is what lets a downloaded series get its ``ingest_locations``
+    rebuilt from the downloaded manifest -- without which DID refuses the
+    document. (The second ``ndic://`` location the manifest used to keep
+    for member fetches was retired with NDI-matlab#986; the id is still
+    needed for the ingest_locations rebuild alone.)
 
     updateFileInfoForLocalFiles takes cloud_dataset_id as an OPTIONAL third
     argument, and every existing test called it directly. So dropping the
     argument at the call site broke nothing any test could see, while a
-    downloaded series lost the reference. This pins the call site.
+    downloaded series lost the reconstruction. This pins the call site.
     """
 
     def _spy(self, monkeypatch):