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

Improve error message for guess engine #5455

Merged
merged 15 commits into from
Jun 23, 2021
7 changes: 5 additions & 2 deletions xarray/backends/cfgrib_.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Collaborator Author

@aurghs aurghs Jun 10, 2021

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

return has_cfgrib

if has_cfgrib:
BACKEND_ENTRYPOINTS["cfgrib"] = CfgribfBackendEntrypoint

BACKEND_ENTRYPOINTS["cfgrib"] = CfgribfBackendEntrypoint
7 changes: 5 additions & 2 deletions xarray/backends/h5netcdf_.py
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_h5netcdf

if has_h5netcdf:
BACKEND_ENTRYPOINTS["h5netcdf"] = H5netcdfBackendEntrypoint

BACKEND_ENTRYPOINTS["h5netcdf"] = H5netcdfBackendEntrypoint
7 changes: 5 additions & 2 deletions xarray/backends/netCDF4_.py
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_netcdf4

if has_netcdf4:
BACKEND_ENTRYPOINTS["netcdf4"] = NetCDF4BackendEntrypoint

BACKEND_ENTRYPOINTS["netcdf4"] = NetCDF4BackendEntrypoint
56 changes: 39 additions & 17 deletions xarray/backends/plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ def sort_backends(backend_entrypoints):


def build_engines(pkg_entrypoints):
backend_entrypoints = BACKEND_ENTRYPOINTS.copy()
backend_entrypoints = {}
for backend_name, backend in BACKEND_ENTRYPOINTS.items():
if backend.are_dependencies_installed():
backend_entrypoints[backend_name] = backend
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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()
}

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

pkg_entrypoints = remove_duplicates(pkg_entrypoints)
external_backend_entrypoints = backends_dict_from_pkg(pkg_entrypoints)
backend_entrypoints.update(external_backend_entrypoints)
Expand All @@ -101,30 +104,49 @@ def guess_engine(store_spec):

for engine, backend in engines.items():
try:
if backend.guess_can_open and backend.guess_can_open(store_spec):
if backend.guess_can_open(store_spec):
return engine
except Exception:
warnings.warn(f"{engine!r} fails while guessing", RuntimeWarning)

compatible = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
compatible = []
# no engine found, find out why
compatible_engines = []

for engine, backend_cls in BACKEND_ENTRYPOINTS.items():
try:
backend = backend_cls()
if backend.guess_can_open(store_spec):
compatible.append(engine)
except Exception:
warnings.warn(f"{engine!r} fails while guessing", RuntimeWarning)

installed = [k for k in engines if k != "store"]
if installed:
raise ValueError(
"did not find a match in any of xarray's currently installed IO "
f"backends {installed}. Consider explicitly selecting one of the "
"installed backends via the ``engine`` parameter to "
"xarray.open_dataset(), or installing additional IO dependencies:\n"
"http://xarray.pydata.org/en/stable/getting-started-guide/installing.html\n"
"http://xarray.pydata.org/en/stable/user-guide/io.html"
)
if not compatible:
if installed:
error_msg = (
"did not find a match in any of xarray's currently installed IO "
f"backends {installed}. Consider explicitly selecting one of the "
"installed engines via the ``engine`` parameter, or installing "
"additional IO dependencies, see:\n"
"http://xarray.pydata.org/en/stable/getting-started-guide/installing.html\n"
"http://xarray.pydata.org/en/stable/user-guide/io.html"
)
else:
error_msg = (
"xarray is unable to open this file because it has no currently "
"installed IO backends. Xarray's read/write support requires "
Comment on lines +134 to +135
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"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 "

"installing optional IO dependencies, see:\n"
"http://xarray.pydata.org/en/stable/getting-started-guide/installing.html\n"
"http://xarray.pydata.org/en/stable/user-guide/io"
)
else:
raise ValueError(
"xarray is unable to open this file because it has no currently "
"installed IO backends. Xarray's read/write support requires "
"installing optional dependencies:\n"
"http://xarray.pydata.org/en/stable/getting-started-guide/installing.html\n"
"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"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
f"backends: {compatible}. But their dependencies may not be installed, see:\n"
f"backends: {compatible}. But they may not be installed, see:\n"

?

Copy link
Collaborator

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.

"http://xarray.pydata.org/en/stable/user-guide/io.html \n"
"http://xarray.pydata.org/en/stable/getting-started-guide/installing.html"
)

