more friendly error message in case no chunk manager is available #9676
Conversation
|
Much better! |
|
Thanks - I had a PR on this but don't mind closing mine in favor of this one. There is also #7963 which seems related. |
|
wow, I don't know how I missed two open PRs that aim to do something similar in different ways. Which one do we take? If we merge this one your PR might still be valuable since it also changes the error message if there are chunk managers but not the one that was requested. |
|
Shall we merge? |
xarray/tests/test_parallelcompat.py
Outdated
| with pytest.raises(ValueError, match="unrecognized chunk manager dask"): | ||
| def test_no_chunk_manager_available(self, monkeypatch) -> None: | ||
| monkeypatch.setattr( | ||
| "xarray.namedarray.parallelcompat.list_chunkmanagers", dict |
There was a problem hiding this comment.
I'm actually not sure whether using dict is an improvement over lambda: {} in this case...
With the latter it is pretty clear that we have a function that returns an empty dict, while the former is more along the lines of "create a new dict", which as far as intent goes is not quite the same (the result is the same, though). Sure, maybe dict is a tiny bit faster, but we only call this once, while running the tests.
TomNicholas
added
topic-error reporting
topic-chunked-arrays
| @@ -95,6 +95,10 @@ def guess_chunkmanager( | |||
| """ | |||
|
|
|||
| chunkmanagers = list_chunkmanagers() | |||
| if len(chunkmanagers) == 0: | |||
| raise ValueError( | |||
There was a problem hiding this comment.
This is definitely better, but perhaps it should throw a ModuleNotFoundError or ImportError?
There was a problem hiding this comment.
that would be kinda weird, though, because those usually need to point to a module that failed to import / was not found (and technically here we don't care about imports, we care about entrypoints).
There was a problem hiding this comment.
I know it's a bit odd, but ImportError doesn't actually require you to give a path to a specific module that it couldn't import (it's an optional argument), and I do think there would be value in the ImportError indicating to users that it's not really xarray's fault, it's their fault for not having set up an environment in which dask is importable.
But I think it's fine either way, and I agree it's not obvious if an entrypoint issue should raise an ImportError or not.
There was a problem hiding this comment.
Also we could actually just special case and state the name of the module that failed to be imported.
There was a problem hiding this comment.
+1 for ImportError. ImportError mentioning dask will do a better job of helping users figure out how to solve this themselves.
Sorry for dropping the ball on reviewing / merging these guys 😞 Let's merge this one.
This change would also be useful but is much less likely to come up. |
Co-authored-by: Tom Nicholas <[email protected]>
The current error message when trying to use a chunked-array related method without actually having a chunk manager available is:
That's pretty confusing, so this catches the case where no chunk manager is available and raises an error with guidance on how to fix that.
whats-new.rst