Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix FSStore.listdir behavior for nested directories - #802

Merged
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested
Sep 1, 2021
Merged

Fix FSStore.listdir behavior for nested directories#802
joshmoore merged 6 commits into
zarr-developers:masterfrom
grlee77:fix-fsstore-listdir-nested

Conversation

@grlee77

@grlee77grlee77 commented Aug 2, 2021

Copy link
Copy Markdown
Contributor

This PR is to address the two test failures seen in #786. The fix is to remove any leading '/' from the key, so that after the replacement of '/' with . it becomes a valid chunk key.

The failures are not seen in the test suite with the currently pinned fsspec 2021.6.0, so I have also bumped the fsspec version to 2021.7.0 (which was showing the same failures as reported in #786 in local testing).

closes#786
closes#797 (specifying fsstore[s3] so that the separate s3fs dependency is not longer needed)

TODO:

  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@joshmoore

Copy link
Copy Markdown
Member

Note: I've updated the required status checks so the hanging build (3.8) and build (3.8, ==1.16.4) should no longer hang. I'm going to see if closing and re-opening will make them go away.

Comment threadzarr/storage.py Outdated
@grlee77

Copy link
Copy Markdown
ContributorAuthor

Actually, I see one other test failure in test_storage.py that occurs with 2021.07.0, but not 2021.06.1 (so not due to the same commit).

Details
=========================================================================================================== FAILURES ===========================================================================================================
___________________________________________________________________________________________________ TestFSStore.test_create ____________________________________________________________________________________________________
self = <zarr.tests.test_storage.TestFSStore object at 0x7fb2efd165b0>
def test_create(self):
import zarr
path1 = tempfile.mkdtemp()
path2 = tempfile.mkdtemp()
g = zarr.open_group("file://" + path1, mode='w',
storage_options={"auto_mkdir": True})
a = g.create_dataset("data", shape=(8,))
a[:4] = [0, 1, 2, 3]
assert "data" in os.listdir(path1)
assert ".zgroup" in os.listdir(path1)
g = zarr.open_group("simplecache::file://" + path1, mode='r',
storage_options={"cache_storage": path2,
"same_names": True})
> assert g.data[:].tolist() == [0, 1, 2, 3, 0, 0, 0, 0]
zarr/tests/test_storage.py:965: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ zarr/core.py:662: in __getitem__
return self.get_basic_selection(selection, fields=fields)
zarr/core.py:787: in get_basic_selection
return self._get_basic_selection_nd(selection=selection, out=out,
zarr/core.py:830: in _get_basic_selection_nd
return self._get_selection(indexer=indexer, out=out, fields=fields)
zarr/core.py:1125: in _get_selection
self._chunk_getitems(lchunk_coords, lchunk_selection, out, lout_selection,
zarr/core.py:1836: in _chunk_getitems
cdatas = self.chunk_store.getitems(ckeys, on_error="omit")
zarr/storage.py:1085: in getitems
results = self.map.getitems(keys_transformed, on_error="omit")
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <fsspec.mapping.FSMap object at 0x7fb2efd56ca0>, keys = ['data/0'], on_error = 'omit'
def getitems(self, keys, on_error="raise"):
"""Fetch multiple items from the store
If the backend is async-able, this might proceed concurrently
Parameters
----------
keys: list(str)
They keys to be fetched
on_error : "raise", "omit", "return"
If raise, an underlying exception will be raised (converted to KeyError
if the type is in self.missing_exceptions); if omit, keys with exception
will simply not be included in the output; if "return", all keys are
included in the output, but the value will be bytes or an exception
instance.
Returns
-------
dict(key, bytes|exception)
"""
keys2 = [self._key_to_str(k) for k in keys]
oe = on_error if on_error == "raise" else "return"
try:
out = self.fs.cat(keys2, on_error=oe)
except self.missing_exceptions as e:
raise KeyError from e
out = {
k: (KeyError() if isinstance(v, self.missing_exceptions) else v)
> for k, v in out.items()
}
E AttributeError: 'bytes' object has no attribute 'items'

This PR fixed the two failurs in test_core.py, but the CI here will apparently still fail due to that.

@martindurant

Copy link
Copy Markdown
Member

I believe that last one is fixed in main

@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from e5b48ca to ac7ed24CompareAugust 3, 2021 18:54
@grlee77
grlee77force-pushed the fix-fsstore-listdir-nested branch from ac7ed24 to 407cd98CompareAugust 3, 2021 19:05
@grlee77

Copy link
Copy Markdown
ContributorAuthor

I rebased on master and switched to "/" as requested, but there is now the other error mentioned previously: https://github.com/zarr-developers/zarr-python/pull/802/checks?check_run_id=3234273246#step:7:139

@martindurant

Copy link
Copy Markdown
Member

Is this using fsspec main?

@grlee77

Copy link
Copy Markdown
ContributorAuthor

No, I just saw that it was fixed in fsspec master. I had missunderstood your previous comment to be referring to zarr-python master.

The CI here is using 2021.7.0 so the failure is expected.

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 17, 2021
@joshmoore

Copy link
Copy Markdown
Member

As a test, I tried 2021.7.0 on #773 without luck. (https://github.com/zarr-developers/zarr-python/pull/773/checks?check_run_id=3350238418)

@martindurant : any thoughts on a release for a 7.1 or should I pin to 6.*?

joshmoore added a commit to joshmoore/zarr-python that referenced this pull request Aug 18, 2021
joshmoore added a commit that referenced this pull request Aug 20, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Fix N5Store
* Re-activate contested N5 test
@bnavigatorbnavigator mentioned this pull request Aug 20, 2021
@bnavigator

Copy link
Copy Markdown

For the record: test_core::*::test_array_2d still fail on 2.9.0 with fsspec-2021.7 and fsspec/filesystem_spec#710 applied to it.

@joshmoore

Copy link
Copy Markdown
Member

Thanks, @bnavigator. Unless someone has another suggestion, I'm going to start actively avoiding 2021.07.0 in the wild:

ome/ome-zarr-py@741e952

@bnavigator

Copy link
Copy Markdown

Oh, sorry, I should clarify that I referred to 2.9.0 without this PR. With this patch plus the patch from fsspec/filesystem_spec#710 applied, all (offline) tests succeed.

@joshmoore

Copy link
Copy Markdown
Member

@bnavigator : noted, but fsspec/filesystem_spec#710 is not yet released, right?

@bnavigator

Copy link
Copy Markdown

AFAICT, no. @martindurant any plans to do so soon?

@martindurant

Copy link
Copy Markdown
Member

Yes, just trying to fix up a couple of s3 things first. If I don't make progress in the next ~day, can release fsspec/gcsfs/s3fs anyway.

@codecov

codecovBot commented Sep 1, 2021

Copy link
Copy Markdown

Codecov Report

Merging #802 (d3fbcc9) into master (1a2eeed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #802 +/- ##
=======================================
Coverage 99.94% 99.94% =======================================
Files 31 31 Lines 10680 10681 +1 =======================================
+ Hits 10674 10675 +1 
Misses 6 6 
Impacted FilesCoverage Δ
zarr/storage.py100.00% <100.00%> (ø)

@joshmoorejoshmoore left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting prep for 2.9.5.

Thanks, @martindurant and @grlee77!

@joshmoore
joshmoore merged commit 4e98567 into zarr-developers:masterSep 1, 2021
joshmoore added a commit that referenced this pull request Sep 19, 2021
* Drop skip_if_nested_chunks from test_storage.py
* Add failing nested test
* Make DirectoryStore dimension_separator aware
* Migrate key logic to core rather than storage
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.
* Fix linting in new test
* Extend the test suite for dim_sep
* add n5fsstore and tests
* slightly smarter kwarg interception
* remove outdated unittest ref and fix the name of a test func
* fix massive string block and fix default key_separator kwarg for FSStore
* flake8
* promote n5store to toplevel import and fix examples in docstring
* Try fsspec 2021.7 (see #802)
* Revert "Try fsspec 2021.7 (see #802)"
This reverts commit 68adca5.
* Add missing core tests for N5FSStore, and rchanges required for making them pass
* tmp: debug
* uncomment N5 chunk ordering test
* more commented tests get uncommented
* add dimension_separator to array metadata adaptor
* Revert "tmp: debug"
This reverts commit ee9cdbc.
* Attempt failed: keeping '.' and switching
* Revert "Attempt failed: keeping '.' and switching"
This reverts commit 51b3109.
* regex: attempt failed due to slight diff in files
* Revert "regex: attempt failed due to slight diff in files"
This reverts commit 3daea7c.
* N5: use "." internally for dimension separation
This allows N5 to detect the split between key and chunks
and pre-process them (re-ordering and changing the separator).
see: #773#793
* move FSSpec import guard
* remove os.path.sep concatenation in listdir that was erroring a test, and add a mea culpa docstring about the dimension_separator for n5 stores
* resolve merge conflicts in favor of upstream
* make listdir implementation for n5fsstore look more like fsstore's listdir, and add crucial lstrip
* Update hexdigest tests for N5Stores to account for the presence of the dimension_separator keyword now present in metadata
* Add tests for dimension_separator in array meta for N5Stores
* N5FSStore: try to increase code coverage
* Adds a test for the dimension_separator warning
* uses the parent test_complex for listdir
* "nocover" the import error since fsspec is ever present
* flake8
* add chunk nesting test to N5FSStore test suite
* make array_meta_key, group_meta_key, attrs_key private
* N5FSStore: Remove ImportError test
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.
Co-authored-by: jmoore <josh@glencoesoftware.com>
Co-authored-by: Josh Moore <j.a.moore@dundee.ac.uk>
Sign up for freeto 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.

4 participants

@grlee77@joshmoore@martindurant@bnavigator