Repository navigation
fix(#0260): write init_checkpoint.pt through _write_file_atomic - #291
Draft
nilsmechtel wants to merge 3 commits into
Draft
nilsmechtel wants to merge 3 commits into
nilsmechtel wants to merge 3 commits into
Conversation
The init_checkpoint download used a plain write_bytes, which opens 'wb' and truncates the name before the data lands. The path is handed to a train worker as the training params' checkpoint_path, so a reader on another node could torch.load a prefix -- the same defect #127 fixed for the community checkpoint, at the one call site that fix did not cover. Routes the write through the existing _write_file_atomic, as the other three cache writes already do.
… injection The bundle suite scored identically with and without the atomic write at entry.py's init_checkpoint branch, so nothing in the repo would have caught that line being reverted. Reproduced before writing this: 113 passed / 2 skipped on both origin/main and the fix. Two shapes were rejected first, and both are why this test looks the way it does. Asserting that _write_file_atomic was CALLED passes on a revert that keeps the call and changes the target. And a write that SUCCEEDS leaves the destination identical either way, so there is nothing to observe unless the write fails partway -- hence the injected fault. The fault is injected below the function under test, at open(), so both implementations meet it in the same place. Both namespaces have to be patched: _write_file_atomic calls the bare builtin open, Path.write_bytes goes through io.open, and they are separately bound. Patching only builtins left the truncating path untouched, so the defect arm wrote successfully and then failed on an unrelated missing key -- which reads as a discriminating result and is not one. The status positive control is what caught that: it asserts the recorded failure message is the injected one, so a run where the write never tore cannot satisfy the destination assertion by accident. Measured, full bundle suite with this file present: origin/main (dest.write_bytes) 2 failed, 113 passed, 2 skipped this branch (_write_file_atomic) 115 passed, 2 skipped and on the revert the failures name the damage rather than a call shape: "expected the previous 4111 bytes to survive, found 32775 bytes (a prefix of the new content)" "exists after a failed write, holding 32775 of 65551 bytes -- a train worker handed this path would torch.load a prefix" Refs: #291
This branch has not been deployed
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.
The
init_checkpointdownload wrote its file with a plainwrite_bytes, which opens'wb'and truncates the name before the data arrives, so a train worker on another node couldtorch.loada prefix of it. This is the same defect #127 fixed for the community checkpoint, at the one call site that fix did not cover.Closes myco #260.
The change
One line, routing the write through the existing
_write_file_atomicexactly as the other three cache writes already do, plus a three-line comment naming the constraint that makes it a defect.Why this site crosses a node boundary
resolved["checkpoint_path"]is set fromdestand then persisted bytraining.write_training_params(...). The train workers read it straight back out of those params --train_worker.py:92(checkpoint_path=p.get("checkpoint_path")) andtrain_worker_cellpose.py:207(resume = p.get("checkpoint_path")) -- and those run as separate actors. So the file is handed across a process and potentially node boundary, which is the condition_write_file_atomicexists for. The filing issue flagged this as untraced; it is now traced, with a positive control (8 files in the bundle mentioncheckpoint_path) and a negative control (0 for a bogus symbol).Evidence
The #127 harness, rebuilt during this work to take a ref argument so it runs against a durable ref rather than a branch the repo deletes on merge (
.dev/verify/0127/check_main.sh). Clause 2 reads the call sites from the AST, not by grepping forfsync:_write_file_atomiccall siteswrite_bytesremainingorigin/main(control)f3bd503Both report
VERDICT: CLAUSES-1-3-HOLD, rc=0. The same instrument on the pre-fix parentce9fafareportsCLAUSES-1-3-FAILrc=1, so it discriminates rather than always passing.No regression in the #127 suites on this branch: ARM A 11 passed rc=0; ARM B (both fsyncs removed) 1 failed, naming
test_the_bytes_and_the_rename_are_both_fsynced; ARM C (length marker reverted) 5 failed of 5. 11 cases ran, none skipped.Not done here, deliberately
No version edit.
apps/model-finetune/**is app-version gated, and PR #260 currently holds the next rung at 0.30.1. Per the Backlog head's rung ordering this PR stays a draft with no manifest change so it cannot race that one; the bump, upload andgh pr readyhappen in one sitting once #260 lands and the live floor is re-measured at that moment.Not tested
No new test. The issue's DONE WHEN defines verification as reading the call site plus the harness clause-2 arm reaching zero, which is what is shown above. No real download,
torch.load, or cross-node page-cache window was exercised, and no cluster was touched -- the defect is argued from the write shape plus #127's measured reproduction at an equivalent site, not from an observed failure at this one.