[c10d][nccl] Fix ProcessGroupNCCL::split OOB when the world PG is not the first NCCL PG - #192109
Closed
tushar00jain wants to merge 4 commits into
Closed
tushar00jain wants to merge 4 commits into
tushar00jain wants to merge 4 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/192109
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 17edab2 with merge base f3849a4 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This was referenced Aug 4, 2026
Closed
Closed
d4l3k
approved these changes
Aug 5, 2026
d4l3k
left a comment
Member
There was a problem hiding this comment.
Re-approving due to switching to ghstack from sl
Collaborator
|
Starting merge as part of PR stack under #192113 |
Collaborator
|
Starting merge as part of PR stack under #192113 |
Collaborator
|
Starting merge as part of PR stack under #192113 |
pytorchmergebot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…process group (#192110) Summary: ProcessGroup::splitGroup did auto backendOpts = opts.has_value() ? *opts : parentBackend->getBackendOptions(); backendOpts->group_name = groupName; backendOpts->timeout = ...; backendOpts->group_desc = groupDesc; auto splitBackend = parentBackend->split(store, ranks, backendOpts); and every getBackendOptions() in tree returns the backend's live options_, not a copy -- ProcessGroupGloo, ProcessGroupNCCL, nccl2 and FakeProcessGroup all just cast options_, and ProcessGroupWrapper/LazyBackend delegate. So a split rewrites the *parent's* group_name, timeout and group_desc, and the child ends up holding the very same Options object. ProcessGroupGloo::split then writes `glooOpts->global_ranks_in_group = std::move(globalRanksInGroup)` onto it, i.e. the parent's rank map is replaced by the child's, and the parent's next split indexes a vector sized for the child: parent BEFORE any split: group_name='0' timeout=0:01:51 ranks=[] parent AFTER split #1: group_name='0:split:[0, 1]' timeout=0:03:42 ranks=[0, 1] child1.options IS parent.options: True [rank2] split #2 child ranks = [139894703605936, 1900999358] (expected [0, 2]) [rank0] split #2 child ranks = [0, 1954417006] (expected [0, 2]) That is a heap read past the end, on a stock 4-rank CPU-only gloo world driven entirely through the public ProcessGroup.split_group(ranks) pybind, whose opts argument defaults to nullopt (test/distributed/test_device_mesh.py already calls it that way). The child it produces has garbage global ranks, so its FlightRecorder identity and any rank translation done through it are wrong; on a different heap layout the same read is a SIGSEGV. mergeRemoteGroup has the same aliasing and no escape hatch at all: it takes no opts parameter, so it always rewrites the parent's options and hands the live object to merge(), leaving parent and merged child permanently sharing one Options for the rest of the process. All four defects fixed here are stock. None needs nccl2, or any custom backend: the aliasing is a 4-rank CPU-only gloo world, the Python options substitution is any "cpu:gloo,cuda:nccl" world, the split-once-per-backend hang is a bare "gloo" world on an accelerator host, and the deepcopy TypeError is any gloo-backed group. What the nccl2 migration changed is only reachability: it routes CPU coordination groups through split_group (see MIGRATION_RULES.md), which is what made every rank start printing the ProcessGroupGloo::split options warning on every compound split. That warning is the thread this change was pulled from. The order of discovery matters for reading the rest of this message. The warning looked cosmetic, so the obvious Python one-liner -- stop substituting pg_options, let the C++ per-backend fallback do its job -- was implemented and probed. It removed the warning and gave the gloo child the right group_name, timeout and ranks, and it turned the cosmetic problem into a memory-safety one, because the fallback path is exactly the aliasing above. That is how the C++ defect was found. The split-once-per-device-type hang was then found while verifying the Python change, because deleting the deepcopy is what first makes dist.split_group reachable at all on a gloo world; and the gloo _Options TypeError surfaced in the same runs. Fix it in C++, at the point where the options are handed to a backend: add a virtual Backend::Options::clone() and copy per backend in splitGroup and mergeRemoteGroup. Threading a per-device opts map through splitGroup instead was considered and is strictly worse -- it does not fix the no-entry fallback (a device with no map entry still lands on the parent's live options), does not fix mergeRemoteGroup at all, changes the signature and every caller, and the right per-device options are simply the parent's own, which clone() gives for free. Making each backend's split()/merge() defensively copy what it is handed was also rejected: the parent's group_name, timeout and group_desc are already overwritten before split() is ever called, so the copy has to happen at the call site or the parent is corrupted regardless. And copying in Python -- which is what the deleted deepcopy was -- requires every Options subclass to carry a pickling pybind, which is precisely the requirement gloo failed. clone() is deliberately not pure. ProcessGroupXCCL::Options derives from Backend::Options but lives out of tree in third_party/torch-xpu-ops, fetched by caffe2/CMakeLists.txt and pinned by third_party/xpu.txt, so a pure virtual would make it abstract and break the XPU build. For the same reason the call site guards against slicing: auto copy = opts->clone(); if (copy == nullptr || typeid(*copy) != typeid(*opts)) { return opts; // subclass has not adopted clone(): keep legacy sharing } so a backend that has not adopted clone() keeps exactly today's behaviour rather than being handed a sliced base Options -- aliasing is a bug, but handing a backend an object of the wrong dynamic type is worse, and the backend cannot detect it. RTTI is already required by c10d (the dynamic_intrusive_pointer_casts in ProcessGroupGloo.cpp and FakeProcessGroup.hpp), so the typeid test costs nothing new. An explicit caller-supplied opts is now applied only to the group's *default* backend type; the other device legs inherit a clone of their own backend's options. That mirrors what pg_options already means for init_process_group, where _new_process_group_helper hands it to the backend creator that understands it, and it is what stops a ProcessGroupNCCL::Options from reaching the gloo leg of a "cpu:gloo,cuda:nccl" group. The caller's object is cloned too, so split_group no longer mutates it. While here, split (and merge) each backend *instance* once rather than once per device type. splitGroup iterates deviceTypeToBackendType_, but several device types can map to the same backend object: init_process_group("gloo") registers one ProcessGroupGloo for both cpu and cuda, and ProcessGroup::setBackend explicitly reuses the backend already registered for a backend type. The loop therefore built a second child that was immediately discarded -- after it had already rendezvoused on the *same* PrefixStore("<groupName>/") keys as the real child. The two gloo contexts race for "<group>/0/rank_N" and peers pair with the discarded group: $ python bug3b_double_split_same_backend.py gloo 5 # cpu+cuda -> one PG [rank1] split 0 done <hang; rank0 never returns from split 0> $ python bug3b_double_split_same_backend.py cpu:gloo 5 # one device type [rank0] split 0..4 done / [rank1] split 0..4 done This is pre-existing and independent, but it blocks the change below: removing the Python deepcopy is what makes dist.split_group reachable at all on a gloo world, and the first thing it then hits is this hang. On the Python side, split_group substituted `pg_options = copy.deepcopy(parent_backend.options)` where parent_backend is the *accelerator* backend, and passed that one object to every device's split(). On a "cpu:gloo,cuda:nccl" world the gloo leg got a ProcessGroupNCCL::Options, warned "Tried to pass options to ProcessGroupGloo::split that are not ProcessGroupGloo::Options. Falling back to default options.", and silently lost the caller's timeout and group_name (123 s and a real name became 0:30:00 and ''; the empty name is what the ctor feeds to setGroupUid, so the child's pg uid was empty too). Not cosmetic even for backends that only warn -- a 30-minute timeout where the caller asked for 123 s changes when a hang is detected -- and a backend that is strict about its Options type hard-errors on the same mismatch instead of warning. Worse, on a *gloo* world the accelerator backend is the gloo backend, and ProcessGroupGloo::_Options has no __copy__/__deepcopy__ pybind (unlike the NCCL, nccl2 and fake Options), so dist.split_group did not merely warn, it threw: TypeError: cannot pickle 'torch._C._distributed_c10d._Options' object i.e. dist.split_group was unusable for every gloo-backed group on an accelerator host, in stock, with no migration involved -- nothing in stock CI splits a bare gloo world on such a host. Delete the deepcopy and leave pg_options as the caller passed it; the C++ side now gives each device backend a copy of its own options, and the pickling requirement disappears with the deepcopy that imposed it. The two halves must land together. Leaving pg_options=None *without* the C++ clone was tried and is a memory-safety regression, not a fix: it routes the gloo leg straight to parentBackend->getBackendOptions(), i.e. exactly the aliasing above, so the parent's global_ranks_in_group is overwritten with the child's and the parent's next split reads out of bounds. The Python-only "one-line fix" is not viable on its own. There is no downstream fix for any of this. A caller can pass explicit pg_options, but there is no per-device options API to pass, so on a multi-device world one of the legs is still wrong; merge_remote_group takes no opts at all; and nothing outside c10d can stop splitGroup writing group_name, timeout and global_ranks_in_group onto the parent's live object. The only downstream "mitigation" is to never split the same parent twice -- which DeviceMesh, vLLM and our own migration all do routinely. While here, close the netName leak this change would otherwise multiply. nccl2's Options::clone(), added above, is c10::intrusive_ptr<::c10d::Backend::Options> clone() const override { auto copy = c10::make_intrusive<Options>(*this); copy->config = cloneNcclConfig(config); return copy; } and cloneNcclConfig strdup's config.netName while ncclConfig_t records no owner. splitGroup and mergeRemoteGroup call cloneOptions() once per backend per split/merge, into a backendOpts local that nccl2's split() never retains -- it builds its own childOpts -- so that strdup is unreachable the moment the loop iteration ends. The allocation itself is not new: cloneNcclConfig and its use in nccl2's split() both predate this stack, so the once-per-process leak is inherited. What is ours is the *rate*: cloning per split/merge turns it into one leaked allocation per split_group / merge_remote_group per PG per rank whenever a caller sets net_name. We did not create the leak, we multiplied it, so the ownership fix belongs in this commit rather than stacked on top of it. clone() now ties the allocation it makes to the Options that made it: copy->config = cloneNcclConfig(config); if (copy->config.netName != nullptr) { copy->owned_net_name_ = std::shared_ptr<const char>( copy->config.netName, [](const char* p) { std::free(const_cast<char*>(p)); }); } with a private `std::shared_ptr<const char> owned_net_name_` member. The ownership model, stated across construct / clone / destroy: - Options(bool): netName is NCCL_CONFIG_UNDEF_PTR (NULL). Owns nothing. - Options(const BaseOptions&): shares the base's netName, and the base keeps owning it -- untracked, unchanged, unfreed. A conversion that deep-copied here would start freeing a string a live stock Options still points at. - the copy constructor and copy assignment stay `= default`, so the shared_ptr is copied along with every other field: copies share the allocation and the reference count, and the string outlives every Options pointing at it and is freed exactly once, when the last of them dies. - clone() is the sole place ownership is established, and only for the one allocation it makes. - ~Options() stays `= default`; the member does the freeing, so there is no hand-written destructor to keep in sync as fields are added. - anything else that lands in config.netName -- the NCCLConfig pybind's strdup, a config assigned wholesale from Python, the clone split() hands to ncclCommSplit -- is untracked and untouched, exactly as before. If Python replaces config on a cloned Options, the shared_ptr still frees our now unreferenced buffer and leaves theirs alone. Ownership is deliberately taken *only* for an allocation an Options makes for itself and never hands to NCCL. The pinned third_party/nccl (v2.29.7-1) copies the caller's string -- src/init.cc:1863-1865 malloc's strlen(tmpNetName)+1 and memcpy's into comm->config.netName, and src/init.cc:349 frees only that copy -- so it never frees the caller's pointer, and everything cloneNcclConfig allocates leaks under this tree. But the comment already at nccl2/ProcessGroupNCCL.hpp:52-56 records that *some* NCCL versions free the caller's string on comm destruction, PyTorch can be built against a system NCCL, and the cutoff version could not be established (third_party/nccl carries one squashed release commit and no history). A destructor that freed whatever is in config.netName would therefore double-free on any version matching that comment, since the same pointer is handed to ncclCommInitRankConfig/ncclCommSplit, and would dangle a live stock Options in the base-adopting case. So the strdup's that split() and ProcessGroupNCCLLazy hand to NCCL are left alone; fixing those means either pinning a version cutoff or strdup'ing separately at every NCCL handoff, which only moves the leak. The `= default` special members are load-bearing and should not be tidied into hand-written ones. An intermediate version of this did exactly that -- a copy constructor that strdup'ed plus a destructor that free'd a raw char* member -- and it compiled, leaked nothing, and silently stopped copying abort_process_on_timeout_or_error, because a hand-written copy constructor initialises the base and then leaves *derived* members at their default initialisers. ProcessGroupNCCL2Test::test_options_type caught it, as "AssertionError: True is not false" through copy.deepcopy. The shared_ptr shape has no such trap: copy semantics stay compiler-generated, and therefore stay correct for fields added later. Test Plan: The four repro scripts, re-run against the rebuilt tree. bug3_gloo_split_alias.py (4 ranks, CPU only) now keeps the parent untouched, reports that the child's options are a distinct object, and gives all four ranks the correct second-split ranks: python triage/bug3_gloo_split_alias.py 29541 parent BEFORE any split: group_name='0' timeout=0:01:51 ranks=[] parent AFTER split #1: group_name='0' timeout=0:01:51 ranks=[] child #1: group_name='0:split:[0, 1]' timeout=0:03:42 ranks=[0, 1] child1.options IS parent.options: False [rank0] split #2 child global_ranks_in_group = [0, 2] (expected [0, 2]) OK [rank1] split #2 child global_ranks_in_group = [1, 3] (expected [1, 3]) OK [rank2] split #2 child global_ranks_in_group = [0, 2] (expected [0, 2]) OK [rank3] split #2 child global_ranks_in_group = [1, 3] (expected [1, 3]) OK Pre-fix the parent was rewritten to group_name='0:split:[0, 1]' timeout=0:03:42 ranks=[0, 1], "child1.options IS parent.options: True", and all four ranks printed CORRUPT with garbage rank lists. bug4_python_options.py (2 ranks, "cpu:gloo,cuda:nccl") emits no warning and the gloo child keeps the caller's 123 s timeout and group name: CUDA_VISIBLE_DEVICES=2,3 torchrun --nproc-per-node 2 \ triage/bug4_python_options.py child gloo opts type=_Options timeout=0:02:03 name='4a2ad14e...' child nccl opts type=Options timeout=0:02:03 name='4a2ad14e...' both with ranks=[0, 1] child gloo options IS parent gloo options: False parent gloo timeout after split: 0:01:51 ranks=[] Pre-fix this printed the "Tried to pass options to ProcessGroupGloo::split" warning and gave the gloo child 0:30:00 and ''. bug4_gloo_world_deepcopy.py splits instead of raising TypeError: CUDA_VISIBLE_DEVICES=2,3 torchrun --nproc-per-node 2 \ triage/bug4_gloo_world_deepcopy.py OK: child gloo timeout=0:30:00 name='4a2ad14e...' ranks=[0, 1] The 0:30:00 there is not a regression: that script passes no timeout= to split_group, and stock split_group resolves timeout=None to _get_default_timeout(pg_backend) = 30 min before the C++ ever sees it. bug3b_double_split_same_backend.py completes 5/5 with a bare "gloo" world, where it previously hung with rank 0 never returning from split 0: CUDA_VISIBLE_DEVICES=2,3 timeout 180 python \ triage/bug3b_double_split_same_backend.py gloo 5 [rank0] split 0 done ... [rank0] split 4 done [rank1] split 0 done ... [rank1] split 4 done EXIT=0 New tests, all of which fail pre-fix: CUDA_VISIBLE_DEVICES=2,3 python test/distributed/test_c10d_gloo.py \ CommTest.test_split_group_does_not_alias_parent_options \ CommTest.test_split_group_keeps_gloo_options -v Ran 2 tests in 7.291s OK python test/distributed/test_c10d_common.py SplitGroupOptionsTest -v test_split_group_clones_parent_options ... ok test_split_group_opts_apply_to_default_backend_only ... ok Ran 2 tests in 0.900s OK Against the pre-fix binary the same three fail as "unexpectedly identical: ProcessGroupGloo._Options object", "unexpectedly identical: Backend.Options object" and "'caller-supplied' != 'cpu-backend'". Full suites, serially on otherwise-idle GPUs, all green: test_c10d_common.py Ran 72 tests in 113.349s OK test_c10d_gloo.py Ran 289 tests in 1098.597s OK (skipped=4) test_c10d_nccl.py Ran 284 tests in 1645.782s OK (skipped=15) test_c10d_nccl2.py Ran 20 tests in 103.397s OK test_c10d_pybackend.py Ran 5 tests in 4.845s OK test_device_mesh.py Ran 76 tests in 297.845s OK (skipped=5) Three known gaps. mergeRemoteGroup's clone and merge-once path has no multi-device test in tree -- test_c10d_pybackend.py covers the single-device merge only -- so the dedupe branch there is argued from symmetry with split, not measured. The typeid slicing guard is dead code in an in-tree build, since every in-tree Options subclass now overrides clone(); its real consumer is the out-of-tree XPU Options, which cannot be built here, so someone with an XPU build should confirm it compiles and does not silently fall back to aliasing. And stock ProcessGroupNCCL::Options::clone() copies ncclConfig_t shallowly, so a caller-set config.net_name/comm_name (strdup'd in init.cpp) stays shared between parent and child -- unchanged from the __deepcopy__ pybind it replaces, so not a regression; nccl2's clone deep-copies via cloneNcclConfig(), and fixing the stock one belongs to whoever owns ProcessGroupNCCL.cpp. The netName ownership half is not meaningfully testable from Python. The leak is one small allocation on a path reachable only when a caller sets net_name; observing it needs heap accounting (ASAN/valgrind) rather than an assertion, and copy.deepcopy(opts).config.net_name reads the same string before and after. Its real regression test is the existing ProcessGroupNCCL2Test::test_options_type, which is what caught the hand-written-copy-ctor version described above; every split exercises the clone() path itself, with netName == nullptr, and neither crashes nor double-frees. Re-run at the tip of the stack, on 4 idle H100s, with the whole of it applied: CUDA_VISIBLE_DEVICES=0,1,2,3 python test/distributed/test_c10d_nccl2.py -v Ran 24 tests in 135.623s OK CUDA_VISIBLE_DEVICES=0,1,2,3 python test/distributed/test_c10d_nccl.py -v Ran 285 tests in 1797.054s OK (skipped=15) Authored with the assistance of an AI coding agent. Pull Request resolved: #192110 Approved by: https://github.com/d4l3k ghstack dependencies: #192104, #192105, #192106, #192107, #192108, #192109
pytorchmergebot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…nd string (#192111) Summary: split_group's `backend=` filter validated the request against a re-expansion of the parent's backend string through Backend.default_device_backend_map, via _parse_backend_string. That is not the expansion the parent process group was built from. _new_process_group_helper iterates BackendConfig(backend), which expands through Backend.backend_capability -- "which devices can this backend run on" -- while default_device_backend_map answers "which devices pick this backend by default". The two agree for every device-qualified parent string, and disagree for a bare one: backend string BackendConfig default_device_backend_map "gloo" cpu, cuda cpu "fake" cpu, cuda, hpu, xpu hpu so on a stock init_process_group("gloo", device_id=cuda:0) world, which really does run gloo on cuda: split_group(backend="cuda:gloo") ValueError: Requested backend for device 'cuda' is not present in the parent process group (parent backends: {'cpu': 'gloo'}) The failure is a hard ValueError at group-construction time on a filter the parent can actually satisfy, so the caller has no way to build the child it asked for. Over a bare "fake" parent every filter form fails -- "cuda:fake", "cpu:fake" and "cpu:fake,cuda:fake" with the ValueError above, bare "fake" with "RuntimeError: splitGroup deviceTypes filter must include the parent process group's default backend device type" -- leaving backend=None as the only usable call. This is not specific to fake process groups -- a bare "fake" world is built exactly like a bare "gloo" world, FakeProcessGroup really does support splitting (supportsSplitting() is true, split() is implemented, and calling pg.split_group(device_types=...) directly past the Python gate works for ['cpu'], ['cpu','cuda'] and the full device list), and test_fake_pg.py already splits one. CI never caught it because every existing test of the filter (test_c10d_nccl.py, test_device_mesh.py) uses a device-qualified parent, where the two expansions coincide. How it was found: out of the Megatron --fake-process-group regression triage during the nccl2 migration. That flag broke on our own wiring commit, and one of the three defects behind it was "a fake parent is unfilterable by split_group", which forced the Megatron side to pass backend=None. Filed that way, the entry read as a fake-PG limitation. Triage of the claim showed the diagnosis was too narrow: the same rejection reproduces with no fake PG anywhere, on a plain stock init_process_group("gloo", device_id=cuda:0) world asked for backend="cuda:gloo" -- so it is a stock c10d bug, found downstream and root-caused upstream, and the register entry was retitled accordingly. Nothing here needs nccl2: the migration only raised exposure, because it routes CPU coordination groups through split_group. It does not belong downstream. The only workarounds available to a caller are to pass backend=None -- which drops the filter entirely, so the child carries every device the parent has -- or to always build device-qualified worlds so the two expansion tables happen to agree. The first is what Megatron had to do and it silently gives up a real capability; the second is impossible for a fake world (fake is registered on four devices and is no device's default, so there is no qualified spelling that survives the round trip) and is not something a library can impose on the application that called init_process_group. The wrong answer is produced by two of torch's own tables disagreeing about what a bare backend name means; only torch can reconcile them. Derive the parent's per-device backends from the BackendConfig the group was actually built from -- already constructed a few lines above, so no new state and no new API -- intersected with parent_pg._device_types, which is the same set the C++ check in ProcessGroup::splitGroup validates against. That is the point of the shape: after this, the Python gate and the C++ gate are answering the same question about the same parent, instead of one guessing from a global default map. A device-qualified request still goes through _parse_backend_string unchanged, deliberately -- that path was never wrong, and narrowing the change keeps the behaviour of every real production world identical. A bare request now selects every parent device running that backend, which is what a user asking for "gloo" over a gloo world means, and raises a clear ValueError naming the parent's real backends if it matches nothing, instead of the misleading per-device message. For a device-qualified parent the new parent map is byte-identical to the old one, and a bare request over such a parent resolves the same way it did before ("nccl" -> {cuda:nccl}, "gloo" -> {cpu:gloo}), so the only behaviour that changes is the bare-parent case that was broken. The same function had a second, independent defect on the other side of the split, found while triaging the first. After a *successful* split it looked the child's backend up unconditionally on torch.accelerator.current_accelerator(), so any filter that legitimately drops the accelerator leg died afterwards with RuntimeError: No backend type associated with device type cuda which reproduces on a stock bare-"gloo" world with backend="cpu:gloo" and has nothing to do with fake either. The child is already built and registered at that point, so the caller gets an exception for a split that succeeded. Prefer the accelerator only when the child actually has it and fall back to cpu otherwise, keeping the pre-existing RuntimeError for a child that has neither. split_backend_class is only consumed by the torchcomms _world.comms.append branch, so this narrows a hard failure to the lookup the caller meant and leaves the non-torchcomms path unchanged. Three things are deliberately left alone. ProcessGroup::splitGroup's "the filter must keep the default backend's device" check picks that device with a find_if over the unordered_map deviceTypeToBackendType_, so for a parent whose devices all share one BackendType (bare gloo on cpu+cuda, fake on four devices) the "default backend device" depends on hash bucket order rather than bound_device_id; cuda:gloo and cuda:fake filters therefore still fail there. That is a real wart but needs a C++ change and its own repro; preferring bound_device_id's device type among the devices sharing backendType_ is the likely fix. _parse_backend_string's bare-name branch is now unreachable from split_group but is left in place: it is a generic helper with its own callers' semantics, and pruning it is not this change's business. And _world.pg_backend_config for the child is still str(BackendConfig(backend)), so a bare filter that matches only some parent devices records a wider config than the child really has -- cosmetic. Test Plan: test/distributed/test_fake_pg.py gains TestFakePG.test_split_group_backend_filter, next to the existing test_split_group* tests and in the same skipIfHpu / HAS_ACCELERATOR / FakeStore conventions. It asserts that bare "fake" keeps every parent device, that "cpu:fake" and "cpu:fake,<accel>:fake" keep exactly the named devices, and that "mps:fake", "cpu:gloo" and bare "gloo" are still rejected: python test/distributed/test_fake_pg.py -k split_group -v test_split_group_backend_filter ... ok test_split_group_consistent_naming_after_partial_split_rank_0..3 ... ok test_split_group_non_member ... ok test_split_group_rank_0..3 ... ok test_split_group_store_not_retained ... ok Ran 11 tests in 0.903s OK Against the old parent expansion the new test fails with the RuntimeError quoted above: full = dist.split_group(split_ranks=[[0, 1]], backend="fake") RuntimeError: splitGroup deviceTypes filter must include the parent process group's default backend device type. FAILED (errors=1) Recursive splits of a filtered child work too (cpu:fake,cuda:fake -> cpu:fake -> all_reduce), since the child's recorded backend string round-trips through the same expansion. The post-fix filter matrix, measured directly: fake parent: cpu:fake -> OK fake -> OK cpu:fake,cuda:fake -> OK None -> OK cuda:fake -> RuntimeError (C++ wart above) bare-gloo: cpu:gloo -> OK gloo -> OK (cpu+cuda) None -> OK cuda:gloo -> RuntimeError (C++ wart above) cpu:gloo,cuda:nccl (device_id=cuda): cuda:nccl -> OK nccl -> OK cpu:gloo / gloo / cuda:gloo rejected exactly as before Whole file and the neighbouring split-filter suites: python test/distributed/test_fake_pg.py Ran 73 tests in 1.357s ... OK python test/distributed/test_c10d_collectives.py -k split_group -v Ran 8 tests in 39.653s ... OK (skipped=2) python test/distributed/test_device_mesh.py Ran 76 tests in 297.845s ... OK (skipped=5) test_c10d_nccl.py's ProcessGroupNCCLGroupTest -k split ran 7 tests; the six split/filter tests pass, and the seventh, test_comm_split_world_pg_created_after_another_nccl_pg, failed at the time for an unrelated reason -- the NCCL split OOB fixed by another commit in this stack, which uses no backend= filter -- and is green in the post-rebuild sweep of the whole stack (Ran 284 tests, OK, skipped=15). Authored with the assistance of an AI coding agent. Pull Request resolved: #192111 Approved by: https://github.com/d4l3k ghstack dependencies: #192104, #192105, #192106, #192107, #192108, #192109, #192110
pytorchmergebot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…cator trace hook (#192112) Summary: cacheAllocatorRegisterHook ran NCCLComm::registerSegment(window=true) for any segment allocated into a pool that had been registered with symm=True, and on NCCL 2.29.7 ncclCommWindowRegister is unconditionally collective: src/dev_runtime.cc enqueues a group task that runs bootstrapIntraNodeAllGather to exchange memory handles, bootstrapIntraNodeBarrier to make sure everyone imported them, bootstrapIntraNodeBroadcast for the multicast handle, and a final bootstrapBarrier for the symmetric window. Every rank of the communicator must call it, the same number of times, in the same order, and none of those bootstrap sockets has a timeout. The hook is the wrong place for that. It is called from record_trace, which CUDACachingAllocator invokes from alloc_block and release_block with the device allocator's recursive_mutex held, immediately under the comment "Callbacks should not include any Pytorch call". So the NCCL rendezvous blocks every other thread in the process that wants to allocate. It also runs on whatever thread happens to allocate or free -- the main thread, an autograd worker, or for SEGMENT_FREE the GC pass that drops the last MemPool reference. And SEGMENT_ALLOC / SEGMENT_FREE are purely local allocator decisions: a rank-specific tensor, or fragmentation-driven release_available_cached_blocks firing out of the OOM retry on whichever rank happens to be tight, is enough to make one rank call and the others not. Reproduced as a hard hang on stock nccl, 2 ranks, no torchcomms in the process: all ranks register a symm pool, rank 0 alone allocates one more tensor into it, and rank 0's next plain torch.zeros() never returns. py-spy --native: accept (libc.so.6) ncclSocketAccept -> socketAccept (bootstrap.cc:1057) bootstrapIntraNodeAllGather (bootstrap.cc:1226) symMemoryMapLsaTeam -> symMemoryObtain -> ncclDevrWindowRegisterInGroup pncclCommWindowRegister (dev_runtime.cc:1102) c10d::NCCLComm::registerSegment c10d::cacheAllocatorRegisterHook ...::record_trace -> alloc_block -> NativeCachingAllocator::malloc at::detail::empty_cuda ... THPVariable_zeros There is no recovery from that state. ProcessGroupNCCL::abortComms() takes ncclCommMemPoolMapMutex before aborting the comms, and the parked hook is holding that mutex for the whole duration of the NCCL call, so the watchdog blocks behind it. Re-run with a 20 s PG timeout and a 130 s budget, both ranks still had to be killed. The defect is in stock ProcessGroupNCCL, not in the nccl2 backend: everything above was measured with backend="nccl" and no torchcomms loaded. nccl2's register half was already correct -- registerAddressLocked() is a plain ncclCommRegister with symmetric windows deferred to the collective ensureSegmentWindow() -- but nccl2 shares the two secondary hazards below, so it is hardened the same way here. How it was found: by inspection while implementing nccl2's register_mem_pool (earlier in this stack). Writing down *why* nccl2 defers window registration out of the allocator hook forced the question of whether stock's version of the same hook was safe, and reading NCCL 2.29.7's dev_runtime.cc answered it. No downstream run failed: Megatron's nccl_allocator always deregisters its pool collectively before allocating into it again, which is why the tp2_dp2_nvls bit-exact cell was clean on both arms. The purpose-built 2-rank harness is what turned the analysis into the reproducible hang above. Our own NVLS work raised the reachability -- more segments carry windows -- but did not create the bug, which predates the migration on both sides. Working around this downstream is not possible in general. The hang happens inside an ordinary torch.zeros() with the allocator mutex held, so the only downstream mitigation is to guarantee that no rank ever allocates into (or frees from) a symm-registered pool without every peer doing the same, in the same order -- unachievable when the divergence source is an OOM-driven release_available_cached_blocks on whichever rank happens to be tight. The alternative mitigation is "never call register_mem_pool(symm=True)", i.e. give up NVLS. And once wedged there is no escape hatch at any layer: NCCL's bootstrap sockets have no timeout, and c10d's own watchdog blocks behind the hook's mutex. The invariant has to be enforced where the hook is installed. The rule this change enforces is that the trace hook may only make NCCL calls that are local to this rank. cacheAllocatorRegisterHook now always registers with window=false and TORCH_WARN_ONCEs when it sees a symm pool, pointing at deregister_mem_pool() + register_mem_pool() as the collective way to get new segments windowed. Symmetric windows are created exclusively by registerMemPool(), which the user calls collectively -- which is what the nccl2 backend had already been doing during the migration, its registerAddressLocked() being a plain ncclCommRegister with windows deferred to ensureSegmentWindow(). The rule is written down as NOTE [NCCL calls from the caching allocator trace hooks] next to the hooks, because the next person to add a call here needs the reason, not just the current shape. This is a deliberate behaviour change: segments allocated into a symm pool *after* register_mem_pool(symm=True) are no longer auto-upgraded to symmetric windows. They stay registered as plain NCCL user buffers, so collectives over them take the non-symmetric algorithm -- the same decision on every rank, so no mismatch. That auto-upgrade is exactly what hangs, and the hook has no way to know whether the peers are making the same allocation. The cheaper-looking alternative -- keep the upgrade but only when the allocation "looks symmetric" -- does not exist: the hook sees one rank's allocator, and any predicate it could evaluate is exactly the local decision that diverges. Losing NVLS on late segments is a performance regression for a workload that grows its pool after registering it; hanging is not a performance regression, so the trade is not close. Callers that want the windows back have a supported, collective way to get them, and the warning names it. Two smaller hazards on the same path are fixed with it. ncclCommMemPoolMap is keyed by shared_ptr<NCCLComm> and hashed by pointer, so its iteration order is heap-address order and differs between ranks; with two communicators sharing a symm pool, rank A could issue windowRegister(commX) then windowRegister(commY) while rank B did the reverse -- two blocking rendezvous in opposite orders, a deadlock even under perfectly symmetric allocation. The new commsInterestedInSegment() returns the interested comms sorted by NCCLComm::getUniqueHash() -- the hex of the ncclUniqueId plus ":<splitCounter>" for split children, the one key every rank agrees on; device index or pointer order would not do. The map mutex is still held across the calls on purpose: that is what keeps abortComms() from retiring a comm underneath the hook. The nccl2 side has the same shape -- registeredComms_ is a std::set<ProcessGroupNCCL*>, i.e. pointer order -- and its new commsForDevice() sorts by getCommName(). NCCLUtils' deregisterSegment(void* ptr) loses its `window` parameter and uses the flag recorded at registration time instead. That is required by the above: a symm pool can now hold both windowed segments (from registerMemPool) and locally registered ones (from the hook), and handing a ncclReg* to ncclCommWindowDeregister, or the reverse, is a handle type confusion. Deriving the API from the registration record rather than from the caller's view of the pool makes that unrepresentable, which is why the parameter is removed rather than merely fixed at the two call sites. One correction to the record while here. ncclCommWindowDeregister is *not* collective on upstream NCCL 2.29.7: symWindowDestroy -> symMemoryDropRef + ncclCommDeregister contains no bootstrap call, and the divergent-free case was verified not to hang here. It is collective under NCCLX with rmaAlgo != orig, where it goes through ctran::ctranWinFree -> CtranWin::free and takes a windowBarrier. So the deregister half is build-dependent rather than unconditionally broken. The teardown is kept regardless -- dropping it would leave a stale window over an address range the allocator is about to hand out again, so a later allocation reusing that range would silently resolve to the stale window, which is memory corruption rather than a hang, and strictly worse -- but both the stock hook and nccl2's deregister_address(addr, from_allocator_hook=true) now warn once that this is the uncollective path. In other words the risky half is kept knowingly, because the safe-looking alternative is unsafe. Test Plan: A 2-rank harness on the four interesting shapes, on GPUs 2,3, each cell under a 200 s cap: CUDA_VISIBLE_DEVICES=2,3 timeout 200 python \ triage/bug6_alloc_hook_collective.py <mode> - div_alloc (divergent allocation into a symm pool) -- the direct regression check -- went from a hang the watchdog could not break (exitcodes [1, 1]) to both ranks printing DONE (exitcodes [0, 0]) - sym_alloc (symmetric allocation): exitcodes [0, 0], stayed green - div_free (divergent free): exitcodes [0, 0], stayed green - div_alloc_local (divergence, symm=False): exitcodes [0, 0], stayed green The existing registration tests, including the one most likely to notice the behaviour change above: python -m pytest test/distributed/test_c10d_nccl.py \ -k "window_registration or user_buffer_registration" -v NcclUserBufferRegistrationTest::test_nccl_user_buffer_registration PASSED NcclUserBufferRegistrationTest::test_nccl_window_registration PASSED 2 passed, 282 deselected python -m pytest test/distributed/test_c10d_nccl2.py -k register_mem_pool -v 3 passed, 17 deselected No CI regression test is added: the pre-fix failure mode is a hang, so such a test can only fail by timing out. Two things this does not establish. The NCCLX collective-deregister path is inferred from source, not measured -- everything ran against upstream NCCL 2.29.7. And all runs were single-node, 2 or 4 ranks, so the getUniqueHash() ordering fix and the multi-communicator iteration-order deadlock it closes remain argued rather than reproduced. Authored with the assistance of an AI coding agent. Pull Request resolved: #192112 Approved by: https://github.com/d4l3k ghstack dependencies: #192104, #192105, #192106, #192107, #192108, #192109, #192110, #192111
pytorchmergebot
pushed a commit
that referenced
this pull request
Aug 5, 2026
…ress (#192113) Summary: _create_c10d_store() began with if _torchelastic_use_agent_store(): return TCPStore(hostname, port, world_size, is_master=False, timeout) so under torchrun every rank got a client-only TCPStore for whatever host and port it had been handed. torchrun's elastic agent exports TORCHELASTIC_USE_AGENT_STORE=True into every worker it spawns, alongside MASTER_ADDR and MASTER_PORT, and the flag promises exactly one thing: the agent hosts a store at that endpoint. _create_c10d_store applied it to every endpoint. So any caller building a private store on a port of its own choosing -- a library making a stateless group, or a script calling init_process_group(init_method="tcp://host:port") -- ended up with a client on every rank, no rank ever started a server, and every rank sat in connect-retry against nothing until the store timeout expired: thirty minutes on gloo, ten on nccl. From the outside that is an unexplained hang at startup with no error and nothing in the logs, on a code path that works perfectly the moment the same program is run without torchrun. Torch already knew the address had to match. TCPStore's C++ constructor only forgives a failed server bind when useAgentStore && masterPort == opts.port -- the same guard, one layer down. The Python layer was contradicting the C++ layer: rendezvous.py behaved as though the agent serves every address, while TCPStore.cpp assumes it serves only its own. Found by root-causing a hang rather than by reading. vLLM's stateless_init_torch_distributed_process_group hangs when called from inside a torchrun-managed process, on both the gloo and the nccl arms and identically on stock nccl and on nccl2. Isolating it ruled out everything torchrun-shaped in turn -- RANK and WORLD_SIZE, MASTER_PORT collisions, store key collisions -- and left one variable: popping TORCHELASTIC_USE_AGENT_STORE out of the environment makes the identical run succeed. That pointed at torch.distributed.rendezvous, which is where vLLM was getting its store, and from there at this branch. This is a stock defect. rendezvous.py is stock torch, the elastic agent that sets the variable is stock torch, and the hang reproduces on a stock init_process_group(init_method="tcp://...") under stock torchrun with nothing from the migration loaded at all. No part of it requires nccl2. It is also only fixable here, which is the whole reason it belongs in c10d rather than downstream. vLLM has already fixed itself, independently, by building the TCPStore directly instead of going through rendezvous -- and that bypass is byte-for-byte the branch _create_c10d_store now takes for a private address, so on a torch carrying this commit the two are equivalent and the workaround is functionally redundant. It was nonetheless deliberately left in place, because vLLM supports torch releases that will not carry this fix for a long time and on a fixed torch it is a no-op that costs nothing. What a downstream patch cannot close is the broader exposure: an API server started under torchrun whose workers call init_distributed_environment(distributed_init_method="tcp://...") reaches this code through init_process_group, which builds its store via _create_c10d_store, and no amount of patching a stateless-group helper reaches that path. Every framework offering a "connect to this address" knob has the same shape, so the guard has to live in torch. The fix is a predicate, _agent_store_serves(hostname, port), that honors TORCHELASTIC_USE_AGENT_STORE only when the hostname and port it is asked about are the agent's own MASTER_ADDR and MASTER_PORT. _create_c10d_store gates on that instead of on the raw env read, and the docstring is corrected to describe what the code now does. _torchelastic_use_agent_store() itself stays, since it is the raw env read and is referenced elsewhere in the ecosystem. This is TCPStore.cpp's existing guard extended to the hostname, which the Python layer has in hand and the C++ layer does not. Comparing the hostname as a string is safe in both directions. Everything that legitimately wants the agent's store arrives through env://, and _env_rendezvous_handler reads MASTER_ADDR verbatim out of the same environment the agent wrote it into, so for that path the comparison is exact by construction. A caller who instead hand-writes tcp://<resolved-ip>:<MASTER_PORT> and so trips the string mismatch does not get the old broken behaviour but the correct one: rank 0 takes the is_master=True branch, its bind fails because the agent already holds that port, and TCPStore.cpp's port-only guard forgives the failure and turns it into a client -- which is precisely what the old code did directly. The degradation is into a slower path, not into a broken one. Test Plan: Two regression tests in test_store.py::RendezvousTCPTest, next to the other dist.rendezvous("tcp://...") tests and in the same create_tcp_url() / common.find_free_port() idiom. They are a matched pair on purpose, so that the fix cannot quietly degrade into deleting the feature: test_agent_store_ignored_for_other_address sets the agent environment but points it at a different port and requires the private store to come up and round-trip a key, while test_agent_store_honored_for_master_address sets it matching, with nothing listening, and requires a DistNetworkError rather than some rank silently starting a server. python test/distributed/test_store.py -v -k agent_store test_agent_store (LibUvTCPStoreTest.test_agent_store) ... ok test_agent_store_honored_for_master_address (RendezvousTCPTest...) ... ok test_agent_store_ignored_for_other_address (RendezvousTCPTest...) ... ok test_agent_store (TCPStoreTest.test_agent_store) ... ok Ran 4 tests in 4.175s OK Pre-fix the same selection is 199.719 s and FAILED (errors=1), with test_agent_store_ignored_for_other_address raising DistNetworkError: The client socket has timed out after 10000ms while trying to connect to (localhost, 37515). Neither new test is decorated with @retry_on_connect_failures, and that is deliberate: that helper's default connect_errors=(ADDRESS_IN_USE) is missing its trailing comma, so it is a plain string rather than a tuple, the membership test iterates it character by character, and consequently almost any RuntimeError matches and is retried ten times, each attempt paying its full timeout. That is what turned the genuine pre-fix failure here into a roughly 190-second hang with the real error buried behind "Failing after 10 retries". It is a pre-existing wart in common_utils.py whose blast radius covers every test currently using the decorator, so it is left untouched here and filed separately. End-to-end under real torchrun, driven by triage/bug14_private_store_torchrun.py (stock torch, no vLLM), which can monkeypatch the predicate back to its legacy form with --legacy so both behaviours are observable on one binary. Two modes: rendezvous (what vLLM's stateless helper did) and init_pg (init_process_group(init_method="tcp://..."), the path vLLM cannot bypass): torchrun --nproc_per_node=2 --master_port=29502 \ triage/bug14_private_store_torchrun.py <mode> --port 29783 \ --store-timeout 15 [--legacy] rendezvous --legacy : [rank1] RAISED after 26.1s: DistNetworkError rendezvous (fixed) : [rank0] OK in 0.19s / [rank1] OK in 0.01s, allreduce -> 2.0 init_pg --legacy : [rank1] RAISED after 20.8s: DistNetworkError init_pg (fixed) : [rank0] OK in 0.05s / [rank1] OK in 0.04s, allreduce -> 2.0 With stock timeouts rather than the repro's fifteen seconds the legacy arms never finish at all. A four-rank env:// run confirms the genuine agent-store path is untouched: _agent_store_serves reports True on every rank and the allreduce returns 4.0. Verified on 4x H100, CUDA 13.0, NCCL 2.29.7, against a freshly rebuilt HEAD: the pre-existing install predated the commits already landed in this stack, so unmodified HEAD was rebuilt first to make the before-numbers honest measurements rather than comparisons against a stale binary. 879 tests across six suites with zero failures, with only the skips those suites already had: test_c10d_nccl.py Ran 285 tests in 1659.524s OK (skipped=15) test_c10d_gloo.py Ran 289 tests in 1089.249s OK (skipped=4) test_c10d_nccl2.py Ran 20 tests in 102.125s OK test_c10d_common.py Ran 72 tests in 117.529s OK test_store.py Ran 140 tests in 23.000s OK (skipped=12) test_fake_pg.py Ran 73 tests in 1.543s OK Authored with the assistance of an AI coding agent. Pull Request resolved: #192113 Approved by: https://github.com/d4l3k ghstack dependencies: #192104, #192105, #192106, #192107, #192108, #192109, #192110, #192111, #192112
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Summary:
groupRanks() gates the identity-ranks fast path on
local_id_ == 0, intending tomean "this is the world PG". But local_id_ is a process-global counter over every
ProcessGroupNCCL ever constructed, so the guard only holds when the world PG
happens to be the first NCCL backend in the process. When it is not,
groupRanks() falls through and returns the world PG's global_ranks_in_group,
which distributed_c10d.py deliberately leaves empty for the default group.
split() then indexes that empty vector:
which is a null deref. This is the NCCL analogue of the gloo fix earlier in this
stack ("Fix ProcessGroupGloo::split OOB when the world PG is not the first gloo
PG"), same file layout, same line.
Nothing exotic is required to reach it -- only public API:
destroy_process_group() frees the object but not the id, and the counter never
rewinds, so the second world PG is local_id_ == 1. TORCH_CPP_LOG_LEVEL=INFO shows
the mechanism directly: the default PG comes up as "[PG ID 1 PG GUID 0
(default_pg)]" and never prints the "ProcessGroupNCCL environments" line, which
is itself gated on local_id_ == 0. Destroy-and-reinit is an ordinary pattern
(test suites, serving frameworks, notebooks, anything that reconfigures
parallelism). The same state is reached by a directly-constructed stateless
ProcessGroupNCCL built before init_process_group -- which is exactly what vLLM's
stateless_init_torch_distributed_process_group does -- or by a fork after any
NCCL PG exists.
This is stock, not nccl2: the sequence above is plain torch.distributed with
backend="nccl", no torchcomms and no vLLM in the process, and it dies with
exitcodes [-11, -11] (SIGSEGV on both ranks) versus [0, 0] for the same script
with the first init/destroy pair removed. PYTHONFAULTHANDLER pins the frame at
distributed_c10d.py:6725 in split_group.
How it was found: by inspection, not by a failing run. The gloo twin was found
through a vLLM data-parallel SIGSEGV; the two backends carry the same
groupRanks() shape, so the NCCL copy was read immediately afterwards and
predicted to crash the same way. The repro above was then written
to confirm it, and did. Saying it plainly: nothing downstream had hit this yet,
but the sequence is public API and two of the three ways into it (stateless PG
first, fork after a PG exists) are shipping patterns.
It belongs in c10d for the same reason the gloo one did. local_id_ is torch's
own internal counter; a downstream caller cannot read it, cannot reset it, and
cannot even tell whether the PG it is about to split was the first NCCL PG in
the process -- that depends on what every library loaded earlier did. The only
downstream "rule" would be "construct your world PG before anything else creates
a NCCL PG", which no library can guarantee about its host application and which
does not survive a fork or a destroy/reinit anyway. And the failure is a null
deref inside c10d, not a diagnosable error.
An empty global_ranks_in_group already means "spans the world in rank order", so
the local_id_ term is both unnecessary and wrong. Drop it.
Dropping it alone is not sufficient here. The fast path returned a reference to
a function-local
static std::vector<uint64_t> globalRanks(size_). That staticwas not misbehaving before this change, and it is worth being precise about why:
a magic static initialises once, and under the old gate the branch is reachable
only by the object with local_id_ == 0 -- and since process_group_id is
monotonic and never reset, exactly one object in the whole process lifetime has
that id. So "whichever PG sizes it first" was a one-element set, and the
std::iota that re-runs on every call is a data race that every writer resolves
to the identical value, on a vector that never resizes: formally UB and
TSAN-visible, benign in practice. As soon as any PG with an empty
global_ranks_in_group can get there, that stops holding: the first arrival
freezes the size and a later, larger PG reads past the end -- the null deref
becomes a silent heap OOB, and groupRanks().size() starts lying to the
unbatched-P2P paths. The fix creates the reachability, so the fix has to remove
the static.
The gloo backend materializes the same mapping into a lazily filled
mutable defaultRanks_, which predates its fix and is safe there for a reason that doesnot transfer: ProcessGroupGloo's constructor itself calls groupRanks() (through
FlightRecorder::record_pg_ranks), so the member is always sized single-threaded
before the object is published, and every later call is a pure read.
ProcessGroupNCCL's constructor never calls groupRanks(), and ProcessGroupNCCL
explicitly supports multi-threaded use, while its callers hold no common lock --
record_pg_ranks() runs outside the mutex_ scope in initNCCLComm, and
pointToPoint reads groupRanks().size() from wherever the user issues a
send/recv. A lazy resize+iota here would therefore be a genuine race, and not a
benign one: two threads first-calling concurrently would both resize(),
reallocating the buffer under the reference the other has already returned.
Materialize the mapping eagerly in the constructor instead, gated on
global_ranks_in_group being empty so the numerous subgroups allocate nothing and
only world-spanning PGs pay size_ * 8 bytes. groupRanks() then becomes a pure
read that needs no synchronization and no
mutable, which is the invariantworth having here rather than the smaller diff.
The emptiness test stays at call time, matching fixed-gloo semantics. That is
deliberate: snapshotting it as a bool in the constructor would make NCCL immune
to a parent whose options object was aliased by a child's split -- and would
thereby mask that defect, which is fixed properly one commit later in this stack
by giving each backend its own Options.
Two behaviour changes fall out of the wider fast path, both corrections: for a
world-spanning PG that is not local_id_ 0, FlightRecorder::record_pg_ranks() now
records the real membership instead of an empty list, and the eager/lazy
unbatched-P2P warnings print the real group size instead of 0.
Not fixed, same defect class, tracked separately: ProcessGroupNCCL::globalRank()
caches the first PG's rank_ in a function-local static, and unlike groupRanks()
it genuinely is seeded by whichever PG is constructed first, because the
constructor calls it while logging. It feeds guessDeviceId(), which split()
itself calls. It needs its own repro and is deliberately out of scope here.
Test Plan:
test/distributed/test_c10d_nccl.py gains
test_comm_split_world_pg_created_after_another_nccl_pg, which burns local_id_ 0
with an init_process_group / destroy_process_group pair over a PrefixStore,
brings up the real default PG over a second prefix, splits it world-wide, and
checks the child's ranks and a broadcast:
It fails against the pre-fix code with "Expected 0 but got -11".
The standalone repro of the public-API sequence above, 2 ranks, with and
without the id-burning init/destroy pair:
Pre-fix the burn=True arm was exitcodes=[-11, -11].
The neighbouring test_comm_split_* tests are unaffected:
Authored with the assistance of an AI coding agent.