zarr-python icon indicating copy to clipboard operation
zarr-python copied to clipboard

Make sure fs exceptions are raised if not MissingFs exceptions (clone)

Open itcarroll opened this issue 2 years ago • 6 comments

note: This PR is a copy of PR #1237 which had become stale. Description from @martindurant is:

(for v2 for now)

When FSStore gets exceptions, these correctly become the fill value when they are translated to KeyError. However, some exceptions do not equate to missing keys but to other permanent problems such as PermissionError. This change makes sure they are correctly raised to the caller even within getitems (getitem already was correct).

TODO:

  • [x] Add unit tests and/or doctests in docstrings
  • (NA) Add docstrings and API docs for any new/modified user-facing classes and functions
  • (NA) New/modified features documented in docs/tutorial.rst
  • [x] Changes documented in docs/release.rst
  • [x] GitHub Actions have all passed
  • [x] Test coverage is 100% (Codecov passes)

itcarroll avatar Dec 10 '23 21:12 itcarroll

It would be nice to get the workflows authorized so I could see what more to fix and document in release notes.

itcarroll avatar Dec 13 '23 04:12 itcarroll

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.99%. Comparing base (2534413) to head (21a5ab7).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1604   +/-   ##
=======================================
  Coverage   99.99%   99.99%           
=======================================
  Files          38       38           
  Lines       14580    14604   +24     
=======================================
+ Hits        14579    14603   +24     
  Misses          1        1           
Files Coverage Δ
zarr/storage.py 100.00% <100.00%> (ø)
zarr/tests/test_storage.py 100.00% <100.00%> (ø)

codecov[bot] avatar Dec 13 '23 07:12 codecov[bot]

Failing test is the same seen in #1611, something with Azure connection.

itcarroll avatar Dec 20 '23 10:12 itcarroll

It would be nice to get the workflows authorized so I could see what more to fix and document in release notes.

Apologies for the slowness. It's the end of year crunch. Actions launched.

Failing test is the same seen in #1611, something with Azure connection.

IIUC, this has been happening more frequently recently. A relaunch usually helps. I'll keep an eye on it.

joshmoore avatar Dec 20 '23 19:12 joshmoore

@joshmoore Trying to help with another merge to main. Maybe not helpful though ...

itcarroll avatar Mar 21 '24 14:03 itcarroll

Scan of the 178 failed tests indicates azure.core.exceptions.HttpResponseError from tests WithABSStore are the culprit.

itcarroll avatar Mar 22 '24 14:03 itcarroll

@itcarroll, it looks like re-running the jobs worked!

sanketverma1704 avatar Mar 27 '24 00:03 sanketverma1704

@joshmoore I know that no issue was created to precipitate @martindurant's original PR, but this fixes an actual, real, silent, bug, so it would be great to finish it off. Thanks!

itcarroll avatar Apr 04 '24 00:04 itcarroll

This has been quite the quest -- thanks @itcarroll for hanging in there. @martindurant - are you happy with this solution? Would love to get this into a release (tomorrow 🤞).

jhamman avatar Apr 05 '24 02:04 jhamman