From b3cfbd5a72f50498799333f435f0b40b386bb382 Mon Sep 17 00:00:00 2001 From: teald Date: Tue, 4 Mar 2025 14:00:24 -0800 Subject: [PATCH 1/4] fix(adfactory): Caught exceptions saved In DRAGONS astrodata, exceptions are just passed. This makes certain frequent debugging issues painful, because relevant exceptions are obscured and therefore the source of a (real) problem is lost. This update keeps the errors around, and reports them if no matching astrodata object is found. This was originally logged, but that choice doesn't coincide with DRAGON's logging principles (and it was also too high of an error level, in any case). This solution captures exceptions and reports them (as part of the error message, with stack traces) if the matching process fails. If there are no exceptions, or the process succeeds, behavior is still preserved as it was. --- astrodata/adfactory.py | 42 +++++++++++++++++++------- tests/unit/test_adfactory.py | 57 ++++++++++++++++++++++++++++++++---- 2 files changed, 82 insertions(+), 17 deletions(-) diff --git a/astrodata/adfactory.py b/astrodata/adfactory.py index ffffe790..b8722e62 100644 --- a/astrodata/adfactory.py +++ b/astrodata/adfactory.py @@ -3,6 +3,7 @@ import copy import logging import os +import traceback from contextlib import contextmanager from copy import deepcopy @@ -92,9 +93,12 @@ def _open_file(source): ) # Handle nonexistent files. + # TODO: Factor this out into a higher except statement. if isinstance(err, FileNotFoundError): raise err + print(type(err), err) + else: if hasattr(fp, "close"): fp.close() @@ -172,6 +176,7 @@ def get_astro_data(self, source): An AstroData instance. """ candidates = [] + exception_list = [] with self._open_file(source) as opened: for adclass in self._registry: try: @@ -182,11 +187,17 @@ def get_astro_data(self, source): raise except Exception as err: - LOGGER.error( - "Failed to open %s with %s, got error: %s", - source, - adclass, - err, + exception_list.append( + ( + adclass.__name__, + type(err), + err, + "".join( + traceback.format_exception( + None, err, err.__traceback__ + ) + ).splitlines(), + ) ) # For every candidate in the list, remove the ones that are base @@ -207,7 +218,20 @@ def get_astro_data(self, source): ) if not final_candidates: - raise AstroDataError("No class matches this dataset") + message_lines = ["No class matches this dataset"] + if exception_list: + n_err = len(exception_list) + message_lines.append(f"Got {n_err} exceptions while matching:") + + for adclass, errname, err, trace_lines in exception_list: + message_lines.append(f"+ {adclass}: {errname}: {str(err)}") + message_lines.extend( + f" {trace_line}" for trace_line in trace_lines + ) + + message = "\n".join(message_lines) + + raise AstroDataError(message) return final_candidates[0].read(source) @@ -215,11 +239,7 @@ def get_astro_data(self, source): "Renamed to create_from_scratch, please use that method instead: " "astrodata.factory.AstroDataFactory.create_from_scratch" ) - def createFromScratch( - self, - phu, - extensions=None, - ): # noqa + def createFromScratch(self, phu, extensions=None): # noqa """Create an AstroData object from a collection of objects. Deprecated, see |create_from_scratch|. diff --git a/tests/unit/test_adfactory.py b/tests/unit/test_adfactory.py index b82910f3..4507cbcd 100644 --- a/tests/unit/test_adfactory.py +++ b/tests/unit/test_adfactory.py @@ -1,15 +1,14 @@ -# Disable pylint -# pylint: skip-file - -import pytest - +from copy import deepcopy import os import astrodata from astrodata import adfactory +from astrodata import AstroData from astropy.io import fits +import pytest + factory = adfactory.AstroDataFactory @@ -58,7 +57,6 @@ def example_dir(tmp_path) -> str: os.remove(dirname) os.mkdir(dirname) - return dirname @@ -77,3 +75,50 @@ def test__open_file_file_not_found(nonexistent_file, example_dir): with pytest.raises(FileNotFoundError): with factory._open_file(example_dir) as _: pass + +def test_report_all_exceptions_on_failure_get_astro_data(example_fits_file, monkeypatch, capfd,): + """Tests that all exceptions are reported if file fails to open. + + This test tries to capture errors that were previously discarded. It does + this by checking what is sent to stderr/stdout. + + In the future, when support for python 3.10 is dropped, exception groups + would vastly simplify this. + """ + # Use local adfactory to avoid spoiling the main one. + factory = astrodata.adfactory.AstroDataFactory() + monkeypatch.setattr(astrodata, "factory", factory) + + class AD1(AstroData): + _message = "This_is_exception_1" + @staticmethod + def _matches_data(source): + raise ValueError(AD1._message) + + class AD2(AstroData): + _message = "This_is_exception_2" + @staticmethod + def _matches_data(source): + raise IndexError(AD2._message) + + class AD3(AstroData): + _message = "This_is_exception_3" + @staticmethod + def _matches_data(source): + raise Exception(AD3._message) + + classes = (AD1, AD2, AD3) + + for _cls in classes: + astrodata.factory.add_class(_cls) + + with pytest.raises(astrodata.AstroDataError) as exception_info: + astrodata.from_file(example_fits_file) + + + caught_err = exception_info.value + assert str(caught_err) + assert "No class matches this dataset" in str(caught_err) + + for message in (_cls._message for _cls in classes): + assert message in str(caught_err), str(caught_err) From d49e485e3efec5d26b70026d32e48770e0400758 Mon Sep 17 00:00:00 2001 From: teald Date: Tue, 4 Mar 2025 14:18:16 -0800 Subject: [PATCH 2/4] fix(adfactory): Handle FileNotFoundError explicitly --- astrodata/adfactory.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/astrodata/adfactory.py b/astrodata/adfactory.py index b8722e62..86289ae2 100644 --- a/astrodata/adfactory.py +++ b/astrodata/adfactory.py @@ -84,6 +84,9 @@ def _open_file(source): except KeyboardInterrupt: raise + except FileNotFoundError: + raise + except Exception as err: # noqa LOGGER.error( "Failed to open %s with %s, got error: %s", @@ -92,13 +95,6 @@ def _open_file(source): err, ) - # Handle nonexistent files. - # TODO: Factor this out into a higher except statement. - if isinstance(err, FileNotFoundError): - raise err - - print(type(err), err) - else: if hasattr(fp, "close"): fp.close() From 60bef72680eebc98b174e54709b7c516fc480e5b Mon Sep 17 00:00:00 2001 From: teald Date: Tue, 4 Mar 2025 14:32:22 -0800 Subject: [PATCH 3/4] fix(adfactory): Capture exceptions when opening file Same as with get_astro_data, but for opening the file. --- astrodata/adfactory.py | 35 +++++++++++++++++++++------ tests/unit/test_adfactory.py | 46 +++++++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/astrodata/adfactory.py b/astrodata/adfactory.py index 86289ae2..fa9deb72 100644 --- a/astrodata/adfactory.py +++ b/astrodata/adfactory.py @@ -75,6 +75,7 @@ def _open_file(source): ) # try vs all handlers + exception_list = [] for func in AstroDataFactory._file_openers: try: fp = func(source) @@ -88,11 +89,17 @@ def _open_file(source): raise except Exception as err: # noqa - LOGGER.error( - "Failed to open %s with %s, got error: %s", - source, - func, - err, + exception_list.append( + ( + func.__name__, + type(err), + err, + "".join( + traceback.format_exception( + None, err, err.__traceback__ + ) + ).splitlines(), + ) ) else: @@ -101,9 +108,23 @@ def _open_file(source): return - raise AstroDataError( + message_lines = [ f"No access, or not supported format for: {source}" - ) + ] + + if exception_list: + n_err = len(exception_list) + message_lines.append(f"Got {n_err} exceptions while opening:") + + for adclass, errname, err, trace_lines in exception_list: + message_lines.append(f"+ {adclass}: {errname}: {str(err)}") + message_lines.extend( + f" {trace_line}" for trace_line in trace_lines + ) + + message = "\n".join(message_lines) + + raise AstroDataError(message) yield source diff --git a/tests/unit/test_adfactory.py b/tests/unit/test_adfactory.py index 4507cbcd..8d3a8b2b 100644 --- a/tests/unit/test_adfactory.py +++ b/tests/unit/test_adfactory.py @@ -76,7 +76,7 @@ def test__open_file_file_not_found(nonexistent_file, example_dir): with factory._open_file(example_dir) as _: pass -def test_report_all_exceptions_on_failure_get_astro_data(example_fits_file, monkeypatch, capfd,): +def test_report_all_exceptions_on_failure_get_astro_data(example_fits_file, monkeypatch,): """Tests that all exceptions are reported if file fails to open. This test tries to capture errors that were previously discarded. It does @@ -122,3 +122,47 @@ def _matches_data(source): for message in (_cls._message for _cls in classes): assert message in str(caught_err), str(caught_err) + +def test_report_all_exceptions_on_failure__open_file(example_fits_file, monkeypatch, ): + """Tests that all exceptions are reported if file fails to open. + + This test tries to capture errors that were previously discarded. It does + this by checking what is sent to stderr/stdout. + + In the future, when support for python 3.10 is dropped, exception groups + would vastly simplify this. + """ + # Use local adfactory to avoid spoiling the main one. + factory = astrodata.adfactory.AstroDataFactory() + + def _open1(source): + raise ValueError("Exception_1") + + def _open2(source): + raise IndexError("Exception_2") + + def _open3(source): + raise Exception("Exception_3") + + factory._file_openers = (_open1, _open2, _open3) + + class AD(AstroData): + @staticmethod + def _matches_data(source): + return True + + monkeypatch.setattr(astrodata, "factory", factory) + monkeypatch.setattr(astrodata.adfactory.AstroDataFactory, "_file_openers", factory._file_openers) + + + with pytest.raises(astrodata.AstroDataError) as exception_info: + astrodata.from_file(example_fits_file) + + + caught_err = exception_info.value + assert str(caught_err) + assert "No access, or not supported format for: " in str(caught_err) + + n_openers = len(factory._file_openers) + for message in (f"Exception_{i}" for i in range(1, n_openers+1)): + assert message in str(caught_err) From 8ba8f7e8b26c1c1c75717d268522cd529ece07d6 Mon Sep 17 00:00:00 2001 From: teald Date: Tue, 4 Mar 2025 15:06:58 -0800 Subject: [PATCH 4/4] feat(adfactory): Add debug logging for exceptions. --- astrodata/adfactory.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/astrodata/adfactory.py b/astrodata/adfactory.py index fa9deb72..c165d38c 100644 --- a/astrodata/adfactory.py +++ b/astrodata/adfactory.py @@ -89,6 +89,12 @@ def _open_file(source): raise except Exception as err: # noqa + LOGGER.debug( + "Failed to open %s with %s, got error: %s", + source, + func, + err, + ) exception_list.append( ( func.__name__, @@ -204,6 +210,13 @@ def get_astro_data(self, source): raise except Exception as err: + LOGGER.debug( + "Failed to open %s with %s, got error: %s", + source, + adclass, + err, + ) + exception_list.append( ( adclass.__name__,