Skip to content

Forward failure callback alignment#2402

Merged
rapids-bot[bot] merged 3 commits into
rapidsai:mainfrom
bdice:pr-forward-failure-callback-alignment
May 22, 2026
Merged

Forward failure callback alignment#2402
rapids-bot[bot] merged 3 commits into
rapidsai:mainfrom
bdice:pr-forward-failure-callback-alignment

Conversation

@bdice
Copy link
Copy Markdown
Collaborator

@bdice bdice commented May 17, 2026

Description

Forwards caller-provided alignment through failure_callback_resource_adaptor allocation and deallocation calls instead of always using CUDA_ALLOCATION_ALIGNMENT. This preserves alignment semantics when the adaptor is stacked with other resources.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@bdice bdice added bug Something isn't working non-breaking Non-breaking change labels May 17, 2026
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 17, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c661fb1d-08f8-4e28-95f6-c7ba59eeb7f3

📥 Commits

Reviewing files that changed from the base of the PR and between d4d4c0a and 6596034.

📒 Files selected for processing (1)
  • cpp/include/rmm/mr/detail/failure_callback_resource_adaptor_impl.hpp

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Memory allocation now correctly respects alignment parameters throughout the failure callback mechanism, ensuring consistent alignment handling across all operations.
  • Tests

    • Added tests validating alignment parameter forwarding in memory allocation operations.

Walkthrough

The PR updates failure_callback_resource_adaptor_impl to accept and forward the caller-provided alignment (defaulting to rmm::CUDA_ALLOCATION_ALIGNMENT) to upstream allocate/deallocate calls; allocate_sync/deallocate_sync defaults were updated. A new test verifies alignment forwarding using a mock resource.

Changes

Alignment parameter forwarding

Layer / File(s) Summary
Allocate and deallocate alignment forwarding
cpp/include/rmm/mr/detail/failure_callback_resource_adaptor_impl.hpp
allocate and deallocate now forward the caller-provided alignment to the upstream resource; allocate_sync/deallocate_sync default alignment changed to rmm::CUDA_ALLOCATION_ALIGNMENT.
Alignment forwarding test
cpp/tests/mr/failure_callback_mr_tests.cpp
New ForwardsAlignment test adds mock_resource.hpp, gmock aliases, and EXPECT_CALL checks that the adaptor forwards the specified alignment for allocate/deallocate.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • rapidsai/rmm#2343: Also updates wrappers to forward explicit rmm::CUDA_ALLOCATION_ALIGNMENT to underlying resources.
  • rapidsai/rmm#2330: Adds alignment forwarding through allocation call paths (device_buffer constructors).

Suggested reviewers

  • miscco
  • rongou
🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: forwarding alignment through the failure callback resource adaptor.
Description check ✅ Passed The description is directly related to the changeset, explaining the purpose of forwarding caller-provided alignment through the adaptor.
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

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

@bdice bdice moved this to In Progress in RMM Project Board May 21, 2026
@bdice bdice self-assigned this May 21, 2026
@bdice bdice marked this pull request as ready for review May 21, 2026 21:52
@bdice bdice requested a review from a team as a code owner May 21, 2026 21:52
@bdice bdice requested review from PointKernel and ttnghia May 21, 2026 21:52
@bdice
Copy link
Copy Markdown
Collaborator Author

bdice commented May 22, 2026

/merge

@rapids-bot rapids-bot Bot merged commit 9e3729d into rapidsai:main May 22, 2026
84 of 85 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in RMM Project Board May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants