default load_namespaces=True #1748

bendichter · 2023-07-28T21:27:31Z

test_outer_import_structure tests that the imports available from the pynwb module are unchanged. This PR removes the warn library, so it has also been removed form this test. This will technically break backwards compatibility if a user is running from pynwb import warn, but that would be highly unusual and unlikely.

codecov · 2023-07-28T21:30:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2aceed0) 91.75% compared to head (ad0e08f) 91.96%.
Report is 1 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1748      +/-   ##
==========================================
+ Coverage   91.75%   91.96%   +0.21%     
==========================================
  Files          27       27              
  Lines        2619     2615       -4     
  Branches      684      682       -2     
==========================================
+ Hits         2403     2405       +2     
+ Misses        141      138       -3     
+ Partials       75       72       -3

Flag	Coverage Δ
integration	`71.28% <100.00%> (+0.18%)`	⬆️
unit	`83.63% <75.00%> (+0.16%)`	⬆️

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

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

…namespaces_true

bendichter · 2023-07-31T17:55:32Z

I don't understand why I am failing flake8. @rly , can you help?

oruebel · 2023-07-31T20:00:45Z

I don't understand why I am failing flake8.

The GitHub Action indicated the following flake8 issues:

./tests/integration/hdf5/test_ecephys.py:3:109: E231 missing whitespace after ','
./tests/unit/test_ecephys.py:5:104: E231 missing whitespace after ','

bendichter · 2023-07-31T20:07:53Z

oh, I see. It's the imports

rly · 2023-07-31T20:43:59Z

Currently, calling load_namespaces=True is resulting in lots of this warning:
UserWarning: No cached namespaces found in [...].nwb

This comes from hdmf. I think we can remove that warning. @bendichter could you make a PR for this?

oruebel · 2023-09-06T14:23:51Z

Fix #1143

tests/unit/test_file.py

oruebel · 2023-09-06T14:36:27Z

@bendichter just FYI, there were a few conflicts in particular in the streaming tutorial, when I updated the PR with the dev branch. Here the changes I made during the merge:

e7023c2

oruebel · 2023-09-06T14:39:46Z

The "coverage / ubuntu-latest" action seems to show a large number of warnings and one test failure that appear to be related to the changes in this PR (see https://github.com/NeurodataWithoutBorders/pynwb/actions/runs/6098358982/job/16548020150?pr=1748)

In particular

======================================================================
FAIL: test_validate_file_cached_ignore (validation.test_validate.TestValidateCLI.test_validate_file_cached_ignore)
Test that validating a file with cached spec against the core namespace succeeds.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/pynwb/pynwb/tests/validation/test_validate.py", line 139, in test_validate_file_cached_ignore
    self.assertEqual(result.stderr.decode('utf-8'), '')
AssertionError: '/opt/hostedtoolcache/Python/3.11.4/x64/li[552 chars]."\n' != ''
- /opt/hostedtoolcache/Python/3.11.4/x64/lib/python3.11/site-packages/hdmf/spec/namespace.py:531: UserWarning: Ignoring cached namespace 'hdmf-common' version 1.0.0 because version 1.8.0 is already loaded.
-   warn("Ignoring cached namespace '%s' version %s because version %s is already loaded."
- /opt/hostedtoolcache/Python/3.11.4/x64/lib/python3.11/site-packages/hdmf/spec/namespace.py:531: UserWarning: Ignoring cached namespace 'core' version 2.1.0 because version 2.6.0-alpha is already loaded.
-   warn("Ignoring cached namespace '%s' version %s because version %s is already loaded."
-

Additionally, there are a lot of warnings where either no cached namespaces are found or another version of the same namespace has already been loaded:

/opt/hostedtoolcache/Python/3.11.4/x64/lib/python3.11/site-packages/hdmf/backends/hdf5/h5tools.py:213: UserWarning: No cached namespaces found in test_RoiResponseSeries.nwb

/opt/hostedtoolcache/Python/3.11.4/x64/lib/python3.11/site-packages/hdmf/spec/namespace.py:531: UserWarning: Ignoring cached namespace 'core' version 2.1.0 because version 2.6.0-alpha is already loaded.

…namespaces_true

bendichter · 2023-10-04T09:33:39Z

@rly it looks like you are altering the NWBHDF5IO call to ignore the "no namespace found error". I guess this wouldn't really come up in practice since the NWB files are generally stored with cached schema. Is that right?

tests/unit/test_file.py

docs/gallery/advanced_io/linking_data.py

Co-authored-by: Ben Dichter <[email protected]>

rly · 2023-10-04T19:19:45Z

@rly it looks like you are altering the NWBHDF5IO call to ignore the "no namespace found error". I guess this wouldn't really come up in practice since the NWB files are generally stored with cached schema. Is that right?

That was done in hdmf-dev/hdmf#926 . But yes, that warning wouldn't really come up in practice except when reading old files where the schema was not cached by default. For those old files without the core/hdmf-common schema cached, even if the schema were cached, they would be ignored because the schema loaded by pynwb would take precedence. So it's not a useful warning.

rly · 2023-11-27T16:20:58Z

@bendichter can we merge this?

h-mayorquin · 2023-12-10T13:13:45Z

src/pynwb/__init__.py

-            # namespaces are not loaded when creating an NWBHDF5IO object in write mode
-            if 'w' in mode or mode == 'x':
-                raise ValueError("cannot load namespaces from file when writing to it")
+        if mode in 'wx' or manager is not None or extensions is not None:


I think this introduced a bug.

h-mayorquin · 2023-12-10T13:13:59Z

src/pynwb/__init__.py

-            if extensions is not None:
-                warn("loading namespaces from file - ignoring 'extensions' argument")
-            # namespaces are not loaded when creating an NWBHDF5IO object in write mode
-            if 'w' in mode or mode == 'x':


Here was correct.

default load_namespaces=True

0345a46

bendichter requested review from rly and oruebel July 28, 2023 21:27

Update CHANGELOG.md

792451d

bendichter and others added 9 commits July 28, 2023 17:56

change logic around to ignore load_namespaces when it cannot be used

382f0fb

Merge remote-tracking branch 'origin/load_namespaces_true' into load_…

acb1c8d

…namespaces_true

flake8

26faa0c

Merge branch 'dev' into load_namespaces_true

955fb8d

Update __init__.py

2c6a415

Merge branch 'dev' into load_namespaces_true

7f1a1e3

fix back-compatibility test

9d5ab33

Merge remote-tracking branch 'origin/load_namespaces_true' into load_…

41ba0db

…namespaces_true

flake8

6c941a0

flake8

f8e35d3

rly previously approved these changes Jul 31, 2023

View reviewed changes

bendichter mentioned this pull request Jul 31, 2023

rmv warning from load_namespaces hdmf-dev/hdmf#926

Merged

remove warning from test

df2a32e

bendichter dismissed rly’s stale review via df2a32e August 1, 2023 00:32

Merge branch 'dev' into load_namespaces_true

e7023c2

oruebel reviewed Sep 6, 2023

View reviewed changes

tests/unit/test_file.py Outdated Show resolved Hide resolved

bendichter added 2 commits September 21, 2023 09:22

rmv useless test

8087153

Merge remote-tracking branch 'origin/load_namespaces_true' into load_…

8a95beb

…namespaces_true

bendichter and others added 5 commits September 21, 2023 15:23

Merge branch 'dev' into load_namespaces_true

28ba0d1

Merge branch 'dev' into load_namespaces_true

6c4d772

Merge branch 'dev' into load_namespaces_true

a833c9e

Update tests, validate script for default load ns

8b86a20

Update CHANGELOG.md

ac71956

rly previously approved these changes Oct 4, 2023

View reviewed changes

bendichter commented Oct 4, 2023

View reviewed changes

tests/unit/test_file.py Outdated Show resolved Hide resolved

docs/gallery/advanced_io/linking_data.py Outdated Show resolved Hide resolved

docs/gallery/advanced_io/linking_data.py Outdated Show resolved Hide resolved

rly dismissed their stale review via 2384a69 October 4, 2023 19:05

rly and others added 2 commits October 4, 2023 12:05

Update docs/gallery/advanced_io/linking_data.py

2384a69

Co-authored-by: Ben Dichter <[email protected]>

Update docs/gallery/advanced_io/linking_data.py

a0daffa

Co-authored-by: Ben Dichter <[email protected]>

rly added 2 commits October 4, 2023 12:23

Update test_file.py

930eef6

Merge branch 'dev' into load_namespaces_true

a8cf307

rly approved these changes Oct 23, 2023

View reviewed changes

Merge branch 'dev' into load_namespaces_true

ad0e08f

bendichter merged commit 9fafde2 into dev Nov 27, 2023
24 checks passed

h-mayorquin reviewed Dec 10, 2023

View reviewed changes

h-mayorquin mentioned this pull request Dec 10, 2023

fix bug with w- io mode #1795

Merged

6 tasks

rly deleted the load_namespaces_true branch December 22, 2023 00:42

rly mentioned this pull request Feb 5, 2024

Load namespaces by default except in write mode and during validation #1196

Closed

stephprince mentioned this pull request Apr 13, 2024

[Documentation]: All docs should load namespaces #1562

Closed

3 tasks

rly mentioned this pull request Jun 3, 2024

[Feature]: NWBZarrIO should have load_namespaces=True by default hdmf-dev/hdmf-zarr#198

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

default load_namespaces=True #1748

default load_namespaces=True #1748

bendichter commented Jul 28, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading

bendichter commented Jul 31, 2023

oruebel commented Jul 31, 2023

bendichter commented Jul 31, 2023

rly commented Jul 31, 2023

oruebel commented Sep 6, 2023

oruebel commented Sep 6, 2023

oruebel commented Sep 6, 2023

bendichter commented Oct 4, 2023

rly commented Oct 4, 2023

rly commented Nov 27, 2023

h-mayorquin Dec 10, 2023

h-mayorquin Dec 10, 2023

default load_namespaces=True #1748

default load_namespaces=True #1748

Conversation

bendichter commented Jul 28, 2023 • edited Loading

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

bendichter commented Jul 31, 2023

oruebel commented Jul 31, 2023

bendichter commented Jul 31, 2023

rly commented Jul 31, 2023

oruebel commented Sep 6, 2023

oruebel commented Sep 6, 2023

oruebel commented Sep 6, 2023

bendichter commented Oct 4, 2023

rly commented Oct 4, 2023

rly commented Nov 27, 2023

h-mayorquin Dec 10, 2023

Choose a reason for hiding this comment

h-mayorquin Dec 10, 2023

Choose a reason for hiding this comment

bendichter commented Jul 28, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading