-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve error message for guess engine #5455
Improve error message for guess engine #5455
Conversation
# Conflicts: # xarray/backends/plugins.py # xarray/tests/test_backends.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I added some small suggestions.
xarray/backends/cfgrib_.py
Outdated
@@ -146,6 +146,9 @@ def open_dataset( | |||
) | |||
return ds | |||
|
|||
@staticmethod | |||
def are_dependencies_installed() -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call this dependencies_installed
, or is_installed
, installed
, or available
?
Could be:
class CfgribfBackendEntrypoint(BackendEntrypoint):
available = has_cfgrib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested not to use is_installed
because the backend is obviously installed, are the dependencies that are missing.
I like available
as a name.
I have a slight preference to keep it a method as it is more powerful, but you can use a properties in the case you need, so if other like it I'll not object to change it to an attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
available
is fine for me and also using an attribute instead of a function
"xarray is unable to open this file because it has no currently " | ||
"installed IO backends. Xarray's read/write support requires " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"xarray is unable to open this file because it has no currently " | |
"installed IO backends. Xarray's read/write support requires " | |
"xarray is unable to open this file because no IO backend " | |
"is currently installed. Xarray's read/write support requires " |
xarray/backends/plugins.py
Outdated
"http://xarray.pydata.org/en/stable/user-guide/io.html" | ||
error_msg = ( | ||
"found the following matches with the input file in xarray's IO " | ||
f"backends: {compatible}. But their dependencies may not be installed, see:\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f"backends: {compatible}. But their dependencies may not be installed, see:\n" | |
f"backends: {compatible}. But they may not be installed, see:\n" |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This in not technically correct. You may have rioxarray
installed that provides the engine, but not GDAL
so you get the error. I would suggest to keep the message like it is.
xarray/backends/plugins.py
Outdated
return engine | ||
except Exception: | ||
warnings.warn(f"{engine!r} fails while guessing", RuntimeWarning) | ||
|
||
compatible = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compatible = [] | |
# no engine found, find out why | |
compatible_engines = [] |
xarray/backends/plugins.py
Outdated
backend_entrypoints = {} | ||
for backend_name, backend in BACKEND_ENTRYPOINTS.items(): | ||
if backend.are_dependencies_installed(): | ||
backend_entrypoints[backend_name] = backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backend_entrypoints = {} | |
for backend_name, backend in BACKEND_ENTRYPOINTS.items(): | |
if backend.are_dependencies_installed(): | |
backend_entrypoints[backend_name] = backend | |
backend_entrypoints = { | |
backend_name: backend | |
for backend_name, backend in BACKEND_ENTRYPOINTS.items() | |
if backend.are_dependencies_installed() | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer avoiding comprehension if they are complex since they are less readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this blackened format I think it's as readable as the normal loop. But I don't feel that strongly about this, if you prefer the other one that's fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still prefer my open suggestion but let's not get hung up over this. I'll merge soon (or you can).
Unit Test Results0 files 0 suites 0s ⏱️ Results for commit eea7673. ♻️ This comment has been updated with latest results. |
* main: Improve error message for guess engine (pydata#5455) Refactor dataset groupby tests (pydata#5506) DOC: zarr note on encoding (pydata#5427) Allow plotting categorical data (pydata#5464)
When open_dataset() fails because no working engines are found, it suggests installing the dependencies of the compatible internal backends, providing explicitly the list.
pre-commit run --all-files