Skip to content

fix: explicit store sync-IO capability; wrappers forward it; LatencyStore fixes - #4206

Open
d-v-b wants to merge 1 commit into
zarr-developers:mainfrom
d-v-b:fix/store-sync-capability-wrappers
Open

fix: explicit store sync-IO capability; wrappers forward it; LatencyStore fixes#4206
d-v-b wants to merge 1 commit into
zarr-developers:mainfrom
d-v-b:fix/store-sync-capability-wrappers

Conversation

@d-v-b

@d-v-b d-v-b commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

The first version of this PR had the following summary:

This PR adds a new supports_sync_io property to the base Store ABC. We need this expressed as a public property in order to allow stores that wrap other stores -- like the latency store or wrapper store -- to correctly advertise that they support synchronous execution. Otherwise, these meta-stores fail to leverage performance advantages gated on the store supporting synchronous execution.

I really dislike this! The basic design here is deeply flawed, and we will fix it correctly by defining stores that either do async, or sync, but not both. However, as long as our public MemoryStore and LocalStore classes are obligatorily async, we don't have much of a choice if we want to offer performance speedups through sync code.

This PR also makes the following fixes:

  • ensures that WrapperStore forwards sync methods + the capability to the wrapped store
  • ensures that LatencyStore sync reads / writes, get_ranges and get_partial_values actually see the configured latency
  • ensures that derived stores keep the (loc, scale) distribution of latencies instead a single frozen sample

The changes in this PR have been amended such that it is better described by this summary:

  • the fused codec pipeline checks a private helper called _store_supports_sync_io that combines Isinstance(SupportsSyncStore) with a _supports_sync_io attribute check that WrapperStore can forward from the store it wraps. This actually matches the same pattern we use for bridging sync / async codecs in the sharding codec, which hopefully makes it easier to move away from this pattern when we formally split async and sync paths.

This PR also makes the following fixes:

  • ensures that WrapperStore forwards sync methods + the capability to the wrapped store
  • ensures that LatencyStore sync reads / writes, get_ranges and get_partial_values actually see the configured latency
  • ensures that derived stores keep the (loc, scale) distribution of latencies instead a single frozen sample

For reviewers

Please check if this new public API on the Store class is correctly scoped.

Based on a claude-authored PR: d-v-b#257

Author attestation

  • I am a human, these are my changes, and I have reviewed and understood every change and can explain why each is correct.

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/user-guide/*.md
  • Changes documented as a new file in changes/
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@d-v-b
d-v-b force-pushed the fix/store-sync-capability-wrappers branch from 186c392 to 1d123cf Compare July 29, 2026 14:32
@d-v-b
d-v-b marked this pull request as ready for review July 29, 2026 14:38
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.03922% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.95%. Comparing base (a88f889) to head (029e05a).

Files with missing lines Patch % Lines
src/zarr/testing/store.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4206      +/-   ##
==========================================
+ Coverage   93.91%   93.95%   +0.03%     
==========================================
  Files          91       91              
  Lines       12723    12768      +45     
==========================================
+ Hits        11949    11996      +47     
+ Misses        774      772       -2     
Files with missing lines Coverage Δ
src/zarr/abc/store.py 95.40% <100.00%> (+0.04%) ⬆️
src/zarr/codecs/sharding.py 96.17% <100.00%> (ø)
src/zarr/core/codec_pipeline.py 96.20% <100.00%> (ø)
src/zarr/storage/_logging.py 100.00% <100.00%> (ø)
src/zarr/storage/_wrapper.py 98.11% <100.00%> (-0.79%) ⬇️
src/zarr/testing/store.py 98.33% <95.00%> (-0.18%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
d-v-b requested a review from maxrjones July 29, 2026 14:46
@maxrjones

Copy link
Copy Markdown
Member

Is it an option to not yet support store composition with FusedCodecPipeline until we have better architected sync/async split between stores? I'm wondering if we can get this performance improvement out for the dominant usage pattern (a non-wrapped Local / Memory store), and save the remaining use cases for when we can implement it without added tech debt.

@d-v-b

d-v-b commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

I'm wondering if we can get this performance improvement out for the dominant usage pattern (a non-wrapped Local / Memory store), and save the remaining use cases for when we can implement it without added tech debt.

It's a bit tricky because our wrapper stores like the latencystore and logging chiefly exist to make testing other zarr components easier. So if we prevented them from working with the fused codec pipeline, we would be denying ourselves a powerful tool for debugging the fused codec pipeline.

@d-v-b

d-v-b commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

@maxrjones I'm open to other solutions here. for example, we could mint totally new SyncLatencyStore and SyncWrapperStore classes that only wrap sync store methods. If we kept these classes private for now, we could use them internally for perf stuff without public churn. Does that look more palatable?

@d-v-b

d-v-b commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

A third alternative, and perhaps the simplest: we just make this attribute private. I'm going to move in that direction for now.

@d-v-b

d-v-b commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

The recent changes no longer introduce new public API. instead, the fused codec pipeline checks a private helper called _store_supports_sync_io that combines Isinstance(SupportsSyncStore) with a _supports_sync_io attribute check that WrapperStore can forward from the store it wraps. This actually matches the same pattern we use for bridging sync / async codecs in the sharding codec, which hopefully makes it easier to move away from this pattern when we formally split async and sync paths.

I will update the PR description accordingly.

@d-v-b
d-v-b force-pushed the fix/store-sync-capability-wrappers branch from c3dfafc to 0227efd Compare July 30, 2026 07:25
…rs forward it

The fused write gate checked only SupportsSetSync, but write_sync also
needs get_sync (partial-chunk read-modify-write) and delete_sync
(all-fill chunk cleanup): a set-sync-only store passed the gate, wrote
some chunks, then died mid-batch with TypeError. And WrapperStore
forwarded no *_sync method, so every wrapped store (e.g. LatencyStore)
silently lost the sync fast path — latency benchmarks measured the async
fallback while claiming to measure the fused sync path.

Both gates now consult _store_supports_sync_io: structural membership in
SupportsSyncStore (the full get/set/delete sync surface) combined with a
per-instance _supports_sync_io opt-out (absent means capable). This is a
private, interim convention pending a formal sync/async store
architecture — the store-side twin of the codec-side _sync_capable
convention from zarr-developers#4179 — deliberately not new public API. WrapperStore
delegates the three sync methods and forwards the wrapped store's
capability, so wrapping a sync store keeps the fast path and wrapping an
async-only store falls back cleanly; LoggingStore logs the delegated
sync calls.

LatencyStore fixes: sync reads/writes now sleep the configured latency
on the worker thread; get_ranges/get_partial_values route through the
latency-injecting get instead of bypassing the wrapper; _with_store
passes the raw (loc, scale) latency config instead of a single sampled
float, so derived stores keep the distribution.

Assisted-by: ClaudeCode:claude-fable-5
@d-v-b
d-v-b force-pushed the fix/store-sync-capability-wrappers branch from 0227efd to 029e05a Compare July 30, 2026 07:30
Sign up for free to 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.

2 participants