Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
This looks like a solid, focused change. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35f904af97
ℹ️ 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".
| local_rank = int(os.environ.get('LOCAL_RANK', dist.get_rank() if dist.is_initialized() else 0)) | ||
| device = local_rank % max(get_accelerator().device_count(), 1) | ||
| get_accelerator().set_device(device) | ||
| return torch.device('cuda', device) |
There was a problem hiding this comment.
When cpu-torch-latest runs this newly added file (the workflow invokes all of unit/ in .github/workflows/cpu-torch-latest.yml), DistributedTest initializes CPU/Gloo because CPU_Accelerator.is_available() is true, so these classes are not skipped; the first _full() then allocates on this unconditional CUDA device and fails with the CPU-only PyTorch build. Mark the distributed classes CUDA-only before launch while leaving the *CPU classes runnable.
Useful? React with 👍 / 👎.
| if get_accelerator().device_count() < self.world_size: | ||
| pytest.skip(f'needs {self.world_size} visible accelerators, found {get_accelerator().device_count()}') |
There was a problem hiding this comment.
On CUDA hosts exposing fewer than eight devices, this check runs too late to skip: DistributedTest first spawns world_size = 8, and DistributedTest._dist_run calls set_device(local_rank) before invoking the test method. Ranks beyond the available device count therefore fail during initialization, so ordinary 2/4-GPU full-suite runs fail or hang instead of reaching this pytest.skip; move the resource check to collection or launch time.
Useful? React with 👍 / 👎.
| return cdb.new_group(ranks) | ||
| if timeout is None: | ||
| return cdb.new_group(ranks) | ||
| return cdb.new_group(ranks, timeout=timeout) |
There was a problem hiding this comment.
When oneCCL is available, init_distributed can install CCLBackend; calling the newly advertised dist.new_group(..., timeout=...) reaches this keyword call, but deepspeed/comm/ccl.py still defines new_group(self, ranks) only, so every timeout-bearing subgroup creation raises TypeError instead of creating a group. Extend that sibling override (and the base interface) or explicitly reject unsupported timeouts.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| if not segment.shape or len(segment.shape) != ndim: | ||
| raise AffineTransferExecutionError('segment dimensionality does not match its logical shape') |
There was a problem hiding this comment.
When source and target maps describe a 0-D scalar (for example replicated_map((), 1)), plan_transfer legitimately emits a one-element TransferSegment whose shape is (), but this truthiness check rejects it before either a local or P2P copy. Consequently the generic executor cannot execute valid planner output for scalar parameters; validate dimensionality with len(segment.shape) != ndim without treating a zero-dimensional shape as an empty segment.
Useful? React with 👍 / 👎.
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
35f904a to
4a4bee9
Compare
This branch has not been deployed
2 participants
The merged #8623 planner describes affine shard copies. This PR executes those plans directly into existing frozen target shards with bounded peer staging, without reconstructing the full logical tensor.
Refs #8230, #8252 and 0z5a#1. This is the executor portion of that work; #8800 supplies live AutoEP metadata separately. The diff is limited to 3 files, 972 added lines: executor, contract tests and API documentation. Historical benchmark drivers and evidence remain in the fork and are linked below.
execute_transfer/execute_transfersaccept explicit source and target rank bindings, preserve strided regions and storage offsets, and collectively reject invalid buffers, aliases or request-order disagreement before writes. Shared plans reuse their schedule within a call. CUDA handles drain every 32 rounds and before return; other work handles retain immediate waits.Review fixes: CUDA tests check accelerator type and visible device count before worker launch; valid scalar plans now execute; the unrelated
new_group(timeout=...)changes are removed, preserving the existing CCL interface.Validation on this exact executor and test source:
On the same two H100s, restoring the original scalar predicate causes all four scalar regressions (single/batched, local/cross-process) to fail with the dimension-rejection defect. The fixed full file passes, including FP32/FP16/BF16 transfer, mixed storage windows, collective refusal, request ordering and a shared-plan batch spanning 48 rounds. The final pytest and Slurm step exit zero and the assigned GPU contexts are gone. Startup/resource failures are retained separately.
Historical eight-H100 non-scalar correctness evidence records 304 BF16/FP32 checks plus mixed-layout and shared-plan checks. Historical paired performance reports 1.289x geometric speedup, 95% CI [1.2703, 1.3080], for a 1-GiB synthetic frozen-weight handoff. These are prior non-scalar runs, not a new eight-GPU qualification of this rebased and reduced branch or a full-model performance claim.
Scope: scale-1 plans and frozen packed weights. No optimizer migration, live AutoEP swap, CPU/NVMe offload integration, training convergence/resumption, serving or non-CUDA vendor qualification. The staging budget applies per peer direction; a payload-time transport failure may partially modify targets. Maintenance contact: @0z5a.