Allow remote.py to cache information about the remote files

[wesleyjcole]

Summary

This pull request speeds up the remote-file check that runs at the start of every runreeds.py call by caching information about files that have already been checked. Previously, reeds.remote.download_remote_files() re-hashed every required remote file's full contents on every run, which can be slow for the very large .h5 files.

Technical details

Implementation notes

I added a small local checksum cache (inputs/remote/.remote_files_cache.json, inside the existing gitignored inputs/remote directory). This cache is created the first time that a file is checked and is keyed by each raw file's size and modification time (mtime). Before hashing a file, the file information is checked against the cache. If size and mtime match the cached entry, the cached MD5 is reused instead of re-reading and re-hashing the file. If the file is new, missing from the cache, or its size/mtime changed, the MD5 is recomputed like normal and the cache updated.. The cached MD5 is still compared against inputs/remote_files.csv, so a changed manifest checksum (e.g. a new Zenodo record) still results in a re-download regardless of what is cached.

Validation, testing, and comparison report(s)

I verified that remote.py will still download files that do not exist, and that once information about a file is cached, the file checking is basically instant.

• First run (cold cache): ~75 s on my Windows machine and ~40 s on Kestrel to verify all 7 files used in a Pacific test case.
• Second run (warm cache): ~0 s to verify the same 7 files.

This change does not impact model solutions or outcomes.

Checklist for author

Details to double-check

• Charge code provided to reviewers
• [ ] Included comparison reports for appropriate test cases
• [ ] Documentation updated if necessary

General information to guide review

• Zero impact on results of default case
• No large data file(s) added/modified
• No substantive impact on runtime for full-US reference case
• No substantive impact on folder size for full-US reference case
• No change to process flow (runreeds.py, reeds/core/solve/solve.py)
• No change to code organization
• No change to package requirements (environment.yml or Project.toml)

Did you use LLM tools (chatbot or copilot) in the preparation of this PR? If so, describe how

Yes, I used Claude to brainstorm ways to speed up remote.py checking, and then to implement the code to perform the caching. I also used the LLM to edit and modify the code and comments to better align with what I was looking for from the code changes. Finally, I used the LLM to test outcomes (and separately performed my own testing).

Tag points of contact here if you would like additional review of the relevant parts of the model

[pesap]

I like the overall direction. The cache should make repeated remote-file checks much faster, and keeping it under each worktree’s inputs/remote directory seems reasonable.

A couple of edge cases stood out:

• How is the cache invalidated if a file’s contents change without its size or modification time changing?
• What happens when two processes use the same worktree concurrently? The current read/modify/write flow could leave a partially written or stale JSON cache.

If we are assuming that the remote files are immutable, that assumption should be documented explicitly. Otherwise, we may want stronger invalidation and atomic (possibly locked) cache writes, although that may be overkill for this change.

This is not a blocker for me, but I think the assumption and limitations should be documented somewhere.

I also added a few inline code suggestions.

[patrickbrown4]

File size is not a reliable indicator (two .h5 files with the same shape and data types can have the same size even if the data differ)

[wesleyjcole]

I've added inode checking as a third element to see if the files are different.

[pesap]

Could we include the file inode in the cache fingerprint? As Patrick noted, file size is not a reliable indicator: two .h5 files with the same shape and data types can have the same size even when their data differ. Size and modification time can also remain unchanged when a file is replaced or modified, allowing a stale MD5 to be reused.

This would not provide an absolute integrity guarantee, but it would catch common file-replacement cases:

         entry is not None
         and entry.get('size') == stat.st_size
         and entry.get('mtime_ns') == stat.st_mtime_ns
+        and entry.get('inode') == stat.st_ino

The inode should also be stored in the cache entry whenever the MD5 is computed, including after a download.

[wesleyjcole]

Yes - added. Thanks for this third check. I wasn't aware of it as an option.

[pesap]

