Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from Apr 5, 2024

Conversation

itcarroll
Copy link
Contributor

@itcarroll itcarroll commented Dec 10, 2023

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:

  • 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
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

zarr/tests/test_storage.py Outdated Show resolved Hide resolved
zarr/storage.py Outdated Show resolved Hide resolved
@itcarroll
Copy link
Contributor Author

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

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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%> (ø)

@itcarroll
Copy link
Contributor Author

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

@joshmoore
Copy link
Member

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.

@itcarroll
Copy link
Contributor Author

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

@itcarroll
Copy link
Contributor Author

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

@MSanKeys963
Copy link
Member

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

@itcarroll
Copy link
Contributor Author

@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!

@jhamman
Copy link
Member

jhamman commented Apr 5, 2024

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 jhamman merged commit 5fde3a2 into zarr-developers:main Apr 5, 2024
18 checks passed
@itcarroll itcarroll deleted the fs_exceptions branch April 5, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants