PLUGINS/UCX: Timeout warning for device memory list creation.

[rakhmets]

What?

Added a warning message in case the time limit for creating the device memory list is exceeded.
Applied logic from the Device API V1 #947.

Summary by CodeRabbit

• Bug Fixes
  • Improved timeout handling for device memory setup with a configurable warning threshold (default 5s); a single warning is emitted once when exceeded and retries continue with backoff.
  • Reduced CPU usage during memory list creation by yielding when progress stalls.
  • Standardized and clarified failure messages for device memory list creation across local and remote paths.

[github-actions]

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

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

🚀

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a centralized std::string error message and timeout-aware retry logic for remote device memory list creation in the UCX plugin; includes <chrono> and <thread> and adjusts error handling to use the new message consistently. No public API/signature changes.

Changes

Cohort / File(s)

Summary

UCX mem_list implementation src/plugins/ucx/mem_list.cpp

Introduced const std::string error_message{"Failed to create device memory list"} in an anonymous namespace; unified error text for remote/local/not-supported cases; added timeout-based retry for remote memory list creation using std::chrono, a configurable NIXL_UCX_TIMEOUT_WARNING, worker.progress() and std::this_thread::sleep_for backoff; swapped a std::string_view to const std::string; added <chrono> and <thread> includes and a config include. No exported signatures changed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble bytes beneath the code-tree's twist,
one steadfast string clenched in my fist,
I wait, I ping, I warn once more,
retrying steps along the shore,
carrot crumbs for every fixed list. 🐇

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The description is incomplete. While it addresses 'What?', it lacks a 'Why?' section explaining the business justification beyond referencing an external PR.	Expand the 'Why?' section to explain the business impact and why this timeout warning is necessary for users or the system.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: adding a timeout warning for device memory list creation in the UCX plugin.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/plugins/ucx/mem_list.cpp (1)

20-35: ⚠️ Potential issue | 🔴 Critical

Move <chrono> and <thread> out of the #else branch.

The function createMemList at line 146 is defined only when HAVE_UCX_GPU_DEVICE_API is defined, yet it uses std::chrono::steady_clock, std::chrono_literals, and std::this_thread::sleep_for (lines 155, 161, 163, 169). However, the headers <chrono> and <thread> are placed in the #else branch (lines 30, 34), making the GPU build dependent on transitive includes. Move these headers outside the preprocessor block or place them inside the #ifdef HAVE_UCX_GPU_DEVICE_API branch to ensure proper compilation.

Suggested fix

+#include <chrono>
+#include <thread>
+
 `#ifdef` HAVE_UCX_GPU_DEVICE_API
 `#include` "common/configuration.h"
 `#include` "rkey.h"
 `#include` "ucx_backend.h"
 `#include` "ucx_utils.h"
@@
-#else
-#include <chrono>
+#else
 `#include` <exception>
 `#include` <string>
 `#include` <string_view>
-#include <thread>
 `#endif`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/ucx/mem_list.cpp` around lines 20 - 35, The build breaks because
createMemList (defined when HAVE_UCX_GPU_DEVICE_API is set) uses std::chrono and
std::this_thread but the <chrono> and <thread> headers are incorrectly placed in
the `#else` branch; move the headers so they are available when
HAVE_UCX_GPU_DEVICE_API is defined—either include <chrono> and <thread> above
the `#ifdef` HAVE_UCX_GPU_DEVICE_API block or add them inside the `#ifdef` branch
alongside the other includes—so functions like createMemList, uses of
std::chrono::steady_clock, std::chrono_literals, and std::this_thread::sleep_for
compile without relying on transitive includes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/plugins/ucx/mem_list.cpp`:
- Around line 20-35: The build breaks because createMemList (defined when
HAVE_UCX_GPU_DEVICE_API is set) uses std::chrono and std::this_thread but the
<chrono> and <thread> headers are incorrectly placed in the `#else` branch; move
the headers so they are available when HAVE_UCX_GPU_DEVICE_API is defined—either
include <chrono> and <thread> above the `#ifdef` HAVE_UCX_GPU_DEVICE_API block or
add them inside the `#ifdef` branch alongside the other includes—so functions like
createMemList, uses of std::chrono::steady_clock, std::chrono_literals, and
std::this_thread::sleep_for compile without relying on transitive includes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b7e733d9-3bef-401d-bdaf-68856dce5186

📥 Commits

Reviewing files that changed from the base of the PR and between 383f9b1 and a141f87.

📒 Files selected for processing (1)

• src/plugins/ucx/mem_list.cpp

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/plugins/ucx/mem_list.cpp`:
- Around line 158-166: The NIXL_WARN message in the retry loop around
ucp_device_remote_mem_list_create (uses variables timeout, start, check_timeout
and env key NIXL_UCX_TIMEOUT_WARNING) currently implies a hard failure; change
the log text to indicate it's only a warning that the retry wait has reached the
configured duration (e.g., "Reached warning threshold of X ms, still
waiting/retrying") and explicitly state that NIXL_UCX_TIMEOUT_WARNING is
interpreted as milliseconds when logging or documenting; keep the existing
behavior of setting check_timeout = false so the message only appears once.
- Around line 40-43: Move the constexpr std::string_view error_message out of
the nixl::ucx namespace and into an anonymous namespace at the top of the .cpp
to give it internal linkage (e.g., namespace { constexpr std::string_view
error_message{"Failed to create device memory list"}; }), remove the original
declaration inside namespace nixl::ucx, and leave all uses of error_message
unchanged so the symbol is now file-local per the project's implementation-file
convention.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6c62554f-d3f2-4e09-8cd1-16b5cb3ed3c4

📥 Commits

Reviewing files that changed from the base of the PR and between a141f87 and c28081d.

📒 Files selected for processing (1)

• src/plugins/ucx/mem_list.cpp

[rakhmets]

/build

[rakhmets]

/build