Conversation
10cb73b to
3aa63ac
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4192 +/- ##
==========================================
+ Coverage 94.22% 94.27% +0.04%
==========================================
Files 92 94 +2
Lines 12942 13177 +235
==========================================
+ Hits 12195 12423 +228
- Misses 747 754 +7
🚀 New features to boost your workflow:
|
effd07c to
b0e7af9
Compare
b0e7af9 to
c744f95
Compare
| def test_round_trip() -> None: | ||
| url = "s3://bucket/a.zip?v=2|zip:b/inner.zip|zip:c|zarr3:" | ||
| segments = parse_pipeline(url) | ||
| assert "|".join(s.raw for s in segments) == url |
There was a problem hiding this comment.
can we get property-based tests that create valid pipelines up to depth 8 or so, sampled from all valid root and adapter schemes, and ensure that these pipelines comply with the invariants tested for a few examples here? And if there's a convenient way to generate invalid pipelines, that would also be nice for property testing
787e785 to
6e6c2f5
Compare
|
This is great! Here's some stuff I found with Claude: 🤖 AI text below 🤖 Review of c744f95. The parser and adapter ABC look solid (the spec corpus passes, ruff/mypy are clean, no import cycles, test order doesn't leak registry state), but the store hooks introduce some regressions and there are a few contract questions worth settling before the Correctness / regressions
Spec conformance at the root
Design / API surface
Smaller things
|
|
a few more findings, this time from codex: 🤖 AI text below 🤖 A few additional findings on
For reference, the updated focused suite passes locally: |
6e6c2f5 to
d29f495
Compare
Implements URL pipeline support (https://github.com/jbms/url-pipeline): '|'-chained URLs resolve through pluggable adapters registered under the 'zarr.url_adapters' entry-point group (entry-point name = URL scheme). - zarr.abc.url_pipeline: PipelineSegment, AdapterResolution, PipelineContext, URLPipelineAdapter (single-classmethod contract) - zarr.storage._url_pipeline: parse_pipeline / resolve_pipeline; the root sub-URL delegates to make_store so existing file/memory/fsspec routing is unchanged - registry: register_url_adapter / get_url_adapter / list_url_adapter_schemes (name check only; no adapter imports) - make_store/make_store_path route strings containing '|' (or a registered root scheme) through the resolver; residual store paths combine with the user-supplied path - StorePath gains a zarr_format attribute (populated by format segments in a follow-up) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d29f495 to
20f2ac8
Compare
…e-core # Conflicts: # src/zarr/errors.py # src/zarr/registry.py
…thoring - make_store validates the access mode before routing a URL pipeline string to adapters, matching every other StoreLike branch - make_store docstring lists URL pipeline strings; StorePath.open documents the invalid-mode ValueError - user guide: StoreLike bullet for pipeline strings and an adapter authoring section in extending.md (runnable example) - changelog: drop PR-relative wording, name the public parser entry points Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
get_url_adapter took a non-reentrant lock across entry_point.load(), so an adapter module that resolved another scheme at import time deadlocked the process. Matching entry points are now taken off the pending list under the lock and imported outside it; import failures are wrapped in URLPipelineError and leave the entry point discoverable for a retry. A same-named entry point is discarded with a ZarrUserWarning when the scheme is already registered (e.g. by a builtin), instead of staying pending forever; duplicate entry-point names warn and the first wins. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ent contracts - The resolver closes the store an adapter returned when it cannot be made read-only for mode 'r', instead of leaking it. - resolve_pipeline documents every error type it lets through; the resolve_preceding docstring states the local-file-root limitation. - AdapterResolution and PipelineContext compare and hash by identity, since a Store is not hashable and frozen=True implied otherwise. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… caller The zarr2:/zarr3: format carried on StorePath.zarr_format was merged only in zarr.api.asynchronous; create_array, from_array, Group.from_store, Group.open, Array.open and the deprecated AsyncArray._create dropped it and swallowed explicit conflicts. The merge now lives on StorePath.resolve_zarr_format and is applied at each site. To let the pipeline's format apply when the caller does not specify one, create_array (sync and async) and Group.from_store default zarr_format to None, resolving to the configured default (3) as before; Array.open gains a zarr_format parameter mirroring Group.open. Tests: registry deadlock/race/import-failure/shadowing, resolver leak, zarr_format merge at each core site, and pins for the documented memory:/ file: divergences and the local-file-root limitation (xfail, strict). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- The extending guide's wrapper example dropped the preceding residual path while its comment claimed the opposite; it now joins the two and a tested snippet shows root|a|b resolving to a/b. - Changelog and storage guide state the memory:// routing split with fsspec, the undecoded percent-escapes in file: roots, the local-file-root limitation, the entry-point collision warnings, and the zarr_format default changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
This PR delivers the core infrastructure for pluggable URL pipeline parsing.
Implements URL pipeline support (https://github.com/jbms/url-pipeline):
|-chained URLs are resolved through pluggable adapters registered under thezarr.url_adaptersentry-point group (entry-point name = URL scheme).zarr.abc.url_pipeline:PipelineSegment,AdapterResolution(store, residual path,zarr_format),PipelineContext(withresolve_preceding(mode=..., storage_options=...)for wrapper adapters andpreceding_urlfor native ones),URLPipelineAdapter(single-classmethod contract)zarr.storage.parse_pipeline/zarr.storage.resolve_pipeline(public): the parser is a permissive segment splitter;memory:andfile:roots are resolved with the spec's semantics, every other root delegates to the existingmake_storeroutingregistry:register_url_adapter(warns on collision) /get_url_adapter(per-scheme lazy loading, thread-safe, case-insensitive) /list_url_adapter_schemes(name check only; no adapter imports)make_store/make_store_pathroute strings containing|(or a registered root scheme) through the resolver; residual store paths combine with the user-supplied path;mode="r"is enforced on whatever the adapter returnsStorePathgains azarr_formatattribute, populated by format segments and merged with the caller'szarr_formatin theopen*/create*/save*APIs (explicit conflicts raise)"a"on a read-only store now serves the "open" half for all stores (moved intoStorePath.open), instead of raising upfrontBehavior notes are in
changes/4192.feature.md. Builtin adapters (zip:,zarr2:/zarr3:) follow in separate PRs.For reviewers
This is PR 2 in a series towards #2943.
The review findings posted on 2026-08-18 (both batches) have been addressed in the current head; the notable design outcomes are:
|is reserved in every string store spec (documented;pathlib.Pathis the escape hatch);?/#are not split on schemeless local rootszip::...) and zarr's nativefile:/memory:schemes are never dispatched to a root adapterstorage_optionskeys before the root sees themAdapterResolutioncarrieszarr_format, and the documented wrapper idiom isdataclasses.replace(preceding, ...)so future fields are never dropped; nested wrappers preserve residual paths (tested)Merged with
mainon 2026-09-16 (conflicts were limited toerrors.py/registry.py__all__and import lines).A final independent review pass (2026-09-16) found and this PR fixes: a deadlock when an adapter module resolved another scheme at import time (entry points are now imported outside the registry lock); the pipeline-selected
zarr_formatbeing dropped bycreate_array/from_array/Group.from_store/Group.open/Array.open(now merged viaStorePath.resolve_zarr_formatat every site;create_arrayandGroup.from_storedefaultzarr_formattoNone,Array.opengains the parameter); a store leak on the read-only-conversion failure path; silent shadowing of same-named entry points; and the extending-guide example dropping the preceding residual path. Documented rather than changed: thememory://routing split with fsspec, undecoded percent-escapes infile:roots, and the local-file-root limitation for wrapper adapters (pinned by a strict xfail).Author attestation
TODO
docs/user-guide/*.mdchanges/