Skip to content

Rebuild series ingest_locations on SyncFiles=false download (companion to VH-Lab/NDI-matlab#988) #302

Closed

Description

@stevevanhooser

Follow-up carved out of the closing note on #300. The workaround retirement was retracted, but this gap survived and is worth its own tighter issue.

What is missing

ndi.cloud.filehandler.updateFileInfoForRemoteFiles rewrites a downloaded document's file_info locations to ndic:// URIs on a SyncFiles=false download. It does not rebuild series_info[k].ingest_locations for any series the document carries. DID strips ingest_locations before storing a document, so a document read back from a cloud dataset returns with n_present populated and ingest_locations empty -- exactly the shape DID's MembersNotLocatable guard (DID-matlab#185 / DID-python's _reject_series_without_ingest_locations) refuses on the following add_docs. The whole SyncFiles=false path is blocked on that guard for any document carrying a populated series.

The SyncFiles=true path is fine: updateFileInfoForLocalFiles already calls reconstructSeriesIngestLocations, which reads the manifest bytes off disk (they were downloaded alongside the doc) and rebuilds ingest_locations from them.

The gap is that on SyncFiles=false the manifest bytes are not on disk -- there is nothing to read. So the reconstruction needs a fetch, and it needs to work the same way DID's read path does (VH-Lab/DID-matlab#201 / VH-Lab/DID-python#88): through the DID customFileHandler contract.

What NDI-matlab does (the shape to port)

VH-Lab/NDI-matlab#988 (three commits: 1a6ec2fc, 414e8572, a4dfb076) taught updateFileInfoForRemoteFiles.m to:

  1. Accept an optional customFileHandler name-value argument.
  2. After the file_info rewrite, if any series in series_info has n_present > 0 and empty ingest_locations, fetch each such series' manifest bytes into a scratch directory (through the handler when supplied, or a direct fetch minted through ndi.cloud.api.files otherwise) and call reconstructSeriesIngestLocations with that scratch directory.
  3. The scratch directory is cleaned up on function exit. Manifest bytes do not land in the DID file cache here; a subsequent member open trips DID#201's lazy fetch, which is what populates the cache. Cache eviction on a later session is answered by the same re-fetch path.
  4. seriesName is deliberately empty in the handler context -- a non-empty seriesName marks a MEMBER fetch, where sourcePath is the manifest and ctx.uid is the member; the manifest fetch itself is the opposite (sourcePath names the manifest, ctx.uid names the same manifest).
  5. A handler failure leaves the entry un-reconstructed so DID's guard fires on add_docs with the document's own identity -- the right signal a partial download deserves.

Offline coverage is tests/+ndi/+unittest/+database/TestUpdateFileInfoForRemoteFilesShape.m (VH-Lab/NDI-matlab#987): a mock handler serves manifest bytes by uid, and the test walks a doc through DID's store/read cycle so ingest_locations gets stripped (the exact shape a cloud round trip returns), then reshapes through updateFileInfoForRemoteFiles with the mock and pins the rebuilt ingest_locations.

What to change on the Python side

src/ndi/cloud/filehandler.py:

  • Add an arity-aware _dispatch_custom_file_handler(handler, dest_path, source_path, context) helper mirroring did.implementations.sqlitedb.SqliteDb._dispatch_custom_file_handler.
  • Add a _fetch_manifest(dest_path, cloud_dataset_id, manifest_uid, document_id, series_name, custom_file_handler=None, client=None) helper. With a handler, dispatch through it with seriesName="" in the context. Without a handler, call fetch_cloud_file directly, matching what the read-side does when a manifest is asked for by uid.
  • Modify updateFileInfoForRemoteFiles to take an optional keyword-only custom_file_handler=None and, after the file_info rewrite, when any series_info entry has n_present > 0 and empty ingest_locations, fetch each qualifying series' manifest bytes to a tempfile.TemporaryDirectory and call reconstructSeriesIngestLocations(doc_props, str(tmp_dir), cloud_dataset_id) before the temp dir is cleaned up. A per-manifest fetch failure is caught and logged; the entry is left un-reconstructed so DID's guard fires on add.

tests/test_cloud_series_reconstruction_remote.py (new file, or extend tests/test_cloud_series_reconstruction.py):

  • Ordinary reshape: every file_info location becomes ndicloud, ingest=0, delete_original=0, uid survives, location is ndic://<dataset>/<uid>.
  • A doc whose ingest_locations is already populated skips reconstruction (no fetch, no handler call).
  • A doc without series_info skips the whole block and still reshapes file_info.
  • End-to-end offline: a mock handler serves manifest bytes by uid; take a doc, run it through DID's add_docs / get_docs cycle so ingest_locations gets stripped, reshape via updateFileInfoForRemoteFiles(..., custom_file_handler=mock), and pin the rebuilt ingest_locations (one entry per present member, ndic:// location, ingest=0, one manifest fetch total with seriesName="").

Bridge yamls:

  • Bump updateFileInfoForRemoteFiles's matlab_last_sync_hash in src/ndi/cloud/ndi_matlab_python_bridge.yaml to the NDI-matlab main head that carries #988 (currently aea0c72c), and update the decision_log to record the port rather than the deferral.
  • Update src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml's downloadNdiDocuments entry if its decision_log calls out the gap.

Not in scope

A Python analog of NDI-matlab's FileSeriesRoundTripTest.testMembersSurviveADownloadFromTheCloudWithSyncFilesFalse -- the live-cloud round-trip test. Belongs in its own change, layered after this one; also flagged in the closing note on #300.

References

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions