Error on NWB files with external links #843

jwodder · 2021-11-24T14:50:01Z

Closes #840.

codecov · 2021-11-24T14:55:06Z

Codecov Report

Merging #843 (247a7cd) into master (dfb9770) will increase coverage by 0.07%.
The diff coverage is 95.91%.

@@            Coverage Diff             @@
##           master     #843      +/-   ##
==========================================
+ Coverage   85.28%   85.35%   +0.07%     
==========================================
  Files          61       61              
  Lines        6322     6366      +44     
==========================================
+ Hits         5392     5434      +42     
- Misses        930      932       +2

Flag	Coverage Δ
unittests	`85.35% <95.91%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dandi/metadata.py	`81.99% <50.00%> (-0.18%)`	⬇️
dandi/pynwb_utils.py	`83.15% <95.00%> (+1.18%)`	⬆️
dandi/tests/test_pynwb_utils.py	`97.72% <100.00%> (+2.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfb9770...247a7cd. Read the comment docs.

jwodder · 2021-11-24T15:22:45Z

@yarikoptic The tests appear to be failing because this call to copy_nwb_file() produces a file with an external link.

@bendichter What's the best way to copy an NWB file while giving it a new object_id without creating an external link?

bendichter · 2021-11-24T18:47:21Z

dandi/pynwb_utils.py

+@metadata_cache.memoize_path
+def nwb_has_external_links(filepath):
+    with h5py.File(filepath, "r") as fp:
+        visited = set()
+
+        # cannot use `file.visititems` because it skips external links
+        # (https://github.com/h5py/h5py/issues/671)
+        def visit(path="/"):
+            if isinstance(fp[path], h5py.Group):
+                for key in fp[path].keys():
+                    key_path = path + "/" + key
+                    if key_path not in visited:
+                        visited.add(key_path)
+                        if isinstance(
+                            fp.get(key_path, getlink=True), h5py.ExternalLink
+                        ) or visit(key_path):
+                            return True
+            elif isinstance(fp.get(path, getlink=True), h5py.ExternalLink):
+                return True
+            return False
+
+        return visit()


Ah yes, thank you for tidying up this function.

bendichter · 2021-11-24T18:50:08Z

@jwodder

What's the best way to copy an NWB file while giving it a new object_id without creating an external link?

I'm surprised you are running into this issue, especially since link_data is False. @rly would be the person to ask about copying NWB files using PyNWB.

yarikoptic · 2021-11-29T20:01:02Z

pinged @rly on slack

oruebel · 2021-11-29T20:40:38Z

What's the best way to copy an NWB file while giving it a new object_id without creating an external link?

I think a combination of the generate_new_id and NWBHDFIO.export functions should work for this.

If you just want to resolve external links, the NWBHDF5IO.copy_file may also do the trick. This function works lower level by using the copy method from h5pyand has been deprecated in favor of ``export`, but I figured I'd mention is just in case the source code of that function are useful for you.

jwodder · 2021-11-29T21:30:44Z

@oruebel

I think a combination of the generate_new_id and NWBHDFIO.export functions should work for this.

How exactly do I combine those two functions? If I try doing this:

with pynwb.NWBHDF5IO(src, "r") as ior, pynwb.NWBHDF5IO(dest, "w") as iow:
    data = ior.read()
    data.generate_new_id()
    iow.export(container=data)

I get TypeError: NWBHDF5IO.export: missing argument 'src_io', unrecognized argument: 'container'.

If I instead do this:

with pynwb.NWBHDF5IO(src, "r") as ior, pynwb.NWBHDF5IO(dest, "w") as iow:
    data = ior.read()
    data.generate_new_id()
    iow.export(src_io=ior)

then the copy will still have the same object ID as the original.

If I do this:

with pynwb.NWBHDF5IO(src, "r") as ior, pynwb.NWBHDF5IO(dest, "w") as iow:
    ior.generate_new_id()
    iow.export(src_io=ior)

then I get AttributeError: 'NWBHDF5IO' object has no attribute 'generate_new_id'.

rly · 2021-11-29T21:37:49Z

Please try:

with pynwb.NWBHDF5IO(src, "r") as ior, pynwb.NWBHDF5IO(dest, "w") as iow:
    data = ior.read()
    data.generate_new_id()
    iow.export(ior, container=data)

jwodder · 2021-11-29T21:40:49Z

@rly That fails with "TypeError: NWBHDF5IO.export: unrecognized argument: 'container'" (I'm using hdmf 3.1.1.)

rly · 2021-11-29T21:48:42Z

My bad. 'container' is the argument name for HDF5IO. Please try:

with pynwb.NWBHDF5IO(src, "r") as ior, pynwb.NWBHDF5IO(dest, "w") as iow:
    data = ior.read()
    data.generate_new_id()
    iow.export(ior, nwbfile=data)

jwodder · 2021-11-29T21:59:34Z

@rly That works; thanks.

yarikoptic · 2021-11-30T00:37:39Z

tox.ini is very popular today - keeps conflicting

yarikoptic · 2021-11-30T00:39:03Z

dandi/pynwb_utils.py

+                        ) or visit(key_path):
+                            return True
+            elif isinstance(fp.get(path, getlink=True), h5py.ExternalLink):
+                return True


@bendichter what use case could trigger this codepath? would be nice to add it to the test

jwodder · 2021-11-30T00:41:17Z

@yarikoptic Conflict resolved.

yarikoptic · 2021-12-13T18:48:28Z

ok, let's proceed with it as is. Thank you!

jwodder added the minor Increment the minor version when merged label Nov 24, 2021

jwodder marked this pull request as draft November 24, 2021 15:29

bendichter reviewed Nov 24, 2021

View reviewed changes

jwodder force-pushed the gh-840 branch from 0ba452f to d625c62 Compare November 29, 2021 21:31

jwodder marked this pull request as ready for review November 29, 2021 21:59

yarikoptic reviewed Nov 30, 2021

View reviewed changes

jwodder added 3 commits November 29, 2021 19:40

Add function for detecting external links in NWB files

d14f1e5

Error on NWB files with external links

fc9e17d

Make copy_nwb_file() not create external links

247a7cd

jwodder force-pushed the gh-840 branch from 423184e to 247a7cd Compare November 30, 2021 00:40

yarikoptic merged commit 7ee9da3 into master Dec 13, 2021

yarikoptic deleted the gh-840 branch December 13, 2021 18:48

yarikoptic mentioned this pull request Apr 6, 2023

Metadata / validation not caught before attempting to upload #1270

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error on NWB files with external links #843

Error on NWB files with external links #843

jwodder commented Nov 24, 2021

codecov bot commented Nov 24, 2021 •

edited

Loading

jwodder commented Nov 24, 2021

bendichter Nov 24, 2021

bendichter commented Nov 24, 2021

yarikoptic commented Nov 29, 2021

oruebel commented Nov 29, 2021 •

edited

Loading

jwodder commented Nov 29, 2021

rly commented Nov 29, 2021

jwodder commented Nov 29, 2021

rly commented Nov 29, 2021

jwodder commented Nov 29, 2021

yarikoptic commented Nov 30, 2021

yarikoptic Nov 30, 2021

jwodder commented Nov 30, 2021

yarikoptic commented Dec 13, 2021

Error on NWB files with external links #843

Error on NWB files with external links #843

Conversation

jwodder commented Nov 24, 2021

codecov bot commented Nov 24, 2021 • edited Loading

Codecov Report

jwodder commented Nov 24, 2021

bendichter Nov 24, 2021

Choose a reason for hiding this comment

bendichter commented Nov 24, 2021

yarikoptic commented Nov 29, 2021

oruebel commented Nov 29, 2021 • edited Loading

jwodder commented Nov 29, 2021

rly commented Nov 29, 2021

jwodder commented Nov 29, 2021

rly commented Nov 29, 2021

jwodder commented Nov 29, 2021

yarikoptic commented Nov 30, 2021

yarikoptic Nov 30, 2021

Choose a reason for hiding this comment

jwodder commented Nov 30, 2021

yarikoptic commented Dec 13, 2021

codecov bot commented Nov 24, 2021 •

edited

Loading

oruebel commented Nov 29, 2021 •

edited

Loading