Refactoring: derive nixlUcxThreadPoolEngine from nixlUcxThreadEngine - #1906
Conversation
|
👋 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. 🚀 |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUCX thread handling now creates shared progress threads only when enabled and workers exist, and thread-pool instances inherit the thread-engine’s notification and progress behavior instead of maintaining separate shared-thread state. ChangesUCX Thread Engine Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/build |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/ucx_backend.h (1)
328-328: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOverride
getNotifs()in the thread-pool engine.nixlUcxThreadPoolEnginecan still be created withenableProgTh == false && num_threads > 0, so inheritingnixlUcxThreadEngine::getNotifs()makes the null-thread_fallback callprogressLoop()while dedicated workers are already active. That can progress the same UCX workers concurrently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/ucx/ucx_backend.h` at line 328, The thread-pool engine inherits getNotifs() from nixlUcxThreadEngine, which can incorrectly fall back to progressLoop() even when nixlUcxThreadPoolEngine has dedicated worker threads running. Override getNotifs() in nixlUcxThreadPoolEngine so notification handling uses the thread-pool path only and never starts the null-thread fallback that can progress UCX workers concurrently; update the override near nixlUcxThreadPoolEngine and align it with the existing thread-pool/progress logic in nixlUcxThreadEngine.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/plugins/ucx/ucx_backend.h`:
- Line 328: The thread-pool engine inherits getNotifs() from
nixlUcxThreadEngine, which can incorrectly fall back to progressLoop() even when
nixlUcxThreadPoolEngine has dedicated worker threads running. Override
getNotifs() in nixlUcxThreadPoolEngine so notification handling uses the
thread-pool path only and never starts the null-thread fallback that can
progress UCX workers concurrently; update the override near
nixlUcxThreadPoolEngine and align it with the existing thread-pool/progress
logic in nixlUcxThreadEngine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 76f8d202-60e9-4b8d-808d-05c26446014e
📒 Files selected for processing (1)
src/plugins/ucx/ucx_backend.h
|
/build |
|
/build |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/ucx/ucx_backend.cpp`:
- Around line 366-368: Validate `num_threads` in `UcxBackend` before computing
`numSharedWorkers_` so it cannot exceed `getWorkers().size()`, and fail fast
with a runtime error if the requested thread count is invalid. Update the logic
around `shared_count` and the worker-indexing path in `UcxBackend::...` so it no
longer relies on `NIXL_ASSERT` for production safety, and ensure the derived
worker range used later when indexing workers is always bounds-checked.
In `@src/plugins/ucx/ucx_backend.h`:
- Around line 315-322: Move getSharedWorkersSize() back under the protected
section alongside appendNotif() in ucx_backend.h so it remains an internal
helper instead of a public API. Keep the UCX backend member grouping consistent
by preserving the intended public/protected/private layout, and make sure any
existing access by the pool still works through the protected override rather
than exposing it through the public interface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 469a4e81-8e2e-47a6-b6f0-3394bff8ec44
📒 Files selected for processing (2)
src/plugins/ucx/ucx_backend.cppsrc/plugins/ucx/ucx_backend.h
|
/build |
|
/build |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" stage exited 42 because the gtest CI harness treats the benign environmental warning Full analysisSummary: Stages 203 and 242 ("Run CPP tests") failed with exit code 42; 69/154 tests reported failed, almost all due to a UCX build lacking CUDA/IB support in the test container. Root cause: The UCX in the test image was built/loaded without CUDA and accelerated-IB transports ( Implicated commit: unknown (recent history on File: Suggested fix: Fix the test container's UCX so it is built with CUDA and IB/RDMA transport support (investigate the failing Related: none found. |
|
🤖 CI Triage Agent — TL;DR: The "Run Nixlbench tests" stage was killed by Jenkins (exit 143) after an ~18-minute hang, not a slow test; the first UCCL+ETCD benchmark hung during engine teardown (one Full analysisSummary: Nixlbench stage (node 356) hung during teardown of the first UCCL+ETCD benchmark and was SIGTERM-killed after ~18 min of no output. Root cause: Engine/worker shutdown deadlock. At 06:47:45–48 both workers reached "Destroying Engine…"; one printed "Engine destroyed" but the peer hit Implicated commit: PR #1906 (adac02f, "Refactoring: derive nixlUcxThreadPoolEngine from nixlUcxThreadEngine") — the change under test; closely related prior fix is 0225cd5 (#1880, "ucx: fix infinite loop dropping pending requests on dedicated-thread exit") by Eylon Eliyahu Krause, same shutdown code path. File: UCX engine shutdown path exercised via Suggested fix: Reproduce the UCCL+ETCD READ VRAM→VRAM case and capture a stack trace (gdb/py-spy) of the stuck Related: PR #1906 (#1906); PR #1880 (#1880); PR #1846 (#1846, forcibly close UCP endpoints). |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" stage crashed (SIGABRT, exit 134) in Full analysisSummary: Jenkins Root cause: UCX on test node Implicated commit: unknown — no code change in the failing test path is implicated; the failure originates from the UCX runtime/device configuration on node File: Suggested fix:
Related: PR #1869 (CI base image switch to pytorch + CUDA 13.3); PR #1743 (run gtest in single process). No existing issue matches the "No such device" UCX worker failure. |
|
🤖 CI Triage Agent — TL;DR: The NIXL build compiled and installed cleanly; the pipeline failed in the "Allocate Environment" stage when the Slurm Full analysisSummary: Build #2789 failed in the "Allocate Environment" stage (node 200/183) while allocating a GPU node via Slurm; the container exec stream disconnected before the job started. Root cause: Infrastructure, not code. The Note on timing: there is an ~8-minute quiet span between Implicated commit: unknown — not code-related (commit adac02f / PR #1906 is not implicated; the build and image push succeeded). File: Jenkins pipeline Slurm allocation step ( Suggested fix: Re-run build #2789 (retry the "Allocate Environment" stage); the failure is a transient Slurm/websocket dropout. If it recurs: (1) check the Related: none (issue search returned no matching reports). |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" stage failed because one gtest, Full analysisSummary: Root cause: The test unconditionally creates a UCX context requesting Implicated commit: unknown — the failing test file wasn't retrievable via the tools and no matching commit was found in the searched history; the failure is environment-driven (UCX build lacking CUDA/IB) rather than a specific code change. File: test gtest Suggested fix: Make Related: #1909 ("TEST/GTEST: Fail on unexpectedly skipped tests and fix device tests"), #1953 (UCX version logging) — possibly relevant to the UCX/device-test handling; no exact matching issue found. |
|
🤖 CI Triage Agent — TL;DR: The "Run Nixlbench tests" stages didn't time-out from being slow — they hung for ~12–16 minutes during UCX/UCCL engine teardown after the last completed benchmark (UCCL WRITE, VRAM→VRAM, ETCD), then Jenkins killed them (exit 143). The hang is in the UCX progress-thread shutdown path that PR #1906 refactors, so that PR is the implicated change. Full analysisSummary: Jenkins Root cause: Log-timestamp analysis shows a hang, not overrun. Stage 356's last application line is Implicated commit: PR #1906 (commit [REDACTED:Hex High Entropy String], "Refactoring: derive nixlUcxThreadPoolEngine from nixlUcxThreadEngine"). Related prior fix in the same area: 0225cd5 (Eylon Eliyahu Krause, #1880, "ucx: fix infinite loop dropping pending requests on dedicated-thread exit"). File: Suggested fix: Audit the refactored destructor/thread-shutdown path introduced by #1906. After deriving the shared base, the progress thread's stop-flag / Related: PR #1906 (the change under test); PR #1880 (recent fix to UCX dedicated-thread exit infinite loop, same shutdown path).
|
|
/build |
|
🤖 CI Triage Agent — TL;DR: The "Run CPP tests" stage failed because CUDA could not initialize on the test node Full analysisSummary: Jenkins Root cause: CUDA failed to initialize inside the test container on Implicated commit: unknown — no code regression identified; evidence points to node/container CUDA environment on File: Suggested fix: Repair GPU access on the test node/container so Related: none found.
|
|
🤖 CI Triage Agent — TL;DR: The GPU node Full analysisSummary: "Run CPP tests" stages failed on GPU node Root cause: Node-level GPU/RDMA fault on Implicated commit: None. The failing test File: Environmental — Suggested fix: Retry the build on a healthy GPU node and drain/reboot Related: none found (issue/PR search returned no matches for the UCX arm-iface error). |
…- v1.4.0 (#2012) ## What? Backport #1906 to v1.4.0 To unify the code and reuse the same functions It's pre-requisite for completions API PR <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added configurable UCX shared and dedicated worker threading. * Added optional CUDA, Torch, Infinia DDN, and UCX plugin build configuration. * Added pre-built wheel-base image support for faster wheel builds. * Added container build verification coverage for multiple platforms. * Updated project and package version to 1.4.0. * **Documentation** * Updated CI workflow, wheel-building, caching, and image-tag documentation. * Added NVIDIA proprietary licensing attribution. * **Chores** * Added NVIDIA license information and updated third-party component notices. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 participants
What?
To unify the code and reuse the same functions
It's pre-requisite for completions API PR
Summary by CodeRabbit
Bug Fixes
Refactor