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

Raise a better error when an array/store already exists #605

Open
eric-czech opened this issue Sep 13, 2020 · 5 comments
Open

Raise a better error when an array/store already exists #605

eric-czech opened this issue Sep 13, 2020 · 5 comments

Comments

@eric-czech
Copy link

It would be helpful for users and downstream libraries if the error thrown when attempting to create an array that already exists on disk was more specific. For example, this error isn't very clear and it isn't easy to wrap in error handling logic:

import zarr
zarr.create((10, 10), store='/tmp/test.zarr')
zarr.create((10, 10), store='/tmp/test.zarr')
# ValueError: path '' contains an array

This could perhaps raise a FileExistsError or at least include a message that describes the condition more clearly.

@Carreau
Copy link
Contributor

Carreau commented Sep 13, 2020

I think the problem is that the store may not be a filesystem, so there may not be files.

The code that fails is not aware whether or not the zarr hierarchy is filesystem base or not; so raising a "FileExistsError" does not make sens.

@eric-czech
Copy link
Author

The code that fails is not aware whether or not the zarr hierarchy is filesystem base or not; so raising a "FileExistsError" does not make sens.

Some potential solutions without knowing the codebase well:

  • err_contains_array could possibly return an error type that is specific to groups or paths within groups already existing (PathExistsError maybe)
  • The message could certainly contain something about the intent of the operation that lead to it since I imagine it's usually the same thing (i.e. trying to open a path within a group).  Perhaps "Path {path} already exists in store".  And if that's not appropriate in some calling contexts, then maybe similar "err_contains_array" functions could be called where they make sense but still provide more information on what went wrong.
  • The errors could be caught and wrapped appropriately so whatever code path interpreted the store as a file path in the first place can add the context necessary to know that the provided path was already present (as a more informative error type)

@jeromekelleher
Copy link
Member

Just +1 @eric-czech's point here, it would be nice to have a more specific error here than a ValueError (even if it's not a FileExistsError).

@rabernat
Copy link
Contributor

👍 We often hit these errors with cloud storage (e.g. s3fs) and users get very confused.

@eric-czech's suggestions make sense to me.

@Carreau
Copy link
Contributor

Carreau commented Sep 14, 2020

#590 from 3 weeks ago does already include subclasses of ValueError with more informative names and make the stacktrace more explicit as to where the error come from.

Though we do need to remove the helper function in favor of custom exception and I'm was waiting for #546 to get in as it did conflict and was bigger.

Once this is in we can more precise error messages, but there will still be some layer of indirection with the store.

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

No branches or pull requests

4 participants