Basic working FsspecStore #1785

martindurant · 2024-04-10T20:40:29Z

This works.

We don't have any list methods, do we need them?
We don't have a bulk delete
No exception handling is done here, which has been a thorny issue. Shall we do the same as in v2 with expected exceptions -> KeyError? I don't see in Store's API what the expectation is.

Fixes #1757

TODO:

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

martindurant · 2024-04-10T20:41:09Z

@jhamman @d-v-b , for discussion

pep8speaks · 2024-04-11T19:17:09Z

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

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

Comment last updated at 2024-04-14 01:40:25 UTC

d-v-b · 2024-04-11T20:03:21Z

thanks for this @martindurant, I will see if I have time to play with this locally.

No exception handling is done here, which has been a thorny issue. Shall we do the same as in v2 with expected exceptions -> KeyError? I don't see in Store's API what the expectation is.

I don't think we have expectations at this point. I will try to get a distillation from the v2 issues around this topic. What do you think we should do here?

… v3_fsspec

martindurant · 2024-04-14T01:44:27Z

I see we have no storage tests, so don't know how to push this any further.

d-v-b · 2024-04-14T08:42:00Z

I will get some tests for you shortly

jhamman · 2024-05-29T03:53:24Z

@martindurant - the test suite is in a better place now. Are you up for picking this up?

martindurant · 2024-05-29T20:15:02Z

For testing ... I could set up a mock s3 or gcs with some pain and CI overhead. Also, I could wrap the fsspec memoryFS in async stuff, but that would not really be testing the async-ness and depends no me doing that correctly. None of the other stores are actually async, right?

jhamman · 2024-05-29T21:17:30Z

@martindurant - which fsspec implementations have an async-api available?

For now, a mocked s3 backend seems like the way to go.

martindurant · 2024-05-30T00:07:50Z

which fsspec implementations have an async-api

I think this is a complete list

s3
gcs
azure (both blob and datalake2)
http
sshfs (not the builtin ssh/sftp)
anaconda (released version is still sync)

The following can pass through async, if they wrap an async FS:

generic
reference
dir/prefix

jhamman · 2024-05-30T06:25:46Z

Got it. Makes sense. For now, let's target a mocked test against s3. We can add a few others as we approach a full release.

martindurant · 2024-05-30T13:36:21Z

a mocked test against s3

Well, I would use the moto server as s3fs does, which is a real S3 implementation with most of the functionality of the real one (minus permissions, object lifetimes and other unimportant things).

jhamman

@martindurant -- this is coming together. I took it for a spin today and ended up needing to make a few changes (suggestions below). But it works!

src/zarr/store/remote.py

martindurant · 2024-06-04T01:23:48Z

I merged from v3 and the suggestions above. I cannot get mypy to play.

By the way, I would suggest that memoryview() and bytes() really ought to work as expected for a Buffer.

src/zarr/store/remote.py

jhamman · 2024-06-04T14:41:10Z

@martindurant -- thanks for pushing this forward...

I merged from v3 and the suggestions above. I cannot get mypy to play.

I'm hoping @dstansby can take a look... we can find a way!

By the way, I would suggest that memoryview() and bytes() really ought to work as expected for a Buffer.

cc @madsbk, @akshaysubr, @normanrz

Co-authored-by: Martin Durant <martindurant@users.noreply.github.com>

into mdurant_v3_fsspec

…as xfail

Mdurant v3 fsspec

Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>

martindurant · 2024-06-10T18:41:48Z

OK, so the problem with the tests, is that they call get/set in blocking sync code, but the store implementation works in async mode. The test instance is created in sync mode, and gets put on a dedicated fsspec IO thread; but then the async store on the main thread calls the same instance in async mode. Note that moto3 is also running on a thread, to complicate things. I am looking at it.

d-v-b · 2024-06-10T19:33:36Z

OK, so the problem with the tests, is that they call get/set in blocking sync code, but the store implementation works in async mode. The test instance is created in sync mode, and gets put on a dedicated fsspec IO thread; but then the async store on the main thread calls the same instance in async mode. Note that moto3 is also running on a thread, to complicate things. I am looking at it.

That's great insight, thank you. Please let me know if there's anything about the StoreTests design that we should change to make this process simpler.

jhamman

I see ✅!

Thanks @martindurant for working out the kinks here! (and @d-v-b and @dstansby for chipping in)

martindurant · 2024-06-11T02:18:57Z

Highlighting this line, which works around tests passing offset-length for data which is b"". Python allows you to slice bytes like that (b""[1:2] == b""), but remote stores do not allow this.

src/zarr/store/remote.py

