Skip to content

Route links through the backend reader and writer interfaces - #866

Merged
ehennestad merged 7 commits into
mainfrom
backend-agnostic-links
Aug 25, 2026
Merged

Route links through the backend reader and writer interfaces#866
ehennestad merged 7 commits into
mainfrom
backend-agnostic-links

Conversation

@ehennestad

@ehennestad ehennestad commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Motivation

Background — MatNWB's storage backends are described by io.backend.base.Reader and io.backend.base.Writer, with HDF5 as the only implementation today, but a Zarr reader in development. This PR closes a hole where the abstraction did not fully cover link types (SoftLink, ExternalLink).

Problemtypes.untyped.SoftLink and types.untyped.ExternalLink reached past the abstraction and called HDF5 directly: both export methods took the writer's raw file id and called H5L.create_soft / H5L.create_external, and ExternalLink.deref opened the target file itself with H5F.open, h5info and H5L.get_val. The Writer interface had no link API at all, so there was nothing for a new backend to implement — supporting links would have meant special-casing these two classes.

Solution — Links are now expressed as capabilities of the backend. Writing a link asks the writer; reading one asks the reader. A new backend supports links by implementing interface methods, and neither link class contains a storage-specific call any more.

What changed

  • Writer gains writeSoftLink and writeExternalLink; Reader gains readLinkInfo and isReferenceDataset. All four are implemented for HDF5.
  • SoftLink and ExternalLink no longer contain any H5F., H5L., h5info or H5T_REFERENCE reference. A backend that has not implemented link support now fails naming the method it is missing, rather than failing inside HDF5.
  • No behaviour change for HDF5 files: links are written, read and dereferenced exactly as before.
  • Two smaller fixes came along with the rewrite:
    • SoftLink.exportScalar contained an unreachable second if isempty(obj.path) branch, dead after the early return above it.
    • ExternalLink.export declared a refs output that it never assigned, so requesting that output errored. It now returns {}, matching SoftLink.export.
Implementation notes

ExternalLink.deref previously opened the target file with H5F.open and h5info. It now resolves a reader through io.backend.BackendFactory and calls readNodeInfo, which returns the same h5info-style struct for any backend, so the group/dataset/link classification is unchanged.

The retry that SoftLink performed by catching an "name already exists" message is now an explicit check in the HDF5 writer: an identical link is left alone, a differing one is replaced. Rewriting a link is routine rather than exceptional, since an object whose link target is not resolvable on the first pass is exported again by NwbFile.resolveReferences.

isReferenceDataset was added because deref decided how to handle a linked dataset by testing LinkedInfo.Datatype.Class against 'H5T_REFERENCE'. HDF5Reader already made the same test inside readDatasetValue, so this was reaching around an existing abstraction rather than filling a gap in one; readDatasetValue now calls the new method too, so a single place decides. How a dataset is marked as holding references is backend specific — a reference datatype class in HDF5, a zarr_dtype attribute of "object" in hdmf-zarr.

deref checks for its target with isfile or isfolder, since a store is a single file for some backends and a directory for others.

How to test

Soft and external links round-trip unchanged:

nwb = NwbFile('identifier', 'LINKS', 'session_description', 'link round trip', ...
    'session_start_time', datetime(2026, 1, 1, 'TimeZone', 'local'));
device = types.core.Device('description', 'a device');
nwb.general_devices.set('dev', device);
nwb.general_extracellular_ephys.set('grp', types.core.ElectrodeGroup( ...
    'description', 'a group', 'location', 'somewhere', ...
    'device', types.untyped.SoftLink(device)));
nwb.acquisition.set('ts', types.core.TimeSeries( ...
    'data', 1:5, 'data_unit', 'n/a', 'timestamps', 1:5));
nwbExport(nwb, 'links.nwb');

other = NwbFile('identifier', 'OTHER', 'session_description', 'external link', ...
    'session_start_time', datetime(2026, 1, 1, 'TimeZone', 'local'));
other.acquisition.set('linked_ts', types.untyped.ExternalLink( ...
    fullfile(pwd, 'links.nwb'), '/acquisition/ts'));
nwbExport(other, 'other.nwb');

back = nwbRead('links.nwb');
disp(class(back.general_extracellular_ephys.get('grp').device))
disp(class(back.general_extracellular_ephys.get('grp').device.deref(back)))

linked = nwbRead('other.nwb').acquisition.get('linked_ts');
disp(class(linked.deref()))
disp(linked.deref().data.load()')
types.untyped.SoftLink
types.core.Device
types.core.TimeSeries
     1     2     3     4     5

Not included

ExternalLink resolves a relative target filename against the current working directory rather than against the file containing the link, which can silently dereference the wrong file. That is reported separately in #865 and deliberately left alone here, since fixing it changes what deref returns rather than where the call is routed.

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

🤖 Generated with Claude Code

ehennestad and others added 4 commits August 24, 2026 16:54
types.untyped.SoftLink and types.untyped.ExternalLink reached past the
backend abstraction and called HDF5 directly: both export methods took
writer.FileId and called H5L.create_soft/H5L.create_external, and
ExternalLink.deref opened the target itself with H5F.open, h5info and
H5L.get_val. The Writer interface had no link API at all, which is why
they had to.

Add writeSoftLink and writeExternalLink to the writer interface and
readLinkInfo to the reader interface, implement them for HDF5, and
route the two link classes through them. Neither class makes an H5
call any more, so a second backend can support links by implementing
three methods rather than by special-casing these classes.

Behaviour is unchanged for HDF5. Two details are worth noting:

- The retry that SoftLink performed by catching "name already exists"
  is now an explicit check in the HDF5 writer, which leaves an
  identical link alone and replaces a differing one. Rewriting a link
  is routine rather than exceptional: an object whose target is not
  resolvable on the first pass is exported again by
  NwbFile.resolveReferences.
- ExternalLink.export never assigned its refs output, so requesting it
  errored. It now returns {} like SoftLink.export does.

deref checks for its target with isfile or isfolder, since a store is
a single file for some backends and a directory for others. The node
classification still reads the h5info-style struct that readNodeInfo
returns by contract for every backend, and one HDF5 term remains in
it -- the H5T_REFERENCE dataset check -- which the backend adding
support for it should generalise.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ExternalLink.deref decided how to handle a linked dataset by testing
LinkedInfo.Datatype.Class against 'H5T_REFERENCE'. That was the last
piece of HDF5 vocabulary in backend-neutral code: every other mention
of the constant lives in the HDF5 backend or the h5 internals.

The knowledge was already behind the interface -- HDF5Reader made the
same test inside readDatasetValue -- so deref was reaching around an
abstraction rather than filling a gap in one. Promote it to a reader
method, isReferenceDataset, and have readDatasetValue use it too so
there is a single place that decides.

How a dataset is marked as holding references is backend specific: a
reference datatype class in HDF5, a "zarr_dtype" attribute of "object"
in hdmf-zarr. Callers need the answer, not the encoding.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Apply MATLAB naming conventions to the code this branch introduces
rather than following the surrounding file.

- The struct returned by readLinkInfo uses lowerCamelCase fields (type,
  targetPath, targetFilename). The h5info-derived structs keep their
  PascalCase fields, since those names are the contract that
  readNodeInfo mirrors for every backend.
- ExternalLink.deref uses lowerCamelCase locals and nested functions
  (linkedInfo, isTyped, isDataset, scalarDeref, derefLink) in place of
  the mixed PascalCase and snake_case it had.
- The abbreviated `plist` is spelled propertyListId.

No behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ehennestad
ehennestad marked this pull request as ready for review August 24, 2026 18:52
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.04762% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.27%. Comparing base (382d71c) to head (51c9ff7).

Files with missing lines Patch % Lines
+types/+untyped/ExternalLink.m 97.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #866      +/-   ##
==========================================
+ Coverage   95.21%   95.27%   +0.05%     
==========================================
  Files         231      232       +1     
  Lines        8252     8292      +40     
==========================================
+ Hits         7857     7900      +43     
+ Misses        395      392       -3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Writing a link over a path already held by a group or dataset silently
replaced the node. That was a side effect of the link-type check added
when the retry logic moved into the HDF5 writer: a hard link matched
neither the soft nor the external case, so it fell through to the
delete-and-recreate branch.

Before this branch, SoftLink reached H5L.get_val on such a node and
failed with the HDF5 library's own error, which named neither the path
nor the conflict. Report it directly instead, naming both. This is
stricter than ExternalLink's previous behaviour, which deleted the
node unconditionally.

Replacing a link with a link is unchanged, including replacing one
kind with the other, since both are links rather than nodes standing
in the way.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ehennestad
ehennestad force-pushed the backend-agnostic-links branch from 7e62552 to c359d06 Compare August 25, 2026 08:03
ExternalLink.deref told a group, a dataset and a link apart by which
fields the node info carried, and its group test required 'Datatypes'.
That field holds HDF5 named datatypes, a concept no other backend has,
so a group read through another backend matched none of the three and
deref raised NWB:ExternalLink:UnknownNodeType.

Nothing anywhere reads 'Datatypes' -- this test was its only mention in
the codebase -- and Groups, Datasets and Links already distinguish a
group from a dataset or a link. Drop it from the test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ehennestad
ehennestad requested a review from bendichter August 25, 2026 08:54
Two paths added by this branch had no test.

The existing-link comparison was only exercised for soft links. Add the
external cases: re-writing an identical link, and replacing one that
differs by file or by path. The replacement test is the one that
discriminates -- a comparison that wrongly reports a match leaves the
old link in place and fails it on both halves. Re-writing an identical
link cannot be told apart from deleting and recreating it by looking at
the result, so that test only shows the path is taken without error;
the same is true of the soft-link case beside it.

ExternalLink.deref routes a dataset either through io.parseDataset or
into a bare stub, and only the stub branch was covered:
testExternalResolution links to a plain dataset. Add a link to a typed
dataset, which comes back as its neurodata type, and one to a dataset
of object references, whose references are resolved rather than handed
back raw. Bypassing io.parseDataset returns a DataStub and fails both.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ehennestad
ehennestad added this pull request to the merge queue Aug 25, 2026
Merged via the queue into main with commit 2cd6964 Aug 25, 2026
20 checks passed
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.

2 participants