N5FSStore #793

d-v-b · 2021-07-06T13:19:55Z

An fsstore-backed implementation of n5

There's a lot of ugly duplicated logic in n5.py. It might be worth some time cleaning that up.

cc @joshmoore, in case you want to test dimension_separator stuff against this branch

TODO:

[] Add unit tests and/or doctests in docstrings
Add docstrings and API docs for any new/modified user-facing classes and functions
New/modified features documented in docs/tutorial.rst
Changes documented in docs/release.rst
GitHub Actions have all passed
Test coverage is 100% (Codecov passes)

Previous tests (now commented out) used logic in the store classes to convert "0/0" keys into "0.0" keys, forcing the store to be aware of array details. This tries to swap the logic so that stores are responsible for passing dimension separator values down to the arrays only. Since arrays can also get the dimension_separator value from a .zarray file they are now in charge.

pep8speaks · 2021-07-06T13:19:59Z

Hello @d-v-b! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-09-19 14:01:04 UTC

codecov · 2021-07-06T14:54:30Z

Codecov Report

Merging #793 (aa4a723) into master (7ec8c64) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #793    +/-   ##
========================================
  Coverage   99.94%   99.94%            
========================================
  Files          31       31            
  Lines       10681    10936   +255     
========================================
+ Hits        10675    10930   +255     
  Misses          6        6

Impacted Files	Coverage Δ
zarr/__init__.py	`100.00% <100.00%> (ø)`
zarr/n5.py	`100.00% <100.00%> (ø)`
zarr/storage.py	`100.00% <100.00%> (ø)`
zarr/tests/test_core.py	`100.00% <100.00%> (ø)`
zarr/tests/test_storage.py	`100.00% <100.00%> (ø)`
zarr/creation.py	`100.00% <0.00%> (ø)`
zarr/tests/test_convenience.py	`100.00% <0.00%> (ø)`

This reverts commit 68adca5.

joshmoore · 2021-08-17T12:28:36Z

Note that #773 is still passing against this branch. Any thoughts on moving forward here, @d-v-b, re: #769 (comment)?

(Personally, I'd be in favor of prioritizing a fix for the state of dimension_separator over code coverage if need be)

…g them pass

d-v-b · 2021-08-17T18:03:19Z

@joshmoore please try again, I discovered that this branch was missing N5FSStore tests in test_core, which were failing until I made some changes. I'm pretty sure anything that changes the chunk keys on the array side is going to break what I have here.

…into n5fsstore

joshmoore · 2021-08-18T07:23:04Z

Good morning, @d-v-b. The merged branches passed for me locally. I've re-activated #794 for fuller testing.

joshmoore · 2021-08-18T18:50:20Z

zarr/tests/test_storage.py

@@ -1207,7 +1207,7 @@ def test_chunk_nesting(self):
        assert 'foo/10.20.30' in store
        assert b'yyy' == store['foo/10.20.30']
        # N5 reverses axis order
-        # assert b'yyy' == store['foo/30/20/10']
+        assert b'yyy' == store['foo/30/20/10']


Per #773 (review), I have a heard time understanding how it's possible to support the requirement that multiple keys be supported for the same item.

This may be part and parcel of the conversation we're having in gitter: https://gitter.im/zarr-developers/community?at=611d562e933ee46b7564044c

Update: ah, now I see that they are re-ordered not just .// swapped, but I think my same concern holds.

I agree with the concern -- it looks like this behavior is expected from the NestedDirectoryStore: https://github.com/zarr-developers/zarr-python/blob/master/zarr/tests/test_storage.py#L1148

On my PR I resurrected this. I at least understand know why it's supported, though I think it's a hackdoor of sorts.

joshmoore · 2021-08-20T13:30:31Z

Sounds good. If you want to test a reversion of the hexdigests, you can pin numcodecs to <0.9. I'm unsure of whether or not to try fixing the NestedDirectoryStore issue. Nothing is failing at the moment so I'm tempted to leave it as in #773.

…stdir, and add crucial lstrip

…e dimension_separator keyword now present in metadata

* Adds a test for the dimension_separator warning * uses the parent test_complex for listdir * "nocover" the import error since fsspec is ever present

joshmoore · 2021-09-17T07:37:26Z

@d-v-b: I pushed a commit trying to get codecov to 100%. I'm still having difficulty getting the likes of:

elif is_chunk_key(key):
    key = self._swap_separator(key)
``
to trigger if you have any ideas.

d-v-b · 2021-09-17T16:46:38Z

@joshmoore I copied a TestN5Store method over into TestN5FSStore, which seems to have done the trick. An alternative to copying code would be to make TestN5FSStore inherit from TestFSStore and TestN5Store, but I'm nervous about using multiple inheritance (and when I ran tests with multiple inheritance, I didn't uncover any surprising failures).

