[Python] Reduce latency of NIXL import - #1895
Conversation
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
|
👋 Hi ovidiusm! 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: Ovidiu Mara <ovidium@nvidia.com>
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — Those sibling jobs don't exist as separate Jenkins jobs (the parallel branches run within the same Let me reconsider the fetched log more carefully. The abort must have occurred in a parallel branch whose output was interleaved or in a branch (e.g., "v1.21.x" — note SLURM job 1483178 for Given my rules: I fetched a log successfully, but it shows a passing branch and does not contain the abort. I must not fabricate a root cause for the Summary: Jenkins Root cause: Undetermined from available evidence. The log I retrieved is the Implicated commit: unknown (the aborting branch's log was not available; cannot attribute). File: unknown — the SIGABRT did not originate in the fetched branch. Suggested fix: Retrieve the console log for the failing parallel branch — the UCX Related: #1895 (the PR under test), #1874 (ci: skip ucx_backend_multi under TSan; lower sanitizer timeout), #1831 (ci: stop non-etcd sanitizer stages spawning etcd/gRPC thread), #1800 (ci: export container on test failure for crash debugging). If you can point me to the |
|
👀 Investigating |
|
🤖 CI Triage Agent — I now have a complete picture. Let me do the final timing math to distinguish "hang" from "legitimately slow." The pytest-timeout is 5.0 seconds per test. Looking at the log, each single The Summary: The Jenkins Root cause: The Implicated commit: unknown for the timeout value itself (the test file was last touched by the telemetry commits [REDACTED:Hex High Entropy String] / d6ae1dc, unrelated). The 5.0s File: Suggested fix: Because UCX backend init is legitimately ~11s in this environment (not hung), raise the per-test/fixture timeout for the agent-creation tests — e.g. add Related: #1869 (Switch CI base image to pytorch + CUDA 13.3) and #1889 (ci: version the dispatcher pipeline) are the most relevant open CI PRs; none directly track this timeout. No existing issue found for the UCX-init-vs-pytest-timeout mismatch.
|
📝 WalkthroughWalkthroughThe PR removes the unconditional torch import in the Python API, adding a sys.modules-based helper for tensor type checks. It also introduces a shared nixl_meta_utils module for CUDA major version detection, used by nixl and nixl_ep backend selection, with corresponding build and packaging updates. ChangesLazy torch tensor detection in Python API
Shared CUDA major detection for NIXL meta packages
Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
|
/build |
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/bindings/python/nixl-meta/pyproject.toml.in (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate copyright year range for this modified file.
This file is modified in this PR (line 40 added) but the header still reads only
2025, while the siblingmeson.buildin the same PR already uses2025-2026.📝 Proposed fix
-# SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.Based on learnings, for modified existing build-related files, use a year range from the earliest year to the current year, e.g. "Copyright (c) 2025-2026".
🤖 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/bindings/python/nixl-meta/pyproject.toml.in` at line 1, The SPDX copyright header in this modified build-related file still uses only a single year, so update the existing copyright notice to the appropriate year range for a changed file. Use the same convention as the sibling build file and adjust the header in pyproject.toml.in so it reflects the earliest year through the current year, keeping the change limited to the copyright line.Source: Learnings
🤖 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/bindings/python/nixl-meta/pyproject.toml.in`:
- Line 1: The SPDX copyright header in this modified build-related file still
uses only a single year, so update the existing copyright notice to the
appropriate year range for a changed file. Use the same convention as the
sibling build file and adjust the header in pyproject.toml.in so it reflects the
earliest year through the current year, keeping the change limited to the
copyright line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0f8c0d9f-d235-408d-9e78-b0f7f623c594
📒 Files selected for processing (5)
src/bindings/python/nixl-meta/meson.buildsrc/bindings/python/nixl-meta/nixl/__init__.pysrc/bindings/python/nixl-meta/nixl_ep/__init__.pysrc/bindings/python/nixl-meta/nixl_meta_utils.pysrc/bindings/python/nixl-meta/pyproject.toml.in
💤 Files with no reviewable changes (1)
- src/bindings/python/nixl-meta/nixl_meta_utils.py
|
👀 Investigating |
|
🤖 CI Triage Agent — I now have enough to make a confident diagnosis. The IPC / local-transfer path ( The evidence:
This is a Summary: The Root cause: The UCCL backend's local (same-node) GPU-IPC transfer path is broken for VRAM buffers. On this single-node run both nixlbench workers are on host Implicated commit: [REDACTED:Hex High Entropy String] "Enable local xfer for UCCL backend (#1428)" — Pravein Govindan Kannan (introduced the File: src/plugins/uccl/uccl_backend.cpp:556-566 (the Suggested fix: Fix the UCCL GPU-IPC local VRAM path so the write/read vector operations actually populate and honor a valid IPC mapping (verify Related: PR #1428 ([REDACTED:Hex High Entropy String], local xfer for UCCL); PR #1151 (Fix UCCL's consistency checks); PR #1595 (unified get param). No open issue found specifically tracking this segfault.
|
|
👀 Investigating |
|
🤖 CI Triage Agent — The Dockerfile is the container the build_helper stage builds and then exports to squashfs. Now the key question: is this a build/test hang or a genuine failure? Let me re-examine the fetched log's timestamps carefully. The log I have is entirely the enroot squashfs export ("Failed to read file … creating empty file" at Regarding timestamps: there is no >2-minute silent gap in the fetched window — the "Failed to read file" lines are continuous milliseconds apart, then a ~68s mksquashfs (normal for a 6.8 GB image). So within the visible window there is no hang; but the visible window does not contain the root-cause command. Per the instructions, I must not speculate about a root cause from PR titles/source alone when the actual error line was not captured. I did successfully fetch a log, but it does not contain the failing error signature. Let me be precise and honest: I have a log, but the specific failing test/build line is absent from it. I'll report what is evidenced and flag that the causal error line wasn't captured, rather than fabricate a diagnosis. Summary: The Root cause: Not determinable from the captured log. The log window I retrieved contains only the crash-debug container-export phase (hundreds of non-fatal "Failed to read file … creating empty file" warnings from mksquashfs packing Implicated commit: unknown (cannot attribute from captured evidence; candidates on this branch include #1868 UCX v1.22.x bump, #1869 base-image switch to pytorch+CUDA 13.3, and #1896 install python bindings from source — but none is confirmed by the log). File: unknown (failing step not present in captured log; the exported container is built from Suggested fix: Retrieve the build_helper branch's earlier log (the test/compile step that produced exit code 1, before the crash-debug export at 10:08:06) — e.g. fetch the full Related: PR #1800 (ci: export container on test failure for crash debugging) explains the export seen in the log; PR #1868 (UCX v1.22.x bump, matching the branch name) is the most likely area to inspect once the real error line is obtained. |
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
|
👀 Investigating |
|
🤖 CI Triage Agent — I now have a clear picture. Here's my diagnosis. DiagnosisSummary: The Root cause: A single UCX Implicated commit: PR #1895 (branch head File: Suggested fix:
Related: PR #1895 (#1895) — the PR under test. Also worth cross-checking recent RW-lock/race changes in
|
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
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/bindings/python/nixl-meta/nixl_meta_utils.py`:
- Around line 23-97: Add unit tests for detect_cuda_major() that exercise each
staged lookup path driven by sys.modules: imported torch, imported
cuda.bindings, imported cupy, torch-on-disk via _torch_cuda_version_from_disk(),
and the last-resort torch import. Include a torch stub where version.cuda is
None to verify it correctly falls through to the later branches instead of
stopping early. Keep the tests focused on detect_cuda_major and
_torch_cuda_version_from_disk so regressions in the fallback ordering are
caught.
- Around line 58-97: The CUDA version detection in detect_cuda_major() still
falls back to importing torch on the common CPU-only path because
_torch_cuda_version_from_disk() returns None for both “no torch” and “CPU-only
wheel”; update _torch_cuda_version_from_disk() to distinguish those cases with a
sentinel so CPU-only installs stop early, and broaden the torch.version.cuda
fallback in detect_cuda_major() to handle non-ImportError import failures by
returning None.
🪄 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: 23600054-7d32-4683-bb7f-bc901e392d7b
📒 Files selected for processing (6)
src/api/python/_api.pysrc/bindings/python/nixl-meta/meson.buildsrc/bindings/python/nixl-meta/nixl/__init__.pysrc/bindings/python/nixl-meta/nixl_ep/__init__.pysrc/bindings/python/nixl-meta/nixl_meta_utils.pysrc/bindings/python/nixl-meta/pyproject.toml.in
|
/build |
|
👀 Investigating |
|
🤖 CI Triage Agent — This is very informative. The successful master run (stage 356) shows the identical UCCL test sequence completing normally — including the first UCCL READ VRAM->VRAM case (at 14:32:44), which printed "Engine destroyed" for both processes and then cleanly proceeded to the next test. The same "Error/HUP on connection" WARN appears there too, but it's benign — both processes exited. In the failing v1.22.x run (stage 318), the very first UCCL READ VRAM->VRAM case reached the same point (both "Engine destroyed", the epoll WARN at 14:30:25.594) but then one of the two Key differences between the runs:
This is a nondeterministic shutdown-race/hang in the UCCL backend engine teardown, sensitive to the UCX version. It is unrelated to PR #1895 ("[Python] Reduce latency of NIXL import"). Summary: The "Run Nixlbench tests" (UCX v1.22.x) stage was aborted after the first UCCL READ VRAM→VRAM benchmark hung during engine teardown; Jenkins SIGTERM'd it (exit 143) after ~49 min of no output. Root cause: A hang (not a timeout) in the UCCL backend's engine shutdown path. In stage 318 the benchmark result printed at 14:30:24, both workers logged "Engine destroyed" and a Implicated commit: unknown (not introduced by this PR). Most relevant prior UCCL work: [REDACTED:Hex High Entropy String] (Pravein Govindan Kannan, "UCCL: Simplify and Optimize for batch transfers") and [REDACTED:Hex High Entropy String] ("Enable local xfer for UCCL backend"). The hang is in the UCCL epoll client / accept-thread shutdown. File: UCCL backend RDMA event loop — Suggested fix:
Related: PR #1895 (unrelated to failure); UCCL history: #1271, #1428, #895.
|
What?
Speedup NIXL wheel import time
How?
Slowest dependency import was profiled to be PyTorch.
Changes:
torchonly as needed. Not all projects usingnixlusetorch.TYPE_CHECKINGto importtorchin IDEs and static analysis tools.nixlandnixl.epmodules to remove repetitionSummary by CodeRabbit
New Features
Bug Fixes