Error on invalid store mode #3068
Conversation
|
this looks good to me, curious to hear your thoughts @jhamman since you are the mode architect |
|
Would it be possible instead to provide a UserWarning and return a read-only copy of the store? I'm concerned about the downstream impacts of this change (e.g., see xarray failures reported in #3105 (comment)). |
|
My thinking here with an error was to avoid situations where one opened an array with At a slightly higher level, I don't really understand why arrays can't have their own read/write permissions, but I missed that design decision. |
I definitely agree with the premise of avoiding situations where one opened an array with mode='r' but writing still works. I still question this solution because it will certainly be disruptive. The decision to defer read/write permissions to only the store level is an internal design detail that could be changed without any user facing disruptions, so IMO it'd be better to reconsider that decision before releasing a disruptive change. IIUC since the array has a reference to the store used at creation rather than a copy, this check doesn't really suffice to solving the situation where one opened an array with mode="r" if the store mode were become mutable (xref #3105). While it seems likely that @d-v-b understands that consequence and is arguing against it, a separate array mode would protect against store changes badly influencing array behavior in a more robust way. |
|
We should decide one way or another before doing a 3.0.9 release. I'm still minded to go ahead with this, but expand the error message to say to open the store in read only mode if you want to open an array in read only mode. I'd also like to look at the xarray test failures, to see if there's an easy fix, or if there is a sensible use case that we're breaking with this change. So before we release 3.0.9 with this, I think the todo is:
@maxrjones does that sound good? Anything I missed? Thanks for the downstream testing, it helps a lot! |
|
Okay, I had a poke around at pydata/xarray#10430, and convinced myself that we should revert this change for now. Long term we really need to sort out the complete mess of permissions and work out what our permission model actually is... |
|
for posterity can you elaborate a bit more on how the linked xarray PR convinced you to revert? |
This reverts commit 481550a.
|
In the xarray tests, there are examples where they create a memory store that is not read only, write an array to it, and then try and open that array in read only mode. I think it's reasonable to ask for a read only array from a writeable store, and not be able to write to the array. |
|
an alternative solution would be to go with ##3138 and modify xarray to use that method as needed. I'd be curious to hear the pros / cons for the revert solution vs the "add functionality" solution. |
|
My sense is #3138 is a bit of a hack anyway, when it should be possible to open an array in read-only mode from a writeable store. Because no-one did a write up (?) of the permission model changes for zarr-python v3 I don't really understand whether this is deliberately not possible, or if it was just an oversight. |
This is the key question. In Zarr-python 2 arrays and groups had a permission model. But in Zarr-python 3, we expressly moved the permissions model to the storage level, and so arrays / groups have defer entirely to the store for that. If we stick with that decision, then it should not be possible to open an array in read-only mode from a writeable store, without creating a new read-only version of that store (which #3138 implements). |
Rather than erroring on invalid store mode, we could emit a warning and create a copy of the store in the specified mode using #3138 (if implemented in the store class). This would balance matching the store mode with the API call and avoiding breaking changes. What would be the downsides of that approach? |
|
👍 that sounds like a good approach to me - I'd even consider not warning and just silently returning a copy. How shall we move forward then? Should we roll that change into #3138? |
Yes. for example I'd like to read from array
👍 great idea! |
I can update #3138 to add |
This avoids instances where mode='r' could be passed, by the resulting array is not read-only. Fixes #2949