Raise an exception for previously set key #2558
Conversation
| @@ -319,8 +319,9 @@ async def test_set_if_not_exists(self, store: S) -> None: | |||
| data_buf = self.buffer_cls.from_bytes(b"0000") | |||
| await self.set(store, key, data_buf) | |||
|
|
|||
| new = self.buffer_cls.from_bytes(b"1111") | |||
| await store.set_if_not_exists("k", new) # no error | |||
| with pytest.raises(Exception): | |||
There was a problem hiding this comment.
can we be more specific about the exception? Ideally we would match the type and the error message
There was a problem hiding this comment.
I'm not quite sure how to manage matching the type with optional dependencies. Obstore raises its AlreadyExistsError and fsspec raises its FileExistsError. One option is to put this as NotImplemented in the base test class and require the individual stores to implement it in their tests, but that seems non-ideal. Do you have advice?
Matching the error message would be easier because we wouldn't need to import those errors.
There was a problem hiding this comment.
if it's not simple to get more specific than Exception than I think it's OK to leave it as-is, and i can open an issue about how we could make this particular test more specific 😄
Co-authored-by: Davis Bennett <[email protected]>
|
@d-v-b when looking into what exceptions fsspec raises, I noticed that fsspec's internal code comments and the docs lists exclusive write as experimental and risky. Should we warn about this in the Zarr docs? |
|
@martindurant what do you think? |
I don't know if this is the behavior we want actually. The application in Zarr that relies on this method is to set an empty group metadata object if it doesn't exist yet. If it does, the desired behavior is to move on. One way to move forward is to catch this error in the Obstore Store and then move on. |
Thanks Joe, that makes sense now. I'll follow your recommendation to suggest a change within ObjectStore instead |
I think this tests was not correctly checking the behavior because the data at
kwas previously set, so callingset_if_not_existson that same key should actually raise an exception. The behavior for obstore in #1661 and kylebarron#3 seem correct, in contrast to the fsspec-based stores which now fail on this test.