[iyastreb]

What?

Simplify notification sending, avoid several redundant allocations

Testing

UCX_TLS=^cuda_ipc NIXL_UCX_SGL_ENABLE=1 numactl -N 0 -m 0 nixlbench --initiator_seg_type=VRAM --target_seg_type=VRAM --start_block_size=65536 --max_block_size=65536 --start_batch_size=1 --max_batch_size=1 --progress_threads=0 --num_iter=1000000

Block Size (B)      Batch Size     B/W (GB/Sec)   Avg Lat. (us)  Avg Prep (us)  P99 Prep (us)  Avg Post (us)  P99 Post (us)  Avg Tx (us)    P99 Tx (us)    
----------------------------------------------------------------------------------------------------------------------------------------------------------------
65536               1              9.374971       7.0            31.0           31.0            1.2            2.0            5.7            9.0
65536               1              9.485419       6.9            11.0           11.0            1.1            2.0            5.7            9.0

Summary by CodeRabbit

• Bug Fixes
  • Improved UCX request handling across regular, scatter-gather, composite, flush, and notification operations.
  • Requests retain their connection and endpoint while processing, helping transfers and notifications use the appropriate endpoint.
  • Updated asynchronous notification delivery to manage message data consistently and handle send completion cleanly.
  • UCX error handling now uses the configured mode when available, with a peer-mode default otherwise.

[github-actions]

👋 Hi iyastreb! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

[iyastreb]

/build

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: ai-dynamo/nixl/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Enterprise
• Run ID: 3e51869e-4c45-46af-a5d5-ecf67606cd9c

📥 Commits

Reviewing files that changed from the base of the PR and between ef46d22 and 5baffe2.

📒 Files selected for processing (5)

• src/plugins/ucx/ucx_backend.cpp
• src/plugins/ucx/ucx_backend.h
• src/plugins/ucx/ucx_backend_req.h
• src/plugins/ucx/ucx_thread_pool_engine.cpp
• src/plugins/ucx/ucx_utils.h

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

UCX request handles now retain their connection and endpoint. Transfer and notification paths initialize and use that handle state. Notification serialization and active-message sending now pass owned std::string payloads.

Changes

UCX request and notification flow

Layer / File(s)	Summary
Request handle lifecycle src/plugins/ucx/ucx_backend_req.h	Request handles gain init and getEp, retain their connection and endpoint, and reset that state on release. Appending requests no longer assigns or checks the connection.
Transfer handle setup src/plugins/ucx/ucx_backend.cpp, src/plugins/ucx/ucx_thread_pool_engine.cpp, src/plugins/ucx/ucx_backend.h	SGL, regular, and composite transfers initialize handles with a connection and endpoint. Request appends no longer pass the connection. Error-handling mode initialization moves to a helper, and errHandlingMode_ is now const.
Owned notification sends src/plugins/ucx/ucx_backend.cpp, src/plugins/ucx/ucx_backend.h, src/plugins/ucx/ucx_backend_req.h, src/plugins/ucx/ucx_utils.cpp, src/plugins/ucx/ucx_utils.h	Notification state stores serialized strings, and notification paths use the handle endpoint. sendAm accepts an owned string payload and retains it for asynchronous sends.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 5baff

No actionable issue remains from the reviewed changes. The PR is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the UCX plugin change and its main goal of simplifying notifications.
Description check	✅ Passed	The description explains the change and its purpose, and includes benchmark results. It omits the template’s Why and How headings, but the purpose is stated and How is optional.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}

[svc-nixl]

🤖 CI Triage Agent — nixl-ci-gpu-vr · commit d0b9d02c

TL;DR: test_prep_mem_view fails because UCX cannot build a device-capable (cuda_ipc/GDA) lane to the peer on node vr-nvl72-ts3-l06-comp137 — ucp_device_remote_mem_list_create returns UCS_ERR_INVALID_PARAM ("lane not found for element 0") and NIXL surfaces NIXL_ERR_BACKEND. This is not caused by commit d0b9d02: the identical failure occurs on build #90, an unrelated PR, on the same node, while build #88 passed on a different node.

Full analysis

