From dc9155729ad29ea10c76245e9041db0462438a4e Mon Sep 17 00:00:00 2001 From: Spicer Bak Date: Thu, 16 Jul 2026 13:47:26 -0400 Subject: [PATCH 1/5] Add helper functions for network fallback patterns (Issue #50 Task 2.4) Consolidates duplicate try-except fallback patterns into reusable helpers: - open_dataset_with_fallback(): FRF -> CHL server fallback pattern - open_dataset_with_retry(): Retry loop with configurable attempts/delay Refactored: - getWaveGaugeLoc(): Now uses open_dataset_with_fallback() - getModelField(): Now uses open_dataset_with_retry() (was 15-iteration loop) Added 10 tests for helper functions covering: - Primary/fallback URL handling - IOError and OSError handling - Retry logic and config defaults - Error message printing Co-Authored-By: Claude Opus 4.5 --- murgtools/getdata/getDataFRF.py | 103 +++++++++++++++++++++---- tests/test_getDataFRF.py | 128 +++++++++++++++++++++++++++++++- 2 files changed, 214 insertions(+), 17 deletions(-) diff --git a/murgtools/getdata/getDataFRF.py b/murgtools/getdata/getDataFRF.py index b24b004..0f3d6eb 100755 --- a/murgtools/getdata/getDataFRF.py +++ b/murgtools/getdata/getDataFRF.py @@ -25,6 +25,83 @@ from murgtools import config from murgtools.exceptions import InvalidGaugeError +# ============================================================================= +# Helper Functions for Network Fallback Patterns +# ============================================================================= + + +def open_dataset_with_fallback(primary_url, fallback_url, error_message=None): + """Open a NetCDF dataset with automatic fallback to secondary URL. + + Attempts to open a NetCDF dataset from the primary URL. If that fails + with an IOError (network issue, file not found), automatically tries + the fallback URL. + + Args: + primary_url (str): Primary URL to attempt first (e.g., FRF local server). + fallback_url (str): Fallback URL if primary fails (e.g., CHL public server). + error_message (str, optional): Custom error message to print if both fail. + If None, no message is printed on failure. + + Returns: + netCDF4.Dataset or None: The opened dataset, or None if both URLs fail. + + Example: + >>> ncfile = open_dataset_with_fallback( + ... self.FRFdataloc + self.dataloc, + ... self.chlDataLoc + self.dataloc + ... ) + """ + try: + return nc.Dataset(primary_url) + except (IOError, OSError): + try: + return nc.Dataset(fallback_url) + except (IOError, OSError): + if error_message: + print(error_message) + return None + + +def open_dataset_with_retry(url, max_retries=None, retry_delay=5, error_message=None): + """Open a NetCDF dataset with retry logic for transient failures. + + Attempts to open a NetCDF dataset, retrying on IOError up to max_retries + times with a delay between attempts. + + Args: + url (str): URL of the NetCDF file to open. + max_retries (int, optional): Maximum number of retry attempts. + Defaults to config.MAX_RETRY_ATTEMPTS. + retry_delay (int or float): Seconds to wait between retries. Default 5. + error_message (str, optional): Custom error message format string. + Can include {url}, {attempt}, {max_retries} placeholders. + + Returns: + netCDF4.Dataset or None: The opened dataset, or None if all retries fail. + + Example: + >>> ncfile = open_dataset_with_retry( + ... "http://server/data.nc", + ... max_retries=3, + ... retry_delay=10 + ... ) + """ + if max_retries is None: + max_retries = config.MAX_RETRY_ATTEMPTS + + for attempt in range(max_retries): + try: + return nc.Dataset(url) + except (IOError, OSError): + if attempt < max_retries - 1: + msg = error_message or 'Error reading {url}, trying again {attempt}/{max_retries}' + print(msg.format(url=url, attempt=attempt + 1, max_retries=max_retries)) + time.sleep(retry_delay) + + return None + + def gettime(allEpoch, epochStart, epochEnd, indexRef=0): """This function opens the netcdf file, and retrieves time. @@ -1518,10 +1595,12 @@ def getWaveGaugeLoc(self, gaugenumber): see help on self.waveGaugeURLlookup for gauge keys """ self._waveGaugeURLlookup(gaugenumber) - try: - ncfile = nc.Dataset(self.FRFdataloc + self.dataloc) - except IOError: - ncfile = nc.Dataset(self.chlDataLoc + self.dataloc) + ncfile = open_dataset_with_fallback( + self.FRFdataloc + self.dataloc, + self.chlDataLoc + self.dataloc + ) + if ncfile is None: + raise IOError(f"Could not open dataset for gauge {gaugenumber}") out = {'Lat': ncfile['latitude'][:], 'Lon': ncfile['longitude'][:]} return out @@ -2946,18 +3025,10 @@ def getModelField(self, var, prefix, local=True, ijLoc=None, model='STWAVE', **k model, prefix, grid, grid) elif model == 'CMS': # this is standard operational model url Structure fname = self.crunchDataLoc + u'waveModels/%s/%s/Field/Field.ncml' % (model, prefix) - finished = False - n = 0 - while not finished and n < 15: - try: - ncfile = nc.Dataset(fname) - finished = True - except IOError: - print('Error reading {}, trying again'.format(fname)) - time.sleep(10) - n += 1 - if not finished: - raise (RuntimeError, 'Data not accessible right now') + + ncfile = open_dataset_with_retry(fname, max_retries=15, retry_delay=10) + if ncfile is None: + raise RuntimeError('Data not accessible right now: {}'.format(fname)) assert var in ncfile.variables.keys(), 'variable called is not in file please use\n%s' % \ ncfile.variables.keys() diff --git a/tests/test_getDataFRF.py b/tests/test_getDataFRF.py index 5a9ce56..76e2b77 100644 --- a/tests/test_getDataFRF.py +++ b/tests/test_getDataFRF.py @@ -9,10 +9,136 @@ from unittest.mock import MagicMock, patch, PropertyMock from pyproj import Transformer -from murgtools.getdata.getDataFRF import get_geotiff_extent, gettime, removeDuplicatesFromDictionary +from murgtools.getdata.getDataFRF import ( + get_geotiff_extent, gettime, removeDuplicatesFromDictionary, + open_dataset_with_fallback, open_dataset_with_retry +) from murgtools.exceptions import InvalidGaugeError +class TestOpenDatasetWithFallback: + """Tests for the open_dataset_with_fallback helper function.""" + + def test_returns_dataset_from_primary_url(self): + """Test that primary URL is tried first and returned on success.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + mock_nc = MagicMock() + mock_dataset.return_value = mock_nc + + result = open_dataset_with_fallback('http://primary/data.nc', 'http://fallback/data.nc') + + assert result == mock_nc + mock_dataset.assert_called_once_with('http://primary/data.nc') + + def test_falls_back_to_secondary_url_on_ioerror(self): + """Test that fallback URL is used when primary fails.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + mock_nc = MagicMock() + mock_dataset.side_effect = [IOError("Primary failed"), mock_nc] + + result = open_dataset_with_fallback('http://primary/data.nc', 'http://fallback/data.nc') + + assert result == mock_nc + assert mock_dataset.call_count == 2 + mock_dataset.assert_any_call('http://primary/data.nc') + mock_dataset.assert_any_call('http://fallback/data.nc') + + def test_returns_none_when_both_fail(self): + """Test that None is returned when both URLs fail.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + mock_dataset.side_effect = IOError("Failed") + + result = open_dataset_with_fallback('http://primary/data.nc', 'http://fallback/data.nc') + + assert result is None + + def test_prints_error_message_when_both_fail(self, capsys): + """Test that error message is printed when both URLs fail.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + mock_dataset.side_effect = IOError("Failed") + + open_dataset_with_fallback( + 'http://primary/data.nc', + 'http://fallback/data.nc', + error_message="Custom error message" + ) + + captured = capsys.readouterr() + assert "Custom error message" in captured.out + + def test_handles_oserror_as_well_as_ioerror(self): + """Test that OSError is also caught (Python 3 compatibility).""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + mock_nc = MagicMock() + mock_dataset.side_effect = [OSError("Primary failed"), mock_nc] + + result = open_dataset_with_fallback('http://primary/data.nc', 'http://fallback/data.nc') + + assert result == mock_nc + + +class TestOpenDatasetWithRetry: + """Tests for the open_dataset_with_retry helper function.""" + + def test_returns_dataset_on_first_try(self): + """Test that dataset is returned immediately on success.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + mock_nc = MagicMock() + mock_dataset.return_value = mock_nc + + result = open_dataset_with_retry('http://server/data.nc', max_retries=3) + + assert result == mock_nc + mock_dataset.assert_called_once() + + def test_retries_on_ioerror(self): + """Test that function retries on IOError.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + with patch('murgtools.getdata.getDataFRF.time.sleep'): + mock_nc = MagicMock() + mock_dataset.side_effect = [IOError("Fail 1"), IOError("Fail 2"), mock_nc] + + result = open_dataset_with_retry('http://server/data.nc', max_retries=3, retry_delay=0) + + assert result == mock_nc + assert mock_dataset.call_count == 3 + + def test_returns_none_after_max_retries(self): + """Test that None is returned after all retries exhausted.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + with patch('murgtools.getdata.getDataFRF.time.sleep'): + mock_dataset.side_effect = IOError("Always fails") + + result = open_dataset_with_retry('http://server/data.nc', max_retries=3, retry_delay=0) + + assert result is None + assert mock_dataset.call_count == 3 + + def test_uses_config_default_for_max_retries(self): + """Test that config.MAX_RETRY_ATTEMPTS is used by default.""" + from murgtools import config + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + with patch('murgtools.getdata.getDataFRF.time.sleep'): + mock_dataset.side_effect = IOError("Always fails") + + open_dataset_with_retry('http://server/data.nc', retry_delay=0) + + assert mock_dataset.call_count == config.MAX_RETRY_ATTEMPTS + + def test_prints_error_message_on_retry(self, capsys): + """Test that error message is printed on each retry.""" + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + with patch('murgtools.getdata.getDataFRF.time.sleep'): + mock_nc = MagicMock() + mock_dataset.side_effect = [IOError("Fail"), mock_nc] + + open_dataset_with_retry('http://server/data.nc', max_retries=2, retry_delay=0) + + captured = capsys.readouterr() + assert "Error reading" in captured.out + assert "http://server/data.nc" in captured.out + + class TestGettime: """Tests for the gettime function.""" From 553ee8be9442c64c8909bb5c1608f2cc5079611a Mon Sep 17 00:00:00 2001 From: Spicer Bak Date: Thu, 16 Jul 2026 14:12:04 -0400 Subject: [PATCH 2/5] Improve helper functions API and use proper logging Refinements based on code review: - open_dataset_with_fallback(): Add raise_on_failure parameter for Pythonic exception handling. Default returns None, but callers can opt-in to exceptions. - open_dataset_with_retry(): Use logging module instead of print() for proper log levels (WARNING for retries, ERROR for final failure). Store last error for better error messages. - Update getWaveGaugeLoc() to use raise_on_failure=True for cleaner code - Update tests to use caplog fixture for logging assertions - Add test for raise_on_failure behavior - Add test for final failure logging Co-Authored-By: Claude Opus 4.5 --- murgtools/getdata/getDataFRF.py | 47 +++++++++++++++++-------------- tests/test_getDataFRF.py | 49 +++++++++++++++++++++------------ 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/murgtools/getdata/getDataFRF.py b/murgtools/getdata/getDataFRF.py index 0f3d6eb..d83c72e 100755 --- a/murgtools/getdata/getDataFRF.py +++ b/murgtools/getdata/getDataFRF.py @@ -30,7 +30,7 @@ # ============================================================================= -def open_dataset_with_fallback(primary_url, fallback_url, error_message=None): +def open_dataset_with_fallback(primary_url, fallback_url, raise_on_failure=False): """Open a NetCDF dataset with automatic fallback to secondary URL. Attempts to open a NetCDF dataset from the primary URL. If that fails @@ -40,42 +40,46 @@ def open_dataset_with_fallback(primary_url, fallback_url, error_message=None): Args: primary_url (str): Primary URL to attempt first (e.g., FRF local server). fallback_url (str): Fallback URL if primary fails (e.g., CHL public server). - error_message (str, optional): Custom error message to print if both fail. - If None, no message is printed on failure. + raise_on_failure (bool): If True, raises IOError when both URLs fail. + If False (default), returns None on failure. Returns: - netCDF4.Dataset or None: The opened dataset, or None if both URLs fail. + netCDF4.Dataset: The opened dataset. + + Raises: + IOError: If both URLs fail and raise_on_failure=True. Example: >>> ncfile = open_dataset_with_fallback( ... self.FRFdataloc + self.dataloc, - ... self.chlDataLoc + self.dataloc + ... self.chlDataLoc + self.dataloc, + ... raise_on_failure=True ... ) """ try: return nc.Dataset(primary_url) except (IOError, OSError): - try: - return nc.Dataset(fallback_url) - except (IOError, OSError): - if error_message: - print(error_message) - return None + pass # Fall through to try fallback + try: + return nc.Dataset(fallback_url) + except (IOError, OSError) as e: + if raise_on_failure: + raise IOError(f"Failed to open dataset from both {primary_url} and {fallback_url}") from e + return None -def open_dataset_with_retry(url, max_retries=None, retry_delay=5, error_message=None): + +def open_dataset_with_retry(url, max_retries=None, retry_delay=5): """Open a NetCDF dataset with retry logic for transient failures. Attempts to open a NetCDF dataset, retrying on IOError up to max_retries - times with a delay between attempts. + times with a delay between attempts. Logs progress using the logging module. Args: url (str): URL of the NetCDF file to open. max_retries (int, optional): Maximum number of retry attempts. Defaults to config.MAX_RETRY_ATTEMPTS. retry_delay (int or float): Seconds to wait between retries. Default 5. - error_message (str, optional): Custom error message format string. - Can include {url}, {attempt}, {max_retries} placeholders. Returns: netCDF4.Dataset or None: The opened dataset, or None if all retries fail. @@ -90,15 +94,17 @@ def open_dataset_with_retry(url, max_retries=None, retry_delay=5, error_message= if max_retries is None: max_retries = config.MAX_RETRY_ATTEMPTS + last_error = None for attempt in range(max_retries): try: return nc.Dataset(url) - except (IOError, OSError): + except (IOError, OSError) as e: + last_error = e if attempt < max_retries - 1: - msg = error_message or 'Error reading {url}, trying again {attempt}/{max_retries}' - print(msg.format(url=url, attempt=attempt + 1, max_retries=max_retries)) + logging.warning(f"Error reading {url}, attempt {attempt + 1}/{max_retries}. Retrying...") time.sleep(retry_delay) + logging.error(f"Failed to open {url} after {max_retries} attempts: {last_error}") return None @@ -1597,10 +1603,9 @@ def getWaveGaugeLoc(self, gaugenumber): self._waveGaugeURLlookup(gaugenumber) ncfile = open_dataset_with_fallback( self.FRFdataloc + self.dataloc, - self.chlDataLoc + self.dataloc + self.chlDataLoc + self.dataloc, + raise_on_failure=True ) - if ncfile is None: - raise IOError(f"Could not open dataset for gauge {gaugenumber}") out = {'Lat': ncfile['latitude'][:], 'Lon': ncfile['longitude'][:]} return out diff --git a/tests/test_getDataFRF.py b/tests/test_getDataFRF.py index 76e2b77..627469c 100644 --- a/tests/test_getDataFRF.py +++ b/tests/test_getDataFRF.py @@ -43,8 +43,8 @@ def test_falls_back_to_secondary_url_on_ioerror(self): mock_dataset.assert_any_call('http://primary/data.nc') mock_dataset.assert_any_call('http://fallback/data.nc') - def test_returns_none_when_both_fail(self): - """Test that None is returned when both URLs fail.""" + def test_returns_none_when_both_fail_default(self): + """Test that None is returned when both URLs fail (default behavior).""" with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: mock_dataset.side_effect = IOError("Failed") @@ -52,19 +52,20 @@ def test_returns_none_when_both_fail(self): assert result is None - def test_prints_error_message_when_both_fail(self, capsys): - """Test that error message is printed when both URLs fail.""" + def test_raises_when_both_fail_and_raise_on_failure_true(self): + """Test that IOError is raised when both URLs fail and raise_on_failure=True.""" with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: mock_dataset.side_effect = IOError("Failed") - open_dataset_with_fallback( - 'http://primary/data.nc', - 'http://fallback/data.nc', - error_message="Custom error message" - ) + with pytest.raises(IOError) as exc_info: + open_dataset_with_fallback( + 'http://primary/data.nc', + 'http://fallback/data.nc', + raise_on_failure=True + ) - captured = capsys.readouterr() - assert "Custom error message" in captured.out + assert "primary" in str(exc_info.value) + assert "fallback" in str(exc_info.value) def test_handles_oserror_as_well_as_ioerror(self): """Test that OSError is also caught (Python 3 compatibility).""" @@ -125,18 +126,32 @@ def test_uses_config_default_for_max_retries(self): assert mock_dataset.call_count == config.MAX_RETRY_ATTEMPTS - def test_prints_error_message_on_retry(self, capsys): - """Test that error message is printed on each retry.""" + def test_logs_warning_on_retry(self, caplog): + """Test that warning is logged on each retry attempt.""" + import logging with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: with patch('murgtools.getdata.getDataFRF.time.sleep'): mock_nc = MagicMock() mock_dataset.side_effect = [IOError("Fail"), mock_nc] - open_dataset_with_retry('http://server/data.nc', max_retries=2, retry_delay=0) + with caplog.at_level(logging.WARNING): + open_dataset_with_retry('http://server/data.nc', max_retries=2, retry_delay=0) + + assert "Error reading" in caplog.text + assert "http://server/data.nc" in caplog.text - captured = capsys.readouterr() - assert "Error reading" in captured.out - assert "http://server/data.nc" in captured.out + def test_logs_error_on_final_failure(self, caplog): + """Test that error is logged when all retries fail.""" + import logging + with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: + with patch('murgtools.getdata.getDataFRF.time.sleep'): + mock_dataset.side_effect = IOError("Always fails") + + with caplog.at_level(logging.ERROR): + result = open_dataset_with_retry('http://server/data.nc', max_retries=2, retry_delay=0) + + assert result is None + assert "Failed to open" in caplog.text class TestGettime: From b22c3217da5e89509c56c54c8baa91230ece6b9e Mon Sep 17 00:00:00 2001 From: Spicer Bak Date: Thu, 16 Jul 2026 14:21:56 -0400 Subject: [PATCH 3/5] Rename max_retries to max_attempts for clarity Addresses PR #60 review comments: - Rename parameter from max_retries to max_attempts to clarify that it represents total attempts (not additional retries after first try) - Add input validation requiring max_attempts >= 1 - Update docstring to explain semantics (max_attempts=3 means 3 total tries) - Update all callers and tests to use new parameter name Co-Authored-By: Claude Opus 4.5 --- murgtools/getdata/getDataFRF.py | 37 ++++++++++++++++++++------------- tests/test_getDataFRF.py | 24 +++++++++++++-------- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/murgtools/getdata/getDataFRF.py b/murgtools/getdata/getDataFRF.py index d83c72e..d221059 100755 --- a/murgtools/getdata/getDataFRF.py +++ b/murgtools/getdata/getDataFRF.py @@ -69,42 +69,49 @@ def open_dataset_with_fallback(primary_url, fallback_url, raise_on_failure=False return None -def open_dataset_with_retry(url, max_retries=None, retry_delay=5): +def open_dataset_with_retry(url, max_attempts=None, retry_delay=5): """Open a NetCDF dataset with retry logic for transient failures. - Attempts to open a NetCDF dataset, retrying on IOError up to max_retries - times with a delay between attempts. Logs progress using the logging module. + Attempts to open a NetCDF dataset up to max_attempts times, with a delay + between failed attempts. Logs progress using the logging module. Args: url (str): URL of the NetCDF file to open. - max_retries (int, optional): Maximum number of retry attempts. - Defaults to config.MAX_RETRY_ATTEMPTS. - retry_delay (int or float): Seconds to wait between retries. Default 5. + max_attempts (int, optional): Total number of attempts to make (not retries). + For example, max_attempts=3 means try up to 3 times total. + Defaults to config.MAX_RETRY_ATTEMPTS. Must be >= 1. + retry_delay (int or float): Seconds to wait between attempts. Default 5. Returns: - netCDF4.Dataset or None: The opened dataset, or None if all retries fail. + netCDF4.Dataset or None: The opened dataset, or None if all attempts fail. + + Raises: + ValueError: If max_attempts is less than 1. Example: >>> ncfile = open_dataset_with_retry( ... "http://server/data.nc", - ... max_retries=3, + ... max_attempts=3, # Try up to 3 times ... retry_delay=10 ... ) """ - if max_retries is None: - max_retries = config.MAX_RETRY_ATTEMPTS + if max_attempts is None: + max_attempts = config.MAX_RETRY_ATTEMPTS + + if max_attempts < 1: + raise ValueError(f"max_attempts must be >= 1, got {max_attempts}") last_error = None - for attempt in range(max_retries): + for attempt in range(max_attempts): try: return nc.Dataset(url) except (IOError, OSError) as e: last_error = e - if attempt < max_retries - 1: - logging.warning(f"Error reading {url}, attempt {attempt + 1}/{max_retries}. Retrying...") + if attempt < max_attempts - 1: + logging.warning(f"Error reading {url}, attempt {attempt + 1}/{max_attempts}. Retrying...") time.sleep(retry_delay) - logging.error(f"Failed to open {url} after {max_retries} attempts: {last_error}") + logging.error(f"Failed to open {url} after {max_attempts} attempts: {last_error}") return None @@ -3031,7 +3038,7 @@ def getModelField(self, var, prefix, local=True, ijLoc=None, model='STWAVE', **k elif model == 'CMS': # this is standard operational model url Structure fname = self.crunchDataLoc + u'waveModels/%s/%s/Field/Field.ncml' % (model, prefix) - ncfile = open_dataset_with_retry(fname, max_retries=15, retry_delay=10) + ncfile = open_dataset_with_retry(fname, max_attempts=15, retry_delay=10) if ncfile is None: raise RuntimeError('Data not accessible right now: {}'.format(fname)) diff --git a/tests/test_getDataFRF.py b/tests/test_getDataFRF.py index 627469c..ce96862 100644 --- a/tests/test_getDataFRF.py +++ b/tests/test_getDataFRF.py @@ -87,7 +87,7 @@ def test_returns_dataset_on_first_try(self): mock_nc = MagicMock() mock_dataset.return_value = mock_nc - result = open_dataset_with_retry('http://server/data.nc', max_retries=3) + result = open_dataset_with_retry('http://server/data.nc', max_attempts=3) assert result == mock_nc mock_dataset.assert_called_once() @@ -99,23 +99,23 @@ def test_retries_on_ioerror(self): mock_nc = MagicMock() mock_dataset.side_effect = [IOError("Fail 1"), IOError("Fail 2"), mock_nc] - result = open_dataset_with_retry('http://server/data.nc', max_retries=3, retry_delay=0) + result = open_dataset_with_retry('http://server/data.nc', max_attempts=3, retry_delay=0) assert result == mock_nc assert mock_dataset.call_count == 3 - def test_returns_none_after_max_retries(self): - """Test that None is returned after all retries exhausted.""" + def test_returns_none_after_max_attempts(self): + """Test that None is returned after all attempts exhausted.""" with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: with patch('murgtools.getdata.getDataFRF.time.sleep'): mock_dataset.side_effect = IOError("Always fails") - result = open_dataset_with_retry('http://server/data.nc', max_retries=3, retry_delay=0) + result = open_dataset_with_retry('http://server/data.nc', max_attempts=3, retry_delay=0) assert result is None assert mock_dataset.call_count == 3 - def test_uses_config_default_for_max_retries(self): + def test_uses_config_default_for_max_attempts(self): """Test that config.MAX_RETRY_ATTEMPTS is used by default.""" from murgtools import config with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: @@ -135,24 +135,30 @@ def test_logs_warning_on_retry(self, caplog): mock_dataset.side_effect = [IOError("Fail"), mock_nc] with caplog.at_level(logging.WARNING): - open_dataset_with_retry('http://server/data.nc', max_retries=2, retry_delay=0) + open_dataset_with_retry('http://server/data.nc', max_attempts=2, retry_delay=0) assert "Error reading" in caplog.text assert "http://server/data.nc" in caplog.text def test_logs_error_on_final_failure(self, caplog): - """Test that error is logged when all retries fail.""" + """Test that error is logged when all attempts fail.""" import logging with patch('murgtools.getdata.getDataFRF.nc.Dataset') as mock_dataset: with patch('murgtools.getdata.getDataFRF.time.sleep'): mock_dataset.side_effect = IOError("Always fails") with caplog.at_level(logging.ERROR): - result = open_dataset_with_retry('http://server/data.nc', max_retries=2, retry_delay=0) + result = open_dataset_with_retry('http://server/data.nc', max_attempts=2, retry_delay=0) assert result is None assert "Failed to open" in caplog.text + def test_raises_valueerror_for_invalid_max_attempts(self): + """Test that ValueError is raised when max_attempts < 1.""" + import pytest + with pytest.raises(ValueError, match="max_attempts must be >= 1"): + open_dataset_with_retry('http://server/data.nc', max_attempts=0) + class TestGettime: """Tests for the gettime function.""" From 449b7d85c1539f7cf54f968aff363dec7042ffe9 Mon Sep 17 00:00:00 2001 From: Spicer Bak Date: Thu, 16 Jul 2026 14:23:22 -0400 Subject: [PATCH 4/5] Fix docstring accuracy for open_dataset_with_fallback Addresses PR #60 review comments: - Document that both IOError and OSError trigger fallback (not just IOError) - Fix return type to show it can return None when raise_on_failure=False Co-Authored-By: Claude Opus 4.5 --- murgtools/getdata/getDataFRF.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/murgtools/getdata/getDataFRF.py b/murgtools/getdata/getDataFRF.py index d221059..a3407d0 100755 --- a/murgtools/getdata/getDataFRF.py +++ b/murgtools/getdata/getDataFRF.py @@ -34,8 +34,8 @@ def open_dataset_with_fallback(primary_url, fallback_url, raise_on_failure=False """Open a NetCDF dataset with automatic fallback to secondary URL. Attempts to open a NetCDF dataset from the primary URL. If that fails - with an IOError (network issue, file not found), automatically tries - the fallback URL. + with an IOError or OSError (network issue, file not found), automatically + tries the fallback URL. Args: primary_url (str): Primary URL to attempt first (e.g., FRF local server). @@ -44,7 +44,8 @@ def open_dataset_with_fallback(primary_url, fallback_url, raise_on_failure=False If False (default), returns None on failure. Returns: - netCDF4.Dataset: The opened dataset. + netCDF4.Dataset or None: The opened dataset, or None if both URLs fail + and raise_on_failure is False. Raises: IOError: If both URLs fail and raise_on_failure=True. From feefd70f20602dca0a6e9afa126031a06d94444c Mon Sep 17 00:00:00 2001 From: Spicer Bak Date: Thu, 16 Jul 2026 14:40:15 -0400 Subject: [PATCH 5/5] Include exception details in error messages for better diagnostics Addresses PR #60 review comments: - open_dataset_with_fallback: Include both primary and fallback error messages when raise_on_failure=True, showing why each URL failed - open_dataset_with_retry: Include exception message in retry warning to help diagnose DNS, timeout, permission issues etc. Co-Authored-By: Claude Opus 4.5 --- murgtools/getdata/getDataFRF.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/murgtools/getdata/getDataFRF.py b/murgtools/getdata/getDataFRF.py index a3407d0..7cd15d3 100755 --- a/murgtools/getdata/getDataFRF.py +++ b/murgtools/getdata/getDataFRF.py @@ -57,16 +57,21 @@ def open_dataset_with_fallback(primary_url, fallback_url, raise_on_failure=False ... raise_on_failure=True ... ) """ + primary_error = None try: return nc.Dataset(primary_url) - except (IOError, OSError): - pass # Fall through to try fallback + except (IOError, OSError) as e: + primary_error = e # Save for diagnostic message try: return nc.Dataset(fallback_url) - except (IOError, OSError) as e: + except (IOError, OSError) as fallback_error: if raise_on_failure: - raise IOError(f"Failed to open dataset from both {primary_url} and {fallback_url}") from e + raise IOError( + f"Failed to open dataset from both URLs.\n" + f" Primary ({primary_url}): {primary_error}\n" + f" Fallback ({fallback_url}): {fallback_error}" + ) from fallback_error return None @@ -109,7 +114,9 @@ def open_dataset_with_retry(url, max_attempts=None, retry_delay=5): except (IOError, OSError) as e: last_error = e if attempt < max_attempts - 1: - logging.warning(f"Error reading {url}, attempt {attempt + 1}/{max_attempts}. Retrying...") + logging.warning( + f"Error reading {url}, attempt {attempt + 1}/{max_attempts}: {e}. Retrying..." + ) time.sleep(retry_delay) logging.error(f"Failed to open {url} after {max_attempts} attempts: {last_error}")