d-v-b · 2024-06-11T12:54:14Z

src/zarr/store/remote.py

-                if byte_range
-                else fs._cat_file(path)
+            if byte_range:
+                # fsspec uses start/end, not start/length


I count this as additional evidence that we should switch to start/end semantics for the rest of the stores

Kerchunk is the exception, storing start/length, mostly because length is generally smaller for chunks in big files.

d-v-b · 2024-06-11T12:54:48Z

src/zarr/store/remote.py

+        except self.exceptions:
+            return None
+        except OSError as e:
+            if "not satisfiable" in str(e):


flagging this as s3 abstraction leakage that we might want to address later on by making an s3-specific storage class

d-v-b · 2024-06-11T12:57:23Z

src/zarr/store/remote.py

+        # TODO: expectations for exceptions or missing keys?
+        res = await self._fs._cat_ranges(list(paths), starts, stops, on_error="return")


i think returning the exceptions is the right thing here

d-v-b · 2024-06-11T12:59:27Z

tests/v2/test_storage.py

@@ -399,7 +399,9 @@ def test_hierarchy(self):
            assert [] == store.listdir(self.root + "c/x/y")
            assert [] == store.listdir(self.root + "c/d/y")
            assert [] == store.listdir(self.root + "c/d/y/z")
-            assert [] == store.listdir(self.root + "c/e/f")
+            # the following is listdir(filepath), for which fsspec gives [filepath]


what's the advantage of going with POSIX semantics here?

fsspec tries to adhere to posix as much as possible. If we want to exclude the [file] case, we'd have to code that special case into our store.

d-v-b · 2024-06-11T13:00:46Z

tests/v3/test_store/test_remote.py

+
+
+@pytest.fixture(autouse=True, scope="function")
+def s3(s3_base):


@martindurant could you explain what's happening in this test fixture? e.g., why do we need to manipulate the cache, why do we need to create an instance of S3FileSystem?

pytest-asyncio creates a new event loop for each async test. When an async-mode s3fs instance is made from async, it will be assigned to the loop from which it is made. That means that if you use s3fs again from a subsequent test, you will have the same identical instance, but be running on a different loop - which fails.

For the rest: it's very convenient to clean up the state of the store between tests, make sure we start off blank each time.

d-v-b

looks good, thank you @martindurant et al

…r-developers#1679) This is adapted from the fixes that were rolled into zarr-developers#1785 for the v3 branch. Signed-off-by: Adam Williamson <awilliam@redhat.com>

