Add AI-assisted Intel CPU issue triage toolkit - #2
Draft
frost-intel with Copilot wants to merge 1 commit into
Draft
frost-intel with Copilot wants to merge 1 commit into
frost-intel with Copilot wants to merge 1 commit into
Conversation
Copilot created this pull request from a session on behalf of
frost-intel
June 26, 2026 15:29
View session
pytorchmergebot
pushed a commit
that referenced
this pull request
Jun 30, 2026
…#188024) Some gfx950 (MI350) 2-GPU runner pods come up with a container that cannot read part of the KFD/HSA topology. RCCL then fails every collective init with "ncclUnhandledCudaError: Call to CUDA function failed / Could not read node #N" (N is a fixed topology-node index for that pod, e.g. #2 or #9). When a distributed shard lands on such a pod, the first collective crashes and a later test hangs the whole shard until the 270-minute job timeout. Host-side rocminfo still enumerates the GPUs on these pods, so the existing "Runner check GPU count" gate does not catch it -- the failure is the container's deeper topology read, not agent enumeration. Diagnosis: the same RCCL error appears across many different distributed tests and both worker-crash and downstream-hang forms; in the logs it is constant per pod and present from the very first collective, and world_size is 2 so "node #N" cannot be a rank -- it is a system topology node. So it is a per-pod container health problem, not a PyTorch or per-test bug. Fix: add a fast in-container RCCL pre-flight (.ci/pytorch/rocm_preflight.py) that spawns a min(2, ngpu)-rank process group and does one all_reduce. For distributed shards _rocm-test.yml runs it (wrapped in `timeout 180`) before the suite; on failure or hang the job fails in seconds with a clear message instead of hanging for 270 minutes, and the bad pod is identifiable for draining. Non-distributed shards are unchanged. Test Plan: ``` python -m py_compile .ci/pytorch/rocm_preflight.py lintrunner -a .ci/pytorch/rocm_preflight.py .github/workflows/_rocm-test.yml ``` Validated the pre-flight directly. Locally on 8x A100 (the nccl backend exercises the same init/collective path RCCL uses) the script passes: ROCm/RCCL pre-flight passed (2-rank all_reduce) and the failure path is exercised too: when the process group cannot initialize, mp.spawn raises, the script prints the "::error::" message and exits 1 -- the job fails fast instead of hanging. In CI, dispatched the gate on the gfx950.2 distributed pool: the pre-flight ran inside the container and passed on healthy pods (exit 0, suite proceeded). The broken-pod case is the same exit-1 fast-fail path; it only reproduces when a pod with the broken KFD topology is in rotation, and the pool was healthy during testing, so that specific case was not caught live. Authored with Claude. Pull Request resolved: pytorch#188024 Approved by: https://github.com/frgossen
Self-contained, stdlib-only toolkit under tools/intel_cpu_triage/ implementing the AI-assisted Intel CPU issue resolution workflow: incremental GitHub ingestion, AI/heuristic triage and scoring into three buckets, duplicate detection, a board state machine with two human gates, PR rate-limiting, and health metrics with a feedback loop. Includes CLI, example scheduled workflow, README, and 26 passing unit tests. Authored with assistance from Claude.
frost-intel
force-pushed
the
copilot/develop-pytorch-workflow
branch
from
July 7, 2026 13:13
5440fdf to
48903ac
Compare
frost-intel
pushed a commit
that referenced
this pull request
Aug 6, 2026
…process group (pytorch#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: pytorch#192110 Approved by: https://github.com/d4l3k ghstack dependencies: pytorch#192104, pytorch#192105, pytorch#192106, pytorch#192107, pytorch#192108, pytorch#192109
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
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.
A small Intel CPU/oneDNN team needs a workflow to surface actionable
pytorch/pytorchissues and track them through to merge — using AI as an accelerator while protecting maintainer trust from low-quality, volume-driven PRs.This adds a self-contained, stdlib-only toolkit under
tools/intel_cpu_triage/(no third-party deps, no PyTorch build required) that implements the workflow end-to-end. The DB is the single source of truth; upstream labels are never modified, and nothing auto-comments or auto-opens PRs.Pipeline
ingest.py,github_client.py) — incremental GitHub Search API sync into SQLite via anupdated:>=watermark; one query per scope label plus a content-keyword sweep to catch mislabeled Intel issues (AMX, oneDNN, AVX-512, bf16, …). PRs filtered out.triage.py,dedup.py) — scores issues into three buckets (ready_to_work,needs_info_repro,needs_maintainer_decision). Quarantines issues that already have a PR, are feature/design requests, or imply API/semantics changes. Ships a deterministicHeuristicTriagerand a pluggableLLMTriager(merges model JSON onto the heuristic baseline, falls back on any failure). Lexical TF-IDF dedup with an optional real-embedder hook.board.py) — Kanban state machine enforcing legal transitions and human gate [RFC] Provide full XPU feature support in Torchvision #1: an engineer must explicitlypulla ready issue onto the board.ratelimit.py) — team-wide open-PR cap + per-reviewer budget; the enforcement point for human gate Add AI-assisted Intel CPU issue triage toolkit #2 before any upstream PR.metrics.py) — merge rate, change-request (churn) rate, triage precision, plus a feedback export of accepted/rejected examples for prompt tuning.Surrounding pieces
cli.pyorchestrates all phases;config.example.jsonexposes labels, keywords, and caps.intel_cpu_triage.yml.example— scheduled Action (.examplesuffix so it can't auto-run from a fork) that caches the DB to keep ingestion incremental.README.mddocuments the workflow and the two human gates;tests/covers ingestion, bucketing/quarantine, board transitions, rate-limits, and metrics.Example
The heuristic path runs fully offline; wire an LLM by passing a
complete(prompt) -> strclient toLLMTriager. Default config and thresholds are starting points intended to be tuned per team.