Expose LatencyStore as public API?

[TomNicholas]

I just learned (from @jhamman) that I don't need to keep writing my own LatencyStore class for benchmarking, because zarr-python already ships with a LatencyStore implementation!

zarr-python/src/zarr/testing/store.py

Line 530 in 926a52f

class LatencyStore(WrapperStore[Store]):

But unlike LoggingStore and WrapperStore, LatencyStore is not exposed or documented publicly. I propose making it public.

[TomNicholas]

Also the other inconsistency is that LatencyStore is defined in zarr.testing.storage whereas WrapperStore and LoggingStore are defined in zarr.storage. Either all the testing-related stores should be in zarr.testing or none of them should be.

[d-v-b]

Also the other inconsistency is that LatencyStore is defined in zarr.testing.storage whereas WrapperStore and LoggingStore are defined in zarr.storage. Either all the testing-related stores should be in zarr.testing or none of them should be.

I don't think this holds -- some stores for testing are designed to be public, and so those are defined in zarr.storage. LatencyStore was designed for internal use, which is why it's not currently public.

That being said, I don't have a problem with making LatencyStore public, i.e. moving it to zarr.testing.

[TomNicholas]

LatencyStore was designed for internal use, which is why it's not currently public.

I don't really see what the difference between LatencyStore and LoggingStore is in that respect. Aren't they both only useful for testing?

[d-v-b]

when I say "designed for internal use", I mean this in the historical sense: I designed LatencyStore to test specific functionality in this library. It might be useful in other libraries, but that's not what it was designed for. However I suspect @jhamman had another library in mind when he wrote LoggingStore, which is why it's public today.

As long as LatencyStore is private, we can change it whenever we want. If we make it public, we lose that freedom. But maybe that's fine.

[d-v-b]

as for zarr.storage vs zarr.testing.store, I can see the appeal of putting all the public Store subclasses in one place, and as you note that should probably be zarr.storage.

[TomNicholas]

I suspect @jhamman had another library in mind when he wrote LoggingStore

I totally independently also had the idea for a LatencyStore for use in testing xarray. I only realised today that a LatencyStore implementation already existed this whole time. LatencyStore is so simple that my version and your version are not meaningfully different. So both LoggingStore and LatencyStore are useful both inside zarr-python and outside of it, and it would be useful to have both be public.

As long as LatencyStore is private, we can change it whenever we want. If we make it public, we lose that freedom. But maybe that's fine.

What changes might we want to make to it?

[d-v-b]

What changes might we want to make to it?

The latency store could be more realistic if, instead of a fixed latency associated with get and set requests, we used a distribution of latencies, or even accepted a user-provided function for generating a latency. This would more accurately model storage backends with rate-limiting behavior.

[d-v-b]

and this is the crux of my point: the latency store was designed to test stuff inside zarr python, and for that purpose, a fixed latency was fine. If we make it public, then we might find that people want new features that make it useful for stuff outside its original scope, and that becomes a maintenance burden.

This is a perfectly fine outcome, but it's also more work and more responsibility for us.

[d-v-b]

The latency store could be more realistic if, instead of a fixed latency associated with get and set requests, we used a distribution of latencies, or even accepted a user-provided function for generating a latency. This would more accurately model storage backends with rate-limiting behavior.

BTW we could add this functionality by widening the type of the get_latency and set_latency fields to include stuff like specifications of distributions

[TomNicholas]

we might find that people want new features

We can always tell them no at that point.

widening the type of the get_latency and set_latency fields to include stuff like specifications of distributions

I think that might be useful but we should apply the YAGNI principle.

My my complaint is that the LatencyStore exists but is not discoverable, and it's easy to make it discoverable.

If you're okay with adding it to zarr.Storage then I can make a PR.

[d-v-b]

If you're okay with adding it to zarr.Storage then I can make a PR.

that would be great!

[maxrjones]

I could see people using WrappingStore or LoggingStore in real applications since they should not add much overhead. By contrast, my impression is that LatencyStore is purely useful for testing. I would be more convinced about moving this store if you could provide an example of when it would be used outside of strict testing use-cases.

[TomNicholas]

I'm not saying it's useful outside of testing - I'm saying it is useful for testing and should be exposed for use in other libraries' tests.