joshmoore · 2021-09-17T18:38:11Z

Sounds good. Three Friday evening points:

@zarr-developers/core-devs : any comments?
I'm still a tiny bit nervous about having two versions but I guess deprecation might eventually be our friend.
What version do you propose? patch or minor?

d-v-b · 2021-09-17T18:43:11Z

I'm still a tiny bit nervous about having two versions but I guess deprecation might eventually be our friend.

By "two versions" do you mean the N5Store and N5FSStore? Given that a) n5 is relatively niche, and this was flagged as experimental, and b) fsspec isn't a super heavy dependency, I agree that a quick deprecation path looks nice.

Version-wise I could think this is minor? since we are adding to the public API.

joshmoore · 2021-09-17T19:06:48Z

zarr/storage.py

@@ -1065,22 +1065,28 @@ class FSStore(MutableMapping):
        Separator placed between the dimensions of a chunk.
    storage_options : passed to the fsspec implementation
    """
+    array_meta_key = array_meta_key


Sorry, I missed these. Would it make sense to have these prefixed with _. Do you really mean these to be public API?

good question! I'm fine with making these private.

… n5fsstore

FSStore only throws ModuleNotFoundError on initialization rather than on import. Therefore N5FSStore does the same. If this *weren't* the case, then the import in zarr/init would need to test the import as well, which isn't the case.

joshmoore · 2021-09-19T14:12:16Z

zarr/__init__.py

@@ -9,7 +9,7 @@
                           zeros_like)
 from zarr.errors import CopyError, MetadataError
 from zarr.hierarchy import Group, group, open_group
-from zarr.n5 import N5Store
+from zarr.n5 import N5Store, N5FSStore


This concerned me that it would need protecting by try/except block. In testing it, I realized FSStore only throws on __init__ and therefore N5FSStore could be less conservative. I've pushed:

aa4a723

joshmoore · 2021-09-19T14:21:32Z

zarr/storage.py

+        protocol, _ = fsspec.core.split_protocol(url)
+        # set auto_mkdir to True for local file system
+        if protocol in (None, "file") and not storage_options.get("auto_mkdir"):
+            storage_options["auto_mkdir"] = True


joshmoore

Thanks for this, @d-v-b! Pushing forward with a minor release.

d-v-b · 2021-09-19T14:35:38Z

Thanks for your help @joshmoore!

joshmoore and others added 12 commits June 14, 2021 13:33

Drop skip_if_nested_chunks from test_storage.py

d26923a

Add failing nested test

c06476d

Make DirectoryStore dimension_separator aware

ce8b2f0

Fix linting in new test

449a67f

Merge 'origin/master' into fix-dstore

10c874e

Extend the test suite for dim_sep

2e4f4d7

add n5fsstore and tests

8660fa5

resolve merge conflicts with main

3b341ed

slightly smarter kwarg interception

bb1121c

remove outdated unittest ref and fix the name of a test func

be8f37f

fix massive string block and fix default key_separator kwarg for FSStore

95b2573

flake8

ceba78d

joshmoore mentioned this pull request Jul 6, 2021

Temporary PR for testing 773 and 793 #794

Closed

promote n5store to toplevel import and fix examples in docstring

02ea91c

joshmoore and others added 3 commits August 17, 2021 13:57

Merge branch 'master' into fix-dstore

cb62c10

Try fsspec 2021.7 (see zarr-developers#802)

68adca5

Revert "Try fsspec 2021.7 (see zarr-developers#802)"

f2f75b7

This reverts commit 68adca5.

d-v-b added 2 commits August 17, 2021 11:04

Merge branch 'master' into n5fsstore

930a821

Add missing core tests for N5FSStore, and rchanges required for makin…

a57b3bc

…g them pass

Merge branch 'fix-dstore' of https://github.com/joshmoore/zarr-python …

9bb058f

…into n5fsstore

joshmoore and others added 3 commits August 18, 2021 09:51

tmp: debug

ee9cdbc

uncomment N5 chunk ordering test

a853a29

more commented tests get uncommented

7d3c879

joshmoore reviewed Aug 18, 2021

View reviewed changes

d-v-b added 5 commits September 13, 2021 19:53

resolve merge conflicts in favor of upstream

eea4aaa

resolve merge conflicts in favor of upstream

b8fe803

make listdir implementation for n5fsstore look more like fsstore's li…

8fec1d6

…stdir, and add crucial lstrip

Update hexdigest tests for N5Stores to account for the presence of th…

46ebb44

…e dimension_separator keyword now present in metadata

Add tests for dimension_separator in array meta for N5Stores

864773d

d-v-b changed the title ~~[WIP] N5FSStore~~ N5FSStore Sep 16, 2021

joshmoore force-pushed the n5fsstore branch 2 times, most recently from b8f12a8 to 5fcf68d Compare September 17, 2021 07:16

N5FSStore: try to increase code coverage

3b56155

* Adds a test for the dimension_separator warning * uses the parent test_complex for listdir * "nocover" the import error since fsspec is ever present

joshmoore force-pushed the n5fsstore branch from 5fcf68d to 3b56155 Compare September 17, 2021 07:36

d-v-b added 2 commits September 17, 2021 12:31

flake8

b0f6d33

add chunk nesting test to N5FSStore test suite

82ce89f

Merge branch 'master' into n5fsstore

267c744

joshmoore reviewed Sep 17, 2021

View reviewed changes

d-v-b and others added 3 commits September 17, 2021 19:06

make array_meta_key, group_meta_key, attrs_key private

2b85410

Merge branch 'n5fsstore' of https://github.com/d-v-b/zarr-python into…

8bd6c41

… n5fsstore

N5FSStore: Remove ImportError test

aa4a723

FSStore only throws ModuleNotFoundError on initialization rather than on import. Therefore N5FSStore does the same. If this *weren't* the case, then the import in zarr/init would need to test the import as well, which isn't the case.

joshmoore reviewed Sep 19, 2021

View reviewed changes

joshmoore approved these changes Sep 19, 2021

View reviewed changes

joshmoore merged commit dca87fc into zarr-developers:master Sep 19, 2021

d-v-b mentioned this pull request Sep 22, 2021

N5Store support of cloud buckets #540

Closed

d-v-b deleted the n5fsstore branch September 22, 2021 18:36

d70-t mentioned this pull request Sep 28, 2021

normalize_keys regression in version 2.10.0 #841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

N5FSStore #793

N5FSStore #793

d-v-b commented Jul 6, 2021

pep8speaks commented Jul 6, 2021 •

edited

codecov bot commented Jul 6, 2021 •

edited

joshmoore commented Aug 17, 2021 •

edited

d-v-b commented Aug 17, 2021

joshmoore commented Aug 18, 2021

joshmoore Aug 18, 2021

joshmoore Aug 18, 2021

d-v-b Aug 18, 2021

joshmoore Aug 19, 2021

joshmoore commented Aug 20, 2021

joshmoore commented Sep 17, 2021

d-v-b commented Sep 17, 2021

joshmoore commented Sep 17, 2021

d-v-b commented Sep 17, 2021

joshmoore Sep 17, 2021

d-v-b Sep 17, 2021

joshmoore Sep 19, 2021

joshmoore Sep 19, 2021

joshmoore left a comment

d-v-b commented Sep 19, 2021

N5FSStore #793

N5FSStore #793

Conversation

d-v-b commented Jul 6, 2021

pep8speaks commented Jul 6, 2021 • edited

Comment last updated at 2021-09-19 14:01:04 UTC

codecov bot commented Jul 6, 2021 • edited

Codecov Report

joshmoore commented Aug 17, 2021 • edited

d-v-b commented Aug 17, 2021

joshmoore commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore commented Aug 20, 2021

joshmoore commented Sep 17, 2021

d-v-b commented Sep 17, 2021

joshmoore commented Sep 17, 2021

d-v-b commented Sep 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshmoore left a comment

Choose a reason for hiding this comment

d-v-b commented Sep 19, 2021

pep8speaks commented Jul 6, 2021 •

edited

codecov bot commented Jul 6, 2021 •

edited

joshmoore commented Aug 17, 2021 •

edited