Feat/concurrent members

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!

[d-v-b]

Makes AsyncGroup.members() fetch keys and fetch metadata concurrently, which provides a big performance win for high-latency storage backends. The number of concurrent operations is limited by the zarr-wide configuration setting.

in main, Group.members() requires ~O(num_members) time to complete, because it does not perform IO concurrently. In this PR, Group.members runs in constant time (until the number of concurrent requests exceeds the concurrency limit).

[d-v-b]

note: this PR depends on #2474

[TomAugspurger]

It's technically possible for KeyError.args to be empty, so this [0] would raise an IndexError:

IndexErrorTraceback (mostrecentcalllast)
CellIn[10], line1---->1KeyError().args[0]

If we're comfortable assuming / requiring that things raising a KeyError here will populate that with the key, then I think this is fine to ignore. Otherwise, we might want to catch that IndexError.

[d-v-b]

I'm pretty sure that args will always be populated, because we are handling exceptions coming downstream of a function that necessarily takes a concrete key to query from storage. For that reason, even if I did add code to handle the case when e.args is not populated, I have no idea how we would test this case with our current Group api, since there's no way that I know of to reach this line without some concrete keys.

[TomAugspurger]

Style nitpick: I like contextlib.nullcontext for cases where you may or may not have a context manager: https://docs.python.org/3/library/contextlib.html#contextlib.nullcontext

3.10 added support for asynchronous context managers, so this should be usable here:

semaphore=semaphoreorcontextlib.nullcontext()
asyncwithsemaphore:
....

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

[d-v-b]

I tried contextlib.nullcontext but I didn't feel like the LOC saved was worth the added indirection in this case. I assume that contextlib.nullcontext doesn't add notable performance overhead, by contrast, I find the current if... else construction easier to reason about, and it requires 1 fewer import.

And I wonder whether getitem_semaphore should be the one to look up async.concurrency from the zarr config? Maybe we want that to be the default so that we don't miss it anywhere (and have some other way to indicate unbounded concurrency?)

As I understand it, we only query the concurrency limit when the semaphore is created, which is necessarily before getitem_semaphore gets called. All invocations of getitem_semaphore have to use the same Semaphore instance, because otherwise there would be no coordination mechanism for rate-limiting. So I think that means getitem_semaphore shouldn't know anything about the config, or even concurrency limits.

[d-v-b]

@dstansby how can I re-rerun the docs build? and do you have any idea why it failed?

[dstansby]

Do you have a readtheodcs account? If you send me the email or username for that, I can add you as a maintainer to the project on readthedocs.org. Then you'll get a "Rebuild this build" button:

I'm not sure why this one failed... I will restart!

[d-v-b]

thanks for fixing it @dstansby!