Conversation
|
👋 Hi tomerdav! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughDoxygen now parses CUDA headers as C++ and scans the GPU API directory. The GPU type and device headers now document public transfer types, memory views, status queries, transfers, atomic operations, pointer lookup, and legacy wrappers. ChangesGPU API documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change restores GPU API reference documentation and Doxygen visibility without changing runtime behavior, so it is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
a751b6a to
5bf9507
Compare
|
🤖 CI Triage Agent — TL;DR: The build never got past Full analysisSummary: Root cause: The git checkout ran anonymously — the log prints Implicated commit: unknown — no repository commit is implicated; the failure precedes checkout of File: Jenkins job configuration for Suggested fix: Have a Jenkins admin re-attach a valid credential to the Related: none — searched issues/PRs for prior checkout-401/credential failures and found no matching reports; recent
|
|
🤖 CI Triage Agent — TL;DR: The build never got past SCM checkout — every Full analysisSummary: Root cause: The pipeline's Implicated commit: unknown — no repository content was fetched; the triggering commit 5bf9507 / merge ref cac415a is not implicated. File: unknown — failure is in the Jenkins job's SCM configuration (the Suggested fix: Have CI infra rotate and re-attach the GitHub HTTPS credential used by the sanitizer agents, and make the checkout use it explicitly instead of ambient auth:
Related: none — searching the project for this signature returned only an unrelated match (PR #401), and no existing issue tracks the 401 checkout failure.
|
|
🤖 CI Triage Agent — TL;DR: The build never got past Full analysisSummary: Stage 13 "Checkout source code" failed after 60s; all clone/fetch attempts died with Root cause: The Git SCM step runs with Two corroborating details:
Implicated commit: unknown — no repo commit is implicated; the failure precedes checkout. Suspect a change to the Jenkins agent image / job credential binding rather than a source commit. File: N/A (Jenkins job SCM configuration for Suggested fix:
Related: none found (searched the repo for this error signature; only unrelated PRs #401 and #2085 matched).
|
|
🤖 CI Triage Agent — TL;DR: The build never got past "Checkout source code" — every Full analysisSummary: Stage 13 "Checkout source code" failed after 5 retries with Root cause: The git SCM step ran with Corroboration that this is not the PR: build #556 of the same job completed "Checkout source code" successfully in 10.7 s and proceeded into the parallel "Setup docker"/"Build image" stages. Nothing in the log touches nixl source at all — no compiler, no test, no repo file was ever read. Implicated commit: none — the failure precedes checkout of any source. Commit File: not a source file — Jenkins job SCM configuration / agent pod template for Suggested fix:
Related: none — issue/PR search for this signature returned only unrelated matches (#401, #2085, #2123).
|
|
🤖 CI Triage Agent — TL;DR: The build never got past Full analysisSummary: Root cause: The Git SCM step on agent pod Implicated commit: unknown — no repo commit is implicated; the failure precedes checkout of the tree. (For context only, the most recent CI-credential-related change in history is File: Jenkins job configuration for Suggested fix: This needs a CI-admin action, not a code change:
Related: PR #2230 (trigger, docs-only — not the cause). No existing issue matches this 401 checkout signature; a search for it returned only an unrelated PR (#401).
|
|
🤖 CI Triage Agent — TL;DR: The build failed in the Full analysisSummary: Stage 414 ("Prepare", the Root cause: Infrastructure/network flake, not a code defect. The Prepare step's registry login produced:
The TLS handshake to Artifactory did not complete within the Go client's default 10s window (login started ~11:57:27, failed 11:57:39). Evidence that this is transient rather than a config/credential problem:
Implicated commit: none — [REDACTED:Hex High Entropy String] is not implicated; this is an infrastructure failure. File: the Jenkins pipeline "Prepare" step that runs Suggested fix:
Note: the Artifactory credential is correctly masked in this log ( Related: none — no existing issue/PR tracks this login flake. (Search hits #2007 and #2147 are unrelated.)
|
|
/build |
ColinNV
left a comment
There was a problem hiding this comment.
I believe device_types.cuh should have an @file.
5bf9507 to
813da51
Compare
Added a file-level Doxygen description to |
|
🤖 CI Triage Agent — TL;DR: One of the six matrix axes failed its Full analysisSummary: Root cause: Infrastructure/workspace race, not a source problem. The pipeline checks out once ("Checkout source code", stage 13): it wipes Implicated commit: none — [REDACTED:Hex High Entropy String] (PR #2230, tomerdav) is not implicated; the failure is in the CI workspace-provisioning path. File: Suggested fix:
Related: PR #2230 (build under test); #2147 (the refactor whose comments PR #2230 restores). No existing issue found for the
|
|
🤖 CI Triage Agent — TL;DR: Build #3532's "Run CPP tests" stage was SIGTERM'd (exit 143) at test 246/255 because the UCX/etcd-dependent gtests ran 5–20× slower than baseline, not because of a hang or the PR's changes — three Full analysisSummary: Stage Root cause: Wall-clock kill, but not a hang — the log shows continuous per-test progress right up to 14:56:10, and the largest inter-line gap (~2.4 min, 14:41:30→14:43:51) is exactly one test's own self-reported 144806 ms runtime. The stage overran because the suite was pathologically slow. Comparing against build #3531, which ran the identical 255-test suite on the same host earlier the same hour:
Pure-CPU tests are bit-for-bit the same duration; only agent/connection-establishing tests (UCX, etcd, metadata exchange) blow up by a fixed additive penalty. That signature is environmental contention on the shared network/IB/GPU fabric, not a code regression — and the commit under test is a docs restore on Infra telemetry corroborates the contention: Implicated commit: none — no code commit implicated; 813da51 (PR #2230, docs-only) is not the cause. File: CI job configuration for Suggested fix:
Related: PR #2230 (build trigger, docs-only); prior build #3531 on the same node for the timing baseline and the device-API test failure. Possibly relevant existing work: #1743 "TEST/GTEST: Run in single process." |
PR ai-dynamo#2147 moved the public GPU device API out of the UCX header and dropped the original comments. Put them back on the namespaced nixl::gpu API and the C-style wrappers.
Add src/api/device/gpu to INPUT, recognize *.cuh, and map .cuh as C++ so Doxygen parses the public device headers.
813da51 to
5936443
Compare
|
🤖 CI Triage Agent — TL;DR: All four Full analysisSummary: Root cause: The Dockerfile computes the PyTorch wheel index from the base image's CUDA version: This is environment-independent (it reproduces identically on x86_64 and aarch64 stages), not a node/infra issue. The failure was introduced when the base images were bumped to CUDA 13.4 while the index remained derived from Implicated commit: [REDACTED:Hex High Entropy String] — "build: bump CUDA and CI base images, stop restating them across CI (#2205)", NirWolfer (bumped File: Suggested fix: Stop deriving the index from
Note PR #2249 ("build: pin the torch wheel index instead of deriving it from CUDA_VERSION") already implements this; land it (or rebase this PR on it) to unblock the container build. Related: PR #2249 (fix in flight); PR #2205 (base image bump that exposed it); this build's PR #2230.
|
|
🤖 CI Triage Agent — TL;DR: Two of the six parallel Build variants failed in Full analysisSummary: Root cause: Git was pushed into interactive credential prompting (i.e. the anonymous HTTPS fetch was refused/redirected) and, with no tty, died instantly. Note that in the same build the tarball-based wraps worked fine in these containers ( Implicated commit: 76275cf — "Use tagged Taskflow git wrap for NIXL builds (#2121)", author bzsuni (converted taskflow to a git wrap; before this the dependency was not fetched via File: Suggested fix:
Related: PR #2121 (introduced the taskflow git wrap), PR #2230 (build under test, unaffected by the failure) |
|
🤖 CI Triage Agent — TL;DR: The two Full analysisSummary: Root cause: i.e. the HTTPS git endpoint returned an auth challenge and git had no TTY/credentials. This is environment-specific, not code-specific: in the same build the tarball fetches from GitHub work everywhere ( Implicated commit: 76275cf "Use tagged Taskflow git wrap for NIXL builds (#2121)" — bzsuni (introduced the File: Suggested fix:
Related: PR #2121 (taskflow git wrap), PR #2205 / commit [REDACTED:Hex High Entropy String] (CI base image bump), PR #2230 (the PR under test — not the cause; same failure present in #3047)
|
|
🤖 CI Triage Agent — TL;DR: All four parallel Full analysisSummary: Root cause: Unauthenticated
That mixed success/failure pattern within a single command is the signature of rate-limiting / a proxy rejecting anonymous git-over-HTTPS, not a bad URL or DNS outage (apt traffic to Implicated commit: unknown — not introduced by [REDACTED:Hex High Entropy String]; the failing File: Suggested fix:
Related: none (no existing issue for this clone failure; #1961 is the precedent for moving external fetches to an internal mirror).
|
|
🤖 CI Triage Agent — TL;DR: All four parallel container builds fail at the same Dockerfile step — Full analysisSummary: Root cause: The Dockerfile computes the PyTorch wheel index from the CUDA version of the base image:
The Implicated commit: File: Suggested fix: Stop deriving the index from export UV_INDEX="${TORCH_INDEX_URL}" && \
uv pip install --system torch torchvision torchaudioOptionally add Related: PR #2249 (pin the torch wheel index instead of deriving it from CUDA_VERSION); PR #2205 / commit [REDACTED:Hex High Entropy String] (CUDA base image bump that triggered it).
|
|
/build |
|
🤖 CI Triage Agent — TL;DR: Full analysisSummary: Stage "Run DL Python tests" (node Root cause: Both spawned ranks die inside the local overload of
This is not related to the commit under test: PR #2230 is "Restore user-facing Device API Doxygen comments" (comments only). Build #2302 (different commit, node Implicated commit: Not the PR commit. The failing code paths come from File: Suggested fix:
Related: PR #2230 (branch under test, docs-only); PR #2194 "bugfix: test: make the device API tests actually run"; prior device-API PRs #1715, #1245; also seen in build #2302 on
|
|
🤖 CI Triage Agent — TL;DR: All four container-image builds hit transient unauthenticated Full analysisSummary: Three of the four parallel "Build image" stages in Root cause: Every failure is the same signature — git got an HTTP 401 from github.com and tried to prompt for credentials with no tty:
The partial-success pattern (same URL succeeding and failing minutes apart in the same build) rules out a bad URL or a missing tag and points to github.com throttling/refusing anonymous requests. Four parallel arch/variant image builds each perform dozens of credential-less clones (grpc's recursive submodules alone are ~30 repos, several fetched twice) from one egress IP. This is not a code defect: PR #2230 is a docs-only Doxygen-comment change, and none of the failing steps touch changed files. A secondary, real bug is exposed by stage 209: UCX's Implicated commit: none — no commit in the repo caused this. Nearest related change is File: Suggested fix:
Related: PR #2230 (the PR under test — docs only, not the cause). Same-signature clone flakiness is plausible in the other currently-open container-build PRs #2250, #2231. |
Summary
nixl::gpuAPI and the C-style wrappers (nixlPut,nixlGpuGetXferStatus,nixlAtomicAdd,nixlGetPtr).@ref nixlPut/@ref nixlAtomicAddcan resolve.Test plan
src/api/device/gpu/nixl_device.cuhanddevice_types.cuhagainst the pre-device api: Layer UCX device API from GPU API #2147 comments onsrc/api/gpu/ucx/nixl_device.cuhnixlPut/nixl::gpu::putappear in the outputSummary by CodeRabbit