Could we avoid silently replacing an unreadable cache with {}? A malformed cache or read error currently looks like a normal cache miss, which hides corruption and may cause every remote file to be rehashed. Since the cache is only an optimization, rebuilding it may be reasonable, but the recovery should be explicit and visible.

     if CACHE_PATH.is_file():
+        msg = f'Invalid remote-file cache: {CACHE_PATH}'
         try:
-            with open(CACHE_PATH, 'r') as f:
+            with CACHE_PATH.open('r', encoding='utf-8') as f:
                 cache = json.load(f)
-        except (json.JSONDecodeError, OSError):
-            cache = {}
+        except json.JSONDecodeError as exc:
+            raise ValueError(msg) from exc
+        except OSError as exc:
+            raise OSError(msg) from exc

This preserves the original exception context while giving both failures a consistent message.

[wesleyjcole]

I'm not too concerned about an explicit error message because if it fails to read the cache it just defaults back to reading in the full file like normal, so there is nothing really lost. I've added more explicit error reporting, but not full error reporting because that feels like overkill for this.

[pesap]

Could we write the cache atomically? If the process is interrupted during json.dump(), the next run may see a partially written JSON file. This also relates to the concurrent use case I mentioned in the general comment.

One possible implementation is:

with tempfile.NamedTemporaryFile(
    mode='w',
    encoding='utf-8',
    dir=CACHE_PATH.parent,
    prefix=f'.{CACHE_PATH.name}.',
    delete=False,
) as f:
    json.dump(cache, f)
    f.flush()
    os.fsync(f.fileno())
    temp_path = Path(f.name)

temp_path.replace(CACHE_PATH)

This ensures readers see either the old complete cache or the new complete cache, rather than a partially written file. A lock would still be needed if concurrent cache updates are supported.

[wesleyjcole]

I changed the write to make it happen atomically. This risk is still low for this because a corrupted cache file just defaults back to the standard treatment, so it just means you'll lose out on the 30-80 s speedup.

[pesap]

Could we avoid adding another private method here? I do not like private methods for this kind of local logic, and LLMs tend to introduce them by default. This logic is only used inside download_remote_files(), so keeping it inline would avoid creating another implementation boundary. If reuse is intended, it should be a public typed helper instead.

One possible inline replacement is:

md5 = ''

if rawpath.is_file():
    stat = rawpath.stat()
    entry = cache.get(str(rawpath))

    if (
        entry is not None
        and entry['size'] == stat.st_size
        and entry['mtime_ns'] == stat.st_mtime_ns
    ):
        md5 = entry['md5']
    else:
        md5 = get_md5sum(rawpath)

This makes the cache lookup behavior visible at the point where the download decision is made.

[wesleyjcole]

The function is called twice, so keeping it as a function seems reasonable to me. I don't see a downside to keeping it as a private method.

[pesap]

I think we should add testing for this :) In particular, it would be useful to cover cache hits, cache misses, malformed cache JSON, changed file metadata, and checksum mismatches. This optimization changes the existing integrity check, so the tests should show that it speeds up valid files without allowing invalid files to pass.

[wesleyjcole]

I think we should add testing for this :) In particular, it would be useful to cover cache hits, cache misses, malformed cache JSON, changed file metadata, and checksum mismatches. This optimization changes the existing integrity check, so the tests should show that it speeds up valid files without allowing invalid files to pass.

I don't understand the value of adding automated testing for this? It seems like one of those files that would get put in and then never touched again. It's easy to create, so happy to add it if you think that would still be a good thing to do.

[wesleyjcole]

I think we should add testing for this :) In particular, it would be useful to cover cache hits, cache misses, malformed cache JSON, changed file metadata, and checksum mismatches. This optimization changes the existing integrity check, so the tests should show that it speeds up valid files without allowing invalid files to pass.

I don't understand the value of adding automated testing for this? It seems like one of those files that would get put in and then never touched again. It's easy to create, so happy to add it if you think that would still be a good thing to do.

I thought through this one some more and decided that tests would 1) make it easier to review the PR and 2) even if they get stale, so really cause issues, so why not add them. I've added a new test_remote_cache.py.