CORE: build UCX with nixl SONAME suffix, load with RTLD_DEEPBIND to avoid collisions - #1673
Conversation
…void collisions Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
👋 Hi roiedanino! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
/build |
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
👀 Investigating |
|
🤖 CI Triage Agent — All the evidence needed is in the logs. Here is the full diagnosis: Summary: Root cause: The
Black modified the file in-place and the hook exited 1 because the working-tree file changed during the check. The exact diff from the log: - module_suffix_pattern = re.compile(
- rf"-{escaped_suffix}(-[0-9a-f]{{8}})?\.so"
- )
+ module_suffix_pattern = re.compile(rf"-{escaped_suffix}(-[0-9a-f]{{8}})?\.so")
modules_without_suffix = [
- name for name in ucx_modules
+ name
+ for name in ucx_modules
if not module_suffix_pattern.search(name.rsplit("/", 1)[-1])
]All other hooks (mypy, isort, flake8, codespell, whitespace checks) passed. Implicated commit: File: Suggested fix: In the Related: none
|
Signed-off-by: Roie Danino <rdanino@nvidia.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the relevant section of the file to confirm current content
sed -n '40,65p' contrib/check_ucx_wheel_bundle.pyLength of output: 1112
It looks like the formatting was applied after the CI run on commit |
|
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:
📝 WalkthroughWalkthroughThis PR adds configurable UCX source and SONAME-suffix handling in build and wheel tooling, adds wheel and binding validation scripts, and introduces deepbind-aware UCX plugin loading plus UCX library path validation. ChangesBuild and Validation Infrastructure
Runtime Plugin Loading with RTLD_DEEPBIND Support
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
# Conflicts: # contrib/Dockerfile.manylinux # contrib/build-wheel.sh
|
@coderabbitai review |
✅ Action performedReview finished.
|
ovidiusm
left a comment
There was a problem hiding this comment.
LGTM please merge once NIXL CI passes
|
👀 Investigating |
|
/ok to test 22d9d4d |
|
👀 Investigating |
|
🤖 CI Triage Agent — I've established the critical evidence from the logs I successfully fetched. Let me analyze the timeline definitively rather than continue hunting for the exact script path. Timeline analysis of the fetched log:
The largest inter-line gap by far is ~39 minutes (11:19:33 → 11:58:46) with zero output. Per the triage guidance, exit code 143 + "timeout" is the symptom. This is a hang, not a slow test — the work was clearly not making progress (no output at all for 39 of the run's minutes). The last meaningful operation was Notably, Summary: The Root cause: Not a slow test — a hang during benchmark shutdown. The final application line was Implicated commit: Most likely the UCCL backend integration (PR #895, "Add UCCL backend integration for NIXL") — it introduces the File: UCCL backend engine shutdown path — Suggested fix:
Related: PR #895 (UCCL backend integration); PR #1743 (TEST/GTEST: run in single process) and #1849 (CI triage trigger) are peripheral. No existing issue specifically tracking the UCCL teardown hang was found. |
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — I have the full log. Let me analyze what actually failed. The build succeeded for both branches. Two parallel branches ran: Looking at the timeline for the
Let me confirm the key lines. The evidence is clear from the log. Let me verify the salloc parameters and the failure signature. The failing branch is the SLURM allocation step for the Key evidence:
This is not a hang inside application code — the This is an infrastructure/resource-contention failure, not a code defect in the PR. The build phase (compile of nixl, UCX, nixlbench, EP kernels) completed cleanly for both variants. Summary: The Root cause: SLURM cluster resource contention, not a code/test bug. Implicated commit: unknown (not a code regression; PR #1673 commit fed787e built and ran cleanly on the master variant) File: Suggested fix: Retry/re-run the job — this is transient GPU-cluster contention. To reduce recurrence: (1) stagger or serialize the two ucx-variant allocations so they don't compete for the same single exclusive GB200 node at once, (2) raise Related: none |
|
👀 Investigating |
|
🤖 CI Triage Agent — The log I fetched (the The critical evidence in the fetched log:
This is a hang, not legitimately-slow work. The largest gap (14:06 → 14:44, ~38 min, most of the remaining runtime) has zero application output. The Let me confirm there isn't a nixlbench-side hang I'm attributing wrongly — the fetched job ( Summary: The Root cause: Infrastructure/agent loss, not a test or code defect. At Implicated commit: unknown — no code commit is implicated; this is a CI agent-connectivity failure (Kubernetes pod File: unknown (Jenkins pipeline/k8s agent infrastructure; not a repo source file). Relevant log signatures: Suggested fix: Retry the build — this is almost certainly a transient agent/pod loss. If it recurs, investigate the k8s node hosting the Related: none found (issue/PR search for the agent-loss/timeout signature returned no matches). |
|
/build |
|
/ok to test aab8ecf |
|
👀 Investigating |
|
🤖 CI Triage Agent — The The actual failure is Given PR #1673 changes plugin loading to use However — per my operating rules, I must diagnose only from evidence actually present in the fetched log. The fetched console does not contain the Test CPP stage's stack trace, error line, or the crashing test name; it only contains the interleaved Python/Rust/Nixlbench output (all passing) plus the final Jenkins abort message Summary: The Root cause: A C++ test binary crashed (exit 245 = 128+... → killed by signal 11, SIGSEGV). The fetched console is interleaved parallel output that does not contain the Test CPP stage's own error/stack trace, so the exact crashing test and cause are not directly evidenced. The change under test (PR #1673) loads the UCX backend plugin with Implicated commit: Cannot confirm from logs. The PR under test is #1673 (roiedanino, "CORE: build UCX with nixl SONAME suffix, load with RTLD_DEEPBIND to avoid collisions"), tip commit [REDACTED:Hex High Entropy String]; the crash is in the CPP test suite but the specific commit/line is not in the fetched output. File: unknown — the Test CPP stage log (stack trace / crashing test) was not in the fetched console, and the CPP-specific log fetch failed (SSH key missing: Suggested fix: Retrieve the standalone Related: PR #1673 (#1673); relevant createXferReq code from commit [REDACTED:Hex High Entropy String] (#1641, "CORE/AGENT: Tune createXferReq."). Note: I diagnosed from the one log I successfully fetched (the Jenkins console), but it did not contain the Test CPP stage's error detail; please attach that stage's log to pinpoint the crash.
|
|
👀 Investigating |
|
🤖 CI Triage Agent — I have enough evidence for a confident diagnosis. Let me summarize. The crash is a SIGSEGV (exit 139) occurring during UCCL engine teardown on the first WRITE case, right after However — critically — this build/branch corresponds to PR #1673, which changes how NIXL loads UCX (custom SONAME suffix + Summary: The Root cause: A SIGSEGV in the UCCL plugin's engine-destroy path ( Implicated commit: [REDACTED:Hex High Entropy String] (PR #1673, "CORE: build UCX with nixl SONAME suffix, load with RTLD_DEEPBIND to avoid collisions"). Teardown code involved: uccl_backend.cpp File: src/plugins/uccl/uccl_backend.cpp:21-61 ( Suggested fix:
Related: PR #1673 (the change under test); PR #1151 "Fix UCCL's consistency checks"; PR #895 (original UCCL backend). none other found.
|
|
👀 Investigating |
|
🤖 CI Triage Agent — I have enough evidence to make a confident diagnosis. Let me summarize. The key facts from the log:
This is a real, non-flaky failure signature caused by the UCCL backend running in a NUMA/NIC-misconfigured state on Summary: Root cause: Not a hang and not a legitimate time-limit shortfall — the log shows continuous progress up to a human abort ("Aborted by Ovidiu Mara"). The run was crawling because the UCCL backend is severely mis-tuned on node Implicated commit: unknown — the UCCL nixlbench backend cases are the trigger, but the UCCL test-matrix / File: Suggested fix:
Related: PR #1673 (#1673) — the change under test; no existing issue found tracking the UCCL NUMA/NIC-selection performance problem. |
|
@svc-nixl The log analysis is not correct. I stopped the job because it was hanged here: |
What?
Add private-UCX loading support for the NIXL wheel by combining:
RTLD_DEEPBINDwhen loading the UCX backend plugin.RTLD_DEEPBINDduringnixl_ep_cppPython extension import.Why?
The NIXL wheel bundles Python code, NIXL core libraries, the NIXL UCX plugin, UCX core libraries, and UCX modules. In environments such as HPC-X/OpenMPI, another UCX version can already be loaded globally before NIXL is imported or before the NIXL UCX plugin is loaded.
Even if the NIXL wheel contains its own UCX libraries, normal ELF symbol resolution can bind NIXL’s UCX references to the globally loaded UCX instead. That can make NIXL silently use the wrong UCX version and can produce hard-to-debug runtime failures.
This change makes the intended private UCX path explicit and testable.
How?
Load the UCX backend plugin with
RTLD_DEEPBINDby default.NIXL_UCX_DEEPBIND.Import
nixl_ep_cppwithRTLD_DEEPBINDwhen supported by the platform.NIXL_UCX_DEEPBINDopt-out behavior.Add UCX build wiring to the container flow.
--ucx-soname-suffix <suffix>passes UCX--with-soname-suffix=<suffix>.--private-ucxis a shortcut for the NIXL private suffix.--ucx-repoallows building against a UCX branch/fork that contains the private SONAME support.Improve
wheel_add_ucx_plugins.pyso auditwheel-renamed libraries such aslibucp-nixl-<hash>.so...are mapped correctly.Add UCX backend diagnostics.
NIXL_UCX_EXPECTED_SONAMEcan be set to fail fast if the backend binds to an unexpected UCX library.Add validation helpers.
contrib/check_ucx_binding.pychecksDT_NEEDEDentries and glibc symbol binding behavior with/withoutRTLD_DEEPBIND.contrib/check_ucx_wheel_bundle.pychecks that a repaired wheel contains private UCX libraries, UCX modules, and the NIXL UCX plugin.This PR should only be merged (and perhaps reviewed) after CONFIGURE: added a configure option to add SONAME suffix openucx/ucx#11483 was merged
Summary by CodeRabbit