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 Overview

Greptile Summary

This PR implements automatic local rank computation from topology when not provided by the user. The implementation adds a new ucc_core_ctx_id_exchange() function to exchange context IDs for topology initialization, and uses the topology to compute the node local rank before creating TL contexts. The perf tool is updated to rely on automatic computation instead of manual configuration.

Key changes:

Added ucc_topo_node_local_rank() helper to retrieve local rank from node subgroup
Context ID initialization moved earlier (before local rank computation) to fix previous double-init issue
Added !ctx->topo check on line 878 to prevent double topo initialization
Automatic local rank computation only triggers when node_local_id == UCC_ULUNITS_AUTO and OOB is available with multiple endpoints
Previous review comments have been addressed regarding context ID timing and topo double-init

Confidence Score: 4/5

Safe to merge with minor dead code that should be cleaned up
The PR addresses most previous review concerns (context ID double-init, topo double-init). The implementation is sound and follows existing patterns. Score reduced by 1 due to dead code at line 632 that should be removed for code cleanliness, though it doesn't affect functionality.
src/core/ucc_context.c contains dead code at line 632 that should be cleaned up

Important Files Changed

Filename	Overview
src/components/topo/ucc_topo.h	Adds `ucc_topo_node_local_rank()` helper function to retrieve local rank from topology
src/core/ucc_context.c	Implements automatic local rank computation from topology, with context ID initialization moved earlier and topo double-init protection added

greptile-apps

_{5 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

src/components/topo/ucc_topo.h

greptile-apps

_{7 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

src/components/topo/ucc_topo.h

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

greptile-apps · 2026-01-29T07:36:06Z

src/core/ucc_context.c

+        if (ctx->topo) {
+            status = ucc_topo_init(set, ctx->topo, &topo);
+            if (UCC_OK != status) {
+                ucc_warn("failed to init topo for computing local rank");
+            } else {
+                b_params.node_local_id = ucc_topo_node_local_rank(topo);
+                ucc_topo_cleanup(topo);
+            }
+        }


When n_oob_eps <= 1, ctx->topo is NULL, so local rank computation is skipped. For single-rank scenarios, b_params.node_local_id remains UCC_ULUNITS_AUTO and gets passed to UCX. Should single-rank cases explicitly set node_local_id = 0?

Suggested change

if (ctx->topo) {

status = ucc_topo_init(set, ctx->topo, &topo);

if (UCC_OK != status) {

ucc_warn("failed to init topo for computing local rank");

} else {

b_params.node_local_id = ucc_topo_node_local_rank(topo);

ucc_topo_cleanup(topo);

}

}

if (ctx->topo) {

status = ucc_topo_init(set, ctx->topo, &topo);

if (UCC_OK != status) {

ucc_warn("failed to init topo for computing local rank");

} else {

b_params.node_local_id = ucc_topo_node_local_rank(topo);

ucc_topo_cleanup(topo);

}

} else if (params->mask & UCC_CONTEXT_PARAM_FIELD_OOB && params->oob.n_oob_eps == 1) {

b_params.node_local_id = 0;

}

src/components/tl/ucp/tl_ucp_context.c

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

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

src/core/ucc_context.c

+    if (addr_storage->addr_len == 0 ) {
+        ucc_free(addr_storage->storage);
+        addr_storage->storage = NULL;
+        return UCC_OK;
+    }


Unreachable code: addr_len is set to a fixed non-zero value at line 618, so this check will never be true. This code was copied from ucc_core_addr_exchange where addr_len varies, but here it's always sizeof(ucc_context_id_t).

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

src/components/tl/ucp/tl_ucp_context.c

        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 · 2026-01-29T09:27:02Z

src/core/ucc_context.c

+    if (addr_storage->addr_len == 0 ) {
+        ucc_free(addr_storage->storage);
+        addr_storage->storage = NULL;
+        return UCC_OK;
+    }


Dead code - addr_len is always set to sizeof(ucc_context_id_t) on line 618, so this check will never be true

Suggested change

if (addr_storage->addr_len == 0 ) {

ucc_free(addr_storage->storage);

addr_storage->storage = NULL;

return UCC_OK;

}

ucc_assert(addr_storage->storage != NULL);

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

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}

src/core/ucc_context.c

greptile-apps

_{3 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

src/core/ucc_context.c

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

src/core/ucc_context.c

+                    goto error_ctx_create;
+                }
+            } while (status == UCC_INPROGRESS);
+            status = ucc_context_topo_init(&ctx->addr_storage, &ctx->topo);


Potential double initialization of ctx->topo. When automatic local rank computation runs (line 747), it initializes ctx->topo. If topo_required is also true, line 880 attempts to initialize ctx->topo again without freeing the first allocation, causing a memory leak of the original topo structure and its procs array.

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

src/core/ucc_context.c

+    if (addr_storage->addr_len == 0 ) {
+        ucc_free(addr_storage->storage);
+        addr_storage->storage = NULL;
+        return UCC_OK;
+    }


Dead code - addr_len is always set to sizeof(ucc_context_id_t) at line 618, so this condition will never be true

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 · 2026-02-08T09:20:54Z

src/core/ucc_context.c

+    if (addr_storage->addr_len == 0 ) {
+        ucc_free(addr_storage->storage);
+        addr_storage->storage = NULL;
+        return UCC_OK;
+    }


dead code - addr_len is always set to sizeof(ucc_context_id_t) at line 618 (non-zero), so this condition will never be true

while this pattern exists in ucc_core_addr_exchange() where addr_len varies based on allgather results, here it's a fixed non-zero value

greptile-apps

_{4 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-08T09:36:39Z

src/core/ucc_context.c

+    if (addr_storage->addr_len == 0 ) {
+        ucc_free(addr_storage->storage);
+        addr_storage->storage = NULL;
+        return UCC_OK;
+    }


dead code - addr_len is always set to sizeof(ucc_context_id_t) at line 618, so this condition will never be true

Suggested change

if (addr_storage->addr_len == 0 ) {

ucc_free(addr_storage->storage);

addr_storage->storage = NULL;

return UCC_OK;

}

ucc_assert(addr_storage->storage != NULL);

…vided by user

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

/build

greptile-apps

_{4 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

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

src/core/ucc_context.c Outdated Show resolved Hide resolved

src/components/topo/ucc_topo.h Outdated Show resolved Hide resolved

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

src/core/ucc_context.c Outdated Show resolved Hide resolved

src/components/topo/ucc_topo.h Outdated Show resolved Hide resolved