[multi-lora v2 3/7] Per-slot checkpoints with optimizer state - #2842
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
5aa9371 to
1900fdd
Compare
630b838 to
86a4575
Compare
1900fdd to
7b4f6ff
Compare
86a4575 to
bbb1215
Compare
7b4f6ff to
e6c02c0
Compare
bbb1215 to
c2ec43d
Compare
e6c02c0 to
974a894
Compare
c2ec43d to
47eefc1
Compare
974a894 to
e6abb46
Compare
47eefc1 to
3dc33bc
Compare
e6abb46 to
e3016e8
Compare
a0bb8d8 to
c3a6fcb
Compare
90bfd2a to
923ad13
Compare
4a22314 to
d9e4f5b
Compare
923ad13 to
fe0c476
Compare
d9e4f5b to
76a25dc
Compare
fe0c476 to
2d57c78
Compare
616f2c8 to
6cea0c6
Compare
90e05d5 to
51f34fa
Compare
04bf60a to
b038a65
Compare
| def apply_shards(): | ||
| loaded = load_adapter(model, slot, shards["weights"]) | ||
| assert loaded > 0, f"loaded 0 adapter tensors from {checkpoint_dir / _weight_shard_name()}" | ||
| optimizer.reload_model_params() |
There was a problem hiding this comment.
optimizer is the outer LayerWise/Chained optimizer, so this reload walks every slot’s child and copies BF16 model weights into FP32 masters. Loading slot S therefore rounds the masters of every live slot; only S is restored from masters below. The preceding lora_executor.load_slot() call does the same. Add a slot-local reload helper and use it in both paths, with a two-slot test that keeps slot B’s FP32 master different from its BF16 model value.
| shards: dict = {} | ||
|
|
||
| def read_shards(): | ||
| shards["weights"] = torch.load(checkpoint_dir / _weight_shard_name(), map_location="cpu", weights_only=True) |
There was a problem hiding this comment.
The read phase validates only optimizer list lengths. It does not validate master/moment tensor shapes or adapter weight keys and shapes. load_adapter() copies matching tensors first and reports missing or unused keys afterward, so a bad shard can mutate part of the live slot before raising. Validate the complete weight and optimizer tensor layout in read_shards() before any copy, or add a validation-only Bridge helper.
| if old_dir.exists(): | ||
| shutil.rmtree(old_dir) | ||
| os.replace(final_dir, old_dir) | ||
| os.replace(tmp_dir, final_dir) |
There was a problem hiding this comment.
This code moved; the current comment is here: #2842 (comment)
| return ckpt._weight_shard_name() | ||
|
|
||
|
|
||
| def test_a_missing_optim_shard_leaves_the_slot_untouched(tmp_path): |
There was a problem hiding this comment.
These tests cover read-phase failures only. They never apply a valid checkpoint, so they do not verify FP32 masters, Adam moments and step counters, weights-only loading, or equivalence of the next Adam update. Add a CPU round-trip test for exact continuation. Include a second live slot whose FP32 master differs from its BF16 model value so the test also catches cross-slot reloads.
5ca4002 to
52ec9d8
Compare
| old_dir = final_dir.parent / f"_old_{final_dir.name}" | ||
| if old_dir.exists(): | ||
| shutil.rmtree(old_dir) | ||
| os.replace(final_dir, old_dir) |
There was a problem hiding this comment.
write_checkpoint_dir() now rolls back a caught failure from the second rename, but the published path is still absent between final_dir -> old_dir and tmp_dir -> final_dir. A process exit in that window leaves the last valid checkpoint only under _old_<name>, so readers lose the public path. Also, shutil.rmtree(old_dir) still runs inside the publish phase, so cleanup failure reports the save as failed after the replacement is committed. Use immutable version directories plus an atomically replaced pointer/exchange for crash atomicity, and make old-version cleanup best-effort.
f958286 to
2c1bbe3
Compare
a72f7f1 to
7c8d7a4
Compare
2c1bbe3 to
33a4a81
Compare
7c8d7a4 to
fec9abe
Compare
33a4a81 to
ab98e53
Compare
7ae642f to
b8af08f
Compare
69bc362 to
d788d23
Compare
50a63b5 to
3c59f65
Compare
d788d23 to
45ff416
Compare
3c59f65 to
53f60d4
Compare
53f60d4 to
f7a9659
Compare
2e97600 to
331ed96
Compare
f7a9659 to
a9f2099
Compare
Co-authored with @yushengsu-thu.
Save and restore one adapter slot independently. Checkpoints contain adapter weight shards, per-rank Adam state and step counters, and FP32 master parameters; loading optimizer state requires the same parallel topology.
Checkpoint load/save execution failures, including filesystem errors, propagate to
TrainerCelland terminate the current single-cell Tinker service.lora/actor.pyand controller commands exposesave_slotandload_slot(..., ckpt_path=..., load_optimizer=...). A TODO incheckpoint_io.pyrecords deferred IO failure isolation.Checkpoint names are relative symlinks to immutable version directories. The shared writer completes shards and metadata before atomically replacing the public link. A process crash leaves the old or new complete checkpoint reachable; older versions remain on disk for readers that already resolved them. Legacy directory checkpoints remain readable, but overwriting them is unsupported: save under a new name. Successful phases retain their collective barriers; execution errors still propagate to the trainer cell.
Stacked on #2841.
Validation:
pre-commit run --all-filesand incrementalgit diff --checkpassed on every updated component head. All previous heads remain ancestors; the final Unify HF snapshot and checkpoint writing #3198 tree exactly matches the reviewed implementation.tests/e2e/lora/test_slot_checkpoint_resume.py(Hopper, LoRA, 2-GPU stage). The tested final tree is Unify HF snapshot and checkpoint writing #3198fd63c7ec8c41c2e0a61d273f0cd5a3504275ada7, with Megatron73b54618f7e58e0f25f619bcfecbe2640765475a, Bridge40b930897717941cfe2bd9806f417e28bf1bfa65, PyTorch 2.13.0+cu130 and Transformer Engine 2.17.0. Checkpoints were on GPFS.pre-commit run --all-filespassed.ci-megatron-pr: 73b54618f7e58e0f25f619bcfecbe2640765475a
ci-sglang-pr: 02b5e123c490c988d21526d0dd7837fefdd0e1da