Summary: Stage "Run Python tests" (node 188) failed: test/python/test_nixl_api.py::test_prep_mem_view — 1 failed, 25 passed, 2 skipped; srun exited 1.

Root cause: The test spawns two ranks, one GPU each, and requires the remote overload of prep_mem_view to succeed. Both spawned workers logged, before the Python traceback:

ucp_device.c:700  UCX ERROR lane not found for element 0
ucp_device.c:819  UCX ERROR failed to create handle: Invalid parameter
E ucx_backend.cpp:752] Failed to prepare remote memory view: Failed to create device memory list(remote): Invalid parameter

nixl::ucx::createMemList(const nixl_remote_meta_dlist_t&, ...) only loops while UCX returns UCS_ERR_NOT_CONNECTED; UCS_ERR_INVALID_PARAM falls through to throw std::runtime_error(...) (src/plugins/ucx/mem_list.cpp:176-178), which nixlUcxEngine::prepMemView converts to NIXL_ERR_BACKEND (src/plugins/ucx/ucx_backend.cpp:751-754), raised into Python as nixlBackendError at test_nixl_api.py:304. "lane not found" means the endpoint to the peer has no device-capable lane — exactly the condition the test's own comment anticipates ("newer UCX rejects it with 'lane not found for element 0'"), but the test has no guard for it: it only skips on not HAVE_UCX_GPU_DEVICE_API or device_count() < 2.

Evidence this is environmental, not from the PR under test:

• Build Add mypy --install-types to pre-commit.yml #91 (commit d0b9d02, PR [5/N] PLUGINS/UCX: Simplify notification #2341) — failed on vr-nvl72-ts3-l06-comp137, slurm allocation 2380150.
• Build Purged unifiedAddr #90 (merge of 1498a38 into 27153aa, a different PR) — byte-for-byte the same UCX error and traceback, same node, slurm allocation 2380141. The ucx_backend.cpp line differs (781 vs 752), confirming a different source tree with identical symptoms.
• Build wheel: initial effort to allow dynamic linking of plugins #88 — the same test passed, on node vr-nvl72-ts3-l11-062-c12 (allocation 2378313).

The PR's diff is confined to notification handling (sendNotif/buildNotif/notifSendPriv/notifAmCb); the notification round-trip in the worker (send_notif/get_new_notifs barrier at test_nixl_api.py:283-290) completed successfully — the failure is strictly after it, in the device mem-list path, which the PR does not touch.

Implicated commit: none — d0b9d02 (Ilia Yastrebov) is not the cause; the same failure reproduces on unrelated build #90 on the same node. The gap is a missing capability guard introduced with the test in [REDACTED:Hex High Entropy String] (x41lakazam, "BINDINGS/PYTHON: Expose prepMemView (local + remote overloads)", #1715).

File: test/python/test_nixl_api.py:304 (failing call); src/plugins/ucx/mem_list.cpp:163-178 and src/plugins/ucx/ucx_backend.cpp:742-755 (error origin)

Suggested fix:

1. Unblock PR [5/N] PLUGINS/UCX: Simplify notification #2341 now: re-run nixl-ci-gpu-vr so it lands on a different vrnvl72 node; this failure is not attributable to the change. Flag vr-nvl72-ts3-l06-comp137 to the cluster owners — two consecutive allocations on it fail the device-API test that passes elsewhere, so check UCX_TLS/cuda_ipc availability and GPU peer topology there.
2. Make the test honest about its hardware requirement rather than failing: have createMemList distinguish "no device-capable lane" from a genuine backend error — return NIXL_ERR_NOT_SUPPORTED when ucp_device_remote_mem_list_create yields UCS_ERR_INVALID_PARAM — and pytest.skip on that in _prep_mem_view_worker. Alternatively probe for a cuda_ipc/GDA lane between the two GPUs in a skipif before mp.spawn.
3. Lower-value but worth it: the NIXL_ERROR message at ucx_backend.cpp:752 should include the peer agent name and worker id, so a failure like this names which endpoint lacked the lane.

Related: PR #2341 (this change, not the cause); PR #1715 (added test_prep_mem_view); PR #2194 "bugfix: test: make the device API tests actually run"; PR #2182 "CI: add nixl-ci-gpu-vr pipeline on the Vera Rubin partition"

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 343b0868-1619-4ec1-90af-b3b338cefc20 in the triage console for the audit trail.

