Repository navigation
fix(model-finetune): fsync cached checkpoints and reject short downloads - #255
Merged
Merged
Conversation
evaluate_community writes a 1.2 GB community checkpoint to shared HOME and hands the PATH to a runtime replica on another node. _write_file_atomic renamed without fsync, so the name went live while the data was still in the writing client's page cache and the remote reader could open a prefix — torch.load then dies with "failed finding central directory", which names a length rather than corrupted bytes. Observed once in the paper campaign on 0.27.13, nondeterministic, with the published pool copy measured whole. Fixing it in the writer covers all three callers (weights, kwargs.json and the community checkpoint) instead of guarding only the reported line. A short GET lands a prefix in exactly the same place with exactly the same symptom, and the reporter's reproduction does not separate the two causes, so both are closed: the checkpoint GETs now verify the declared length, the same contract runtime_cellpose._download_member already implements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Why this is a draft with no version bump. The published head of So the order wants to be: #239 lands → this branch rebases onto it → bump to one above the re-measured floor → upload that combined bundle → mark ready. That is minutes of work and it is queued behind a merge decision, not behind more code. Tracked as backlog #127, paused on exactly that question. For the record, measured rather than assumed: |
…e marker
The zoo cache's hit branch returned a cached checkpoint whenever weights.pt and
kwargs.json both existed, treating kwargs.json as the commit marker for the pair.
That holds only for entries written by a version that has _write_file_atomic.
Before it, both files were written in place, so an interrupted or truncated write
leaves exactly the state the gate reads as committed -- and the cache lives in an
app HOME that is per-application_id on shared storage and survives redeploys, so
such an entry outlives the version that wrote it and is permanent. The write-side
fix in the previous commit stops one being created and says nothing about reading
one; production is pinned to 0.16.1, which has no _write_file_atomic at all.
Record len(content) in kwargs.json and compare it against the file on disk. An
entry with no recorded length is a miss, so a legacy entry re-downloads exactly
once and is self-healing after that. pop() keeps the key out of the returned
contract, which stays {weights_path, model_type}.
_get_verified compared the body against Content-Length but never looked at the status. _http_retry keeps 403/404 in its retry set and returns the last response unraised, and an S3 error document declares its own length correctly — so a few hundred bytes of XML passed the length check and were written as a .pt. torch.load then reports "failed finding central directory", the same symptom as a prefix, which is why the length check reads like it already covers this. Worse on the zoo path, whose hit branch gates on weights_path and meta_path existing: an error body satisfies that gate for every later caller asking for the same model_id, so one transient 403 poisons the cache until the file is removed. The rdf GET was unchecked too, so a 403 body reached yaml.safe_load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Oct 27, 2026
Clearance-time bump for PR #255, measured at this moment rather than read off a row: origin/main's apps/model-finetune/manifest.yaml is 0.29.0 and the published bioimage-io/model-finetune artifact's newest committed version is also 0.29.0 (54 versions), so 0.30.0 is strictly greater than both baselines the two gates compare against. Minor rather than patch because both rows riding this PR change how the app treats data it already has: the zoo cache gate stops trusting kwargs.json as the commit marker for weights.pt (#128), and the checkpoint GET now rejects a body shorter than its declared length instead of caching it (#127). Refs: #127, #128
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.
evaluate_communityhands a 1.2 GB checkpoint path to a runtime replica on a different node after anos.replacewith no fsync, so the reader can open the new name and see a prefix.torch.loadthen dies withPytorchStreamReader failed finding central directory— an error that names a length, not corrupted bytes, because a zip's central directory lives at the end of the file.Reported by the
bioengine-papersession from a real campaign failure on0.27.13; tracked as backlog #127.Why it is the writer, not the reported line
_write_file_atomicis used by three callers, all with the same cross-node exposure:entry.py:548entry.py:554kwargs.json, the commit marker the cache-hit branch gates onentry.py:1692So the fsyncs go inside the writer — data synced before the rename, the containing directory synced after it — and all three are covered by the one change. A guard at the reported call site would have left the other two exposed.
The function was not careless: its docstring reasons explicitly about the concurrency mode it does fix (a uuid temp name plus an atomic rename, so two writers on the request-keyed path cannot expose a prefix). What was missing is durability across nodes, and from inside one machine the two modes are indistinguishable — a local reader is served from the same page cache that makes the remote read short.
Both candidate causes are closed, because the reproduction does not separate them
What is established: the published pool copy is whole (exact recorded byte length, 356 members, every member CRC passing), and the reader got a prefix. Re-running the identical command cleared the failing chunk. That is consistent with a timing window and not with a bad artifact — but a short download lands a prefix in the same cache path with the same symptom, and nobody has reproduced a cross-node prefix read directly. So the checkpoint GETs now verify the declared length through a new
_get_verified, the same contractruntime_cellpose._download_memberalready implements atruntime_cellpose.py:366. Neither gap is proven to be the one that fired; both are real.os.fsync,flush()andO_SYNCappeared zero times inentry.pybefore this change. Positive control that the grep fires at all:apps/cellpose-finetuning/main.py:1267hasos.fsync(f.fileno()).Tests
apps/model-finetune/test_atomic_cache_write.pygains two cases. The fsync one asserts on the syscalls rather than on bytes, for the reason above — a local byte-level read cannot see this defect.test_soup_math.pytorch-gated,test_pool.py:291fixture-gated — both pre-existing)HEAD:apps/model-finetune/entry.py: 2 failed, 3 passed — exactly the two new cases fail (saw [('replace', 51664701)], i.e. zero fsyncs), while the three pre-existing ones still pass. So the new cases discriminate.A deployed replica does not re-fetch its source, so picking this up requires a redeploy — it is not live anywhere until then.