Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

object-store-based Store implementation #1661

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

kylebarron
Copy link

@kylebarron kylebarron commented Feb 8, 2024

A Zarr store based on obstore, which is a Python library that uses the Rust object_store crate under the hood.

object-store is a rust crate for interoperating with remote object stores like S3, GCS, Azure, etc. See the highlights section of its docs.

obstore maps async Rust functions to async Python functions, and is able to stream GET and LIST requests, which all make it a good candidate for use with the Zarr v3 Store protocol.

You should be able to test this branch with the latest pre-release version of obstore:

pip install --pre --upgrade obstore

TODO:

  • Examples
  • 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)

@jhamman
Copy link
Member

jhamman commented Feb 8, 2024

Amazing @kylebarron! I'll spend some time playing with this today.

@kylebarron
Copy link
Author

With roeap/object-store-python#9 it should be possible to fetch multiple ranges within a file concurrently with range coalescing (using get_ranges_async). Note that this object-store API accepts multiple ranges within one object, which is still not 100% aligned with the Zarr get_partial_values because that allows fetches across multiple objects.

That PR also adds a get_opts function which now supports "offset" and "suffix" ranges, of the sort Range:N- and Range:-N, which would allow removing the raise NotImplementedError on line 37.

@martindurant
Copy link
Member

martindurant/rfsspec#3

async def get_partial_values(
self, key_ranges: List[Tuple[str, Tuple[int, int]]]
) -> List[bytes]:
# TODO: use rust-based concurrency inside object-store
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

@kylebarron kylebarron Feb 9, 2024

Choose a reason for hiding this comment

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

object-store has a built-in function for this: get_ranges. With the caveat that it only manages multiple ranges in a single file.

get_ranges also automatically handles request merging for nearby ranges in a file.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I know, but mine already did the whole thing, so I am showing how I did that.

@normanrz
Copy link
Member

Great work @kylebarron!
What are everbody's thoughts on having this in zarr-python vs. spinning it out as a separate package?

@martindurant
Copy link
Member

What are everbody's thoughts on having this in zarr-python vs. spinning it out as a separate package?

I suggest we see whether it makes any improvements first, so it's author's choice for now.

@kylebarron
Copy link
Author

While @rabernat has seen some impressive perf improvements in some settings when making many requests with Rust's tokio runtime, which would possibly also trickle down to a Python binding, the biggest advantage I see is improved ease of use in installation.

A common hurdle I've seen is handling dependency management, especially around boto3, aioboto3, etc dependencies. Versions need to be compatible at runtime with any other libraries the user also has in their environment. And Python doesn't allow multiple versions of the same dependency at the same time in one environment. With a Python library wrapping a statically-linked Rust binary, you can remove all Python dependencies and remove this class of hardship.

The underlying Rust object-store crate is stable and under open governance via the Apache Arrow project. We'll just have to wait on some discussion in object-store-python for exactly where that should live.

I don't have an opinion myself on where this should live, but it should be on the order of 100 lines of code wherever it is (unless the v3 store api changes dramatically)

@jhamman
Copy link
Member

jhamman commented Feb 12, 2024

I suggest we see whether it makes any improvements first, so it's author's choice for now.

👍

What are everbody's thoughts on having this in zarr-python vs. spinning it out as a separate package?

I want to keep an open mind about what the core stores provided by Zarr-Python are. My current thinking is that we should just do a MemoryStore and a LocalFilesystemStore. Everything else can be opt-in by installing a 3rd party package. That said, I like having a few additional stores in the mix as we develop the store interface since it helps us think about the design more broadly.

@martindurant
Copy link
Member

A common hurdle I've seen is handling dependency management, especially around boto3, aioboto3, etc dependencies.

This is no longer an issue, s3fs has much more relaxed deps than it used to. Furthermore, it's very likely to be already part of an installation environment.

@normanrz
Copy link
Member

I want to keep an open mind about what the core stores provided by Zarr-Python are. My current thinking is that we should just do a MemoryStore and a LocalFilesystemStore. Everything else can be opt-in by installing a 3rd party package.

I agree with that. I think it is beneficial to keep the number of dependencies of core zarr-python small. But, I am open for discussion.

That said, I like having a few additional stores in the mix as we develop the store interface since it helps us think about the design more broadly.

Sure! That is certainly useful.

@jhamman jhamman added the V3 label Feb 13, 2024
@itsgifnotjiff
Copy link

This is awesome work, thank you all!!!

@kylebarron
Copy link
Author

The object-store-python package is not very well maintained roeap/object-store-python#24, so I took a few days to implement my own wrapper around the Rust object_store crate: https://github.com/developmentseed/object-store-rs

I'd like to update this PR soonish to use that library instead.

@martindurant
Copy link
Member

If the zarr group prefers object-store-rs, we can move it into the zarr-developers org, if you like. I would like to be involved in developing it, particularly if it can grow more explicit fsspec compatible functionality.

@kylebarron
Copy link
Author

kylebarron commented Oct 22, 2024

I have a few questions because the Store API has changed a bit since the spring.

  • There's a new BufferPrototype object. Is the BufferPrototype chosen by the store implementation or the caller? It would be very nice if this prototype could be chosen by the store implementation, because then we could return a RustBuffer object that implements the Python buffer protocol, but doesn't need to copy the buffer into Python memory.
  • Similarly for puts, is Buffer guaranteed to implement the buffer protocol? Contrary to fetching, we can't do zero-copy puts right now with object-store

I like that list now returns an AsyncGenerator. That aligns well with the underlying object-store rust API, but for technical reasons we can't expose that as an async iterable to Python yet (apache/arrow-rs#6587), even though we do expose the readable stream to Python as an async iterable.

@TomAugspurger
Copy link
Contributor

Is the BufferPrototype chosen by the store implementation or the caller? It would be very nice if this prototype could be chosen by the store implementation, because then we could return a RustBuffer object that implements the Python buffer protocol, but doesn't need to copy the buffer into Python memory.

This came up in the discussion at https://github.com/zarr-developers/zarr-python/pull/2426/files/5e0ffe80d039d9261517d96ce87220ce8d48e4f2#diff-bb6bb03f87fe9491ef78156256160d798369749b4b35c06d4f275425bdb6c4ad. By default, it's passed as default_buffer_prototype though I think the user can override at the call site or globally.

Does it look compatible with what you need?

@maxrjones
Copy link
Member

maxrjones commented Dec 16, 2024

Test 2-4
Obstore has the end byte on a range request as exclusive whereas Zarr-Python tests have this as inclusive. I want to confirm the intended behavior before proposing any more changes.

Obstore defines range requests as start and end where the end is exclusive. I do explicitly document that the range is exclusive in the docs. While the HTTP spec defines range request strings as inclusive byte ranges I believe. Perhaps fsspec and zarr-python implement the latter.

Sorry for all the churn here, but I realized the actual difference seems to be that ByteRangeRequest is actually [start, length] rather than [start, end]. I guess this makes the inclusive/exclusive end debate moot, but was still super surprising to me. See #2437 for any further discussions on byte range representations.

@maxrjones
Copy link
Member

maxrjones commented Dec 19, 2024

An update for anyone following this PR following a sync with @kylebarron today - the implementations obstore.store.LocalStore and obstore.store.MemoryStore backed Zarr stores seem good to go, but AzureStore, GCSStore, and S3Stores will require more work to make the Zarr storage backend work relative to a specified root prefix (the object_store rust library only supports relativity to the bucket). Kyle and I plan to pair on finishing up this PR in the new year. I hope the work early next year will unblock performance testing of Zarr V3 + obstore relative to Zarr V2 and Zarr V3 + icechunk, and would like to collaborate with Earthmover + others on those performance tests.

@JoshCu

This comment was marked as off-topic.

@jhamman
Copy link
Member

jhamman commented Jan 6, 2025

@JoshCu - I think your comment is sufficiently different from the content of this PR to warrant a dedicated issue. Mind creating one and we can discuss there?

@JoshCu
Copy link

JoshCu commented Jan 6, 2025

@JoshCu - I think your comment is sufficiently different from the content of this PR to warrant a dedicated issue. Mind creating one and we can discuss there?

Thanks :)
#2662


return bool(self.store.__eq__(value.store))

def __init__(self, store: _ObjectStore, *, read_only: bool = False) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

if store is not picklable, you'll need to implement __getstate__ and __setstate__ below. The basic strategy you'll want to use is:

  • in __getstate__, extract the needed config to recreate an the _ObjectStore
  • in __setstate__, use the config from above to recreate the _ObjectStore and set self.store directly

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @jhamman. @kylebarron and I discussed xfailing the pickling tests in this PR and implementing __getstate__ and __setstate__ later on, since ObjectStore could be used for non-distributed reading/writing in the meantime. Would you be open to punting this a bit down the road? IIUC Kyle was supportive of adding pickling eventually and I'd be glad to help but it doesn't seem crucial to us right now given that this store will be marked as experimental.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, let's add the __getstate__ methods with a NotImplementedError so we can provide a clear error message when things go wrong.

@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025
Use new ByteRequest syntax and raise NotImplementedError on pickling
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 13, 2025
@kylebarron
Copy link
Author

I released obstore 0.3 today, so I think we're primed to get this PR ready to go.

The failing test is this? #1661 (comment)

@@ -128,6 +128,7 @@ async def test_get(self, store: S, key: str, data: bytes, byte_range: ByteReques
"""
Ensure that data can be read from the store using the store.get method.
"""
data = b"\x01\x02\x03\x04"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data = b"\x01\x02\x03\x04"

We'll either need to xfail this or address the difference in behavior between stores mentioned in #2560, but I agree with Davis about not removing the test. Sorry this line snuck in with one of my PRs.

@maxrjones
Copy link
Member

I released obstore 0.3 today, so I think we're primed to get this PR ready to go.

The failing test is this? #1661 (comment)

Amazing! There are two remaining issues that I know of:

IMO the two major decision affecting how much work is left are:

  1. do we want this PR to focus only on LocalStore, which is pretty well tested, or also PrefixStore-wrapped S3Stores, etc which is currently untested?
  2. should this PR be merged into Zarr-Python or exist outside via some extension/registry mechanism?

@jhamman
Copy link
Member

jhamman commented Jan 17, 2025

do we want this PR to focus only on LocalStore, which is pretty well tested, or also PrefixStore-wrapped S3Stores, etc which is currently untested?

I'll leave this up to you. With the FsspecStore, we only test against moto (s3) and assume it works everywhere else.

should this PR be merged into Zarr-Python or exist outside via some extension/registry mechanism?

I want to hear from others here at @zarr-developers/python-core-devs on this. Personally, I'd like to have it here as an "Experimental" store but it wouldn't be too hard to put in a standalone package either.

@martindurant
Copy link
Member

I don't see why we would need to move the implementation elsewhere, so long as the documentation is clear about the current state. So long as it is "experimental", we are not supposing that other members of zarr can necessarily handle maintenance of this piece (not that I expect @kylebarron to abandon it for any reason).

@normanrz
Copy link
Member

should this PR be merged into Zarr-Python or exist outside via some extension/registry mechanism?

I want to hear from others here at @zarr-developers/python-core-devs on this. Personally, I'd like to have it here as an "Experimental" store but it wouldn't be too hard to put in a standalone package either.

How mature is obstore at this point?
I am a bit torn. On the one hand, I would like zarr-python to remain a small library with lots of external extensions. On the other hand, I also want it to be batteries-included for users to get going quickly. However, this store seems to duplicate a lot of functionality that the FsspecStore already provides.

@kylebarron
Copy link
Author

How mature is obstore at this point?

Eh, that's hard to answer. You could define maturity as the length of time between breaking releases?

Yesterday's 0.3 release had a bunch of new features like improved DX and async streaming uploads, but it only had two very minor breaking changes: using (start, end) for range requests consistently throughout the API, and returning Rust Bytes objects that implement the buffer protocol instead of returning a Python bytes. 1

However, this store seems to duplicate a lot of functionality that the FsspecStore already provides.

Yes, you could argue this isn't providing anything totally "new" that fsspec doesn't do.

But I hope and expect it will provide performance improvements for large concurrent downloads, and thus be useful for Zarr. I'd certainly like to see similar numbers as what Earthmover found in their icechunk announcement.

We have multiple efforts going on to perform obstore benchmarking. @maxrjones is doing awesome work in https://github.com/maxrjones/zarr-obstore-performance/ specific to Zarr and https://github.com/geospatial-jeff/pyasyncio-benchmark is progressing on totally-general high-throughput benchmarking.

Footnotes

  1. Though incidentally, I'm thinking of making another tiny breaking change before 0.3 is too widespread, to require fully explicit semantics of the range request (similar to https://github.com/zarr-developers/zarr-python/pull/2585#issuecomment-2578172415)

obs.store.HTTPStore,
obs.store.S3Store,
obs.store.LocalStore,
obs.store.MemoryStore,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
obs.store.MemoryStore,
obs.store.MemoryStore,
obs.store.PrefixStore,

@maxrjones
Copy link
Member

maxrjones commented Jan 17, 2025

But I hope and expect it will provide performance improvements for large concurrent downloads, and thus be useful for Zarr. I'd certainly like to see similar numbers as what Earthmover found in their icechunk announcement.

For a super simple first estimate, ObjectStore loads a 3.7 GB array of shape (24, 37, 721, 1440) with chunks (1, 1, 721, 1440) in ~65% of the time required by FsspecStore (4.5s vs 7.0s with n=50; from https://github.com/maxrjones/zarr-obstore-performance). I'd like to add Icechunk to this comparison and possibly include other benchmarks, but IMO this is super promising for the value of an object-store (via obstore) backed Zarr store.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.