TL/UCP: enable host-side SRA allreduce pipelining

[wfaderhold21]

Overlap the reduce-scatter and allgather phases across fragments on the host path so fragment i's allgather (pure network) runs concurrently with fragment i+1's reduce-scatter (network + CPU reduction). Previously the host branch of the SRA-knomial pipeline heuristic disabled pipelining (n_frags=1), leaving the NIC idle during every reduce step and the CPU idle during the entire allgather.

The host else-branch of get_pipeline_params() now sets conservative defaults: threshold 256KB, 512KB fragments, nfrags floor 2, pdepth 2, parallel order. Env override UCC_TL_UCP_ALLREDUCE_SRA_KN_PIPELINE still takes precedence. Correctness is unchanged: the CUDA in-place path already runs the identical fragmented schedule, and per-frag scratch is sized from max_frag_count (smaller frags -> smaller scratch).

[svcnbu-swx-hpcx]

🤖 CI Triage Agent — ASAN · commit dbea8e26

TL;DR: The gtest test_schedule.pipelined_depth_16_exact_lifecycle failed because the PR's new depth-16 pipeline tests expect UCC_SCHEDULE_PIPELINED_MAX_FRAGS to be 16, but the header still defines it as 4, so ucc_schedule_pipelined_init clamps the depth to 4 and only 4 fragments/8 subscriptions are created instead of the expected 16/32. Bump UCC_SCHEDULE_PIPELINED_MAX_FRAGS to 16 (and size frags[] accordingly).

Full analysis

Summary: ASAN gtest run failed 1 test — test_schedule.pipelined_depth_16_exact_lifecycle — while the build itself succeeded.

Root cause: UCC_SCHEDULE_PIPELINED_MAX_FRAGS is still 4 in src/schedule/ucc_schedule_pipelined.h (line 14), and the frags[] array is sized to it (line 95). ucc_schedule_pipelined_init clamps any requested n_frags down to this max (ucc_schedule_pipelined.c lines 238-244). The PR's new depth-16 tests request 16 fragments and assert g_fault_frag_inits == 16 and g_fault_subscribe_attempt == 32 (test_schedule.cc lines 1102-1112), but the clamp produces only 4 fragments (8 subscribe attempts), so the EXPECT_EQ(16, ...) assertion fails. The test/source are inconsistent: the tests were written for MAX_FRAGS=16 but the constant wasn't raised.

Implicated commit: d56c68d — "TL/UCP: enable host-side SRA allreduce pipelining" (Ferrol Aderholdt) on branch topic/ar-sra-host-pipelining; the depth-16 tests were added without bumping the max-frags constant.

File: src/schedule/ucc_schedule_pipelined.h:14 (#define UCC_SCHEDULE_PIPELINED_MAX_FRAGS 4); array at :95; clamp at src/schedule/ucc_schedule_pipelined.c:238-244; failing assertions at test/gtest/core/test_schedule.cc:1111-1112.

Suggested fix: Change #define UCC_SCHEDULE_PIPELINED_MAX_FRAGS 4 to 16 in src/schedule/ucc_schedule_pipelined.h so the frags[] array can hold 16 entries and the init no longer clamps a depth-16 request. (Alternatively, if 16 is not intended, adjust the new depth-16 tests to match the real max — but the PR's purpose of "host-side SRA pipelining" and the pipelined_init_depth_16_success test comment "matches UCC_SCHEDULE_PIPELINED_MAX_FRAGS" indicate the constant should become 16.) Verify no static allocations depending on the old value of 4 overflow after the bump.

Related: PR #1354; prior schedule-pipeline fixes c8314d3 (#1262), 4f67436 (#1051).

[svcnbu-swx-hpcx]

🤖 CI Triage Agent — Build & Test · commit dbea8e26

TL;DR: Two new test_schedule gtests (pipelined_real_subscription_failure_first, pipelined_depth_16_exact_lifecycle) fail because the fault-injection harness installs process-global callbacks/counters in fault_reset() but never tears them down, so their exact-count assertions get corrupted by other tests in the same binary; fix by clearing the fault hooks and resetting counters in a gtest TearDown().

Full analysis

Summary: The gtest job (shard 0) failed: 2 of 7759 tests failed — test_schedule.pipelined_real_subscription_failure_first and test_schedule.pipelined_depth_16_exact_lifecycle. Build and all other stages passed.

Root cause: These tests use a fault-injection harness that registers process-global hooks and counters — ucc_event_manager_set_subscribe_fault_cb(fault_subscribe_cb) and ucc_schedule_pipelined_set_lock_observer(fault_lock_observe) (test/gtest/core/test_schedule.cc:1025-1026), plus static globals g_fault_subscribe_attempt, g_fault_subscribe_fail_at, g_fault_lock_inits/destroys, etc. (lines 927-936). They are set at the start of each fault test in fault_reset() but are never uninstalled/reset in a TearDown. Once installed, fault_subscribe_cb stays active for the rest of the single gtest process and fires for every subsequent ucc_event_manager_subscribe() call — including from unrelated test_schedule tests — incrementing the shared g_fault_subscribe_attempt counter (and potentially returning a spurious UCC_ERR_NO_MEMORY when a stale g_fault_subscribe_fail_at matches). The two failing tests are exactly the ones with the tightest exact-equality oracles (EXPECT_EQ(fail_at+1, g_fault_subscribe_attempt) == 1, and EXPECT_EQ(32, g_fault_subscribe_attempt)), so cross-test contamination / gtest's non-source execution order makes them fail while the looser fault tests pass. The ucc_schedule_pipelined_init/_finalize unwind logic itself (src/schedule/ucc_schedule_pipelined.c) is internally consistent with the expected counts, confirming the defect is the leaked global test state rather than the production code path.

Implicated commit: d56c68d1 (Ferrol Aderholdt, "TL/UCP: enable host-side SRA allreduce pipelining") which added this harness, refined by 67ae2ddd (ferrol aderholdt, "REVIEW: remove unnecessary comments") — both on PR #1354.

File: test/gtest/core/test_schedule.cc:1013-1027 (fault_reset installs hooks; no matching teardown) and the assertions at lines 1058, 1072-1076, 1111-1119.

Suggested fix: Make the fault harness self-contained per test. Add a TearDown() (or explicit cleanup at the end of each fault test / fault_expect_subscription_unwind) that unregisters the global hooks and resets counters, e.g.:

ucc_event_manager_set_subscribe_fault_cb(NULL);
ucc_schedule_pipelined_set_lock_observer(NULL);
g_fault_subscribe_fail_at = -1;
g_fault_subscribe_attempt = 0;

so no fault callback survives into other tests and each test starts from a clean, deterministic global state. Alternatively, gate fault_subscribe_cb so it is a no-op unless a fault test is actively running. This removes the order-dependence and makes the exact-count assertions reliable.

Related: PR #1354 (topic/ar-sra-host-pipelining); prior pipelined-schedule fixes for context — #1262 (task restart in ppln schedule) and #1051 (n_deps overflow in pipelined schedule).

[svcnbu-swx-hpcx]

🤖 CI Triage Agent — Lint (codestyle) · commit aa14f863

TL;DR: The codestyle lint failed only on the commit-title policy check: HEAD commit aa14f863 has a 53-character subject, exceeding the 50-char limit. Reword/amend that commit title to ≤50 chars (e.g. REVIEW: raise pipelined MAX_FRAGS back to 16) and force-push.

Full analysis

Summary: The "Check commit title" step of the codestyle job exited 1 because one commit in the PR range has a title longer than 50 characters.

Root cause: .github/workflows/codestyle.yaml enforces a 50-character maximum on every non-merge commit subject in remotes/origin/$GITHUB_BASE_REF..HEAD. The log shows:

• Commit title is too long: 53
• Bad commit title: 'REVIEW: raise pipelined MAX_FRAGS capacity back to 16' (53 chars)

The other three commits in the PR passed (REVIEW: address feedback, REVIEW: remove unnecessary comments, TL/UCP: enable host-side SRA allreduce pipelining). This is purely a commit-message policy violation — no clang-format/code issue was reported, and the later "Check code format" step never ran because the earlier step failed.

Implicated commit: [REDACTED:Hex High Entropy String] — "REVIEW: raise pipelined MAX_FRAGS capacity back to 16" (author of PR #1354, branch topic/ar-sra-host-pipelining)

File: .github/workflows/codestyle.yaml:39 (the ${#msg} -gt 50 check); offending artifact is the commit subject of aa14f863, not a source file.

Suggested fix: Amend the offending commit subject to 50 characters or fewer and force-push the branch:

git rebase -i remotes/origin/master   # or: git commit --amend if it's HEAD
# reword aa14f863 to e.g.:
#   "REVIEW: raise pipelined MAX_FRAGS back to 16"   (44 chars)
git push --force-with-lease

Since the three REVIEW: commits are review fixups, the cleanest option is to squash all four into the single TL/UCP: enable host-side SRA allreduce pipelining commit — that removes the long title and leaves one properly-tagged commit. Note the format check (Check code format) has not yet been validated on this branch; expect it to run only after the title check passes.

Related: PR #1354 (openucx/ucc) — no existing issue found for this failure signature.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 72daf3a8-29eb-4b67-a682-58d75979eba2 in the triage console for the audit trail.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness, security, or repository-rule violations identified.

Findings

1.  Global Test Hooks Leak ▶

Summary

The PR enables host-side SRA knomial allreduce pipelining on eligible single-node teams and strengthens pipeline validation, failure cleanup, and test coverage.

• Selects conservative host pipeline defaults while retaining monolithic behavior for tagged, unsupported-topology, and non-host cases.
• Validates pipeline parameters and propagates initialization errors across hierarchical and UCP collectives.
• Corrects schedule/task cleanup on initialization and finalization paths.
• Adds focused tests for pipeline selection, invalid configuration, fragment cleanup, lock lifecycle, and bounded diagnostic formatting.
• The previously reported global test-hook issue is fixed by atomic hook access and fixture teardown that restores both hooks.

_{Reviews (2) · Last reviewed commit: "REVIEW: make test fi hooks atomic and te..."}

[greptile-apps]

Global Test Hooks Leak

These fault-injection callbacks are process-wide globals that scheduler threads read without synchronization. The tests install both callbacks but never restore them, so later unrelated schedules in the same process can still invoke test instrumentation or race with callback updates. Please keep these hooks test-scoped or synchronize them and reliably restore them after each test.

Knowledge Base Used: Runtime utilities and infrastructure