Skip to content

restore v2-like exceptions #3012

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Apr 24, 2025

This PR brings the following changes:

new (old) exceptions

zarr.errors gets new exceptions ArrayNotFoundError, GroupNotFoundError, PathNotFoundError, NodeNotFoundError (for when neither an array nor a group were found, in a context where either one was a valid result). The first three existed in zarr-python 2, so this PR is restoring compatibility. The new NodeNotFoundError is an instance of the zarr-python 2 exception PathNotFoundError. This subclass relationship exists for zarr-python 2 compatibility -- users coming from zarr-python 2 can use try..., except PathNotFoundError and catch the NodeNotFoundError. PathNotFoundError is deprecated but does not emit a warning on use. We should decide when to remove it.

PathNotFoundError is deprecated because the name of that exception is inconsistent with ArrayNotFoundError and GroupNotFoundError, which take the form <thing at a path>NotFoundError. PathNotFoundError is raised when a path fails to resolve to a node, i.e. an array or group, even if something is at that path. Because the name is bad (IMO), we should remove this exception entirely in the future. Until then, NodeNotFoundError inherits from PathNotFoundError, and we should use NodeNotFoundError in the codebase.

We were using FileNotFoundError and KeyError to model "failure to find an array or group at a path." In this PR, we consistently use ArrayNotFoundError when failing to find an array, GroupNotFoundError when failing to find a group, and NodeNotFoundError when failing to find either an array or group, in a context where either would have been OK (e.g., zarr.open() or group['nodea_name']).

relaxed exception formatting

In this PR, exceptions defined in zarr.errors take a single string in their constructor. This is a change from main, where the exceptions take a tuple of values that will be inserted into an f-string defined in the exception class.

That is, in main, raise FooError('a', 'b') will emit FooError: predefined message with two args, 'a' and 'b', but in this PR, you would need to invoke raise FooError("predefined message with two args, 'a', 'b'") to get the same message.

The acute reason for this change is to allow the same exception to contain a contextualized message. In the case of missing arrays or groups, it's useful to tell the user in e.g. an ArrayNotFoundError which zarr format the code was looking for, which means failing to find a v2 array or a v3 array (or either, if zarr_format was unspecified) each requires a slightly different message.

A more general reason for this change is that (IMO) pre-defining an exception message in the exception class is not good design. The exception text is read by humans, so it should convey whatever is most helpful in the context of that exception, and this might vary in different contexts. The exception message does not need to be highly structured for this purpose (it will not be read by machines), and if we do for some reason want our exceptions to be highly structured, then we can write separate functions that generate a pre-formatted exception message, and use this function before calling the exception. A cost of this approach is that it's a little more work to write exception messages, but the gain in expressiveness is worth it IMO.

partially addresses #3009 and #2821

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Apr 24, 2025
@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 24, 2025

this ballooned in scope, and while the end result is super useful, I think it's a bit too much for one PR (happy to be corrected here). I will spin out the lessons from this PR into separate issues / patches and then I can return to this one.

@joshmoore
Copy link
Member

@d-v-b: how breaking would this be to current v3.0.x code?

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 25, 2025

@d-v-b: how breaking would this be to current v3.0.x code?

For anyone relying on the current set of exceptions relating to missing arrays, groups, it's as breaking as it gets. This is the price of our failure to replace the v2 exceptions with something better.

In terms of releases, I'd say these changes should go in 3.1.0

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 25, 2025

We could introduce some mitigation by having the new exceptions inherit from the old one (I.e., 'FileNotFoundError')

@d-v-b d-v-b added this to the 3.1.0 milestone May 9, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants