Add data caching system with disk persistence (Issue #50 Task 2.1)#58
Merged
Conversation
Previously, get_thredds_server() ran socket.gethostbyname() on every call to detect FRF vs external network. This adds module-level caching so the IP address and server selection are computed once at first use. Thread safety improvements: - Use threading.RLock (reentrant lock) to prevent deadlock when get_thredds_server() calls _get_cached_ip() while holding the lock - Double-checked locking pattern for safe concurrent initialization - Catch OSError (Python 3 standard) instead of socket.error Input validation: - Validate ip_address is a string to prevent type confusion attacks - Return safe defaults (empty string, False) for invalid inputs Changes: - Add _cache_lock (RLock), _cached_ip_address, _cached_server_result - Add _get_cached_ip() helper for lazy thread-safe IP detection - Modify get_thredds_server() to return cached result for default case - Modify is_frf_network() to use cached IP with type validation - Add clear_server_cache() for testing and network change scenarios - Add 9 tests for caching behavior including thread safety Addresses issue #50 task 2.1. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implements a comprehensive data caching system for murgtools that stores retrieved data to disk to avoid repeated network requests. Features: - Caching is OFF by default, must be explicitly enabled - Enable via code: enable_cache() or DataCache(enabled=True) - Enable via environment: MURGTOOLS_CACHE_ENABLED=1 - Default cache directory: /data/getdata (configurable) - Default TTL: 180 days / 6 months (configurable) - Force refresh: cache.get(..., force_refresh=True) - Clear all or by age: clear_cache(older_than_days=30) - Thread-safe with RLock Configuration options: - cache_dir: Custom cache directory path - ttl_days: Time-to-live in days (0 = never expire) - Environment variables for all settings New files: - murgtools/cache.py: DataCache class and helper functions - tests/test_cache.py: 34 comprehensive tests Addresses issue #50 (partial - data caching infrastructure). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds caching support to getObs for caching retrieved oceanographic data
to disk. Caching is OFF by default and must be explicitly enabled.
getObs class changes:
- Add use_cache, cache_dir, cache_ttl_days parameters to __init__
- Add force_refresh parameter to control cache bypass
- Add _get_with_cache() helper method for cache integration
- Refactor getWaveData() to use caching via _getWaveData_impl()
Cache module improvements:
- Add NumpyJSONEncoder for handling numpy types in metadata
- Fix JSON serialization of numpy int64/float64 values
Usage example:
obs = getObs(d1, d2, use_cache=True, cache_dir='/data/getdata')
data = obs.getWaveData(gaugenumber='waverider-26m')
# Second call returns cached data:
data = obs.getWaveData(gaugenumber='waverider-26m')
# Force refresh:
data = obs.getWaveData(gaugenumber='waverider-26m', force_refresh=True)
Tests:
- Add 5 integration tests for getObs caching
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extends caching integration to all commonly used data retrieval methods
in the getObs class. Each method now accepts force_refresh parameter.
Methods with caching support:
- getWaveData (already done in previous commit)
- getCurrents - ocean currents data
- getWind - wind measurements
- getWL - water level (NOAA tide)
- getGaugeWL - gauge water level
- getCTD - CTD measurements
- getALT - altimeter data
- getLidarRunup - lidar runup measurements
- getBathyTransectFromNC - bathymetry transects
Each method follows the same pattern:
1. Add force_refresh parameter
2. Create _impl method with actual logic
3. Wrap with _get_with_cache helper
Usage:
obs = getObs(d1, d2, use_cache=True)
# All these calls are now cacheable:
waves = obs.getWaveData(gaugenumber='waverider-26m')
currents = obs.getCurrents(gaugenumber='awac-11m')
wind = obs.getWind()
wl = obs.getWL()
bathy = obs.getBathyTransectFromNC()
# Force refresh any cached data:
waves = obs.getWaveData(gaugenumber='waverider-26m', force_refresh=True)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in, disk-persistent caching layer to reduce repeated network requests in murgtools, and adds module-level caching for THREDDS server detection (Issue #50 Task 2.1). It also updates getObs retrieval methods to support cached fetches with TTL and a per-call force_refresh override, plus comprehensive unit tests.
Changes:
- Add
murgtools.cachewith a disk-backedDataCache(TTL, force refresh, thread-safety) and convenience helpers exported at package level. - Cache THREDDS server auto-detection results in
murgtools.configwith a thread-safe module-level cache and test coverage. - Integrate optional caching into
getObsmethods ingetDataFRF.pyand add extensive tests for cache behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
murgtools/config.py |
Adds thread-safe module-level caching for THREDDS server auto-detection and cache reset helper. |
murgtools/cache.py |
New disk-persistent caching implementation (metadata + pickle payload) with TTL and global-cache helpers. |
murgtools/getdata/getDataFRF.py |
Adds use_cache/TTL configuration to getObs and wraps major getters with a cache helper. |
murgtools/__init__.py |
Exposes cache APIs (DataCache, get_cache, enable_cache, disable_cache, clear_cache) at package import level. |
tests/test_config.py |
Adds server-detection caching tests (including thread-safety expectations). |
tests/test_cache.py |
Adds extensive unit tests for DataCache, env var config, global cache helpers, and getObs integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+57
to
+64
| def default(self, obj): | ||
| if isinstance(obj, np.integer): | ||
| return int(obj) | ||
| if isinstance(obj, np.floating): | ||
| return float(obj) | ||
| if isinstance(obj, np.ndarray): | ||
| return obj.tolist() | ||
| return super().default(obj) |
Comment on lines
+253
to
+257
| if extra_params: | ||
| # Sort keys for deterministic ordering | ||
| sorted_params = sorted(extra_params.items()) | ||
| key_parts.append(json.dumps(sorted_params, sort_keys=True)) | ||
|
|
Comment on lines
+295
to
+309
| with self._lock: | ||
| # Check cache unless force_refresh is requested | ||
| if not force_refresh: | ||
| cached_data = self._read_cache(cache_key) | ||
| if cached_data is not None: | ||
| return cached_data | ||
|
|
||
| # Fetch fresh data | ||
| data = fetch_func() | ||
|
|
||
| # Store in cache | ||
| if data is not None: | ||
| self._write_cache(cache_key, data, data_source, time_range, extra_params) | ||
|
|
||
| return data |
Fixes: 1. Remove unused `get_cache` import from getDataFRF.py 2. NumpyJSONEncoder: Add fallback to str() for unknown types (datetime, Path, etc.) 3. _generate_cache_key: Use NumpyJSONEncoder for consistent serialization 4. DataCache.get(): Release lock during fetch_func() to allow concurrent fetches Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Comment on lines
+337
to
+339
| # Check if cache files exist | ||
| if not data_path.exists() or not meta_path.exists(): | ||
| return None |
Comment on lines
+353
to
+356
| # Read data | ||
| with open(data_path, 'rb') as f: | ||
| data = pickle.load(f) | ||
|
|
Comment on lines
+1
to
+5
| { | ||
| "vscodeInstanceId": "vscode-2474-1774366165262", | ||
| "port": 8890, | ||
| "pid": 2474, | ||
| "workspacePath": "/home/slug/repos/murgTools", |
Comment on lines
+1
to
+5
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(gh pr *)", | ||
| "Bash(pip show *)", |
Comment on lines
+321
to
+325
| # Include server in extra_params for cache key | ||
| cache_params = {'server': self.server} | ||
| if extra_params: | ||
| cache_params.update(extra_params) | ||
|
|
Comment on lines
+182
to
+194
| # Determine TTL | ||
| if ttl_days is not None: | ||
| self._ttl_days = ttl_days | ||
| else: | ||
| env_ttl = os.environ.get(config.ENV_CACHE_TTL_DAYS) | ||
| if env_ttl: | ||
| try: | ||
| self._ttl_days = int(env_ttl) | ||
| except ValueError: | ||
| logger.warning(f"Invalid TTL value '{env_ttl}', using default") | ||
| self._ttl_days = config.DEFAULT_CACHE_TTL_DAYS | ||
| else: | ||
| self._ttl_days = config.DEFAULT_CACHE_TTL_DAYS |
Comment on lines
+238
to
+248
| def _ensure_cache_dir(self): | ||
| """Create cache directory if it doesn't exist.""" | ||
| try: | ||
| self._cache_dir.mkdir(parents=True, exist_ok=True) | ||
| except PermissionError as e: | ||
| logger.warning(f"Cannot create cache directory {self._cache_dir}: {e}") | ||
| logger.warning("Caching will be disabled") | ||
| self._enabled = False | ||
| except OSError as e: | ||
| logger.warning(f"Error creating cache directory {self._cache_dir}: {e}") | ||
| self._enabled = False |
Comment on lines
+1
to
+7
| { | ||
| "vscodeInstanceId": "vscode-2474-1774366165262", | ||
| "port": 8890, | ||
| "pid": 2474, | ||
| "workspacePath": "/home/slug/repos/murgTools", | ||
| "workspaceName": "murgTools" | ||
| } No newline at end of file |
Comment on lines
+1
to
+10
| { | ||
| "permissions": { | ||
| "allow": [ | ||
| "Bash(gh pr *)", | ||
| "Bash(pip show *)", | ||
| "Bash(find /home/slug/repos/murgTools -maxdepth 3 -name \"*.tif\" 2>&1 | head -20; ls /tmp/*.tif 2>/dev/null | head -5)", | ||
| "Read(//tmp/**)" | ||
| ] | ||
| } | ||
| } |
- Remove unused timedelta import from cache.py - Fix thread safety in _get_cached_ip() by always using lock (clear_server_cache can reset the value, so lock-free fast path was unsafe) - Remove accidentally committed .claude/ and .mcp-debug-tools/ directories - Add .claude/ and .mcp-debug-tools/ to .gitignore Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused pickle import from test_cache.py - Use TTL setter in __init__ for validation (prevents negative TTL) - Include resolved server URL in cache key (not just self.server which can be None for auto-detect, causing cache collisions across networks) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Features
getObs(d1, d2, use_cache=True)MURGTOOLS_CACHE_ENABLED=1/data/getdata(configurable viacache_dirorMURGTOOLS_CACHE_DIR)cache_ttl_daysorMURGTOOLS_CACHE_TTL_DAYS)method(..., force_refresh=True)Methods with Caching Support
getWaveData- Wave measurementsgetCurrents- Ocean currentsgetWind- Wind datagetWL- Water level (NOAA tide)getGaugeWL- Gauge water levelgetCTD- CTD measurementsgetALT- Altimeter datagetLidarRunup- Lidar runupgetBathyTransectFromNC- Bathymetry transectsUsage Example
Changes
DataCacheclass and helper functionsgetObsclass and all major get methodsTest plan
🤖 Generated with Claude Code