Skip to content

feat!: store-defined concurrency limits - #3547

Open
d-v-b wants to merge 33 commits into
zarr-developers:mainfrom
d-v-b:feat/global-concurrency-limit
Open

d-v-b wants to merge 33 commits into
zarr-developers:mainfrom
d-v-b:feat/global-concurrency-limit

Conversation

@d-v-b

@d-v-b d-v-b commented Oct 24, 2025 •

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Removes Zarr's global async.concurrency limit and concurrent_map helper, using asyncio.gather for concurrent work. Concurrency control becomes the responsibility of backends and any store adapters that need it.

The current implementation does not contain the previously proposed ConcurrencyLimiter mixin or per-store limit of 50. It also does not warn when setting the removed async.concurrency key. Existing custom stores that relied on Zarr's limit can now receive more concurrent work and should be assessed accordingly.

Some recorded benchmarks improved, but performance is workload-dependent. The current CodSpeed comment reports both improvements and regressions and uses an older successful main baseline, so it does not establish a universal performance improvement or that this change is safe for every backend.

@github-actions github-actions Bot added the needs release notes Automatically applied to PRs which haven't added release notes label Oct 24, 2025
@github-actions github-actions Bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Oct 27, 2025
@dcherian

Copy link
Copy Markdown
Contributor

Nice, the other one that bugs me when profiling is

return await asyncio.to_thread(

@maxrjones

Copy link
Copy Markdown
Member

@d-v-b do you have thoughts on how downstream libraries would interact with Zarr's concurrency limit? Would those set there own concurrency approaches (e.g., global or per function) or could there be a way to share the limit with Zarr?

@d-v-b

d-v-b commented Dec 22, 2025

Copy link
Copy Markdown
Contributor Author

@maxrjones this PR is moving away from a global concurrency limit because it's actually very hard to implement a top-down concurrency control in our codebase. the new direction I'm taking with this PR is for each store to set its own concurrency limit. for stateless stores like local and remote (basically anything except zip), concurrency-sensitive applications should be able to cheaply create store instances with concurrency limits tuned to the needs of the application.

@d-v-b d-v-b changed the title feat/global concurrency limit feat!: store-defined concurrency limits Feb 20, 2026
@d-v-b

d-v-b commented Feb 20, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

Audit clarification: This describes an earlier revision. At bd836f3 the ConcurrencyLimiter mixin and per-store defaults are absent; backends/adapters must supply any limit.

Original comment (historical):

this is ready for a look. The basic idea is that we make concurrency limits an implementation detail of a particular store and thereby remove all concurrency limiting logic from array / group routines. Some stores will have no concurrency limits (e.g., memory and local storage) and so their performance will go up. Stores that actually benefit from a concurrency limit (remote stuff) has the default limit of 10 but that's something we can play with.

To cut down on boilerplate we (claude and I) define a ConcurrencyLimiter base class that encapsulates the concurrency limiting logic. This means taking a concurrency limit (int | none) and creating a semaphore (or a nullcontext if there's no limit). We also define a decorator that an be used to wrap a should-be-concurrency-limited method on a class that inherits from the ConcurrencyLimiter base class.

@d-v-b
d-v-b requested a review from jhamman February 20, 2026 12:44

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmark Code will be benchmarked in a CI job.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants