feat(sc): recover replay buffer from native TQ checkpoints - #3480
Conversation
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
3af936c to
e31f42c
Compare
|
/ok to test e31f42c |
macandro96
left a comment
There was a problem hiding this comment.
Team review — 16 inline findings across bug/guideline/test-coverage/docstring categories, plus a leader design pass.
Summary: no critical bugs. Two BUG-level actionable items: (a) is True guard silently skips CustomSamplerConfig, letting a bad config waste cluster time; (b) _validate_replay_inventory runs after the native TQ snapshot is written, leaving a stale tmp bundle on validation failure. The rest are guideline/typing polish, docstring alignment, and 3 concrete test-coverage gaps (verifier tool, back-to-back checkpoint serialization, print-monkeypatch brittleness).
Devil's advocate: 12 confirmed, 5 disputed, 2 downgraded. Notable disputes: barrier mutation-counter leak on cancellation (no await between +=1 and try:); sampler_name=None handling (call site uses config, not save_state).
Design (leader pass — the automated design-reviewer agent could not get file-read access under its permission mode):
DataPlaneCheckpointBarrieris hand-rolled aroundasyncio.Conditionbut this is the idiomatic build (no stdlib async rwlock); exception paths on bothmutation()andcheckpoint()correctly notify waiters. LGTM.- Data-plane adapters (
noop.py,transfer_queue.py) are symmetric and match theinterfaces.pyProtocol. Clean seam. - Only real design smell is the
ClassVarcapability on config classes — flagged as a GUIDELINE finding.
Pre-commit: FAIL. Details in the inline comment anchored at single_controller.py:47. The untracked local file examples/configs/recipes/llm/grpo-qwen3-1.7b-1n8g-super-swe1-sc-tq-recovery.yaml also fails configs-minimize-check, but it's not part of this PR and is excluded from findings.
Filtered findings (below confidence threshold or premise disputed):
- barrier mutation-counter leak on cancellation (no await point between the
+= 1and thetry:— cancellation cannot land there) sampler_name=Nonehandling in_maybe_restore_native_data_plane_checkpoint(misreading — call site passesmaster_config.async_rl.sampler.name, notsave_state.sampler_name)_validate_replay_inventoryrunning afterload_state_dictin the restore path (exception is terminal; partial state is discarded)- PERF-EVIDENCE ask for barrier hold time (marginal — barrier only holds during a checkpoint, which is already blocking)
docs/update ask (pre-existing SC docs don't cover this area at length; reasonable follow-up)
Generated by Claude Code
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
75fdeb1 to
c781d74
Compare
|
/ok to test c781d74 |
Ruff dropped the unused re-export from replay_buffer.py; the tests still needed the constant. Import it from its canonical location instead. Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
c712c63 to
83eafa3
Compare
|
/ok to test 83eafa3 |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test da8b0a9 |
GRPOSaveState now has trainer_version; SingleControllerActorArgs now has data_plane_checkpoint_metadata. Two unit tests were still asserting the old shapes. Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test 0b4cecc |
The train-pump epilogue now enters the data-plane barrier before clearing consumed samples. Two SC tests built controllers via object.__new__ and had no _data_plane_checkpoint_barrier attribute; the second test hit it once dispatch actually happened, raising AttributeError. Also add data_plane_checkpoint_metadata=None to the SetupTimingMetrics test's actor_args, matching the field _init_ now reads. Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test 608e9bd |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test c3b8624 |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
@macandro96, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
|
/ok to test bca9a72 |
terrykong
left a comment
There was a problem hiding this comment.
Re-review of the rollout-recovery ledger work (commits since the last round, at bca9a721) by the same six-agent team, every finding adversarially challenged before staging.
The headline is positive. Two agents independently tried to break the single-durable-owner invariant across every dispatch, failure, substitute, abort, and checkpoint-cut interleaving — and could not. The snapshot-skew bug from the last round is properly fixed (every restart-facing read now happens in one exclusive barrier cut, and admission was split so the cursor advance takes a mutation slot while the gate wait stays outside — the exact two-part shape we asked for). The round-trip tests are real, not mocked: real buffer, real samplers, real barrier, genuine disk restart. We re-ran all 287 in-scope unit tests locally on CPU: 287 passed, 0 failed. The functional test recovers a deliberately parked rollout and asserts on its specific group id.
8 comments below: one deliberate behavior change whose justifying comment needs broadening, one stale doc, two missing test assertions (selectability and stamp preservation - both verified passing against this branch before asking), and four low-severity notes including an endorsed future-proofing pattern for the barrier. 11 team findings were raised; 1 was disputed and dropped, 3 downgraded, 2 later removed as too minor, and the raise-scoping ask was inverted after the reviewer challenged it. Plus replies on the three prior-round threads.
Explainers for reviewers joining fresh: the ledger · the atomic cut · the test evidence
Generated by Claude Code
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test 2dd8212 |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test 54d9e82 |
|
/ok to test b8fa75b |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
b8fa75b to
2017b60
Compare
|
/ok to test 2017b60 |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test a4a2a18 |
Signed-off-by: Anish Mahishi <amahishi@nvidia.com>
|
/ok to test ee210a0 |
PR3 (#3591) was SQUASH-merged into main as b3b6713, so none of its commits are ancestors of main while PR4 still carries all of them. Git therefore sees PR3's whole diff as independently added on both sides, which is why all 16 conflicts name b3b6713 and why the PR showed CONFLICTING despite the content being identical. That made the classification, not the content, the work. For each conflicted file: is main's version byte-identical to PR3's head (3d9ce21), and does PR4 add anything beyond it? Three groups fell out. GROUP A -- pure squash artefacts, resolved by taking OURS (10 files) fleet_health.py, collective_weight_synchronizer.py, membership.py, nccl_reshard_weight_synchronizer.py, grpo_sc_generation_shard_recovery.sh, test_watchdog_pump.py, test_membership.py, test_reconcile_communicator.py, test_reshard_rebuild.py, test_weight_synchronizer.py main == PR3 exactly and no other PR touched them, so PR4's side is main's content plus PR4's delta. Taking ours loses nothing. GROUP B -- PR4 contributes nothing, resolved by taking THEIRS (2 files) single_controller_utils/setup.py (#3480, #3727, #3821 on top of PR3) tests/unit/single_controller/test_refit_recovery.py (#3480 on top of PR3) GROUP C -- genuine merges (4 files), one per upstream PR below. The six upstream PRs that contributed real content, and what each needed: #3480 recover replay buffer from native TQ checkpoints single_controller.py: rollout_recovery imports. Kept alongside ours. setup.py, test_refit_recovery.py, L1 harness: group B / additive. #3765 log toolcall and thinktag violation rate single_controller.py: VIOLATION_TAG_KEYS. Auto-merged, verified present. #3727 support non-colocated MInf single_controller.py: MegatronGeneration import, kept alongside ours. L1 harness: grpo_megatron_generation_gym_single_controller.sh entry. #3821 warm-start the value model from a critic-pretrain checkpoint config.py: the max_num_epochs validator. Ours only adds restart_dead_shards to FleetHealthConfig, so both survive; verified the field landed in the right class and the validator is intact. #3655 nemo-lens telemetry vllm_generation.py: the @trace_fn decorator on generate. Ours adds restart_shard in a different region; both kept. #3839 pause generation during in-flight refit vllm_generation.py: pause_generation_for_refit / resume_generation_after_refit. Auto-merged, verified present -- worth knowing it exists, since it pauses engines around a refit and this PR restarts them. Verified after resolving: no conflict markers; all four lint hooks clean (the single pyrefly error is the pre-existing unrelated transfer_queue import); 1122 unit tests pass; both submodule pointers and uv.lock/pyproject byte-identical to main. Both sides' work was checked individually rather than assumed: EngineSupervisor wiring, restart_dead_shards, restart_shard, recreate_worker, desired_membership and the report_refit call on our side; the six items above on main's. Note for anyone reproducing locally: #3655 adds a nemo-lens dependency that the pre-merge container image does not carry, so tests fail at import with ModuleNotFoundError: nemo until the venv is refreshed. Plain upstream/main fails the same way in that image; it is not a merge defect. Signed-off-by: asolergibert <asolergibert@nvidia.com>
What does this PR do ?
Summary
Adds crash-consistent SingleController recovery for two rollout states:
Completed, unconsumed groups
Unfinished prompt groups
This builds on the merged TQ v0.19 bump (#3423) and SingleController checkpoint lifecycle (#3429).
Motivation
Previously, SC checkpoint recovery either serialized rollout tensors separately in
replay_buffer.ptor restarted with an empty rollout buffer.Serializing tensors in both the replay-buffer checkpoint and TQ:
This change makes the native TQ snapshot authoritative for tensors and keeps only the controller metadata required to resume consuming those rows.
Checkpoint contents
replay_buffer_metadata.ptcontains no rollout tensors or fields_data.rollout_recovery.ptstores:Consistency model
TQ mutations, sampler admission commits, rollout-ledger mutations, and checkpoint capture participate in the same data-plane checkpoint barrier.
The barrier ensures a checkpoint observes transitions atomically, including:
Mutation methods require a live barrier capability, preventing future call sites from accidentally changing recovery state outside the checkpoint boundary.
If a group exists in both canonical TQ metadata and the unfinished ledger, canonical TQ ownership wins and the stale ledger entry is discarded.
Save workflow
flowchart LR Dataloader["Dataloader fetch"] --> Ledger["Reserve prompt group in ledger"] Ledger --> Admission["Sampler admission"] Admission --> Generation["Generate rollout"] Generation --> Commit["Canonical TQ commit"] Commit --> Replay["Completed replay index"] Commit --> Remove["Remove unfinished ledger entry"] Checkpoint["SC checkpoint"] --> Barrier["Exclusive checkpoint cut"] Barrier --> TQ["Save native TQ state"] Barrier --> ReplayState["Save replay metadata"] Barrier --> LedgerState["Save unfinished ownership"] TQ --> Bundle["Finalize step_N"] ReplayState --> Bundle LedgerState --> BundleGeneration may continue while a checkpoint is written, but commits and destructive mutations wait at the barrier.
A failed TQ save, inventory mismatch, sidecar serialization failure, or incomplete cleanup prevents the new checkpoint from becoming the latest finalized checkpoint.
Restore workflow
flowchart LR Bundle["Latest finalized checkpoint"] --> TQ["Restore native TQ"] Bundle --> Replay["Restore completed replay index"] Bundle --> Ledger["Restore unfinished ledger"] Replay --> Reconcile["Canonical TQ wins"] Ledger --> Reconcile Reconcile --> Rehydrate["Rehydrate prompts from dataset"] Rehydrate --> Redispatch["Redispatch unfinished groups"] Redispatch --> Pumps["Run rollout and train pumps"]Recovery:
Sampler support
Replay and unfinished-group recovery are supported by all built-in samplers:
Gated samplers persist and restore their exact dispatch index so target-step scheduling does not rewind after restart.
Custom samplers must explicitly declare:
NOTE: Custom gated samplers participating in unfinished recovery should implement the transactional admission interface so gate waiting happens outside the checkpoint mutation cut.
Unsupported custom samplers use shadow-mode data-plane checkpoints and do not restore buffered rollout ownership.
Configuration
Current native restore support requires the TQ
simplebackend. Unsupported configurations fail during setup rather than silently producing incomplete checkpoints.Failure handling
Scope and limitations
This PR recovers unfinished work through whole prompt-group regeneration.
It does not preserve:
Prompt rehydration currently assumes:
Only completed groups already committed to TQ preserve their exact generated tokens and tensor payloads.
Partially resolves #3594 to an extent where we do not lose out on any prompt during checkpoint / restore.
The remaining ones are:
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information