-
-
Notifications
You must be signed in to change notification settings - Fork 365
feat: Implement ZEP 8 URL syntax support for zarr-python #3369
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
base: main
Are you sure you want to change the base?
Conversation
- Add comprehensive ZEP 8 URL parsing and resolution system - Implement StoreAdapter ABC for extensible storage adapters - Add built-in adapters for file, memory, S3, GCS, HTTPS schemes - Support pipe-chained URLs like s3://bucket/data.zip|zip:|zarr3: - Add URLSegment parsing with validation - Integrate with zarr.open_group and zarr.open_array APIs - Include demo script and comprehensive test suite - Pass all existing tests + 35 new ZEP 8-specific tests
…into feature/zep8-url-support
|
One tricky thing about The "path" syntax is generally the default when running a regular s3-compatible server, but the "virtual host" syntax can commonly occur when someone defines a CNAME DNS entry to map their own domain or subdomain to an AWS S3 bucket. When designing this syntax for Neuroglancer, it seemed like it would be annoying to require users to use separate syntax to disambiguate the two cases. Instead, for operations where it matters (namely List), Neuroglancer just automatically determines which of the two cases applies by trying both ways and seeing which one succeeds, and then caching the result so that subsequent list operations don't require two requests. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3369 +/- ##
===========================================
- Coverage 94.92% 60.87% -34.06%
===========================================
Files 79 86 +7
Lines 9500 10231 +731
===========================================
- Hits 9018 6228 -2790
- Misses 482 4003 +3521
🚀 New features to boost your workflow:
|
| Examples:: | ||
|
|
||
| # Basic ZIP file storage | ||
| zarr.open_array("file:data.zip|zip", mode='w', shape=(10, 10), dtype="f4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a bad idea to break file uris.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is considered in the spec doc this PR is building on: zarr-developers/zeps#48
And later is says:
Implementations SHOULD not support `file://relative/path` since that
is ambiguous with the `file://hostname/path` syntax defined by
[RFC8089](https://datatracker.ietf.org/doc/html/rfc8089).
If you forsee serious issues here I'd encourage commenting on that PR on the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partway through the code at this point (currently following the logic of store resolution). Posting some comments now so I don't lose them (i've been burned by this before)
| **In-memory storage:** | ||
|
|
||
| >>> # Create array in memory | ||
| >>> z = zarr.open_array("memory:", mode='w', shape=(5, 5), dtype="f4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I then access this from somewhere else using this syntax?
e.g. memory:aesr80s9e8ra?
| .. warning:: | ||
| The :class:`zarr.storage.ObjectStore` class is experimental. | ||
|
|
||
| URL-based Storage (ZEP 8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what this section is missing is a showcasing of what the equivalent zarr-pyhthon code would be to put it in terms people are more familiar with.
So each section would be:
zarr.open_array("file:zep8-data.zip|zip" ....)
# is equivalent to
zarr.open_array(zarr.storage.ZipStore(...)...)| .. note:: | ||
| When using ZEP 8 URLs with third-party libraries like xarray, the URL syntax allows | ||
| seamless integration without requiring zarr-specific store creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .. note:: | |
| When using ZEP 8 URLs with third-party libraries like xarray, the URL syntax allows | |
| seamless integration without requiring zarr-specific store creation. |
This is already effectively stated above.
| URL-based Storage (ZEP 8) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Zarr supports URL-based storage following the ZEP 8 specification, which allows you to specify storage locations using URLs with chained adapters:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a link to the more extensive docs.
|
|
||
| - ``file:path.zip|zip`` - ZIP file on local filesystem | ||
| - ``s3://bucket/data.zip|zip`` - ZIP file in S3 bucket | ||
| - ``memory:`` - In-memory storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - ``memory:`` - In-memory storage |
not an example of piping
| from zarr.abc.store_adapter import StoreAdapter | ||
| from zarr.storage._local import LocalStore | ||
| from zarr.storage._logging import LoggingStore | ||
| from zarr.storage._memory import MemoryStore | ||
| from zarr.storage._zep8 import URLStoreResolver | ||
| from zarr.storage._zip import ZipStore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth considering makign these lazy, or not a big enough gain?
| >>> is_zep8_url("s3://bucket/data.zarr") | ||
| False | ||
| >>> is_zep8_url("file:///data.zarr") | ||
| False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really follow these returning false. seems like a downgrade in functionality, also having looked a the spec a bit I don't see where this is explicitly disallowed (though very possible i misread or misunderstood)
| if not is_zep8_url(url): | ||
| # Check if it's a simple scheme URL that we can handle | ||
| if "://" in url or ((":" in url) and not url.startswith("/")): | ||
| # Parse as a single segment URL - the parser should handle this | ||
| try: | ||
| segments = self.parser.parse(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I think this answers my question above. Since these are valid urls I think ideally is_zep8_url would handle these simple cases correctly.
| except Exception: | ||
| raise ValueError(f"Not a valid URL: {url}") from None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the expected exception's we are catching here. As written this might silently quash an exception that is a real bug.
I think this whole section can be consolidated and simplified a bit, especially as we don't actually do anyhthing differently here then in the else branch that calls the same parse
| for i, segment in enumerate(segments): | ||
| if segment.adapter in ("zarr2", "zarr3"): | ||
| # Skip zarr format segments - they don't create stores | ||
| # TODO: these should propagate to the open call somehow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling out this TODO as importnat before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this is crucial functionality:
import zarr
arr = zarr.open_array("memory:|zarr2:", mode='w', shape=(5,), dtype='i4')
print(f"Array: {arr}")
print(f"Format version: {arr.metadata.zarr_format}")Array: <Array memory://4411020480 shape=(5,) dtype=int32>
Format version: 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accidentally submitted too early!
Ack! Hit submit instead of a different button! Below is still being filled out! I will comment when properly done.
Here are some edge cases I ran into:
Throws a FileNotFoundError:
Not sure what it should do, but I think this needs a Zep8 error
import zarr
import numpy as np
import tempfile
from pathlib import Path
tmpdir = Path(tempfile.mkdtemp())
zip_path = tmpdir / "mydata.zip"
arr_write = zarr.open_array(
f"file:{zip_path}|zip:",
mode='w',
shape=(10, 10),
chunks=(5, 5),
dtype='f4'
)
arr_write[:, :] = np.random.random((10, 10))
# throws FileNotFoundError
arr_wrong = zarr.open_array(
f"file:{zip_path}", # Missing |zip:
mode='r'
)Accidentally missing an adapter.
I think an absolutely killer feature here is to have automatic suggestions for how to correct a URL (like the python repr does for syntax errors)
e.g.
arr_wrong3 = zarr.open_array(
f"file:{zip_path}|log:", # Missing |zip:
mode='r'
)
print(f"Got data: {arr_wrong3[:]}")gives
ArrayNotFoundError: No array found in store logging-file:///var/folders/tc/fkgp35zn7z913f9cmsxcl6pc0000gn/T/tmpxp7e5hc3/logged_data.zip at path
I would love to have a: "did you forget a |zip: as part of that error message
Nested Zips/nested adapters
I tried to replicate a URL from the spec locally of nested zip files. It simplifies to this:
import zarr, zipfile, tempfile
from pathlib import Path
# Create inner.zip with data
tmpdir = Path(tempfile.mkdtemp())
inner = tmpdir / "inner.zip"
arr = zarr.open_array(f"file:{inner}|zip:", mode='w', shape=(3,), dtype='i4')
arr[:] = [1, 2, 3]
del arr # Close
# Put inner.zip inside outer.zip
outer = tmpdir / "outer.zip"
with zipfile.ZipFile(outer, 'w') as zf:
zf.write(inner, "inner.zip")
# Try nested read - THIS FAILS
url = f"file:{outer}|zip:inner.zip|zip:"
arr_read = zarr.open_array(url, mode='r') # ❌ FileNotFoundErrorFileNotFoundError: [Errno 2] No such file or directory: '[/var/folders/tc/fkgp35zn7z913f9cmsxcl6pc0000gn/T/tmp9kf2z2ks/outer.zip](http://localhost:8888/var/folders/tc/fkgp35zn7z913f9cmsxcl6pc0000gn/T/tmp9kf2z2ks/outer.zip)|zip:inner.zip'
somewhere in the URL reconstruction we lose the zep8 ness and try to just open the whole string as a file
| Zarr supports URL-based storage following the ZEP 8 specification, which allows you to specify storage locations using URLs with chained adapters:: | ||
|
|
||
| >>> # Store data directly in a ZIP file using ZEP 8 URL syntax | ||
| >>> z = zarr.open_array("file:data/example-zep8.zip|zip", mode='w', shape=(50, 50), chunks=(10, 10), dtype="f4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably be adding a .zarr in here somewhere. so we aren't creating otherwise unopenable zip files
| """Validate the URL segment.""" | ||
| import re | ||
|
|
||
| from zarr.storage._zep8 import ZEP8URLError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to errors.py to avoid circular import shenanigans
| class URLParser: | ||
| """Parse ZEP 8 URL syntax into components.""" | ||
|
|
||
| def parse(self, url: str) -> list[URLSegment]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently failing to error on what ought to be invalid URLs with multiple schemes.
z = zarr.open_array("file:memory:example.zip|zip", mode='w', shape=(100, 100), chunks=(10, 10), dtype="i4")
executes and creates teh file: memory:example.zip
I think this is prohibited according to the rules here: https://zeps--48.org.readthedocs.build/en/48/draft/ZEP0008.html#absolute-url-syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also
z = zarr.open_array("memory:memory:", mode='w', shape=(100, 100), chunks=(10, 10), dtype="i4")
creates a standard zarr folder on disk named memory:memory:
| path = preceding_url[5:] # Remove 'file:' prefix | ||
| if not path: | ||
| path = "." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For security, implementation SHOULD place limits on where this scheme is permitted.
refering to .. which seems important here to not expose unlimited things in the filesystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really sure how to get into the business of defining what is safe here. But flagging for consideration
| # Only extract paths from adapter segments, not scheme segments | ||
| # Scheme segments (like file:, s3:, https:) contain paths to the resource, not zarr paths within it | ||
| # Special handling for icechunk: paths with metadata references | ||
| # Both old format "branch:main", "tag:v1.0", "snapshot:abc123" | ||
| # and new format "@branch.main", "@tag.v1.0", "@abc123def456" | ||
| if segment.adapter in ("icechunk", "ic"): | ||
| # Check old format: branch:main, tag:v1.0, snapshot:abc123 | ||
| if ":" in segment.path and segment.path.split(":")[0] in ( | ||
| "branch", | ||
| "tag", | ||
| "snapshot", | ||
| ): # pragma: no cover | ||
| continue # Skip icechunk metadata paths # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on here? what is the old vs new?
can this all just be removed an handled by icechunk?
| except Exception: # pragma: no cover | ||
| # If parsing fails, treat as regular path # pragma: no cover | ||
| pass # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not do this. Or at least be much more careful about what we pass through. e.g. my file:memory: example I think should fail even if it's techinically valid to have a colon in a file name, i think this implementation will just make that illegal for zarr users perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but we would need an escape hatch for whoever is already doing that :/
| for i, segment in enumerate(segments): | ||
| if segment.adapter in ("zarr2", "zarr3"): | ||
| # Skip zarr format segments - they don't create stores | ||
| # TODO: these should propagate to the open call somehow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed this is crucial functionality:
import zarr
arr = zarr.open_array("memory:|zarr2:", mode='w', shape=(5,), dtype='i4')
print(f"Array: {arr}")
print(f"Format version: {arr.metadata.zarr_format}")Array: <Array memory://4411020480 shape=(5,) dtype=int32>
Format version: 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hitting shift enter sending the review off is absolutely killing me. Sorry I keep opening this too early....
A weird one. Adding a log store via zep8 breaks some S3 permissions, but does not if you open with explicit stores
import zarr
from zarr.storage import FsspecStore, LoggingStore
storage_options = {'anon': True}
# WORKS!
s3_store = FsspecStore.from_url(
"s3://noaa-nwm-retro-v2-zarr-pds",
storage_options=storage_options
)
logging_store = LoggingStore(s3_store)
group = zarr.open_group(logging_store)
keys = list(group.keys())
print(f"Keys: {keys[:3]}")
# DIES WITH PERMISSION ERROR
url = "s3://noaa-nwm-retro-v2-zarr-pds|log:"
group = zarr.open_group(url, storage_options=storage_options)
keys = list(group.keys())
print(f"Keys: {keys[:3]}")i suspect that this is related the storage_options not being passed through correctly
| from zarr.storage._builtin_adapters import ( | ||
| FileSystemAdapter, | ||
| GSAdapter, | ||
| HttpsAdapter, | ||
| LoggingAdapter, | ||
| MemoryAdapter, | ||
| S3Adapter, | ||
| S3HttpAdapter, | ||
| S3HttpsAdapter, | ||
| ZipAdapter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add LatencyStore to this list with some sensible defaults it would be super helpful for testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've started making some changes on a branch off of this. fixing the issues i found.
One architectural change is that in this PR as stands URLs can either go directly into fsspec (as a special case) or go through zep8 to end up in fsspec. I think it makes more sense to route all URLs through zep8 machinery. And possibly will for local stores.
I also think i found a bug with the inteded usage of local:// bypassing fsspec stores in is_fsspec_uri the second part of the if condition could never be triggered
Although
A weird one. Adding a log store via zep8 breaks some S3 permissions, but does not if you open with explicit stores
import zarr
from zarr.storage import FsspecStore, LoggingStore
storage_options = {'anon': True}
# WORKS!
s3_store = FsspecStore.from_url(
"s3://noaa-nwm-retro-v2-zarr-pds",
storage_options=storage_options
)
logging_store = LoggingStore(s3_store)
group = zarr.open_group(logging_store)
keys = list(group.keys())
print(f"Keys: {keys[:3]}")
# DIES WITH PERMISSION ERROR
url = "s3://noaa-nwm-retro-v2-zarr-pds|log:"
group = zarr.open_group(url, storage_options=storage_options)
keys = list(group.keys())
print(f"Keys: {keys[:3]}")
| if storage_options: | ||
| store_kwargs.update(storage_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effectively unpacks the storage_options dict which I think is what breaks my example with S3+ log store.
needs somethign like:
| if storage_options: | |
| store_kwargs.update(storage_options) | |
| if storage_options: | |
| store_kwargs['storage_options'] = storage_options |
| def get_supported_schemes(cls) -> list[str]: | ||
| return ["s3", "s3+http", "s3+https"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the s3+ urls we aren't reaching here unfortunately:
zarr.open(
"s3+https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.5/idr0062A/6001240_labels.zarr",
storage_options={'anon': True}
)ValueError: Protocol not known: s3+https
i think this relates to the prior mentioned issues with is_zep8_url
This PR implements support for the ZEP 8 URL syntax in Zarr Python.
Some examples of what now works:
TODO:
docs/user-guide/*.rstchanges/closes #2943
fixes #2831
xref: zarr-developers/zeps#48
cc @jbms