[iyastreb]

/build

[svc-nixl]

🤖 CI Triage Agent — nixl-ci-gpu-vr · commit 5ac0350a

TL;DR: The build didn't fail on code — the "Allocate Environment" stage's salloc sat queued on the vrnvl72 partition for the full --immediate=3600 window and exited with Unable to allocate resources: Connection timed out; this is a Slurm capacity/queue-wait failure, so the fix belongs in the allocation step (retry/queue-wait, smaller exclude list, non-exclusive request), not in the PR.

Full analysis

Summary: nixl-ci-gpu-vr #100 failed in the Allocate Environment stage (node 168) when salloc -p vrnvl72 --immediate=3600 could not get a node within one hour.

Root cause: Everything up to allocation succeeded — UCX/NIXL built, nixlbench built, PR image nixl-ci-gpu-vr-test:100 pushed at 14:01:47. The allocation stage then ran:

14:01:59 + ssh ... salloc -N 1 -p vrnvl72 --job-name=nixl-ci-gpu-vr-100 --immediate=3600 --time=02:20:00 --no-shell
          --account=rubin --exclude=vr-nvl72-ts3-l06-comp137,...comp139,...comp141,vr-nvl72-ts2b-l10-comp102
15:02:08 salloc: lua: Setting job to exclusive mode as no gpu count was specified
15:02:08 salloc: Pending job allocation 2381270
15:02:08 salloc: job 2381270 queued and waiting for resources
15:02:08 salloc: error: Unable to allocate resources: Connection timed out

The single 60-minute gap (14:01:59 → 15:02:08) is exactly --immediate=3600: the job was queued the whole time and Slurm gave up when the immediate window expired. Nothing in nixl hung — the build never reached a test. Three things make this likely to recur: (a) SLURM_EXCLUDE removes four nodes from an already small partition, (b) no GPU count is requested so the Slurm lua plugin promotes the job to whole-node exclusive, and (c) the job is concurrent: true — build #99's Allocate Environment (stage 209, started 14:20) was still pending at the same time, so two runs of the same job competed for the same pool. Secondary issue: because salloc never returned an ID, jobIdFile was never written and pipeline_stop logged WARNING: No job ID files found in /mnt/pvc/nixl-ci-gpu-vr/ — pending job 2381270 had no cleanup path from the pipeline.

