Skip to content

Commit e622688

Browse files
authored
[rl] Revert generator initialization race condition fix (#3809) (#3871)
Reverts the initialization reordering from #3809, which spawned the trainer first and moved TorchStore init + `policy_version` read before generator spawn. Restores the original ordering: spawn trainer and generators together, then init TorchStore and read `policy_version`.
1 parent ea3562e commit e622688

1 file changed

Lines changed: 22 additions & 30 deletions

File tree

torchtitan/experiments/rl/controller.py

Lines changed: 22 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -533,11 +533,7 @@ async def setup_async(
533533
for generator_mesh in generator_meshes:
534534
await setup_torch_elastic_env_async(generator_mesh)
535535

536-
# Spawn the trainer first; generators wait until it is ready (see below).
537-
# A generator's first MoE dispatch (vLLM warm-up in __init__) can race the
538-
# trainer's model build + weight load and fault a partial-NVLink-domain
539-
# HybridEP generator (cudaErrorIllegalAddress, hybrid_ep_backend.cuh:5693),
540-
# so sequencing keeps that first dispatch on a quiescent system.
536+
# Spawn actors on their respective meshes
541537
self.trainer = trainer_mesh.spawn(
542538
"trainer",
543539
PolicyTrainer,
@@ -549,31 +545,7 @@ async def setup_async(
549545
output_dir=config.dump_folder,
550546
)
551547

552-
# Initialize TorchStore for weight sync between trainer and generator.
553-
# StorageVolumes are spawned on the trainer mesh so they are colocated
554-
# with the weight source for faster data access in the non-RDMA path.
555-
# LocalRankStrategy: routes each process to a storage volume based on
556-
# LOCAL_RANK, so colocated processes share the same volume.
557-
# https://github.com/meta-pytorch/torchstore
558-
with sl.log_trace_span("torchstore_init"):
559-
await ts.initialize(mesh=trainer_mesh, strategy=ts.LocalRankStrategy())
560-
561-
# Barrier on trainer readiness BEFORE spawning generators (see spawn comment):
562-
# returns only after the trainer's __init__ (model build + checkpoint load), so
563-
# generators init on a quiescent system. Also reads the restored policy_version
564-
# (0 if fresh) for resume.
565-
# TODO(resume): only model/optimizer/policy_version are restored; the rollout
566-
# buffer (in-flight rollouts) and dataset stream position are NOT -- a resumed
567-
# run refills the buffer and re-reads data from the start.
568-
# TODO: investigate why we need to spawn generator later
569-
self.start_step = self._get_rank_0_value(
570-
await self.trainer.get_policy_version.call()
571-
)
572-
if self.start_step > 0:
573-
logger.info(f"Resuming RL training from step {self.start_step}")
574-
575-
# TODO: torch.compile with aot_eager backend (inductor crashes the vLLM engine on the shared model path).
576-
with sl.log_trace_span("mesh_spawn_generators"):
548+
# TODO: torch.compile with aot_eager backend (inductor crashes the vLLM engine on the shared model path).
577549
generators = []
578550
for idx, generator_mesh in enumerate(generator_meshes):
579551
actor_name = (
@@ -592,6 +564,26 @@ async def setup_async(
592564
generators.append(generator)
593565
self.generator_router = config.generator_router.build(generators=generators)
594566

567+
# Initialize TorchStore for weight sync between trainer and generator.
568+
# StorageVolumes are spawned on the trainer mesh so they are colocated
569+
# with the weight source for faster data access in the non-RDMA path.
570+
# LocalRankStrategy: routes each process to a storage volume based on
571+
# LOCAL_RANK, so colocated processes share the same volume.
572+
# https://github.com/meta-pytorch/torchstore
573+
with sl.log_trace_span("torchstore_init"):
574+
await ts.initialize(mesh=trainer_mesh, strategy=ts.LocalRankStrategy())
575+
576+
# Resume: __init__ ran CheckpointManager.load(); read back the restored policy_version
577+
# (0 if fresh) so the loop resumes at the right step and generators pull at that version.
578+
# TODO(resume): only model/optimizer/policy_version are restored. The active-slot rollout
579+
# buffer (in-flight rollouts) and the dataset stream position are NOT restored -- a resumed
580+
# run refills the buffer and re-reads data from the start. Need to recycle prompts.
581+
self.start_step = self._get_rank_0_value(
582+
await self.trainer.get_policy_version.call()
583+
)
584+
if self.start_step > 0:
585+
logger.info(f"Resuming RL training from step {self.start_step}")
586+
595587
# Start each generator's engine loop on all ranks once, before any
596588
# rank-0-only generate / pull (rank 0 drives the followers through this
597589
# loop, so every rank must be running it first).

0 commit comments

Comments
 (0)