Skip to content

Bump RDMA memory check UCX version#1445

Merged
ovidiusm merged 1 commit into
ai-dynamo:mainfrom
ovidiusm:mem-check-ucx
Mar 19, 2026
Merged

Bump RDMA memory check UCX version#1445
ovidiusm merged 1 commit into
ai-dynamo:mainfrom
ovidiusm:mem-check-ucx

Conversation

@ovidiusm

@ovidiusm ovidiusm commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

What?

Bump RDMA memory check UCX version

Why?

To be handled in UCX 1.22

Summary by CodeRabbit

  • Bug Fixes
    • Updated hardware support detection for RDMA to use the correct library version threshold, improving compatibility verification accuracy.

Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
@github-actions

Copy link
Copy Markdown

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

🚀

@ovidiusm

Copy link
Copy Markdown
Contributor Author

/build

@coderabbitai

coderabbitai Bot commented Mar 19, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

A UCX version threshold for RDMA memory type validation in the warnAboutHardwareSupportMismatch() function was updated from version 1.21 to 1.22, with the associated comment revised accordingly.

Changes

Cohort / File(s) Summary
UCX Version Threshold Update
src/plugins/ucx/ucx_utils.cpp
Updated UCX version check for RDMA memory type support from UCP_VERSION(1, 21) to UCP_VERSION(1, 22); adjusted corresponding comment.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 A version bump, so small and neat,
From twenty-one to twenty-two so sweet,
The RDMA check now knows its place,
With updated threshold, keeping pace! 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Bump RDMA memory check UCX version' is clear and specific, accurately describing the main change of raising the UCX version threshold for RDMA memory checks.
Description check ✅ Passed The description includes the required 'What?' and 'Why?' sections with appropriate content, though lacks the optional 'How?' section for additional technical context.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

@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 the current code and only fix it if needed.

Inline comments:
In `@src/plugins/ucx/ucx_utils.cpp`:
- Around line 643-644: The code incorrectly checks against a non-existent UCX
version UCP_VERSION(1,22); change the threshold in ucx_utils.cpp where
ucpVersion_ is compared (the if (ucpVersion_ >= UCP_VERSION(1, 22)) check) to
the correct existing release macro (use UCP_VERSION(1, 20) to match UCX
v1.20.0), and also update the corresponding test reference in
test/gtest/hw_warning_test.cpp (replace UCP_VERSION(1, 21) with UCP_VERSION(1,
20)) so both the runtime check and the test use the same valid UCX version
macro.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0be1637e-1428-41db-a5b8-49fcabe9be24

📥 Commits

Reviewing files that changed from the base of the PR and between 200f649 and 925e670.

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

Comment thread src/plugins/ucx/ucx_utils.cpp
@ovidiusm

Copy link
Copy Markdown
Contributor Author

/build

@ovidiusm ovidiusm merged commit 5e2ac5e into ai-dynamo:main Mar 19, 2026
15 checks passed
@ovidiusm ovidiusm deleted the mem-check-ucx branch March 19, 2026 19:15
nv-nmailhot pushed a commit that referenced this pull request Mar 19, 2026
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
ovidiusm added a commit to ovidiusm/nixl that referenced this pull request Apr 7, 2026
Signed-off-by: Ovidiu Mara <ovidium@nvidia.com>
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