Implicated commit: No code commit caused this; 5ac0350a (PR #2341, iyastreb/tp-5-notification) is unrelated to the failing stage. Contributing CI change: 1f87bd7b "CI: exclude misconfigured Vera Rubin nodes from nixl-ci-gpu-vr (#2339)", NirWolfer, 2026-10-05 — merged ~3.5h before this build and it is what narrows the eligible node set.

File: .ci/jenkins/lib/test-vr-matrix.yaml:60-62 (SLURM_EXCLUDE, SLURM_JOB_TIMEOUT, SLURM_IMMEDIATE_TIMEOUT) and the Allocate Environment step at .ci/jenkins/lib/test-vr-matrix.yaml:131-150

Suggested fix:

1. Make allocation tolerant of a busy partition instead of hard-failing the build: wrap slurmCI/allocation in a retry, or drop --immediate in favour of a plain queue wait bounded by the stage timeout so a transient queue backlog doesn't burn an entire 1h+ build.
2. Request an explicit GPU count (e.g. --gpus-per-node=<n>) so the lua plugin stops promoting the job to whole-node exclusive — a non-exclusive request will schedule on partially-busy nodes.
3. Set concurrent: false for nixl-ci-gpu-vr (or gate it behind a Jenkins lockable resource keyed on vrnvl72) so two builds of this job cannot queue against the same handful of nodes — Does NIXL work with AWS EFA? #99 and pytest: initial pytest implementation #100 were doing exactly that.
4. Get the four excluded nodes (vr-nvl72-ts3-l06-comp137/139/141, vr-nvl72-ts2b-l10-comp102) repaired and shrink SLURM_EXCLUDE; with them out, the partition has little headroom.
5. Have the allocation helper write jobIdFile as soon as the job ID is known (it is printed with Pending job allocation <id>), so stopAllForBuild can cancel a still-pending allocation like 2381270.
6. For this specific build: just re-run it — the image was already built and pushed successfully.

Related: PR #2339 (node exclusions, merged same day), PR #2182 (added this pipeline), PR #2341 (the PR under test, unaffected)

[iyastreb]

/build

[iyastreb]

/build

[svc-nixl]

🤖 CI Triage Agent — nixl-ci-gpu-vr · commit 5baffe22

TL;DR: The build never ran any tests — it died in the "Allocate Environment" stage because salloc on the vrnvl72 (Vera Rubin) partition waited out its full --immediate=3600 window and returned Unable to allocate resources: Connection timed out. This is a Slurm capacity/queue problem on the Rubin partition, not a defect in the PR's code.

Full analysis

Summary: Stage Allocate Environment (node 168) of nixl-ci-gpu-vr #115 failed after ~60 min: Slurm job 2388127 stayed PENDING and salloc aborted with error: Unable to allocate resources: Connection timed out.

Root cause: The pipeline requests an exclusive whole-node allocation on partition vrnvl72 with --immediate=3600:

salloc -N 1 -p vrnvl72 --job-name=nixl-ci-gpu-vr-115 --immediate=3600 --time=02:20:00 \
  --no-shell --account=rubin \
  --exclude=vr-nvl72-ts3-l06-comp137,vr-nvl72-ts3-l06-comp139,vr-nvl72-ts3-l06-comp141,vr-nvl72-ts2b-l10-comp102

Timestamps show the request submitted at 07:23:33Z, salloc: Pending job allocation 2388127 / queued and waiting for resources, and the failure at 08:23:42Z — exactly the 3600 s --immediate budget. The one-hour gap is salloc legitimately blocking in the Slurm queue (it reported pending state and then timed out on schedule), so nothing hung: no free node in vrnvl72 satisfied the request within an hour. The preceding stages (Docker build, NIXL compile at 07:18–07:23) all succeeded, and the commit under test (5baffe2, PR #2341 "PLUGINS/UCX: Simplify notification") was never exercised. Contributing factor: four nodes are hard-excluded from this partition by commit 1f87bd7b ("CI: exclude misconfigured Vera Rubin nodes from nixl-ci-gpu-vr", #2339), shrinking an already small Rubin pool, and the job asks for exclusive mode (salloc: lua: Setting job to exclusive mode as no gpu count was specified), so it cannot share a partially-used node.

Implicated commit: Not a code commit. Infra-side; the relevant CI change narrowing the node pool is 1f87bd7b (NirWolfer, "CI: exclude misconfigured Vera Rubin nodes from nixl-ci-gpu-vr", #2339). The commit under test, 5baffe2, is not implicated.

File: Jenkins stage Allocate Environment → slurm.allocation(partition: vrnvl72, immediateTimeout: 3600, extraArgs: [--account=rubin, --exclude=…]) in the nixl-ci-gpu-vr job definition under .ci/jenkins/ (the JJB/pipeline file added by #2182 that carries the --exclude list from #2339)

Suggested fix: Retry the build — this is a transient allocation failure, not a test failure. To stop it recurring:

1. Check capacity/health of the vrnvl72 partition (sinfo -p vrnvl72 -o '%n %t %E') and whether the four excluded nodes (vr-nvl72-ts3-l06-comp137/139/141, vr-nvl72-ts2b-l10-comp102) can be repaired and returned, since excluding them leaves too few schedulable nodes.
2. Request a GPU count explicitly (e.g. --gpus-per-node=N) instead of letting the site Lua plugin promote the job to exclusive mode, so it can land on a partially-allocated node.
3. Make allocation failure non-fatal/retryable in the pipeline: wrap the slurm.allocation step in a retry, or mark the build UNSTABLE/ABORTED rather than FAILURE when salloc returns "Unable to allocate resources", so queue contention isn't reported as a code regression against the PR.

Related: PR #2341 (commit under test, unaffected); PR #2339 (1f87bd7b, added the node exclusions); PR #2182 (2dd278bf, added the nixl-ci-gpu-vr pipeline)