Skip to content

Retire the manifest cloud-reference workaround (#300) - #301

Closed
stevevanhooser wants to merge 2 commits into
mainfrom
claude/retire-manifest-workaround
Closed

stevevanhooser wants to merge 2 commits into
mainfrom
claude/retire-manifest-workaround

Conversation

@stevevanhooser

Copy link
Copy Markdown
Contributor

Closes #300. Companion to VH-Lab/NDI-matlab#986; ports the sister retirement PR VH-Lab/NDI-matlab#989.

What changes

ndi.cloud.filehandler.updateFileInfoForLocalFiles no longer appends a second ndicloud location alongside the local file one on a series manifest. That workaround gave DID's custom_file_handler a non-file source path for member fetches, because download_file_from_cloud returned without producing anything on a plain local path -- so every member of a downloaded series read as absent, silently (VH-Lab/NDI-matlab#966).

VH-Lab/DID-matlab#191 taught the read path to reach a local-file manifest directly, and VH-Lab/DID-matlab#201 closed the last case (cloud-only manifest fetched through the same handler). Both landed; both were ported to VH-Lab/DID-python#88 (merged). That makes the second location dead weight -- DID never consults it anymore.

Code

  • src/ndi/cloud/filehandler.py: drop the manifest second-location block, remove the now-unused helpers _ndic_location_record and _declared_series_names, simplify the write-back (no more grown-list case), and update the docstring. cloud_dataset_id stays in the signature because reconstructSeriesIngestLocations still needs it (Cloud-downloaded documents refused by DID's MembersNotLocatable guard VH-Lab/NDI-matlab#958).
  • src/ndi/cloud/orchestration.py: comment update at the call site.
  • src/ndi/cloud/ndi_matlab_python_bridge.yaml and src/ndi/cloud/sync/ndi_matlab_python_bridge.yaml: bump updateFileInfoForLocalFiles's matlab_last_sync_hash to 2ad46c0f (the NDI-matlab#989 head) and drop "second location beside the local copy" language from the decision_log.

Tests

  • Remove TestTheManifestKeepsItsCloudReference from tests/test_cloud_series_reconstruction.py -- analog of TestDownloadedSeriesManifestKeepsItsCloudLocation.m, its whole subject was pinning the second location.
  • Remove test_a_member_of_a_downloaded_series_can_be_read: it asserted the handler receives an ndic:// URI built from the manifest's second location, which no longer exists. End-to-end coverage of a member of a downloaded series lives on the cloud side, mirroring NDI-matlab's testMembersSurviveADownloadFromTheCloud.
  • Add TestSeriesManifestHasExactlyOneLocation as the regression guard: the downloaded series manifest's file_info must have exactly one location, and its location_type must be file. Mirrors testSeriesManifestHasExactlyOneLocation added in Retire the manifest cloud-reference workaround (#986) VH-Lab/NDI-matlab#989. Reintroducing a second ndic:// location would fail that test immediately.
  • Update TestTheCloudDatasetIdReachesTheRewrite's docstring in tests/test_cloud_download_ordering.py: the id is still threaded, but now for the ingest_locations rebuild alone.

Not in scope

VH-Lab/NDI-matlab#988 also taught the SyncFiles=false path (updateFileInfoForRemoteFiles) to rebuild series ingest_locations by fetching the manifest bytes through the DID customFileHandler contract. NDI-python's updateFileInfoForRemoteFiles does not yet do this. That is a separate gap and belongs in its own change -- filed as a note in #300.

Verification

pytest tests/test_cloud_series_reconstruction.py tests/test_cloud_download_ordering.py tests/test_cloud_filehandler.py tests/test_cloud_file_uid_enumeration.py -- 63 passed, 2 skipped, 0 failed. The 7 failures observed on the broader -k "cloud or filehandler or series" sweep are all in tests/test_cloud_profile_secret_field_names.py and stem from an unrelated environment issue (_cffi_backend missing).

References

🤖 Generated with Claude Code

https://claude.ai/code/session_01AdGZ8fRPeY9JSpUHgXm9ao


Generated by Claude Code

updateFileInfoForLocalFiles no longer appends a second 'ndicloud'
location alongside the local 'file' one on a series manifest. That
workaround gave DID's custom_file_handler a non-'file' source path for
member fetches, because download_file_from_cloud returned without
producing anything on a plain local path -- so every member of a
downloaded series read as absent, silently. See NDI-matlab#966.

VH-Lab/DID-matlab#191 taught the read path to reach a local-file
manifest directly, and VH-Lab/DID-matlab#201 closed the last case
(cloud-only manifest fetched through the same handler). Both landed;
both were ported to VH-Lab/DID-python#88 (merged). That makes the
second location dead weight -- it is never consulted anymore.

Ports VH-Lab/NDI-matlab#989 (the NDI-matlab retirement) commit-for-
commit: the workaround block is dropped, its supporting helpers
(_ndic_location_record, _declared_series_names) are removed, and the
docstring stops describing the second location. cloudDatasetId stays
in the signature because reconstructSeriesIngestLocations still needs
it (NDI-matlab#958).

TestTheManifestKeepsItsCloudReference in
tests/test_cloud_series_reconstruction.py was the analog of
TestDownloadedSeriesManifestKeepsItsCloudLocation.m -- its whole
subject was pinning the second location -- so it is removed. In its
place, TestSeriesManifestHasExactlyOneLocation guards the new
invariant: the downloaded series manifest has exactly one location,
of type 'file'. Reintroducing a second ndic:// location would fail
that test immediately.

test_a_member_of_a_downloaded_series_can_be_read is removed with it:
it asserted the handler receives an ndic:// URI built from the
manifest's second location, which no longer exists. End-to-end
coverage of a member of a downloaded series lives on the cloud side,
mirroring NDI-matlab's testMembersSurviveADownloadFromTheCloud.

Bridge yaml hash for updateFileInfoForLocalFiles bumped to 2ad46c0f
(the NDI-matlab#989 head), and the decision_log stops advertising the
second location as a feature. Companion changes to the sync yaml's
downloadNdiDocuments entry.

The related NDI-matlab#988 fix (rebuilding series ingest_locations in
the SyncFiles=false path so DID#185's guard is satisfied on a cloud
round trip) is NOT yet in NDI-python's updateFileInfoForRemoteFiles.
That is a separate gap and belongs in its own change.

Refs: VH-Lab/NDI-matlab#986, VH-Lab/NDI-matlab#989,
      VH-Lab/DID-matlab#191, VH-Lab/DID-matlab#201,
      VH-Lab/DID-python#88.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AdGZ8fRPeY9JSpUHgXm9ao
The bridge drift check reads NDI-matlab HEAD, and two entries drifted
between this branch's base commit and CI time:

updateFileInfoForRemoteFiles (fe64a9f5 → aea0c72c): three commits from
NDI-matlab#988 landed on it (1a6ec2fc, 414e8572, a4dfb076) -- rebuilds
series ingest_locations on a SyncFiles=false download so DID's #185
guard is satisfied on add_docs. That is the follow-up already flagged
out of scope in NDI-python#300; the port belongs in its own change,
not in this retirement. Hash bumped with a decision_log note naming
the deferral.

GEFManager (06f31a4e5 → 59c96e57): NDI-matlab#984 makes the App
Designer window unique per session -- MATLAB uitools only, nothing
to port on this side, which draws no such window. GEFManager itself
stays porting_deferred. Hash bumped with a decision_log note.

Neither change is of this PR; both are pre-existing drifts that turn
CI red on any branch opened after the NDI-matlab head moved. Acknowledged
here rather than left blocking.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AdGZ8fRPeY9JSpUHgXm9ao

Copy link
Copy Markdown
Contributor Author

Retracting this PR. The sister retirement on the MATLAB side, VH-Lab/NDI-matlab#989, was closed unmerged after its Cloud CI failed; both PRs shared the same bad premise about DID's post-#191/#201 behavior. Closing this without merging.

Why the premise was wrong

DID-matlab#191 taught fetchSeriesMemberBytes to offer a local-file manifest sourcePath to the customFileHandler on member fetches (it iterates every manifest location, remote-first then local). It did not teach the handler to service one. NDI-matlab's download_file_from_cloud refuses any sourcePath that doesn't start with ndic://, so once the workaround's second ndicloud location was removed on SyncFiles=true, the handler had nothing servable and every member of a downloaded series read as absent -- silently, because DID reads a handler that produces nothing as a plain miss.

Cloud CI 1 caught it on two tests:

  • FileSeriesRoundTripTest.testMembersSurviveADownloadFromTheCloud (SyncFiles=true) -- DID:SQLITEDB:open, chunkdata.bin_1 ... cannot be accessed ... bytes are not on this machine and could not be retrieved.
  • FileSeriesRoundTripTest.testTheBatchScopeIsTheSeriesAndNotTheWholeDocument -- same error, same site.

SyncFiles=false was unaffected because the manifest there starts as ndic://.

Retraction analysis on the NDI-matlab side:

NDI-python has the same reject-on-non-ndic:// behavior

Confirmed in code. ndi.cloud.filehandler.download_file_from_cloud (src/ndi/cloud/filehandler.py:677-678) returns silently when source_path does not start with NDIC_SCHEME:

if not str(source_path).startswith(NDIC_SCHEME):
    return

So the same regression would occur here: with the workaround gone, DID-python offers the local-file manifest location to the handler on a member fetch (post DID-python#88), the handler returns without producing bytes, and every member of a downloaded series reads as absent -- silently, exactly as on the MATLAB side.

Would NDI-python's own tests have caught it?

Partly, but not through the analog we'd expect.

tests/test_cloud_series_reconstruction.py::TestThroughARealDatabase::test_a_member_of_a_downloaded_series_can_be_read is a real integration test through DID -- session.database_openbinarydoc exercises the actual customFileHandler dispatch, with only fetch_cloud_file mocked. It would fail post-retirement, because the handler would return without setting asked["uri"]. So on the current NDI-python HEAD (workaround intact), the test guards this exact path.

But this PR removed that test at the same time as the workaround, on the reasoning that "end-to-end coverage of a member of a downloaded series lives on the cloud side." That live-cloud coverage does not exist in NDI-python: tests/test_cloud_live.py and tests/test_cloud_hello_matlab.py have no round-trip / SyncFiles=true / member-fetch test analogous to FileSeriesRoundTripTest.testMembersSurviveADownloadFromTheCloud. So had this PR merged as written, the regression would have slipped through NDI-python's tests too.

No bridge YAML bump lands

Verified: origin/main still has matlab_last_sync_hash: "504c5bcf4" for updateFileInfoForLocalFiles in both bridge YAMLs. Since this PR is closing unmerged, the bump toward 2ad46c0f on the branch never reaches main. No follow-up revert needed.

What next

The paths forward for the underlying retirement item are documented in the NDI-matlab#986 wrap-up. Not filing anything from here per the retraction scope.


Generated by Claude Code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Retire the manifest cloud-reference workaround (companion to VH-Lab/NDI-matlab#986)

2 participants