* v3: (22 commits) [v3] `Buffer` ensure correct subclass based on the `BufferPrototype` argument (zarr-developers#1974) Fix doc build (zarr-developers#1987) Fix doc build warnings (zarr-developers#1985) Automatically generate API reference docs (zarr-developers#1918) Update `RemoteStore.__str__` and add UPath tests (zarr-developers#1964) [v3] Elevate codec pipeline (zarr-developers#1932) 0 dim arrays: indexing (zarr-developers#1980) `parse_shapelike` allows 0 (zarr-developers#1979) Clean up typing and docs for indexing (zarr-developers#1961) add json indentation to config (zarr-developers#1952) chore: update pre-commit hooks (zarr-developers#1973) Bump pypa/gh-action-pypi-publish in the actions group (zarr-developers#1969) chore: update pre-commit hooks (zarr-developers#1957) Update release.rst (zarr-developers#1960) doc: update release notes for 3.0.0.alpha (zarr-developers#1959) Basic working FsspecStore (zarr-developers#1785) Feature: Top level V3 API (zarr-developers#1884) Buffer Prototype Argument (zarr-developers#1910) Create issue-metrics.yml fixes bug in transpose (zarr-developers#1949) ...

Basic working FsspecStore

c21da45

Merge branch 'v3' into v3_fsspec

5489408

martindurant added 5 commits April 12, 2024 14:06

upath to be optional

29fa0a4

Merge branch 'v3_fsspec' of https://github.com/martindurant/zarr into…

c2adda5

… v3_fsspec

fill out methods

320a671

add fsspec to deps (I believe we want this)

79477f5

fixes

9104bdc

jhamman added the V3 Affects the v3 branch label Apr 22, 2024

jhamman added this to the 3.0.0.alpha milestone Apr 22, 2024

martindurant added 4 commits May 29, 2024 09:59

Merge branch 'v3' into v3_fsspec

581eafe

importable

20c9387

exceptions

b6bfa11

Merge branch 'v3' into v3_fsspec

02d8149

jhamman reviewed May 30, 2024

View reviewed changes

src/zarr/store/remote.py Outdated Show resolved Hide resolved

src/zarr/store/remote.py Outdated Show resolved Hide resolved

src/zarr/store/remote.py Show resolved Hide resolved

src/zarr/store/remote.py Show resolved Hide resolved

jhamman mentioned this pull request Jun 1, 2024

Fix final typing errors #1939

Merged

Merge branch 'v3' into v3_fsspec

9c402b6

jhamman reviewed Jun 4, 2024

View reviewed changes

src/zarr/store/remote.py Outdated Show resolved Hide resolved

d-v-b and others added 6 commits June 10, 2024 16:54

Update tests/v3/test_store/test_remote.py

a4ca371

Co-authored-by: Martin Durant <martindurant@users.noreply.github.com>

don't use fsmap, and don't use os.path.join

0896cb8

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

a4e1c20

into mdurant_v3_fsspec

scope s3 fixture to session, mark test_store_supports_partial_writes …

93a2c6a

…as xfail

Merge pull request #23 from d-v-b/mdurant_v3_fsspec

7c22f74

Mdurant v3 fsspec

Update src/zarr/store/remote.py

3c037c3

Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>

Fix most

3454da9

martindurant added 4 commits June 10, 2024 15:44

fixed more

9b6a6d3

fix rest

266fa37

Massage old v2 tests

221cebc

just skip them..

1bfcbb5

jhamman approved these changes Jun 11, 2024

View reviewed changes

d-v-b reviewed Jun 11, 2024

View reviewed changes

src/zarr/store/remote.py Outdated Show resolved Hide resolved

d-v-b reviewed Jun 11, 2024

View reviewed changes

Attribute rename to allowed_exceptions

362951e

d-v-b approved these changes Jun 11, 2024

View reviewed changes

Merge branch 'v3' into v3_fsspec

3fb24da

jhamman merged commit 7ded5d6 into zarr-developers:v3 Jun 11, 2024
18 checks passed

AdamWill mentioned this pull request Jun 17, 2024

Adapt storage tests for changes in fsspec (#1819) (#1679) #1970

Closed

2 tasks

martindurant mentioned this pull request Jul 16, 2024

Add Zarr compatibility functions fsspec/kerchunk#478

Open

TomAugspurger mentioned this pull request Aug 7, 2024

pytest is an unnecessary dependency? #2068

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic working FsspecStore #1785

Basic working FsspecStore #1785

martindurant commented Apr 10, 2024

martindurant commented Apr 10, 2024

pep8speaks commented Apr 11, 2024 •

edited

Loading

d-v-b commented Apr 11, 2024

martindurant commented Apr 14, 2024

d-v-b commented Apr 14, 2024

jhamman commented May 29, 2024

martindurant commented May 29, 2024

jhamman commented May 29, 2024

martindurant commented May 30, 2024

jhamman commented May 30, 2024

martindurant commented May 30, 2024

jhamman left a comment

martindurant commented Jun 4, 2024

jhamman commented Jun 4, 2024

martindurant commented Jun 10, 2024

d-v-b commented Jun 10, 2024

jhamman left a comment

martindurant commented Jun 11, 2024

d-v-b Jun 11, 2024

martindurant Jun 11, 2024

d-v-b Jun 11, 2024

d-v-b Jun 11, 2024

d-v-b Jun 11, 2024

martindurant Jun 11, 2024

d-v-b Jun 11, 2024

martindurant Jun 11, 2024

d-v-b left a comment

		# TODO: expectations for exceptions or missing keys?
		res = await self._fs._cat_ranges(list(paths), starts, stops, on_error="return")



		@pytest.fixture(autouse=True, scope="function")
		def s3(s3_base):

Basic working FsspecStore #1785

Basic working FsspecStore #1785

Conversation

martindurant commented Apr 10, 2024

martindurant commented Apr 10, 2024

pep8speaks commented Apr 11, 2024 • edited Loading

Comment last updated at 2024-04-14 01:40:25 UTC

d-v-b commented Apr 11, 2024

martindurant commented Apr 14, 2024

d-v-b commented Apr 14, 2024

jhamman commented May 29, 2024

martindurant commented May 29, 2024

jhamman commented May 29, 2024

martindurant commented May 30, 2024

jhamman commented May 30, 2024

martindurant commented May 30, 2024

jhamman left a comment

Choose a reason for hiding this comment

martindurant commented Jun 4, 2024

jhamman commented Jun 4, 2024

martindurant commented Jun 10, 2024

d-v-b commented Jun 10, 2024

jhamman left a comment

Choose a reason for hiding this comment

martindurant commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-v-b left a comment

Choose a reason for hiding this comment

pep8speaks commented Apr 11, 2024 •

edited

Loading