No file-series support in the cloud path: a downloaded series' members are unreachable #215

Activity

1. claude commented on Sep 8, 2026

   

   claudebot

   on Sep 8, 2026 – with Claude

   More actions

   Step 1 answered: the representation belongs in DID-python, and it is already there

   I cloned DID-python (7e92695) to check. did.document models file series in full, and it landed a day before this issue was written — commit f292b16, "Port DID-matlab #173 file series + per-uid fetch lock; refresh bridge", merged as DID-python#65. The bridge entries are dated PORTED 2026-09-07.

   layer	what DID-python has
   did.document	series_names, is_file_series, series_member_of, is_series_member, series_count → (n, n_present), series_source_root, series_ingest_locations, add_file_series, remove_file_series, strip_series_ingest_locations — reading and writing files.file_series, files.series_info, n_present
   did.file	read_series_manifest, read_series_manifest_uid, write_series_manifest — the shared binary wire format
   did.database	series_has, series_members, _series_manifest_path
   did.implementations.sqlitedb	_ingest_series_members on add, _open_series_member on read, _manifest_locations_for_handler, and the DID-matlab#191 guard against a handler that returns the manifest's own bytes

   So the fork in step 1 resolves cleanly, and it resolves the way AGENTS.md §4 says it should: NDI-python must model no part of files.series_info itself. Not series_info, not n_present, not the manifest format, not member enumeration. All of it goes through did.

   What steps 2–4 describe is not representation, though — it is the cloud round trip, and that is genuinely NDI's layer. DID is explicit that it stays out of it: _open_series_member's docstring says "DID composes no URL and learns no scheme", and ndic:// is NDI's scheme. So this issue is cleared for NDI-python, with one precondition and one prerequisite that is larger than the issue assumes. Both below.

   reset_file_info also has no Python counterpart to worry about

   did.document._reset_file_info clears series_info alongside file_info, same as MATLAB — so MATLAB's #945/#946 hazard is real in Python too, for any code that calls it. Python's updateFileInfoForLocalFiles doesn't, which is why the bridge decision_log says #945 has nothing to fix here. That stays true. It is not evidence that the Python shape is fine; see below.

   The guard: yes, DID-python carries it

   sqlitedb._reject_series_without_ingest_locations (src/did/implementations/sqlitedb.py:1522), called from do_add_doc for a document new to the database, before any write:

   for name in document_obj.series_names():
       _n, n_present = document_obj.series_count(name)
       if n_present > 0 and not document_obj.series_ingest_locations(name):
           raise ValueError(...)

   So the issue's suspicion is confirmed, and it matters exactly as much as it says: ingest_locations is stripped at store time by strip_series_ingest_locations, so the first Python document with a populated series is refused at add_docs outright, not merely mis-resolved. It is a ValueError, not a warning. Any download path that reconstructs ingest_locations is not an optimisation — it is what makes the add possible at all.

   The precondition: DID-python's series API is implemented but untested

   This is the one thing I would not build on without saying out loud. DID-python's suite passes (522 passed, 8 skipped, 1 xfailed), but run it under coverage and look at the series functions — statements covered / total:

   did.document
     add_file_series                          1/54     series_ingest_locations   1/10
     remove_file_series                       1/14     series_count               1/5
     series_member_of                         6/18     series_source_root         1/5
     is_file_series                            1/3     strip_series_ingest_locations 9/17
   did.implementations.sqlitedb
     _open_series_member                      1/53     _ingest_series_members    3/32
     _manifest_locations_for_handler          1/21     _reject_series_without_ingest_locations 2/5
   did.database
     series_has 1/10      series_members 1/16      _series_manifest_path 1/9
   did.file
     read_series_manifest 37/48   read_series_manifest_uid 21/30   write_series_manifest 37/41
   

   did.file's manifest codec is genuinely covered — by tests/symmetry/make_artifacts/file/test_series_manifest.py, which writes five manifests (dense, sparse, named, allEmpty, wideUid) and reads them back. Everything above the codec is at "the def line ran and nothing else did". 1/54 and 1/53 mean those bodies never execute; grep -rn "add_file_series\|series_count\|series_has" tests/ returns nothing.

   One sentence: no test in DID-python ever constructs a document with a populated series. The bytes are proven; every behaviour built on them is not — including the MembersNotLocatable guard that NDI's download path exists to satisfy.

   Filed one layer down rather than worked around here, per §4. A DID-python session is opening that issue and writing the tests; I will link it here when it lands.

   What NDI-python would need

   Four things, and one of them is not in the issue's list.

   A. The handler must accept context — this is the ndic:// problem in its Python form.

   src/ndi/database.py:_cloud_file_handler(dest_path, source_path) is what DID actually receives, and src/ndi/cloud/filehandler.py:download_file_from_cloud(dest_path, source_path, client=None) is what it calls. Both are two-argument.

   DID's _dispatch_custom_file_handler inspects the signature and calls with three arguments only when the handler declares three positional parameters. So NDI's handler is called with two — and for a series member, source_path is the manifest's location. Parsing the uid out of it fetches the manifest and stores it under the member's uid.

   The Python failure is not silent in the way the issue describes MATLAB's, but it is not better: DID-python's #191 guard compares the fetched size against the manifest it already holds and raises DID:SQLITEDB:FileSeries:HandlerReturnedManifest, so every member read fails with an error about the wrong thing. (And the guard is skipped when os.path.getsize(manifest_path) raises, in which case the manifest bytes are cached under the member's uid and every later read of that member returns the manifest, quietly and forever.)

   The fix is the same as MATLAB's: widen both functions to (dest_path, source_path, context) and take the uid from context["uid"] only when context["seriesName"] is non-empty. NDI-matlab's didsqlite.m is explicit about why "prefer context.uid whenever present" is the wrong rule — DID passes a uid for ordinary files too, and there it is the files-table row, not what the ndic:// reference names. DID-python passes {"documentId", "filename", "seriesName", "uid", "mode"}.

   Separately, download_file_from_cloud currently returns silently for a non-ndic:// location, and _cloud_file_handler swallows ImportError the same way. DID reads both as "the handler produced no file". That is the silent-miss shape, arriving by a different route than MATLAB's UnsupportedFileLocationType.

   B. Reconstruct ingest_locations on download. A counterpart to reconstructSeriesIngestLocations.m: for each series with n_present > 0 and no ingest_locations, find the manifest's uid from the file_info entry whose name is the series name, read the downloaded manifest with did.file.read_series_manifest, and build one entry per present slot — {index, uid, location: ndic://<dsid>/<uid>, location_type: "ndicloud", ingest: 0, delete_original: 0}. ingest: 0 matters: the point is to satisfy the guard, not to trigger 28,000 downloads at add time. A series whose manifest cannot be read is left alone so the guard fires — that is the right signal when the download was incomplete.

   C. Keep the manifest's ndic:// reference beside the local copy. ndi.document.add_file already detects ndic:// → ndicloud (src/ndi/document.py:257) and DID's _is_remote_location is a "://" test, so an ndicloud location is classified remote and _manifest_locations_for_handler offers it first. The primitive is in place; only manifests need the second location.

   D — the one not in the issue's list: there is no Python download path for this to hang on.

   updateFileInfoForLocalFiles has no callers. Not in src/, not in tests/ — grep -rn "updateFileInfoForLocalFiles" --include=*.py . finds only its own definition and docstring. And sync/operations.py:downloadNdiDocuments fetches document JSON and stops; it never downloads files and never rewrites file_info to local paths. MATLAB's does both, then threads cloudDatasetId into updateFileInfoForLocalFiles for exactly the reconstruction in B.

   So the SyncFiles=true half of the download path is missing, not just its series support. Step 3 has nowhere to attach until that exists. Worth confirming with a maintainer whether that is known and deliberate (deferred), or a gap of its own — it changes the size of this issue considerably.

   On the shape of updateFileInfoForLocalFiles

   The issue is right that a transliteration will not work, and the reason compounds. MATLAB takes and returns a did.document, calls reset_file_info(), and rebuilds through add_file — which is how it gets add_file's ndic:// detection and its per-location uid minting for free, and why carrying series_info across the reset was a fix worth making. Python takes a properties dict and edits locations[i]["location"] in place. From a bare dict it cannot call add_file, so C would mean hand-rolling a second location record, minting a uid by hand, and duplicating the scheme detection that ndi.document.add_file already does correctly.

   My suggestion: give the Python function a document-shaped signature — (doc, file_directory, cloud_dataset_id="") -> doc — rather than growing the dict-editing version. It is the same operation on the same object MATLAB uses, it lets B and C go through did/ndi.document instead of around them, and since the function has no callers today the change costs nothing. Keeping a thin dict-based wrapper for anything that later wants one is easy; the reverse is not.

   Proposed order of work

   0. DID-python tests for the series API (one layer down, already in motion). Not strictly blocking, but everything below rests on add_file_series, the guard, and _open_series_member, and none of the three has ever run in a test.
   1. Widen the handler to three arguments — _cloud_file_handler and download_file_from_cloud, uid from context["uid"] when context["seriesName"] is set. Self-contained, testable with a fake DID caller, and it is the piece the whole feature is pointless without. One PR.
   2. Decide D with a maintainer. If the Python download path is to grow file downloading + local rewrite, that is its own issue and its own PR, and it is the prerequisite for 3–4.
   3. Reshape updateFileInfoForLocalFiles to take and return a document, with cloud_dataset_id. One PR.
   4. reconstructSeriesIngestLocations + the manifest's second location (B and C). They are the same round trip and want reading together. One PR.
   5. extract_doc_files — the member walk from extract_docs_files.m, keeping each member's ORIGINAL uid, since the copied manifest names members by uid. Independent of the cloud path; can go any time after 0.

   The test that matters most, and the one the issue is really asking for: a downloaded series whose member cannot be resolved must fail a test, not a user. Concretely — build a document with a populated series, run it through strip/store/read-back, reconstruct, add, then open a member through a fake handler and assert the handler was asked for the member's uid and not the manifest's. That single test covers A, B and C at once and is the one I would write first.

   What I am not sure about

   • Whether D is deliberate. I can see the Python download path does not download files; I cannot tell from the code whether that is deferred-on-purpose or an oversight. It changes this issue's scope more than anything else here, so it wants a maintainer's answer before step 3.
   • Whether the API supports batching signed URLs. NDI-matlab's handler has moved past what this issue describes: it now keeps a per-document signed-URL cache, one API round trip per document instead of one per uid, "closing DID sessiontable: land at ~/.ndi/ (parity with MATLAB); fix stale preferences docstring #173's step 3 for the read path". For a 28,000-member series that is the difference between usable and not. I have not checked whether NDI-python's api/files.py can express the same batch call, and I did not look at the cloud API surface for it.
   • The exact contents of context are DID-python's today; I read them from sqlitedb._open_series_member. If DID-matlab#186's contract is still moving, NDI's handler should read defensively (context.get(...)) rather than assume the five keys.
   • _reject_series_without_ingest_locations runs only for a document new to the database (inside the else of the docs row lookup). I have not worked out whether a re-add of an existing doc_id can reach the member-ingest loop with an empty record and quietly do nothing.
   • I have not run any of this. There is no populated-series document anywhere in either Python repo to run it against — which is the same fact as the coverage table above.

   Coordination

   I have edited nothing. claude/burn-down-cloud-drift has not merged into main (main is at 767ab85), and every file steps 1–4 touch is in that branch's set, so there is nothing to write here yet regardless. tests/bridge_drift_allowlist.yaml untouched; the updateFileInfoForLocalFiles and extract_doc_files decision_log notes still say NOT PORTED, which is still true, and should be rewritten by whoever does steps 3–5 rather than now.

   ---

   Generated by Claude Code
2. stevevanhooser

   added a commit that references this issue on Sep 8, 2026

   Burn the +cloud cluster off the drift allowlist: 18 down to 1

   ...

   c8a7b35
3. claude commented on Sep 8, 2026

   

   claudebot

   on Sep 8, 2026 – with Claude

   More actions

   Done. All five PRs are merged; main is at 2b8cf12.

   What shipped

   PR
   #256	Give DID the context, so a series member is fetched by its own uid
   #257	Download every file a document records, not the one it usually does not have
   #258	Download the files before adding the documents, as MATLAB does
   #259	Rebuild ingest_locations, keep the manifest's cloud reference
   #260	Extract a file series' members too, so the copy can be stored somewhere else

   Against this issue's five scope items:

   1. Where the representation belongs — DID-python, and it was already there. f292b16 / DID-python#65 landed the day before this issue was written, which is presumably why the issue did not know. So NDI models no part of files.series_info: series_names, series_count, strip_series_ingest_locations and the manifest codec are all reached through did.document and did.file. What is NDI's is the ndic:// round trip, which DID deliberately knows nothing about ("DID composes no URL and learns no scheme").
   2. Read and write files.series_info through the cloud round trip — Make a downloaded file series readable: rebuild ingest_locations, keep the manifest cloud reference #259.
   3. Reconstruct ingest_locations on download, ndic:// from the dataset id — Make a downloaded file series readable: rebuild ingest_locations, keep the manifest cloud reference #259, reconstructSeriesIngestLocations.
   4. Keep the manifest's cloud reference beside its local copy — Make a downloaded file series readable: rebuild ingest_locations, keep the manifest cloud reference #259.
   5. The MembersNotLocatable guard — present in DID-python as sqlitedb._reject_series_without_ingest_locations, a hard ValueError in do_add_doc before any write. The suspicion in this issue was right: without Make a downloaded file series readable: rebuild ingest_locations, keep the manifest cloud reference #259 the first document with a populated series is refused outright, not mis-resolved.

   Three things this issue did not know about

   DID-python's series API was implemented but untested. Coverage on main at the time: add_file_series 1/54 statements, _open_series_member 1/53, the guard 2/5, series_has 1/10 — only did.file's manifest codec was genuinely covered. No test anywhere constructed a document with a populated series. Filed and worked one layer down rather than around: nine PRs merged in DID-python, four of tests and three of real defects the tests exposed — VH-Lab/DID-python#77 (ingest-path divergences), #78 (the #191 guard poisoning the file cache), #79 (missing file-declaration guards), plus #81 (slot bounds). Those functions are 49/54, 50/54, 5/5, 10/10 now.

   Python's failure mode differed from MATLAB's. _cloud_file_handler declared two positional parameters, and DID chooses between the two- and three-argument forms by counting them — silently. So NDI never saw the context, and a member fetch would have downloaded the manifest under the member's uid. DID's #191 guard catches that when it can compare sizes and cannot when the manifest's size is unreadable, in which case every later read of that member returns the manifest for good.

   Two prerequisites that were not series bugs at all. downloadFilesForDocument read a single top-level document["file_uid"], which ordinary documents do not have — so an ordinary document downloaded nothing (#257). And downloadDataset added documents before downloading files, into target/.ndi/files, which is DID's own FileDir — bypassing ingestion entirely, which is why it looked fine and why updateFileInfoForLocalFiles had never had a caller (#258). Neither is visible from the series side, and neither could be worked around from it: the guard fires at add_docs, so reconstruction needs the manifest on disk before the add.

   Where I changed my mind

   I said here and on #258 that updateFileInfoForLocalFiles should be reshaped to take a document, as MATLAB's does. It was not, and I think that was the wrong plan: the whole download pipeline mutates properties dicts in place — updateFileInfoForRemoteFiles does too — and ndi_document(props) deep-copies, so a document-shaped rewrite would have silently stopped mutating the objects the pipeline holds. It takes cloud_dataset_id as a third argument instead, matching MATLAB's arity. The cost is that the manifest's second location record is built directly rather than through ndi.document.add_file; a test pins the two shapes together.

   One deliberate deviation from MATLAB, in #258: every location gets its ndic:// reference before the local rewrite overwrites what actually arrived, so a file that failed to download keeps a reference that still resolves on demand rather than being dropped from the document. A short download degrades to a slower read instead of a lost file.

   Bookkeeping

   Both decision_log notes that said NOT PORTED — updateFileInfoForLocalFiles and extract_doc_files — were rewritten in the PRs that made them untrue. tests/bridge_drift_allowlist.yaml was never touched: extract_doc_files had already been delisted by an earlier burn-down and its hash was already current, so no matlab_last_sync_hash moved in any of the five.

   One behaviour change worth remembering: #260 moved extract_doc_files' output to MATLAB's layout (target_path/<uid>, flat, no per-document document.json). Nothing in this repo called the old one, but a downstream script could have.

   What is not covered

   • Batch signed URLs. NDI-matlab's handler keeps a per-document signed-URL cache — one API round trip per document instead of one per uid, closing DID#173's step 3 for the read path. For a 28,000-member series that is the difference between usable and not. Python has none; the API surface for it is recorded as porting_deferred (batchSignedUrlLookup, and the signed-URL-set family under issue Bridge completeness red again: 6 signed-URL-set cloud APIs have no entry #206). This is the obvious next piece and probably wants its own issue.
   • No test here has run against a real cloud dataset. Every test uses a fake handler or a stubbed transfer. The chain is proven end to end through a real database — a member of a downloaded series is opened, and an extract is stored in a second session — but the cloud side of it is simulated.

   Closing; the five listed items are all merged.

   ---

   Generated by Claude Code
4. claude

   closed this as completedon Sep 8, 2026