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

fix warning from scipy backend guess_can_open on directory #9911

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

germasch
Copy link

When passing a directory to open_dataset(), the scipy backend fails and a "RuntimeWarning: 'scipy' fails while guessing" is produced.

This affects me since I'm implementing a backend to read data written by the adios2 package, whose data "files" are actually a directory.

This tiny patch treats this case just like file not found, that is, the scipy backend will now return that it cannot open such a "file", but without raising an exception.

Copy link

welcome bot commented Dec 19, 2024

Thank you for opening this pull request! It may take us a few days to respond here, so thank you for being patient.
If you have questions, some answers may be found in our contributing guidelines.

Copy link
Collaborator

@headtr1ck headtr1ck left a comment

Choose a reason for hiding this comment

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

Looks like a useful addition, thanks!
Should we maybe catch a PermissionError as well?

And feel free to add an entry in whats-new.

When passing a directory to open_dataset(), the scipy backend fails
and a "RuntimeWarning: 'scipy' fails while guessing" is produced.

This affects me since I'm implementing a backend to read data written
by the adios2 package, whose data "files" are actually a directory.

This tiny patch treats this case just like file not found, that is, the
scipy backend will now return that it cannot open such a "file", but
without raising an exception.
@germasch germasch force-pushed the pr/backend-scipy-not-a-directory branch from 4505e6f to 133e3dd Compare December 25, 2024 15:05
@germasch
Copy link
Author

I figured this change is too minor to justify a whats-new entry, but I'd be happy to do so if you'd like.

Good point about considering PermissionError, too. I just looked, and PermissionError is already special cased in the case of built-in backends, in that it's re-raised. I think that's the right thing to do -- a PermissionError is very likely not happening because it's a mismatched backend, but rather because of an actual permission problem, and the user will benefit from the specific failure (exception), as opposed to a generic "no matching backend found" error. So I think the answer is probably to propagate a PermissionError from plugin backends to the user as well. If you agree, I can make that change, and make sure that it behaves as desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants