Skip to content

Support for prepared transfer API in nixlbench - #1713

Merged
brminich merged 8 commits into
ai-dynamo:mainfrom
iyastreb:nixlbench-make-prep-workflow
Jun 8, 2026
Merged

brminich merged 8 commits into
ai-dynamo:mainfrom
iyastreb:nixlbench-make-prep-workflow

Conversation

@iyastreb

@iyastreb iyastreb commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

What?

Added --prepared_xfer option to nixlbench

Why?

To be able to execute make+prep NIXL API in nixlbench. Currently this API is used by several LLMs, so we need this path to be profiled in NIXL

Summary by CodeRabbit

  • New Features

    • Added an option to enable a "prepared transfer" mode that caches prepared transfer descriptors and uses them when building requests.
    • Runtime configuration now reports prepared-transfer status.
  • Configuration

    • Validation prevents enabling prepared-transfer together with the memory reregistration option.
  • Stability

    • Cleanup and teardown now release cached prepared-transfer resources to avoid leaks on exit or errors.

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

👋 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.

🚀

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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 new prepared_xfer boolean config/CLI flag, validates it against reregister_mem, prints the setting, and implements an optional worker path that prepares and caches per-slot local/remote dlist handles and indices to build transfer requests; cleanup releases those cached resources.

Changes

Prepared transfer descriptor configuration and implementation

Layer / File(s) Summary
Config flag declaration and registration
benchmark/nixlbench/src/utils/utils.h, benchmark/nixlbench/src/utils/utils.cpp
Declare xferBenchConfig::prepared_xfer static boolean, register CLI flag --prepared_xfer, and initialize it to false.
Config loading, validation, and output
benchmark/nixlbench/src/utils/utils.cpp
Load prepared_xfer via NB_ARG in loadParams(), reject enabling it together with reregister_mem (error + return -1), and print the setting in printConfig().
Worker prepare and cleanup with cached dlists
benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
Add <numeric> include; extend per-slot state with prep_local_dlist, prep_remote_dlist, and indices; when enabled, prepare and cache dlists once via agent->prepXferDlist, build requests via agent->makeXferReq(...) using cached dlists and indices, and release/null cached dlists and clear indices in recycleSlot() and cleanupSlots().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • ai-dynamo/nixl#1474: Modifies xferBenchConfig validation and interacts with the same reregister_mem/prepared-xfer relationship referenced by this PR.

Suggested reviewers

  • aranadive
  • ovidiusm
  • guy-ealey-morag
  • brminich

Poem

🐰 I nibble flags in code tonight,
Prepared hops ready, snug and tight,
Dlists cached in every slot,
Requests spring out — no extra thought,
Cleanup twitches whiskers — all is right.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding support for the prepared transfer API in nixlbench.
Description check ✅ Passed The description covers What and Why sections but lacks the optional How section. The provided information is sufficient and directly addresses the PR's purpose.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

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

@brminich

brminich commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

/build

@iyastreb

iyastreb commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

/build

Comment thread benchmark/nixlbench/src/utils/utils.cpp Outdated

@benlwalker benlwalker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--prepped_xfer is missing from the option list in benchmark/nixlbench/README.md. Document it there, including how it behaves under --recreate_xfer (prep re-runs each iteration).

Comment thread benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp
@iyastreb
iyastreb requested a review from a team as a code owner June 5, 2026 12:39
@guy-ealey-morag

Copy link
Copy Markdown
Contributor

I think --prepared-xfer would be a better flag name than --prepped-xfer
(it's only 1 character longer, and uses the actual terminology)

@iyastreb

iyastreb commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

I think --prepared-xfer would be a better flag name than --prepped-xfer (it's only 1 character longer, and uses the actual terminology)

ok

@dpressle

dpressle commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

👀 Investigating Clang Format Check

@dpressle

dpressle commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

🤖 CI Triage AgentClang Format Check · commit 4029e909

I have a clear and complete picture of the failure. Here is the full diagnosis:


Summary: Clang Format Check failed on PR #1713 due to a line that is too long in benchmark/nixlbench/src/utils/utils.cpp

Root cause: A newly added NB_ARG_BOOL macro call for the prepared_xfer argument was written as a single long line (~95 characters), violating the project's clang-format column limit. The CI tool (clang-format-diff-19) detected that the line must be broken into a multi-argument style consistent with the surrounding code. The current code in the PR reads:

NB_ARG_BOOL(prepared_xfer, false, "Use prepared transfer API (prepare+make), incompatible with reregister_mem");

But clang-format requires it to be split as:

NB_ARG_BOOL(prepared_xfer,
            false,
            "Use prepared transfer API (prepare+make), incompatible with reregister_mem");

This is exactly what clang-format-diff-19 reported in the diff at benchmark/nixlbench/src/utils/utils.cpp (around line 86 relative to the diff, line ~89 in the actual file).

Implicated commit: [REDACTED:Hex High Entropy String] (PR #1713, branch nixlbench-make-prep-workflow)

File: benchmark/nixlbench/src/utils/utils.cpp — the NB_ARG_BOOL(prepared_xfer, ...) call

Suggested fix: In benchmark/nixlbench/src/utils/utils.cpp, reformat the prepared_xfer macro call to wrap its arguments across three lines, matching the style of the adjacent NB_ARG_BOOL(recreate_xfer, ...) call:

NB_ARG_BOOL(prepared_xfer,
            false,
            "Use prepared transfer API (prepare+make), incompatible with reregister_mem");

You can also run clang-format-19 -i -style=file benchmark/nixlbench/src/utils/utils.cpp locally to auto-apply the fix before pushing.

Related: PR #1713 (nixlbench-make-prep-workflow) — none found in issue tracker beyond this PR itself.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 6b1ed24c-46ce-40ff-9b9b-98cadb8a4525 in the triage console for the audit trail.

@dpressle

dpressle commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

👀 Investigating Clang Format Check

@dpressle

dpressle commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

🤖 CI Triage AgentClang Format Check · commit ae1a5bbc

The log confirms exactly what happened. The NB_ARG_BOOL(prepared_xfer, ...) line that was added in commit ae1a5bbc is missing from the file as read — that's because clang-format-diff-19 is flagging the un-reformatted version that exists in the PR diff. I have all the evidence needed.


Summary: Clang Format Check failed because a newly added NB_ARG_BOOL(prepared_xfer, ...) macro call in benchmark/nixlbench/src/utils/utils.cpp exceeds the column width limit and was written as a single long line instead of the multi-line format required by .clang-format.

Root cause: Commit ae1a5bbc (branch nixlbench-make-prep-workflow, PR #1713) added the following line to benchmark/nixlbench/src/utils/utils.cpp (around line 88):

// BEFORE (what was committed — fails clang-format check):
NB_ARG_BOOL(prepared_xfer, false, "Use prepared transfer API (prepare+make), incompatible with reregister_mem");

// AFTER (what clang-format-19 requires):
NB_ARG_BOOL(prepared_xfer,
            false,
            "Use prepared transfer API (prepare+make), incompatible with reregister_mem");

The line is too long for the project's column limit. The adjacent NB_ARG_BOOL(recreate_xfer, ...) and NB_ARG_INT32 macros in the same file already use the correct multi-line style — this one inconsistency is the sole cause of the exit code 1 from clang-format-diff-19.

Implicated commit: [REDACTED:Hex High Entropy String] (PR #1713, branch nixlbench-make-prep-workflow)

File: benchmark/nixlbench/src/utils/utils.cpp — the NB_ARG_BOOL(prepared_xfer, …) call around line 88

Suggested fix: Replace the single-line macro call with the split form that clang-format-19 expects:

NB_ARG_BOOL(prepared_xfer,
            false,
            "Use prepared transfer API (prepare+make), incompatible with reregister_mem");

The fastest way to apply this is to run clang-format-19 (or clang-format-diff-19) locally and commit the result:

# From repo root, reformat the changed files:
git diff HEAD^1 HEAD -- benchmark/nixlbench/src/utils/utils.cpp \
  | clang-format-diff-19 -p1 -style=file -i
git add benchmark/nixlbench/src/utils/utils.cpp
git commit --amend --no-edit

Related: PR #1713nixlbench-make-prep-workflow

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 500e8b4c-2a28-470b-830f-bca5191f71f9 in the triage console for the audit trail.

@iyastreb

iyastreb commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/build

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@benchmark/nixlbench/src/utils/utils.cpp`:
- Line 89: The NB_ARG_BOOL macro invocation for prepared_xfer exceeds the column
limit; reformat it by breaking the arguments onto multiple lines (similar to
adjacent macros) so each parameter is on its own line: keep
NB_ARG_BOOL(prepared_xfer, on the first line, then split false, and the string
"Use prepared transfer API (prepare+make), incompatible with reregister_mem"
onto subsequent lines, aligning commas and closing parenthesis to match
surrounding style and satisfy clang-format.
🪄 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: Pro

Run ID: da667c90-590e-42cd-a97c-7a8e8b96f4d1

📥 Commits

Reviewing files that changed from the base of the PR and between 85a0c82 and ae1a5bb.

📒 Files selected for processing (3)
  • benchmark/nixlbench/src/utils/utils.cpp
  • benchmark/nixlbench/src/utils/utils.h
  • benchmark/nixlbench/src/worker/nixl/nixl_worker.cpp

Comment thread benchmark/nixlbench/src/utils/utils.cpp Outdated
@dpressle

dpressle commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

👀 Investigating AWS NIXL Validation

@dpressle

dpressle commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🤖 CI Triage AgentAWS NIXL Validation · commit 4405ad52

I now have all the evidence needed for a complete diagnosis. Let me compile the findings.


Summary: AWS Batch job failed immediately because the wget download of libfabric-1.21.0.tar.bz2 from GitHub Releases received three consecutive HTTP 504 Gateway Time-out responses, causing the build step to abort before any NIXL code was ever compiled or tested.

Root cause: The build script .gitlab/build.sh (line 184) downloads the libfabric source tarball directly from https://github.com/ofiwg/libfabric/releases/download/v1.21.0/libfabric-1.21.0.tar.bz2 using wget --tries=3 --waitretry=5. During this CI run, GitHub's release CDN returned HTTP 504 on all three attempts (at 06:54:03, 06:54:04, and 06:54:06 UTC), with wget giving up after the third failure. The build script's set -euo pipefail then caused immediate termination, the AWS Batch job transitioned straight to FAILED, and the GHA workflow's wait_for_status detected FAILED after just 1 second — making this look like a near-instant test failure rather than a transient network issue.

The root issue is a transient external network failure (GitHub CDN 504) combined with an insufficient retry strategy (--tries=3 with only a 5-second wait between attempts is far too short for a CDN outage). There is no bug in NIXL's own code; the job never even reached the compile stage.

Implicated commit: [REDACTED:Hex High Entropy String]"NIXL: Move to C++20" (Adit Ranadive, 2026-06-02) — most recent commit touching .gitlab/build.sh, though the actual failure is a transient infra issue rather than a code regression in that commit.

File: .gitlab/build.sh:184

Suggested fix:

  1. Immediate / re-run: Simply re-trigger the CI job — this is almost certainly a transient GitHub CDN 504 that will not recur.
  2. Resilience improvement (preferred long-term fix): Increase wget's retry budget and backoff on line 184, e.g.:
    wget --tries=10 --waitretry=30 --retry-connrefused \
        -O "${TMPDIR}/libfabric-${LIBFABRIC_VERSION#v}.tar.bz2" \
        "https://github.com/ofiwg/libfabric/releases/download/${LIBFABRIC_VERSION}/libfabric-${LIBFABRIC_VERSION#v}.tar.bz2"
    Apply the same improvement to other wget --tries=3 --waitretry=5 calls in the same file (e.g. DOCA host deb on line 154, rustup-init on line 167, etc.) for consistent transient-failure tolerance.
  3. Alternative: Cache the libfabric tarball in an S3 bucket (the CI already has AWS credentials) and download from there as a primary source, falling back to GitHub only if S3 misses. This eliminates any dependency on GitHub's release CDN availability.

Related: None — no open issues or PRs match this specific CDN 504 failure pattern.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 162d38dc-05e6-4341-b54c-4f76bfdb43b7 in the triage console for the audit trail.

@iyastreb

iyastreb commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

/build

@brminich
brminich merged commit c1cd38d into ai-dynamo:main Jun 8, 2026
16 of 17 checks passed
@iyastreb
iyastreb deleted the nixlbench-make-prep-workflow branch June 8, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants