UCC/CORE: Added local rank from topo if not provided by user by MaayanGadishNvidia · Pull Request #1245 · openucx/ucc

MaayanGadishNvidia · 2025-12-17T16:00:11Z

What

Adding automation for calculate local rank from topo if not provided by user

Why ?

Continue of #1189

greptile-apps · 2026-01-22T16:18:22Z

Greptile Summary

This PR adds automatic local rank detection when NODE_LOCAL_ID is set to auto and OOB is available. It introduces a new ucc_core_ctx_id_exchange() function that performs a lightweight prefix allgather (exchanging only ctx_id + host_info, i.e., offsetof(ucc_context_addr_header_t, n_components) bytes per rank) before the main address exchange. The result feeds ucc_context_topo_init() and ucc_topo_init() to compute the per-rank node-local index, which is then passed to TL contexts and ultimately to UCX.

Key improvements over previous iterations:

ctx->id is now initialised once (lines 772–773) before both exchanges, removing the earlier double-initialisation bug.
The topo_required && !ctx->topo guard at line 962 prevents double topo allocation.
Error paths at error_ctx now call ucc_context_topo_cleanup(ctx->topo) and free addr_storage.storage, plugging previously reported memory leaks.
The prefix exchange allocates (n+1) entries—slot n is the local scratch buffer—and sets addr_len = ctx_addr_prefix_len, making the storage layout compatible with ucc_context_topo_init() which only reads h->ctx_id.pi and h->host_info, both within that prefix.

Remaining concern: ucc_topo_node_local_rank() returns UCC_RANK_INVALID (UINT32_MAX) when UCC_SBGP_NODE is not enabled (e.g., all ranks on separate nodes in a multi-rank OOB scenario). This value is assigned to b_params.node_local_id without a validity check. The TL UCP guard is params->node_local_id != UCC_ULUNITS_AUTO; since UCC_RANK_INVALID ≠ UCC_ULUNITS_AUTO, UINT32_MAX would be forwarded to UCX as the node-local ID. Callers should validate the return value and fall back to 0 for single-rank-per-node scenarios.

Confidence Score: 4/5

Safe to merge for multi-rank, multi-node topologies with ppn>1; single-rank-per-node in multi-rank OOB may pass UINT32_MAX to UCX as node_local_id

Most previously raised P1 concerns (double ctx->id init, topo double-init, error-path leaks) appear resolved in this iteration. One P1 remains: UCC_RANK_INVALID is propagated unchecked to UCX when UCC_SBGP_NODE is not enabled (all ranks on separate nodes with n_oob_eps > 1), since UCC_RANK_INVALID (UINT32_MAX) != UCC_ULUNITS_AUTO and the TL UCP guard does not filter it out.

src/components/topo/ucc_topo.h (ucc_topo_node_local_rank return value for disabled sbgp) and src/core/ucc_context.c line 845 (missing validation of UCC_RANK_INVALID before assigning to b_params.node_local_id)

Important Files Changed

Filename	Overview
src/components/topo/ucc_topo.h	Adds ucc_topo_node_local_rank() helper; returns UCC_RANK_INVALID when node sbgp is not enabled, which propagates unchecked to UCX in single-rank-per-node multi-rank OOB scenarios
src/core/ucc_context.c	Adds ucc_core_ctx_id_exchange() for a prefix-only allgather (ctx_id + host_info); early exchange cleanup, double topo-init guard, and error-path topo/storage cleanup are all handled correctly
src/core/ucc_context.h	Declares ucc_core_ctx_id_exchange; updates NODE_LOCAL_ID doc string to document the new three-allgather contract required of OOB backends

_{Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.}

_{Reviews (54): Last reviewed commit: "UCC/CORE: Added automation to loading lo..." | Re-trigger Greptile}

greptile-apps

_{5 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{7 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-29T08:16:11Z

        ucp_params.estimated_num_eps = params->estimated_num_eps;
    }

 #ifdef HAVE_UCX_NODE_LOCAL_ID


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!}

greptile-apps

_{6 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-01-29T10:28:11Z

Additional Comments (1)

src/core/ucc_context.c
Added components/topo/ucc_topo.h include to support automatic local rank computation from topology

greptile-apps

_{4 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

janjust · 2026-02-12T15:05:58Z

/build

janjust · 2026-03-20T19:27:22Z

/build

janjust · 2026-03-30T15:36:12Z

/build

Sergei-Lebedev · 2026-03-31T07:41:03Z

+    ucc_sbgp_t *sbgp;
+
+    if (!topo) {
+        return 0;


i think it makes sense to return rank_invalid in this case

Sergei-Lebedev · 2026-03-31T07:41:20Z

+    sbgp = ucc_topo_get_sbgp(topo, UCC_SBGP_NODE);
+
+    if (sbgp->status != UCC_SBGP_ENABLED) {
+        return 0;


invalid rank

Sergei-Lebedev · 2026-03-31T07:42:12Z

    cfg_mod = std::to_string(bootstrap->get_ppn());
    UCCCHECK_GOTO(ucc_context_config_modify(ctx_config, NULL,
                  "ESTIMATED_NUM_PPN", cfg_mod.c_str()), free_ctx_config, st);
-    cfg_mod = std::to_string(bootstrap->get_local_rank());


why remove?

I removed it for validation of my function. Ill return it

Sergei-Lebedev · 2026-03-31T07:42:58Z

    return status;
 }

+


rem extra blank line

Sergei-Lebedev · 2026-03-31T07:54:19Z

+            do {
+                /* UCC context create is blocking fn, so we can wait here for the
+                completion of addr exchange */
+                status = ucc_core_ctx_id_exchange(ctx, &ctx->params.oob,


second oob allgather does host info exchange as well, which is not always necessary since we already have topo info

I currently clean topo after the local id clac. Do you think I should create another addr exchange function which complete the rest of the original addr exchange, in case that the ucc_core_ctx_id_exchange was used?

janjust · 2026-04-06T02:18:26Z

/build

janjust · 2026-04-09T15:11:50Z

@MaayanGadishNvidia let's rework so that init tim doesn't increase, please run osu_mpi_init tests to make sure we don't impact init

janjust marked this pull request as ready for review January 22, 2026 16:03

janjust requested review from Sergei-Lebedev and janjust January 22, 2026 16:15

janjust added Ready-for-Review Code-Review-Completed labels Jan 22, 2026

greptile-apps Bot reviewed Jan 22, 2026

View reviewed changes

Comment thread src/core/ucc_context.c Outdated

Comment thread src/components/topo/ucc_topo.h Outdated

MaayanGadishNvidia force-pushed the NIC_bid_auto branch from 211c631 to 51f149d Compare January 22, 2026 17:10

greptile-apps Bot reviewed Jan 22, 2026

View reviewed changes

Comment thread src/core/ucc_context.c Outdated

Comment thread src/components/topo/ucc_topo.h Outdated