Suppress FileNotFoundError when deleting keys in the obstore adapter

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lukasbindreiter]

Suppress FileNotFoundError in delete operation of the obstore adapter.

Since some underlying stores allow this, while others raise an error, supressing the error results in consistent behaviour across all stores. This also mimics how the same thing is implemented in the fsspec adapter.

Fixes#3136

TODO:

• Add unit tests and/or doctests in docstrings
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[dstansby]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

[d-v-b]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key, and for the caller of the store method to handle that exception specifically in the context of attempting to remove an empty chunk. But this fix also works!

[kylebarron]

I think a better long-term solution would be for the stores to all raise the same exception when attempting to delete a missing key

From the obstore side I don't have total control over that. The upstream s3 implementation does not raise an error: https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#method.delete_stream

[lukasbindreiter]

Thanks for the PR/fix! This is slightly surprising behaviour to me - I would expect trying to delete a non-existent file to raise an error. But I guess if this is existing behaviour/design across other stores, this is good.

Yes this seems to be the behaviour across stores:

• local store uses path.unlink(missing_ok=True)
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py#L218
• memory store suppresses KeyError
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_memory.py#L143
• fsspec store also suppresses FileNotFoundError:
  https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_fsspec.py#L359

so I believe it makes sense to do the same for obstore as well.

And when thinking about the main purpose of the function (to clear chunks) this behaviour makes sense - it seems the function isn't really about deleting an object, instead it is to make sure that an object doesn't exist in the store.

To be sure that we're getting consistent behaviour across stores, could you move the test you added to the StoreTests class, so it's run across all stores that are tested?

Good idea, I've moved the test now to StoreTests 👍

Across stores is a bit ambiguous in this context though - there is the zarr adapter stores (fsspec, local, memory, obstore) - the test now runs across all of those.

However the issue / bugfix was only for the obstore adapter, since there is inconsistent behaviour across obstore stores (S3, GCS, Azure, Local, InMemory) - but as @kylebarron has mentioned that seems to be out of control of obstore itself.

[maxrjones]

My approval doesn't mean anything really, but I'm submitting it because I ran into this issue and tested your branch locally to confirm that it solved the problem. I also made two code suggestions to apply Kyle's recommendation for convenience.

[kylebarron]

In developmentseed/obstore#487 I propose deprecating the obstore.exceptions.NotFoundError class

[lukasbindreiter]

Thanks @maxrjones and @kylebarron for the review - I've now updated the comment and removed the obstore.exceptions.NotFoundError again

[lukasbindreiter]

@d-v-b Thanks for enabling the merge.

I checked the failing test for the windows-latest py3.11 run, which is this one.

deftest_sync_timeout() ->None:
duration=0.02asyncdeffoo() ->None:
awaitasyncio.sleep(duration)
withpytest.raises(asyncio.TimeoutError):
sync(foo(), timeout=duration/10)

It seems completely unrelated to the changes in the PR, probably a flaky failure? Could you potentially retry it?

[d-v-b]

@lukasbindreiter the flaky test theory seems valid! thanks for this contribution.

(I accidentally posted this in #3136 (comment))

[d-v-b]

@meeseeksdev backport to 3.0.9

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.48%. Comparing base (5731c6c) to head (a6fa2c5).
⚠️ Report is 130 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3140 +/- ##
=======================================
Coverage 94.47% 94.48% =======================================
Files 78 78 Lines 8585 8590 +5 =======================================
+ Hits 8111 8116 +5 
Misses 474 474 

Files with missing lines	Coverage Δ
src/zarr/storage/_obstore.py	94.17% <100.00%> (+0.02%)	⬆️
src/zarr/testing/store.py	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.