Repository navigation
Keep empty gradient-norm ranks in collective reductions - #8918
Levius-Fubuki wants to merge 3 commits into
Conversation
Signed-off-by: levius <2114377220@qq.com>
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
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. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| Total norm of the tensors (viewed as a single vector), as a detached float32 scalar tensor. | ||
| """ | ||
| assert isinstance(input_tensors, Iterable), f'expected Iterable type not {type(input_tensors)}' | ||
| input_tensors = list(input_tensors) |
There was a problem hiding this comment.
@Levius-Fubuki it would be very helpful to simplify this code by refactoring out the local norm computation into a helper. That would reduce code duplication and ensure consistent handling across similar branches.
There was a problem hiding this comment.
Addressed in 3db49c4.
Extracted _get_local_norm_contribution to centralize FP32/device placement, empty-input handling, and the local MAX or sum-of-pth-powers contribution. The outer function now shares the collective dispatch between finite-p and infinity branches, retaining the existing group selection and collective order. Finite-p norms take the root only after SUM reduction; infinity uses MAX.
Local validation on Apple M3, macOS ARM64, Python 3.12.13, torch 2.14.1, CPU/Gloo:
DS_ACCELERATOR=cpu CUDA_VISIBLE_DEVICES=0,1 LOCAL_SIZE=2 GLOO_SOCKET_IFNAME=lo0 DS_UNITTEST_TIMEOUT=90 python -m pytest --forked tests/unit/runtime/test_runtime_utils.py --torch_ver=2.14.1 -qResult: 27 passed, both before and after refactoring. This includes real two-process Gloo reductions for empty model/expert norm contributors, iterable inputs, and the BF16 clipped engine update. Changed-file pre-commit, syntax checks and git diff --check pass. CUDA/NCCL validation was not rerun for this follow-up because the GPU server is offline; the earlier GPU results apply to the previous PR revision.
Signed-off-by: levius <2114377220@qq.com>
|
I independently validated this on the current head ( ResultsFull PR test file: → Distributed repros on the same setup:
One remaining edge caseAn iterable containing a zero-element tensor is still not covered: get_global_norm_of_tensors([torch.empty(0)], norm_type=2) # 0.0, OK
get_global_norm_of_tensors([torch.empty(0)], norm_type=float("inf"))
# RuntimeError: max(): Expected reduction dim to be specified for input.numel() == 0.Zero-size tensors are reachable (zero-size parameters / sharded grads), and Suggested change: def _get_local_norm_contribution(input_tensors, norm_type, device):
"""Compute the FP32 input to the global MAX or SUM reduction."""
- if not input_tensors and norm_type > 0:
- return torch.zeros((), dtype=torch.float32, device=device)
-
all_norms = []
for tensor in input_tensors:
if norm_type == inf:
+ if tensor.numel() == 0:
+ continue
tensor_norm = tensor.detach().abs().max().float()
else:
tensor_norm = tensor.detach().float().norm(norm_type)
all_norms.append(tensor_norm.to(device))
+ if not all_norms and norm_type > 0:
+ return torch.zeros((), dtype=torch.float32, device=device)
+
local_norms = torch.stack(all_norms)Regression case to add next to @pytest.mark.parametrize("norm_type", [2, float("inf")])
def test_zero_element_tensor(norm_type):
norm = ds_utils.get_global_norm_of_tensors([torch.empty(0)], norm_type=norm_type)
assert norm.item() == 0.0With that applied locally, all of the above pass, including mixed inputs: I can't push to your branch, so please take the diff as-is if useful. The only other thing I'd flag is that the CUDA/NCCL path has not been rerun since the refactor because CI is blocked; a green CUDA run would be good before merge. |
|
Thank you for the detailed reproduction and suggested fix. Addressed in Zero-element tensors are now skipped for infinity contributions, and a device-local FP32 zero is returned when no contributions remain. Collective group selection and ordering remain unchanged. The existing tests now cover one/all zero-element ranks, mixed tensors, and generator inputs. On Apple M3, macOS 15.1 ARM64, Python 3.12.13, torch 2.14.1, CPU/Gloo, the full runtime-utils file reports 38 passed; changed-file pre-commit checks pass. The new zero-element iterable test fails on the previous head and passes with this fix. CUDA/NCCL has not been rerun for this follow-up. Thank you again for checking this edge case. |
Signed-off-by: levius <2114377220@qq.com>
Fixes #8912.
get_global_norm_of_tensorsfails when a rank has no local norm inputs, leaving peers in a collective. Materialize iterable inputs once and use a device-local FP32 zero for empty positive p-norm and infinity-norm contributions. Keep the existing model/expert/pipeline group choices and collective order. Nonempty results retain their input-device placement.Validation: 2 x NVIDIA GeForce RTX3090 24GB, driver580.105.08, torch2.12.1+cu130, CUDA13.0, NCCL2.29.7, Python3.12.3. On upstream bf222a2, a two-rank NCCL reproduction fails with
stack expects a non-empty TensorListon the empty rank; a nonempty generator regression fails for the same reason. With the fix:Coverage includes L1/L2/L3/infinity, one/all/no empty ranks, real model/expert groups, generators, output dtype/device, and a public-engine BF16 ZeRO1 TP2/DP1 clipped-SGD update with an empty norm rank. Changed-file pre-commit checks pass. The committed-diff CI selector identifies 87 impacted v1 test files; those broader tests have not all been run locally.
Duplicate check: no open PR found for #8912 or this empty-input fix. Existing #8478/#8635 address norm accounting rather than empty collective participants. Sibling flattened/ZeRO norm helpers were inspected and are unchanged. No production convergence or throughput claim; other accelerators, full folded-MoE models, offload, and combined pipeline/expert topologies are unvalidated.
AI assistance (OpenAI Codex) was used for investigation, implementation and device tests. The human submitter confirmed manual diff review and test-result verification and authorized submission. Device tests above were executed on the requested server with Codex; no additional human test execution is asserted.
Review follow-up (3db49c4): extracted the local norm contribution helper and shared collective dispatch, retaining group selection/order and finite-p versus infinity semantics. On Apple M3/macOS ARM64, Python 3.12.13, torch 2.14.1, CPU/Gloo, the complete
tests/unit/runtime/test_runtime_utils.pysuite reports 27 passed before and after refactoring, including real two-process reductions and a BF16 clipped engine update. Changed-file pre-commit, syntax and diff checks pass. The GPU results above were obtained on the original PR revision9ed909a4; CUDA/NCCL tests were not rerun for this follow-up because the server is offline.Published follow-up on 2026-10-30 (
5cffa02f): handle zero-element tensors inside a nonempty iterable for infinity norms. Zero-element infinity contributions are skipped, and a device-local FP32 zero is used when no contributions remain; collective group selection/order and finite-p behavior are unchanged. Expanded existing distributed and iterable tests cover one/all zero-element ranks, mixed tensors, and generators. On Apple M3/macOS 15.1 ARM64, Python 3.12.13, torch 2.14.1, CPU/Gloo, the new zero-element iterable regression fails on prior head3db49c4awith the reported max() error; the complete updated runtime-utils file reports 38 passed after the fix. All applicable changed-file pre-commit checks pass. These checks were executed with Codex; no new CUDA/NCCL validation is claimed. The submitting human confirmed reviewing the final diff and running the relevant tests, and authorized DCO sign-off and publication.