From aaff79e62adbd21b5714dfbdbf575cb8743b4119 Mon Sep 17 00:00:00 2001 From: Panu Lahtinen Date: Wed, 16 Oct 2024 11:33:58 +0300 Subject: [PATCH 1/6] Log an ERROR level message on YAMLErrors when reader loading fails --- satpy/readers/__init__.py | 9 ++++++++- satpy/tests/test_readers.py | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/satpy/readers/__init__.py b/satpy/readers/__init__.py index 64055c7232..3a05bd0c16 100644 --- a/satpy/readers/__init__.py +++ b/satpy/readers/__init__.py @@ -567,10 +567,12 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): reader_instance = load_reader( reader_configs, **reader_kwargs[None if reader is None else reader[idx]]) - except (KeyError, IOError, yaml.YAMLError) as err: + except (KeyError, IOError) as err: LOG.info("Cannot use %s", str(reader_configs)) LOG.debug(str(err)) continue + except yaml.YAMLError as err: + _log_yaml_error(reader_configs, err) if not readers_files: # we weren't given any files for this reader @@ -590,6 +592,11 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): return reader_instances +def _log_yaml_error(reader_configs, err): + LOG.error("Problem with %s", str(reader_configs)) + LOG.error(str(err)) + + def _early_exit(filenames, reader): if not filenames and not reader: # used for an empty Scene diff --git a/satpy/tests/test_readers.py b/satpy/tests/test_readers.py index eb8983d3cf..a3aef9114e 100644 --- a/satpy/tests/test_readers.py +++ b/satpy/tests/test_readers.py @@ -258,6 +258,11 @@ class TestReaderLoader(unittest.TestCase): Assumes that the VIIRS SDR reader exists and works. """ + @pytest.fixture(autouse=True) + def inject_fixtures(self, caplog): # noqa: PT004 + """Inject caplog to the test class.""" + self._caplog = caplog + def setUp(self): """Wrap HDF5 file handler with our own fake handler.""" from satpy.readers.viirs_sdr import VIIRSSDRFileHandler @@ -439,6 +444,24 @@ def test_almost_all_filtered(self): assert "abi_l1b" in readers assert len(list(readers["abi_l1b"].available_dataset_ids)) == 0 + @mock.patch("satpy.readers.load_reader") + def test_yaml_error_message(self, load_reader): + """Test that YAML errors are logged properly.""" + import logging + + import yaml + + from satpy.readers import load_readers + + filenames = ["AVHR_xxx_1B_M01_20241015100703Z_20241015114603Z_N_O_20241015105547Z.nat"] + error_message = "YAML test error message" + load_reader.side_effect = yaml.YAMLError(error_message) + + with self._caplog.at_level(logging.ERROR): + with pytest.raises(UnboundLocalError): + _ = load_readers(filenames=filenames, reader="avhrr_l1b_eps") + assert error_message in self._caplog.text + class TestFindFilesAndReaders: """Test the find_files_and_readers utility function.""" From ca987bc50a932b15192f483062f554562bf700bc Mon Sep 17 00:00:00 2001 From: Panu Lahtinen Date: Wed, 16 Oct 2024 12:00:59 +0300 Subject: [PATCH 2/6] Refactor load_readers and update the test --- satpy/readers/__init__.py | 26 +++++++++++++++++--------- satpy/tests/test_readers.py | 2 +- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/satpy/readers/__init__.py b/satpy/readers/__init__.py index 3a05bd0c16..ef0d8713c3 100644 --- a/satpy/readers/__init__.py +++ b/satpy/readers/__init__.py @@ -563,16 +563,9 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): else: readers_files = remaining_filenames - try: - reader_instance = load_reader( - reader_configs, - **reader_kwargs[None if reader is None else reader[idx]]) - except (KeyError, IOError) as err: - LOG.info("Cannot use %s", str(reader_configs)) - LOG.debug(str(err)) + reader_instance = _get_reader_instance(reader, reader_configs, idx, **reader_kwargs) + if reader_instance is None: continue - except yaml.YAMLError as err: - _log_yaml_error(reader_configs, err) if not readers_files: # we weren't given any files for this reader @@ -592,6 +585,21 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): return reader_instances +def _get_reader_instance(reader, reader_configs, idx, **reader_kwargs): + reader_instance = None + try: + reader_instance = load_reader( + reader_configs, + **reader_kwargs[None if reader is None else reader[idx]]) + except (KeyError, IOError) as err: + LOG.info("Cannot use %s", str(reader_configs)) + LOG.debug(str(err)) + except yaml.YAMLError as err: + _log_yaml_error(reader_configs, err) + + return reader_instance + + def _log_yaml_error(reader_configs, err): LOG.error("Problem with %s", str(reader_configs)) LOG.error(str(err)) diff --git a/satpy/tests/test_readers.py b/satpy/tests/test_readers.py index a3aef9114e..bff268fdd2 100644 --- a/satpy/tests/test_readers.py +++ b/satpy/tests/test_readers.py @@ -458,7 +458,7 @@ def test_yaml_error_message(self, load_reader): load_reader.side_effect = yaml.YAMLError(error_message) with self._caplog.at_level(logging.ERROR): - with pytest.raises(UnboundLocalError): + with pytest.raises(match=ValueError): _ = load_readers(filenames=filenames, reader="avhrr_l1b_eps") assert error_message in self._caplog.text From dc3bc1ca376287c6bd9979248f8f217486457b8b Mon Sep 17 00:00:00 2001 From: Panu Lahtinen Date: Wed, 16 Oct 2024 13:08:42 +0300 Subject: [PATCH 3/6] Fix reader_kwargs handling --- satpy/readers/__init__.py | 7 +++++-- satpy/tests/test_readers.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/satpy/readers/__init__.py b/satpy/readers/__init__.py index ef0d8713c3..eb2b73d681 100644 --- a/satpy/readers/__init__.py +++ b/satpy/readers/__init__.py @@ -557,13 +557,16 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): reader, filenames, remaining_filenames = _get_reader_and_filenames(reader, filenames) (reader_kwargs, reader_kwargs_without_filter) = _get_reader_kwargs(reader, reader_kwargs) + if reader_kwargs is None: + reader_kwargs = {} + for idx, reader_configs in enumerate(configs_for_reader(reader)): if isinstance(filenames, dict): readers_files = set(filenames[reader[idx]]) else: readers_files = remaining_filenames - reader_instance = _get_reader_instance(reader, reader_configs, idx, **reader_kwargs) + reader_instance = _get_reader_instance(reader, reader_configs, idx, reader_kwargs) if reader_instance is None: continue @@ -585,7 +588,7 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): return reader_instances -def _get_reader_instance(reader, reader_configs, idx, **reader_kwargs): +def _get_reader_instance(reader, reader_configs, idx, reader_kwargs): reader_instance = None try: reader_instance = load_reader( diff --git a/satpy/tests/test_readers.py b/satpy/tests/test_readers.py index bff268fdd2..343613cb0c 100644 --- a/satpy/tests/test_readers.py +++ b/satpy/tests/test_readers.py @@ -458,7 +458,7 @@ def test_yaml_error_message(self, load_reader): load_reader.side_effect = yaml.YAMLError(error_message) with self._caplog.at_level(logging.ERROR): - with pytest.raises(match=ValueError): + with pytest.raises(ValueError, match="No supported files found"): _ = load_readers(filenames=filenames, reader="avhrr_l1b_eps") assert error_message in self._caplog.text From 4745774dcdd1771201ea6b1fce32d6cc3af0644f Mon Sep 17 00:00:00 2001 From: Panu Lahtinen Date: Wed, 16 Oct 2024 13:40:32 +0300 Subject: [PATCH 4/6] Switch YAMLError to ConstructorError --- satpy/readers/__init__.py | 2 +- satpy/tests/test_readers.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/satpy/readers/__init__.py b/satpy/readers/__init__.py index eb2b73d681..3d1cdf2d40 100644 --- a/satpy/readers/__init__.py +++ b/satpy/readers/__init__.py @@ -597,7 +597,7 @@ def _get_reader_instance(reader, reader_configs, idx, reader_kwargs): except (KeyError, IOError) as err: LOG.info("Cannot use %s", str(reader_configs)) LOG.debug(str(err)) - except yaml.YAMLError as err: + except yaml.constructor.ConstructorError as err: _log_yaml_error(reader_configs, err) return reader_instance diff --git a/satpy/tests/test_readers.py b/satpy/tests/test_readers.py index 343613cb0c..c01e201067 100644 --- a/satpy/tests/test_readers.py +++ b/satpy/tests/test_readers.py @@ -455,7 +455,7 @@ def test_yaml_error_message(self, load_reader): filenames = ["AVHR_xxx_1B_M01_20241015100703Z_20241015114603Z_N_O_20241015105547Z.nat"] error_message = "YAML test error message" - load_reader.side_effect = yaml.YAMLError(error_message) + load_reader.side_effect = yaml.constructor.ConstructorError(error_message) with self._caplog.at_level(logging.ERROR): with pytest.raises(ValueError, match="No supported files found"): From 8c9dd7e52922df89b0a242a903cb2a2ffc3550e8 Mon Sep 17 00:00:00 2001 From: Panu Lahtinen Date: Wed, 16 Oct 2024 14:03:36 +0300 Subject: [PATCH 5/6] Reduce cyclomatic complexity in load_readers --- satpy/readers/__init__.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/satpy/readers/__init__.py b/satpy/readers/__init__.py index 3d1cdf2d40..04f98e2182 100644 --- a/satpy/readers/__init__.py +++ b/satpy/readers/__init__.py @@ -561,18 +561,12 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): reader_kwargs = {} for idx, reader_configs in enumerate(configs_for_reader(reader)): - if isinstance(filenames, dict): - readers_files = set(filenames[reader[idx]]) - else: - readers_files = remaining_filenames - + readers_files = _get_readers_files(filenames, reader, idx, remaining_filenames) reader_instance = _get_reader_instance(reader, reader_configs, idx, reader_kwargs) - if reader_instance is None: + if reader_instance is None or not readers_files: + # Reader initiliasation failed or no files were given continue - if not readers_files: - # we weren't given any files for this reader - continue loadables = reader_instance.select_files_from_pathnames(readers_files) if loadables: reader_instance.create_storage_items( @@ -580,6 +574,7 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): fh_kwargs=reader_kwargs_without_filter[None if reader is None else reader[idx]]) reader_instances[reader_instance.name] = reader_instance remaining_filenames -= set(loadables) + if not remaining_filenames: break @@ -588,6 +583,12 @@ def load_readers(filenames=None, reader=None, reader_kwargs=None): return reader_instances +def _get_readers_files(filenames, reader, idx, remaining_filenames): + if isinstance(filenames, dict): + return set(filenames[reader[idx]]) + return remaining_filenames + + def _get_reader_instance(reader, reader_configs, idx, reader_kwargs): reader_instance = None try: From 3db654715280240f8fbc00690811e3350106bb7c Mon Sep 17 00:00:00 2001 From: Panu Lahtinen Date: Wed, 16 Oct 2024 15:53:01 +0300 Subject: [PATCH 6/6] Write a reader config instead of mocking the parsing failure --- satpy/tests/test_readers.py | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/satpy/tests/test_readers.py b/satpy/tests/test_readers.py index c01e201067..f11d181833 100644 --- a/satpy/tests/test_readers.py +++ b/satpy/tests/test_readers.py @@ -259,9 +259,10 @@ class TestReaderLoader(unittest.TestCase): """ @pytest.fixture(autouse=True) - def inject_fixtures(self, caplog): # noqa: PT004 + def inject_fixtures(self, caplog, tmp_path): # noqa: PT004 """Inject caplog to the test class.""" self._caplog = caplog + self._tmp_path = tmp_path def setUp(self): """Wrap HDF5 file handler with our own fake handler.""" @@ -444,22 +445,29 @@ def test_almost_all_filtered(self): assert "abi_l1b" in readers assert len(list(readers["abi_l1b"].available_dataset_ids)) == 0 - @mock.patch("satpy.readers.load_reader") - def test_yaml_error_message(self, load_reader): + def test_yaml_error_message(self): """Test that YAML errors are logged properly.""" import logging - import yaml - + import satpy from satpy.readers import load_readers - filenames = ["AVHR_xxx_1B_M01_20241015100703Z_20241015114603Z_N_O_20241015105547Z.nat"] - error_message = "YAML test error message" - load_reader.side_effect = yaml.constructor.ConstructorError(error_message) + reader_config = "reader:\n" + reader_config += " name: nonreader\n" + reader_config += " reader: !!python/name:notapackage.notareader.BadClass\n" + + os.mkdir(self._tmp_path / "readers") + reader_fname = self._tmp_path / "readers" / "nonreader.yaml" + with open(reader_fname, "w") as fid: + fid.write(reader_config) + + filenames = ["foo.bar"] + error_message = "No module named 'notapackage'" with self._caplog.at_level(logging.ERROR): - with pytest.raises(ValueError, match="No supported files found"): - _ = load_readers(filenames=filenames, reader="avhrr_l1b_eps") + with satpy.config.set({"config_path": [str(self._tmp_path)]}): + with pytest.raises(ValueError, match="No supported files found"): + _ = load_readers(filenames=filenames, reader="nonreader") assert error_message in self._caplog.text