Repository navigation
Fix/checkpoint writes - #10
Merged
Merged
Conversation
zyhu-hu
force-pushed
the
fix/checkpoint-writes
branch
from
November 5, 2026 18:09
5f7a6b3 to
e1b3e0c
Compare
nbren12
requested changes
Nov 7, 2026
… broken latest.pth fabric.save goes through lightning's _atomic_save, which writes a temp file in the system temp dir and shutil.moves it onto the target. When the temp dir is on another filesystem than the run dir (node-local /tmp vs. a shared run dir), the move copies INTO the existing file in place. Two bugs follow: - best.pth is a hard link to latest.pth, so every later save of latest.pth also overwrites best.pth: it tracks the latest state, not the best one. - A walltime kill during that copy leaves a truncated latest.pth that the next job cannot load. screamcast/checkpoint_io.py saves to <path>.tmp in the run dir, fsyncs and CRC-checks it, and os.replaces it onto the path, so the file is never rewritten in place. best.pth is re-linked through a temp name. On resume, a latest.pth that fails its CRC check falls back (loudly) to the newest valid steps_*.pth, keeping the lower best_valid_loss recorded in best.pth; with no usable fallback the run stops with a clear error. Every failure on rank 0 (save, verify, link, resume selection) is shared so all ranks raise instead of waiting in a barrier. tests/test_checkpoint_io.py emulates the cross-filesystem move and a kill mid-copy (each also run against plain fabric.save as a strict xfail), the resume fallback, and a two-rank gloo run where rank 0 fails and both ranks must raise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Zeyuan Hu <zeyuanh@nvidia.com>
Four stages, each resuming the previous stage's best.pth: 64x64 pretraining at 1e-4 for ~10M images or until convergence, a cosine anneal to 1e-6 over ~3M images, a 128x128 (better: tile + 2*halo) fine-tune at 1e-5 for ~0.5-1M images, then 2-step and 4-step fine-tunes at 1e-5. Also recommends training with pipeline.do_precip_relu=False and clipping precipitation at inference, since the in-forward ReLU can leave a precipitation head stuck below zero with no gradient. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Zeyuan Hu <zeyuanh@nvidia.com>
…le tmps Addresses the review of PR 10: - The resume selection in train() moves into checkpoint_io as choose_resume_checkpoint() and apply_best_valid_loss_floor(), with tests for the floor, the steps_* fallback, the error and a two-rank failure. - best.pth is linked to the verified .tmp before it is renamed onto latest.pth (save_checkpoint_atomic(..., best=)), and the floor from best.pth now applies whenever a run resumes, not only on a steps_* fallback. A kill between the link and the rename no longer leaves best.pth on an older state for good. link_best_collective is gone. - A leftover latest.pth.tmp is removed before fabric.save: fsspec ignores a failed copy onto an existing file, so a stale complete tmp could pass the CRC check and replace a newer latest.pth. Leftover *.pth.tmp files are removed when a run starts, and the tmp is removed after a failed save. - A missing latest.pth next to steps_*.pth now prints a warning. - README stage 2 names the scheduler that actually reaches 1e-6 (dampened_cosine_with_hard_restarts, cosine_n_cycles=1, warmup_steps=0). - Docstrings corrected (what "left as it was" covers, the CRC read), and the strict xfails now require AssertionError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Zeyuan Hu <zeyuanh@nvidia.com>
--halo-adjoint-cross-face now defaults to False, so --halo-adjoint keeps the overlap-add at intra-face tile seams and drops it across cube-face edges. Cross-face halo targets are KNN-interpolated off the neighbour face's grid, and mixing them back in over-smooths the outermost pixels of every face (docs/halo-adjoint-cross-face.md). --halo-adjoint-cross-face restores the old behaviour; the setting is still recorded in the output zarr attrs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Zeyuan Hu <zeyuanh@nvidia.com>
zyhu-hu
force-pushed
the
fix/checkpoint-writes
branch
from
November 10, 2026 17:23
e1b3e0c to
27e4f8a
Compare
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.
Title: Crash-safe checkpoint writes, correct best.pth, own-face halo adjoint by default
Summary
1. Checkpoint writes no longer corrupt
latest.pthorbest.pthfabric.saveuses lightning's_atomic_save. That writes a temp file in the system temp dir and thenshutil.moves it onto the target. When the temp dir is on a different filesystem from the run dir (node-local/tmpvs. a shared run dir), the move copies into the existing file in place. Two bugs follow:best.pthtracks the latest state, not the best.best.pthis a hard link tolatest.pth, so every later save oflatest.pthoverwrites it too.latest.pth.New
screamcast/checkpoint_io.py:<path>.tmpin the run dir, fsyncs it, CRC-checks it andos.replaces it onto the path. No checkpoint file is ever rewritten in place.best.pth: it is linked to the verified.tmpbefore the rename. A kill between the two steps can no longer leavebest.pthon an older state.*.pth.tmpfiles are removed when a run starts and before each save. fsspec ignores a failed copy onto an existing file, so a stale but complete tmp could otherwise pass the CRC check and replace a newerlatest.pth.latest.pthfails its CRC check, the run falls back with a loud warning to the newest validsteps_*.pth, keeping the lowerbest_valid_lossrecorded inbest.pth. With no usable fallback it stops with a clear error. A missinglatest.pthnext tosteps_*.pthprints a warning. The selection logic lives inchoose_resume_checkpoint()andapply_best_valid_loss_floor().2. Rollout: the halo adjoint stays within each cube face by default
This changes the default behaviour of
scripts/ace/run_screamcast_nudged.py.--halo-adjoint-cross-facenow defaults to False.--halo-adjointkeeps the overlap-add at tile seams inside a face and drops it across cube-face edges.docs/halo-adjoint-cross-face.md).--halo-adjoint-cross-facerestores the old behaviour. The setting is still recorded in the output zarr attrs.3. README: recommended training schedule
The production checkpoint was trained in four stages, each resuming the previous stage's
best.pth:dampened_cosine_with_hard_restarts,cosine_n_cycles=1,warmup_steps=0).The README also recommends training with
pipeline.do_precip_relu=Falseand clipping precipitation at inference instead. With the ReLU in the forward pass, a precipitation head can get stuck below zero with no gradient.Tests
New
tests/test_checkpoint_io.pycovers:fabric.saveas a strict xfail, to show the bug);steps_*, thebest_valid_lossfloor, the no-fallback error;Run on
e1b3e0cin a GPU container, with S3 access to the SCREAM data. Each result was compared withmain(8db1c74):python ci/check_licenses.pyandpre-commit run -apass.tests/distributed: all pass apart fromtest_training_launch, the same as onmain. That includes the dataloader tests against the real SCREAM store, andtest_characterization_inference: PR 10's outputs for all three production configs match reference outputs generated frommain.test_training_launch: times out at its 180 s limit on bothmainand this branch, while reading data from S3. It runs thepytestconfig, which doesn't save checkpoints, so the new write path is covered bytests/test_checkpoint_io.pyonly.tests/distributed.🤖 Generated with Claude Code