Allow mode casting for Stores

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix

[TomAugspurger]

Following the discussion in #2186 (comment), this is a POC for opening "reopening" a store with a new mode.

The semantics are that reopening a store with a new mode should return a new Store that's backed by the same underlying data.

AFAICT, our current stores use / enforce mode "logically". There's nothing about the actual underlying store that's readonly when mode="r". You could imagine some cases where it's not possible to cast between modes (e.g. you have a readonly database connection). I think it's fine for those Stores to handle that.

By default, the method is abstract meaning this is a new bit of code required for stores to implement. We could also raise NotImplementedError by default.

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)

[TomAugspurger]

This probably needs some work... would we want the new Store to share the same Lock?

[TomAugspurger]

This wasn't straightforward to implement, because of a need to close the old ZipStore (and writing the ending records) before opening the new store.

For now I've opted to raise NotImplementedError for ZipStore.

[TomAugspurger]

The GPU test failed. I think that's because at

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

Lines 170 to 171 in 692593b

	ifstore_dict:
	self._store_dict= {k: gpu.Buffer.from_buffer(store_dict[k]) forkiniter(store_dict)}

we create a new dictionary where, so that store.with_mode('r') no longer views the same dict as the first store.

@akshaysubr I suspect that those lines are there to ensure that all of the values in the dictionary are GPU buffers. What do you think about documenting that as a requirement to use the store (and maybe narrowing the type of the dict values in __init__ to a gpu.Buffer)?

[jhamman]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

[d-v-b]

Thanks @TomAugspurger -- a few suggestions but this is great.

cc @d-v-b who was also looking at this problem today.

second this, once those suggestions are integrated then I think this will be a great addition.

[TomAugspurger]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

[jhamman]

It's tangentially related, but I wanted to confirm that people are OK with changing the GpuMemoryStore.__init__ method to not automatically copy the data from an arbitrary Buffer into a gpu.Buffer. I added a from_dict method for people who want that, but I imagine that the common case is a user who knows where the data is and doesn't want to pay the cost of walking that dictionary and potentially moving data in the __init__.

cc @akshaysubr

[jhamman]

This looks good to me @TomAugspurger - some failing tests in GPU land but otherwise looks good to me.

[jhamman]

I like this API!

[jhamman]

pre-commit.ci autofix