Skip to content

Raise error if ucx detects VRAM mem as host mem - #1393

Merged
brminich merged 1 commit into
ai-dynamo:mainfrom
x41lakazam:eshukrun/ucx-fail-register-on-vram-misclassify
Mar 6, 2026
Merged

brminich merged 1 commit into
ai-dynamo:mainfrom
x41lakazam:eshukrun/ucx-fail-register-on-vram-misclassify

Conversation

@x41lakazam

@x41lakazam x41lakazam commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

Summary

In the case where UCX is built without CUDA, ucp_mem_map silently succeeds on GPU pointers but misclassify them as host memory.
Previously, nixlUcxContext::memReg only logged a warning in this case and returned success.
This caused agent's registerMem, making the UCX backend appear fully functional for VRAM — but actual transfers would fail much later in prepXferDlist, makeXferReq, or during the transfer itself, producing confusing errors far from the root cause.

This change promotes the warning to an error and makes memReg fail immediately when VRAM is detected as host memory by UCX. The memory mapping is properly cleaned up (ucp_mem_unmap) before returning.

Without this fix, users with a UCX installation lacking CUDA support see the following behavior:
registerMem(VRAM_SEG, ...) succeeds (with only a log warning).
The backend appears available for GPU transfers.
Transfer preparation or execution fails with an unrelated error, making the root cause (missing UCX CUDA support) very hard to diagnose.

With this fix, registerMem fails immediately with a clear error message pointing to the missing UCX CUDA support, allowing callers to gracefully fall back or report the issue at the right time.

Summary by CodeRabbit

Bug Fixes

  • Enhanced error detection and resource cleanup for GPU memory registration. The system now validates configurations more strictly and immediately terminates registration with proper resource cleanup when issues are detected, preventing continuation with problematic setups.

@copy-pr-bot

copy-pr-bot Bot commented Mar 4, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Mar 4, 2026

Copy link
Copy Markdown

👋 Hi x41lakazam! 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 Mar 4, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: bc0fca59-380a-4d71-9da9-882773c1249a

📥 Commits

Reviewing files that changed from the base of the PR and between ae80d8d and bd4219e.

📒 Files selected for processing (1)
  • src/plugins/ucx/ucx_utils.cpp

📝 Walkthrough

Walkthrough

Modified error handling in nixlUcxContext::memReg to treat VRAM memory reported as HOST by UCX as a hard error. When this condition occurs, the function now logs an error, unmaps allocated memory, and returns -1 instead of issuing a warning and continuing.

Changes

Cohort / File(s) Summary
VRAM Registration Error Handling
src/plugins/ucx/ucx_utils.cpp
Enhanced error handling in memReg to convert a UCX HOST classification of VRAM memory into a fatal error with cleanup, replacing the previous warning path with immediate unmapping and -1 return code.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • yosefe
  • brminich
  • gleon99

Poem

🐰 A VRAM mixed up as HOST, what a plight,
Now caught with an error, handled just right!
We unmap and return with a swift -1,
No more warnings whispered—the fix is done! 🎯

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: raising an error when UCX detects VRAM as host memory, matching the core objective of the PR.
Description check ✅ Passed The description comprehensively covers what the change does, why it's needed (with detailed context about the problem), and the before/after behavior, aligning well with the template structure.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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 merged commit 383f9b1 into ai-dynamo:main Mar 6, 2026
29 checks passed
staryxchen pushed a commit to staryxchen/nixl that referenced this pull request Mar 7, 2026
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.

2 participants