Fix/atomic parse cache writes - #98
Open
hwendler wants to merge 4 commits into
Open
Conversation
Cache entries were written straight to their target path. A process that died while serialising - a worker hitting a wall-clock limit or being preempted, which is routine when the cache is populated from a batch scheduler - left a truncated file behind that subsequent runs accepted as a valid cache entry, so the failure surfaced later as an unrelated parse error. Entries are now written to a temporary file that is moved into place once complete. The temporary name carries host and process id so that several workers sharing a cache directory, possibly on a network filesystem, cannot overwrite each other's partial writes. An existing entry is not normally rewritten, but two workers can pass that check at the same time and both proceed, so the move has to tolerate an occupied destination: Path.replace does, whereas Path.rename raises on Windows in that case. Compression is now passed explicitly. pandas infers it from the file name, and the temporary name does not carry the suffix that the destination has, so without this the entries would silently be stored uncompressed. It is derived from the destination, which keeps the stored format unchanged. Tests cover an interrupted write leaving neither a cache entry nor a temporary file, a destination created concurrently, and the stored format still being gzip compressed.
Running `make format` reformats this file: the html_js_files entry uses five spaces of indentation and single quotes, and the file lacks a trailing newline. The change is whitespace and quoting only. Kept as a separate commit so that the accompanying cache fix stays confined to its own scope.
Collaborator
|
Thanks for this fix! Just kicked-off the tests - once those pass likely good to merge |
nscorley
requested changes
Aug 18, 2026
nscorley
left a comment
Collaborator
There was a problem hiding this comment.
Thank you very much for catching this problem and putting up this PR! A few relatively minor comments, then would be excited to merge!
Co-authored-by: Nathaniel Corley <nscorley@gmail.com>
Handle every suffix utils/compression recognises (.gz, .gzip, .zst) instead of .gz alone, so an entry is never written uncompressed under a compressed name. Note pandas infers .gz and .zst but not .gzip. Drop two tests that add no coverage: the gzip-format test, since read_pickle raises BadGzipFile on an uncompressed .pkl.gz and the end-to-end cache test already round-trips; and the concurrent-destination test, which only discriminates on Windows while CI runs ubuntu-latest. Co-Authored-By: Claude Opus 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.
📋 PR Checklist
This PR is tagged as a draft if it is still under development and not ready for review.
I have ensured that all my commits follow angular commit message conventions.
I have run
make formaton the codebase before submitting the PR (this autoformats the code and lints it).I have named the PR in angular PR message format as well (c.f. above), with a sensible tag line that summarizes all the changes in the PR.
ℹ️ PR Description
What changes were made and why?
parse() wrote cache entries straight to their target path. If the process died while serialising, the truncated file stayed on disk and every later run accepted it as a valid cache entry, so the real failure surfaced much later as a confusing parse error somewhere else.
This is not a rare corner case where the cache is populated from a batch scheduler, which is the setting the sharded cache directory appears designed for. Workers reach a wall-clock limit, get preempted, or are killed by the OOM handler, and any of those can land in the middle of a write. We hit it repeatedly while parsing the PDB with 64 workers against a shared cache on a network filesystem, and it cost a fair amount of time to diagnose because the symptom appeared in an unrelated structure days later.
Entries are now written to a temporary file and moved into place once complete, so an interrupted write leaves either the previous entry or nothing at all. Two details are worth pointing out:
The temporary name carries host and process id, so that concurrent workers sharing a cache directory cannot overwrite each other's partial writes. The move uses Path.replace rather than Path.rename: an existing entry is not normally rewritten, but two workers can pass that check at the same time and both go on to write, so the second move finds its destination occupied. Path.replace overwrites it, while Path.rename would raise FileExistsError on Windows and strand the temporary file.
Compression is now passed explicitly. pandas infers it from the file name, and _build_cache_file_path produces names ending in .pkl.gz, so entries have always been gzip compressed. The temporary name does not carry that suffix, so without passing the compression the entries would silently have been stored uncompressed. It is derived from the destination, so the stored format is unchanged.
The change is confined to the cache write. Parsing, the cache key and the read path are untouched, and no public signature changes.
How were the changes tested?
One test in tests/io/components/test_caching.py, using a structure from the checked-in test data: an interrupted write leaves neither a cache entry nor a temporary file. Verified to fail when the write is pointed back at the destination directly.
Two further tests were dropped during review as they added no coverage: the stored-format test, since read_pickle raises BadGzipFile on an uncompressed .pkl.gz and the existing end-to-end test already round-trips through the cache; and a concurrent-destination test, which only discriminates on Windows while CI runs ubuntu-latest.
The compression mapping covers the suffixes utils/compression.py recognises. _build_cache_file_path produces .pkl.gz today, so .gzip and .zst are not reachable through the normal path and are not separately tested.
Additional Notes
The one failing test, test_regression_against_stored_result[1twr], is unrelated and pre-existing: it compares a freshly parsed structure against a stored pickle and reports a bond mismatch at 232 indices, and it fails identically on production without this branch. It also parses without any cache arguments, so none of the code touched here runs in it. Flagging it in case it is news; the stored reference may simply predate a change in bond assignment.
The second commit only applies make format to docs/conf.py, which had drifted on production (indentation, quote style, missing trailing newline). It is kept separate so the fix stays scoped.