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 Missing #1237

Closed
wants to merge 10 commits into from

Conversation

martindurant
Copy link
Member

(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
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • 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)

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Nov 21, 2022
@martindurant
Copy link
Member Author

Will pass when fsspec/filesystem_spec#1120 makes it to release

@joshmoore
Copy link
Member

Re-launched to see the state of the tests.

@joshmoore
Copy link
Member

@martindurant: still red.

@martindurant
Copy link
Member Author

I haven't released fsspec since. Due this week.

Comment on lines +1348 to +1349
arr = g.create_dataset("data", data=[1, 2, 3, 4],
dtype="i4", compression=None, chunks=[2])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
arr = g.create_dataset("data", data=[1, 2, 3, 4],
dtype="i4", compression=None, chunks=[2])
arr = g.create_dataset("data", data=[1, 2, 3, 4], dtype="i4", compression=None, chunks=[2])

black/ruff line length is 100, this will fix one red x

Comment on lines +1352 to +1355
assert g.store.getitems(["data/1"]) == {} # not found
with pytest.raises(Exception):
# None is bad data, as opposed to missing
g.store.getitems(["data/0", "data/1"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert g.store.getitems(["data/1"]) == {} # not found
with pytest.raises(Exception):
# None is bad data, as opposed to missing
g.store.getitems(["data/0", "data/1"])
assert g.store.getitems(["data/1"], contexts={}) == {} # not found
with pytest.raises(Exception):
# None is bad data, as opposed to missing
g.store.getitems(["data/0", "data/1"], contexts={})

missing required contexts kwarg is causing one (of two) test failures

@itcarroll
Copy link
Contributor

Red because of (at least) two test failures and one pre-commit.ci formatting check. I've added comments to address two of those.

One test failure is confusing. It's zarr/test/test_storage.py:TestN5FSStore.test_exceptions but it only fails if pytest has also run TestFSStore. Maybe something involving sharing/conflict with the memory file store?

@jhamman
Copy link
Member

jhamman commented Dec 7, 2023

This has unfortunately gone stale. Seems like it was quite close. Will it be revived or should we close this?

@jhamman jhamman added the stale label Dec 7, 2023
@itcarroll
Copy link
Contributor

If you do close, I have a fork with the commits from @martindurant and fixes from my review. If I can resolve the 3rd failure, I will create a new PR (although I hope OP beats me to it)!

@jhamman
Copy link
Member

jhamman commented Dec 8, 2023

@itcarroll - no need for us to close this first. Please go ahead and open a PR of your own with the remaining fixes.

@joshmoore
Copy link
Member

Closing this in favor of #1604

@joshmoore joshmoore closed this Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs release notes Automatically applied to PRs which haven't added release notes stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants