Getitems: support meta_array

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.

[madsbk]

BaseStore now implements getitems(), which takes a meta_array argument. Besides simplifying the code, this enable stores to read directly to GPU memory: rapidsai/kvikio#131

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/tutorial.rst
• Changes documented in docs/release.rst
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[lgtm-com]

This pull request introduces 1 alert when merging 26b7d2d into b677db3 - view on LGTM.com

new alerts:

• 1 for Signature mismatch in overriding method

[codecov]

Codecov Report

Merging #1131 (a1d3520) into main (4b0705c) will not change coverage.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1131 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 36 37 +1 Lines 14799 14855 +56 =========================================
+ Hits 14799 14855 +56 

Impacted Files	Coverage Δ
zarr/_storage/store.py	100.00% <100.00%> (ø)
zarr/context.py	100.00% <100.00%> (ø)
zarr/core.py	100.00% <100.00%> (ø)
zarr/storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage.py	100.00% <100.00%> (ø)
zarr/tests/test_storage_v3.py	100.00% <100.00%> (ø)
zarr/tests/test_util.py	100.00% <100.00%> (ø)
zarr/tests/util.py	100.00% <100.00%> (ø)
zarr/util.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

[quasiben]

Is this the last PR needed by kvik for full Zarr support ?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

[jakirkham]

Thanks Mads! This is very helpful 🙏

Asked one question above about API design. Curious to hear yours and others thoughts 🙂

[jakirkham]

Know fsspec-based storage layers are using a dict return value, but am wondering if we should be doing something different (like returning an iterable). Asking since this would make the read blocking vs. a bit more lazy. The latter can be useful when working with operations that take a bit longer (like reading from the cloud or parallelizing several reads)

cc @martindurant (who also may have thoughts)

[martindurant]

The point of it being blocking, is that many keys may be being fetched concurrently. If you make it an iterator, you lose that, unless you have something that can wait on an async iterator, which in turn has first-completed working.

[madsbk]

I agree with both of you.
The API specifies a return type of Mapping[str, Any] thus it is possible to return a lazy mapping that only reads self[k] when accessed. But let's do that in a follow up PR?

[martindurant]

Understood, and will be interested in how that looks.

By the way, I mentioned elsewhere the possibility of passing key-specific metadata to the get function; that would happen in this same signature. I wonder if you have any use for an array where only some of it is destined for the GPU (perhaps because that data already exists there and doesn't need loading at all).

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[jakirkham]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

[madsbk]

As noted above, think this discussion gets hairy enough it should be broken out into an issue (likely two) and discussed separately.

If we want to go the contexts route, we don't need a meta_array argument

[joshmoore]

Does this also imply that getitem() should have a contexts parameter?

[madsbk]

Does this also imply that getitem() should have a contexts parameter?

Good point, yes getitem() should also take a contexts parameter

[madsbk]

Wait a second, there is no getitem() and _chunk_getitem() anymore :)

[jakirkham]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

[madsbk]

Is this the last PR needed by kvik for full Zarr support ?

Yes with this PR and rapidsai/kvikio#131, all building blocks should be in place to use Zarr backed by CuPy arrays!

Just to clarify, this is a quite useful optimization (and much appreciated!). Though is it a requirement (in that an error was hit somewhere)? Is there a bit more context about how this came up (particularly if an issue was encountered)?

It is required in the sense that a backend doesn’t know the memory destination of a read without this PR. In KvikIO’s backend, we must rely on key names to infer if the data goes to host or device memory. E.g. the following code check for keys that should go to host memory always:

ifos.path.basename(fn) in [
zarr.storage.array_meta_key,
zarr.storage.group_meta_key,
zarr.storage.attrs_key,
]:

This is a problem since a Store can define its own special keys with arbitrary names.

[jakirkham]

Thanks for clarifying. Do we need setitems as well?

[madsbk]

Thanks for clarifying. Do we need setitems as well?

I don't think it is as important as the getitems since a backend can always probe the input data and determine if it is host or device memory.
However, setitems would make it possible to implement concurrent write, which could benefit backends such as KvikIO significantly.

[jakirkham]

@joshmoore do you have thoughts on this one? 🙂

[joshmoore]

Some minor comments, @jakirkham. I do get the feeling that after the refactoring that enabled V3, we likely do need some storage-extension-specific documentation notes.

[joshmoore]

Do you have an example of what they would look like, @martindurant?

[joshmoore]

Is this so private that no one could have made use of it in a subclass?

[madsbk]

Yes, AFAICT.

[jakirkham]

Yeah that's my understanding as well. Or at least by prefixing with _ we have warned users this is an implementation detail (subject to change) that they shouldn't rely on.

[joshmoore]

@jakirkham: this pre-dates me, so I defer. But if it's not documented somewhere we might want to review if that holds across the board. For me, _x is typically valid for subclassing by developers, otherwise it would be a __x. (i.e. public, protected, private in Java-parlance)

[jakirkham]

It's a Python thing generally (not specific to Zarr).

[martindurant]

Single underscore if by far the most common thing to do, to show intent rather than enforce any privateness (which double underscore doesn't do either).

[joshmoore]

@jakirkham: see also though https://stackoverflow.com/questions/41761645/when-to-use-one-or-two-underscore-in-python/41761857#comment99492184_41762806

[joshmoore]

❤️ for removing hasattr hacks in general. 👍

[madsbk]

Resolved conflict.
I suggest we go with this approach for now and then unify get_partial_values() and getitems() later.

[rabernat]

@joshmoore & @jakirkham - any objections to merging this as-is and deferring the broader discussions for future work?

[joshmoore]

Thanks for the ping, @rabernat. I have to say I'm liking the typed Context better. I will read through again ASAP.

[martindurant]

Notice the link to FITS reading: another example of per-chunk parameters that could be fixed by passing around a context, although it's not the only way to do it. I'd say that there are a number of efforts (some more and some less well developed) waiting to see what zarr will do.

[madsbk]

@joshmoore, how can I help progress here? Maybe an online meeting?

[joshmoore]

Sorry, @madsbk. My last comment was shortly before leaving on several weeks of travel and I lost of track of this. It wasn’t my intention to hold you up, and big 👍 for the ping. A few thoughts (from smallest to biggest) though I get the feeling that you are being ping-ponged back and forth between design issues:

• If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.
• Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.
• Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

[madsbk]

If we're less than confident on this API, perhaps we should make this a keyword-only argument. That way the second position could potentially be used for something else and deprecation/renaming would be more straight-forward.

Agree, fixed.

Unlike some of the context that was going to get passed, I have the feeling that the meta_array is more likely to be used for all chunks equally. Does that sound right? If so, I'm a bit hesitant to force someone to create dictionaries of N objects all with the same meta_array instance wrapped in new Context object. As I started to think about "wildcard" keys in the context argument or an additional default_context argument, I realized that perhaps we've not gotten this abstract right yet.

I have added ConstantMap, which is a read-only map that maps all keys to the same constant value. This fixes the issue of having to create a Context for each key without complicating the semantic of getitems().

Finally, I've still not got a clear picture of what other fields on the Context object are going to help with the likes of partial read. Obviously, that's less to do your PR but just to callback to the original discussions about whether or not we knew what the API should look like.

I don't think any of us have a clear answer to this. Therefore, I suggest that we merge this PR and let people explore its possibilities. As long as Context is optional and we force Stores to implement a fallback if it doesn't exist, I think we are fine.

[madsbk]

Thanks all!

[rabernat]

Thank you for your patience Mads.