CORE: Get local rank from topology if unset - #1245
MaayanGadishNvidia wants to merge 1 commit into
Conversation
|
211c631 to
51f149d
Compare
| ucp_params.estimated_num_eps = params->estimated_num_eps; | ||
| } | ||
|
|
||
| #ifdef HAVE_UCX_NODE_LOCAL_ID |
There was a problem hiding this comment.
Trailing whitespace added.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
a3f3680 to
36a073b
Compare
Additional Comments (1)
|
44108af to
a32583c
Compare
a32583c to
c4f6048
Compare
c4f6048 to
a6456d8
Compare
|
/build |
5ef6a7d to
b9cebe1
Compare
|
/build |
| addr_len == 0 iff storage == NULL. */ | ||
| ucc_status_t status; | ||
| ucc_rank_t i; | ||
| const size_t ctx_addr_prefix_len = |
There was a problem hiding this comment.
declared and initialized vars go first.
| const size_t ctx_addr_prefix_len = | ||
| offsetof(ucc_context_addr_header_t, n_components); | ||
| ucc_context_addr_header_t *h; | ||
| void *scratch; |
b9cebe1 to
44ac5b4
Compare
|
/build |
|
🤖 CI Triage Agent — TL;DR: The build broke with a hard compile error in Full analysisSummary: Root cause: Commit 44ac5b4 ("CORE: Get local rank from topology if unset", PR #1245) added
Since UCC builds with Implicated commit: 44ac5b4 — Noam Tsemah, "CORE: Get local rank from topology if unset" (2026-08-19) File: Suggested fix: Make the identifier consistent within const size_t ctx_id_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);
...
addr_storage->storage = ucc_malloc(
(addr_storage->size + 1) * ctx_id_prefix_len, "ctx_ids_storage");
...
ucc_error("failed to allocate %zd bytes for ctx_ids storage",
(addr_storage->size + 1) * ctx_id_prefix_len);
...
addr_storage->addr_len = ctx_id_prefix_len;
scratch = PTR_OFFSET(addr_storage->storage,
ctx_id_prefix_len * addr_storage->size);
memset(scratch, 0, ctx_id_prefix_len);
...
status = oob->allgather(scratch, addr_storage->storage,
ctx_id_prefix_len, oob->coll_info,
&addr_storage->oob_req);(Alternatively rename the declaration to Related: #1245 (the PR under test — this is a self-inflicted failure on its own HEAD commit, not a pre-existing master breakage) |
|
🤖 CI Triage Agent — TL;DR: The Linter job's Full analysisSummary: GitHub Actions "Linter" run 35077588882 failed in the Root cause: Pure compile-time bug, not environment-related. In Implicated commit: [REDACTED:Hex High Entropy String] — Noam Tsemah, "CORE: Get local rank from topology if unset" (most recent commit touching File: src/core/ucc_context.c:678 (declaration) with broken references at :709, :713, :716, :719, :720, :726 Suggested fix: Pick one name and use it everywhere. Simplest is to rename the declaration to match the uses: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);or alternatively Related: PR #1245 (branch
|
|
🤖 CI Triage Agent — TL;DR: The ASAN job failed at compile time, not in the sanitizer run: Full analysisSummary: Root cause: In
No such identifier exists in scope, so clang-17 errors out. This is a pure build break independent of ASAN — every compiler job on this branch will fail identically. Note the log also shows the UCC configure summary is missing CUDA/IB/ROCm (all "not found"), which is expected for this runner and unrelated. Implicated commit: [REDACTED:Hex High Entropy String] — Noam Tsemah, "CORE: Get local rank from topology if unset" (the run's head commit, and the most recent commit touching this file) File: src/core/ucc_context.c:678 (declaration) with unresolved uses at 709, 713, 716, 719, 720, 726 Suggested fix: Complete the rename — replace all six occurrences of addr_storage->storage = ucc_malloc(
(addr_storage->size + 1) * ctx_id_prefix_len, "ctx_ids_storage");
...
ucc_error("failed to allocate %zd bytes for ctx_ids storage",
(addr_storage->size + 1) * ctx_id_prefix_len);
...
addr_storage->addr_len = ctx_id_prefix_len;
scratch = PTR_OFFSET(addr_storage->storage,
ctx_id_prefix_len * addr_storage->size);
memset(scratch, 0, ctx_id_prefix_len);
status = oob->allgather(scratch, addr_storage->storage,
ctx_id_prefix_len, oob->coll_info,
&addr_storage->oob_req);Before pushing, build locally ( Related: PR #1245 (branch
|
|
🤖 CI Triage Agent — TL;DR: The build step of the "Lint (ROCm)" job failed to compile Full analysisSummary: Root cause: In Implicated commit: File: Suggested fix: Make the identifier consistent — simplest is a mechanical rename of the six usages: i.e. Related: none found (no existing issue/PR references |
|
🤖 CI Triage Agent — TL;DR: The UCC build in the Linter-NVIDIA job failed to compile Full analysisSummary: Root cause: A pure compile error from an inconsistent variable rename in the PR's new Implicated commit: [REDACTED:Hex High Entropy String] — Noam Tsemah, "CORE: Get local rank from topology if unset" (the PR #1245 head commit, which is the most recent change to File: src/core/ucc_context.c:678 (declaration) with errors at :709, :713, :716, :719, :720, :726 Suggested fix: Make the identifier consistent throughout the function — simplest is to replace all six (Alternatively rename the line-678 declaration to Related: PR #1245 (branch NIC_bid_auto) — this is a self-inflicted failure in the PR under test, not a pre-existing master breakage; master commit [REDACTED:Hex High Entropy String] was the merge base.
|
|
🤖 CI Triage Agent — TL;DR: The hpcsdk (nvc) build failed compiling Full analysisSummary: Stage "Build UCC" failed: Root cause: In Implicated commit: [REDACTED:Hex High Entropy String] (PR #1245, new File: src/core/ucc_context.c:678 (declaration) and :709, :713, :716, :718, :720, :726 (uses) Suggested fix: Pick one name and use it consistently — simplest is to rename the declaration to match the uses: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);or alternatively Related: PR #1245 (the change under test); no existing issue found for this error signature.
|
|
🤖 CI Triage Agent — TL;DR: The Coverity job failed in Full analysisSummary: Stage "Run Coverity" (node 53) failed at Root cause: Compile error, not a Coverity defect report:
In Implicated commit: [REDACTED:Hex High Entropy String] — "CORE: Get local rank from topology if unset", Noam Tsemah (the exact commit under test, PR #1245) File: src/core/ucc_context.c:678 (declaration) and :709, :713, :716, :719, :720, :726 (uses) Suggested fix: Make the name consistent — simplest is to rename the declaration at line 678: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);(or alternatively Related: PR #1245 (the PR under test); no pre-existing issue found for this error signature.
|
|
🤖 CI Triage Agent — TL;DR: The Coverity build failed at compile time, not in analysis: Full analysisSummary: Root cause: In Implicated commit: [REDACTED:Hex High Entropy String] (PR #1245, the commit under test — the File: src/core/ucc_context.c:678 (declaration) and :709, :713, :716, :719, :720, :726 (uses) Suggested fix: Pick one name and apply it everywhere in - const size_t ctx_id_prefix_len = sizeof(ucc_context_id_t) +
- sizeof(ucc_host_info_t);
+ const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
+ sizeof(ucc_host_info_t);(or conversely Related: PR #1245 (the change under test); no existing issue found matching this error signature.
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: In Implicated commit: File: Suggested fix: Make the names consistent — either rename the declaration to const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);or, preferably (keeps the "ctx id" semantics of the new function), replace all six Related: PR #1245 (branch
|
|
🤖 CI Triage Agent — TL;DR: The build (not the gtest run) failed: Full analysisSummary: Stage "Build Docker" ( Root cause: In
Since UCC configures with Implicated commit: [REDACTED:Hex High Entropy String] (PR #1245 head); the most recent upstream commit to this file is 44ac5b4 "CORE: Get local rank from topology if unset" by Noam Tsemah, but the mismatched identifier is introduced by the PR's changes to File: src/core/ucc_context.c:678 (declaration) and src/core/ucc_context.c:709,713,716,719,720,726 (uses) Suggested fix: Make the names consistent — simplest is to rename the declaration at line 678: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);(or, alternatively, change the six body references to Related: none found (no matching issues/PRs for
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: UCC/ucc-test-gtest #150 failed in the "Build Docker" stage at Root cause: A pure compile error introduced by the commit under test. In
Since UCC is configured with Implicated commit: File: Suggested fix: Make the identifier consistent — simplest is to rename the declaration to match the body: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);(or alternatively Related: PR #1245 (branch
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Pure source/compile error, not infrastructure. In
Because UCC configures with Implicated commit: [REDACTED:Hex High Entropy String] (PR #1245); the most recent upstream commit on this file is 44ac5b4 "CORE: Get local rank from topology if unset" (Noam Tsemah), which does not contain this code. File: src/core/ucc_context.c:678 (declaration) and src/core/ucc_context.c:709, 713, 716, 719, 720, 726 (uses) Suggested fix: Make the names consistent — simplest is to rename the declaration at line 678: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);(or alternatively Note that Related: PR #1245 (the change under test). No existing issue found matching
|
|
🤖 CI Triage Agent — TL;DR: The Docker build stage failed because Full analysisSummary: Root cause: Genuine compile error, not infra. In
UCC builds with Implicated commit: File: Suggested fix: Make the identifier consistent — simplest is to rename the declaration to the name the body uses: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);(or, alternatively, Related: PR #1245 (branch
|
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: UCC build inside the NVLS Docker image failed at Root cause: Pure compile error from an inconsistent identifier rename in const size_t ctx_id_prefix_len = sizeof(ucc_context_id_t) + sizeof(ucc_host_info_t);but the function body references
The build is configured with Implicated commit: [REDACTED:Hex High Entropy String] (PR #1245) — this hunk is not present in any of the prior commits to the file (latest being File: src/core/ucc_context.c:678 (declaration) and :709, :713, :716, :718–:720, :726 (uses) Suggested fix: Pick one name and use it consistently. Minimal change — rename the declaration to match the body: const size_t ctx_addr_prefix_len = sizeof(ucc_context_id_t) +
sizeof(ucc_host_info_t);(or, alternatively, Related: none found (searched for
|
|
🤖 CI Triage Agent — TL;DR: The Docker build stage failed compiling Full analysisSummary: Root cause: Variable-name mismatch introduced by the commit under test. In
Since UCC configures with Secondary concern worth fixing at the same time: the two functions compute the prefix length differently — Implicated commit: [REDACTED:Hex High Entropy String] — "CORE: Get local rank from topology if unset", Noam Tsemah (the exact commit this build polled) File: src/core/ucc_context.c:678 (declaration) and src/core/ucc_context.c:709,713,716,719,720,726 (uses) Suggested fix: Make the name consistent, and share the length definition between both functions. Minimal patch: ucc_status_t ucc_core_ctx_id_exchange(ucc_context_t *context, ucc_oob_coll_t *oob,
ucc_addr_storage_t *addr_storage)
{
- const size_t ctx_id_prefix_len = sizeof(ucc_context_id_t) +
- sizeof(ucc_host_info_t);
+ const size_t ctx_addr_prefix_len =
+ offsetof(ucc_context_addr_header_t, n_components);Using Related: PR #1245 (branch
|
Head branch was pushed to by a user without write access
44ac5b4 to
49de1ae
Compare
|
/build |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Log evidence:
That combination points at the pipelined-schedule restart path. In
Both interleavings are harmful and match the two observed symptoms exactly:
Secondary inconsistency in the same area: restart resets the internal status ( Implicated commit: File: Suggested fix:
Related: PR #1245 (trigger only, #1245), PR #1262 (restart logic), PR #428 (original split_rail allreduce), PR #616 "SCHEDULE: fixes race on n_deps_satisfied" (same class of bug) |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Stage 198 "Run UCC MPI tests (bulk)" hung and was aborted by Jenkins ( Root cause: Evidence from the stage log:
Both symptoms (wrong reduction results, then a stalled fragment) point at the pipelined-schedule fragment restart bookkeeping used by split_rail: in Importantly, this is not attributable to the commit under test: Implicated commit: File: Suggested fix:
Related: PR #1245 (commit under test, #1245 — not the cause); prior work on this code: #1262 (
|
|
🤖 CI Triage Agent — TL;DR: The build's code compiled fine; it failed because the Full analysisSummary: Stage Root cause: Both The gap 13:34:08 → 14:34:17 is exactly 3600 s, i.e. the A second, genuine pipeline bug is visible in the cleanup path:
Implicated commit: unknown for the allocation failure (infrastructure, not code — commit be1f000 / PR #1245 built successfully). The cleanup defect traces to the Slurm-lib conversion commits File: Suggested fix:
Related: PR #1245 (the triggering PR, unaffected — its build passed); CI plumbing commits |
|
🤖 CI Triage Agent — TL;DR: The build failed in the "Allocate Slurm job" stage — Full analysisSummary: Root cause: In stage 174 ( Secondary (real but non-fatal) bug exposed by the failure path: the cleanup stage 224 called Implicated commit: unknown — no UCC source commit is implicated. The Slurm allocation wiring dates to cc35997 (Daniel Pressler, "CI: convert nvls tests to use slurm jenkins lib") and the stop-stage call pattern to 817fc96 / 6d54511 (Daniel Pressler), but the trigger here is cluster capacity, not a code change. File: Suggested fix: Retry the build — this is a transient/capacity failure with no code signal. Then, to stop it recurring and to stop it being misreported as a code failure:
Related: PR #1245 (commit under test, not implicated). No existing issue tracks |
What
Adding automation for calculate local rank from topo if not provided by user
Why ?
Continue of #1189