Skip to content

fix(train_dflash): atomic per-file checkpoint writes + _SUCCESS-gated resume#659

Open
jessiewei7 wants to merge 1 commit into
sgl-project:mainfrom
jessiewei7:pr/checkpoint-atomicity
Open

fix(train_dflash): atomic per-file checkpoint writes + _SUCCESS-gated resume#659
jessiewei7 wants to merge 1 commit into
sgl-project:mainfrom
jessiewei7:pr/checkpoint-atomicity

Conversation

@jessiewei7

@jessiewei7 jessiewei7 commented Jul 7, 2026

Copy link
Copy Markdown

Summary

train_dflash.py's save_checkpoint() writes every checkpoint file
(training_state.pt, draft weights, dflash.py) directly to its final path,
and get_last_checkpoint() (specforge/utils.py) trusts any directory whose
name matches epoch_X_step_Y. So a crash mid-write (OOM kill, preemption, any
interruption during torch.save/save_pretrained/shutil.copy) can leave a
partially-written file at its final, "looks complete" name, and a
--resume run has no way to tell that apart from a finished checkpoint — it
silently loads the corrupt one.

This is worse on multi-node training, where args.output_dir is often not a
single, fully POSIX-coherent filesystem (per-node mounts bridged together,
weaker-consistency network filesystems, etc.). There a directory can look
complete by name while genuinely missing content another rank wrote that
hasn't propagated yet. We hit exactly this on real 2-node hardware: a save
logged "Saved checkpoint to .../epoch_0_step_180" with no error, but the
on-disk directory was missing content.

Fix

  • Every file is written to a <name>.tmp sibling and atomically
    os.rename()'d into place by the process that created it, with an
    os.fsync() of the data before the rename. A crash can now only leave a
    stray .tmp behind — never a partial file at the final name, and not even a
    final-named file with unflushed content after a hard crash (rename() gives
    atomic ordering, not durability; the fsync gives durability).
  • Completeness is signaled by a _SUCCESS marker written last by rank 0,
    only after every other file is renamed into place and a dist.barrier()
    confirms all ranks are done.
  • get_last_checkpoint() gained an opt-in require_success_marker (default
    False). When True, a directory is trusted only if _SUCCESS is present.
    train_dflash.py's --resume passes True; the default stays False so
    train_domino.py / train_eagle3.py / train_peagle.py (whose savers
    don't write this marker) are unaffected.
  • Directories that match the name pattern but lack _SUCCESS are now
    logger.warning'd when skipped, so a --resume right after upgrading (older
    checkpoints predate the marker) doesn't silently restart from scratch — the
    warning names them and shows how to opt one back in (touch <dir>/_SUCCESS).

Cost: the fsync adds some wall-time per save (most visible for large weight
shards) — a deliberate correctness-over-speed trade.

Test plan

  • Live, real multi-node hardware: ran the full pipeline and inspected a
    checkpoint directory on disk, confirming all expected files plus _SUCCESS
    before it was treated as complete; then killed and --resume'd, confirming
    the log showed a real decayed LR from the restored scheduler state (not a
    fresh-init default) — genuine state restored, not silently reinitialized.
  • Unit test tests/test_utils/test_get_last_checkpoint.py (CUDA-free):
    latest-by-(epoch, step) selection; default (no marker) behavior preserved;
    require_success_marker=True selects only marked dirs and returns None
    when all are unmarked; the upgrade-warning is emitted for skipped dirs.

Scope

The same unguarded-write pattern exists in train_domino.py /
train_eagle3.py / train_peagle.py; this PR fixes only the path we
validated live (kill + resume) on real multi-node hardware. The shared
mechanism lives in specforge/utils.py (atomic_torch_save / fsync_path /
require_success_marker, default False), so extending it to the other
scripts is mechanical —
happy to do that in this PR or a follow-up, whichever you prefer.

… resume

save_checkpoint() wrote every file (training_state.pt, draft weights,
dflash.py) straight to its final path, and get_last_checkpoint() trusted
any directory matching epoch_X_step_Y. A crash mid-write (OOM kill,
preemption) could leave a partial file at its final "looks complete"
name, which --resume would then silently load. This is worse on
multi-node setups where args.output_dir isn't a single coherent POSIX
filesystem: a directory can look complete by name while missing content
another rank wrote that hasn't propagated yet (hit live on 2-node HW).

Fix:
- Each file is written to a <name>.tmp sibling, fsync'd, then atomically
  os.rename()'d into place -- rename gives atomic ordering, fsync gives
  durability against a hard crash resurrecting a truncated final-named file.
- A _SUCCESS marker is written last by rank 0, after all files are renamed
  and a barrier confirms all ranks are done.
- get_last_checkpoint() gains an opt-in require_success_marker (default
  False, so train_domino/eagle3/peagle are unaffected); train_dflash's
  --resume passes True. Skipped name-matching dirs lacking _SUCCESS are
  warned about, so a resume right after upgrading (older checkpoints
  predate the marker) doesn't silently restart from scratch.

Adds a CUDA-free unit test for the marker gating. Two fork-only issues
(per-rank makedirs visibility, collective rotation rmtree) are out of
scope: neither has an applicable target on main's single-writer,
no-rotation save_checkpoint().
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant