Synchronize ZeRO-3 rollout early stopping - #8390
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 020a184db6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sequence_has_eos = (response_ids.unsqueeze(-1) == eos_token_ids).any(dim=-1).any(dim=-1) | ||
| local_work_remaining = (~sequence_has_eos).any().to(dtype=torch.int32) | ||
| global_work_remaining = self.work_remaining(local_work_remaining) | ||
| return torch.full((input_ids.shape[0], ), not global_work_remaining, dtype=torch.bool, device=input_ids.device) |
There was a problem hiding this comment.
Preserve compatibility with legacy stopping criteria
On installations using the supported Transformers 4.32.x range (requirements/requirements-inf.txt permits >=4.32.1), the legacy StoppingCriteriaList.__call__ applies Python any() to each criterion result. This batch-shaped tensor therefore raises “Boolean value of Tensor with more than one value is ambiguous” whenever a ZeRO-3 rollout has more than one sequence, so the new synchronized generation path crashes; either provide version-compatible scalar semantics for those releases or raise the Transformers dependency floor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8a5b17. The stopping criterion now returns a scalar Python bool because the decision is already rank-global. This preserves the intended all-local-sequences behavior, is compatible with Transformers 4.32 (which applies Python any() to criterion results), and also broadcasts correctly in newer StoppingCriteriaList implementations. I added a two-sequence regression assertion covering the 4.32-style any() path.
|
|
||
| import pytest | ||
| import torch | ||
| from torch import distributed as torch_dist |
There was a problem hiding this comment.
Replace the direct torch distributed import
This directly imports PyTorch's distributed module under an alias, bypassing the repository's required deepspeed.comm abstraction and the textual check-torchdist hook. Rework the Gloo setup through the DeepSpeed communication layer rather than adding this unapproved direct dependency.
AGENTS.md reference: AGENTS.md:L10-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8a5b17. I removed the direct torch.distributed import and initialize/query process groups only through deepspeed.comm. Since this is specifically a CPU/Gloo subgroup topology test, the test temporarily mocks DeepSpeed's accelerator as CPU during deepspeed.init_distributed(); this avoids binding Gloo to a visible GPU on accelerator hosts while retaining the DeepSpeed communication abstraction. The 4-rank subgroup test and the 2-GPU scripted-EOS ZeRO-3 test both pass in Slurm job 400417.
Signed-off-by: LiRunGuo <li19107254665@gmail.com>
020a184 to
d8a5b17
Compare
|
@LiRunGuo I found one remaining subgroup liveness issue at exact head
The current 4-rank Gloo test calls The narrow fix is to pass |
Summary
max_new_tokensThe rendezvous uses
optimizer.dp_process_group, which is the group used by ZeRO-3 parameter gathers, rather than assuming the default world group. CUDA/HIP graph-capture rollout remains outside this change because it bypasses Hugging Facegenerate().Closes #8321.
Validation
[5, 5]), while preserving local response lengths0.6784,0.1554)Exact validation head:
020a184db69e2f7aead70fb03e0144f09675a6dc.