raise ValueError(error_msg)


def get_backend(engine):
"""Select open_dataset method based on current engine."""
Expand Down
7 changes: 5 additions & 2 deletions xarray/backends/pseudonetcdf_.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_pseudonetcdf

if has_pseudonetcdf:
BACKEND_ENTRYPOINTS["pseudonetcdf"] = PseudoNetCDFBackendEntrypoint

BACKEND_ENTRYPOINTS["pseudonetcdf"] = PseudoNetCDFBackendEntrypoint
7 changes: 5 additions & 2 deletions xarray/backends/pydap_.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_pydap

if has_pydap:
BACKEND_ENTRYPOINTS["pydap"] = PydapBackendEntrypoint

BACKEND_ENTRYPOINTS["pydap"] = PydapBackendEntrypoint
9 changes: 6 additions & 3 deletions xarray/backends/pynio_.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,13 @@ def open_dataset(
mode="r",
lock=None,
):
filename_or_obj = _normalize_path(filename_or_obj)
store = NioDataStore(
filename_or_obj,
mode=mode,
lock=lock,
)

filename_or_obj = _normalize_path(filename_or_obj)
store_entrypoint = StoreBackendEntrypoint()
with close_on_error(store):
ds = store_entrypoint.open_dataset(
Expand All @@ -133,6 +133,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_pynio


if has_pynio:
BACKEND_ENTRYPOINTS["pynio"] = PynioBackendEntrypoint
BACKEND_ENTRYPOINTS["pynio"] = PynioBackendEntrypoint
7 changes: 5 additions & 2 deletions xarray/backends/scipy_.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_scipy

if has_scipy:
BACKEND_ENTRYPOINTS["scipy"] = ScipyBackendEntrypoint

BACKEND_ENTRYPOINTS["scipy"] = ScipyBackendEntrypoint
4 changes: 4 additions & 0 deletions xarray/backends/store.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,9 @@ def open_dataset(

return ds

@staticmethod
def are_dependencies_installed() -> bool:
return True


BACKEND_ENTRYPOINTS["store"] = StoreBackendEntrypoint
14 changes: 12 additions & 2 deletions xarray/backends/zarr.py
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,13 @@ def open_zarr(


class ZarrBackendEntrypoint(BackendEntrypoint):
def guess_can_open(self, filename_or_obj):
try:
_, ext = os.path.splitext(filename_or_obj)
except TypeError:
return False
return ext in {".zarr"}

def open_dataset(
self,
filename_or_obj,
Expand Down Expand Up @@ -756,6 +763,9 @@ def open_dataset(
)
return ds

@staticmethod
def are_dependencies_installed() -> bool:
return has_zarr


if has_zarr:
BACKEND_ENTRYPOINTS["zarr"] = ZarrBackendEntrypoint
BACKEND_ENTRYPOINTS["zarr"] = ZarrBackendEntrypoint
14 changes: 9 additions & 5 deletions xarray/tests/test_plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,20 @@ def test_build_engines_sorted():
mock.MagicMock(return_value={"dummy": DummyBackendEntrypointArgs()}),
)
def test_no_matching_engine_found():
with pytest.raises(
ValueError, match="match in any of xarray's currently installed IO"
):
with pytest.raises(ValueError, match=r"did not find a match in any"):
plugins.guess_engine("not-valid")

with pytest.raises(ValueError, match=r"found the following matches with the input"):
plugins.guess_engine("foo.nc")


@mock.patch(
"xarray.backends.plugins.list_engines",
mock.MagicMock(return_value={}),
)
def test_no_engines_installed():
with pytest.raises(ValueError, match="no currently installed IO backends."):
def test_engines_not_installed():
with pytest.raises(ValueError, match=r"xarray is unable to open"):
plugins.guess_engine("not-valid")

with pytest.raises(ValueError, match=r"found the following matches with the input"):
plugins.guess_engine("foo.nc")