Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the aligned_resource_adaptor to allocate bloom filter device buffers #17758

Open
wants to merge 14 commits into
base: branch-25.02
Choose a base branch
from
Open

Use the aligned_resource_adaptor to allocate bloom filter device buffers #17758

wants to merge 14 commits into from

Conversation

mhaseeb123
Copy link
Member

@mhaseeb123 mhaseeb123 commented Jan 17, 2025 •
edited
Loading

Description

Related to #17164
Related to NVIDIA/cuCollections#660

This PR creates and uses a rmm::mr::aligned_resource_adapter to allocate device buffers for bloom filter data in accordance with bloom filter alignment requirements. This PR also updates the query_bloom_filter function to use the new bloom_filter_ref constructors introduced in NVIDIA/cuCollections#660.

Checklist

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

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 17, 2025

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.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jan 17, 2025
@mhaseeb123 mhaseeb123 marked this pull request as ready for review January 17, 2025 01:53
@mhaseeb123 mhaseeb123 requested a review from a team as a code owner January 17, 2025 01:53
@mhaseeb123 mhaseeb123 added bug Something isn't working 3 - Ready for Review Ready for review by team non-breaking Non-breaking change cuco cuCollections related issue labels Jan 17, 2025
@mhaseeb123 mhaseeb123 changed the title Use an aligned mr adapter to allocate bloom filter device buffers Use an aligned_resource_adaptor to allocate bloom filter device buffers Jan 17, 2025
@mhaseeb123 mhaseeb123 added 5 - DO NOT MERGE Hold off on merging; see PR for details and removed 3 - Ready for Review Ready for review by team labels Jan 17, 2025
@mhaseeb123 mhaseeb123 marked this pull request as draft January 17, 2025 02:22
@mhaseeb123 mhaseeb123 added 2 - In Progress Currently a work in progress and removed 5 - DO NOT MERGE Hold off on merging; see PR for details labels Jan 17, 2025
@mhaseeb123 mhaseeb123 changed the title Use an aligned_resource_adaptor to allocate bloom filter device buffers 🚧 Use an aligned_resource_adaptor to allocate bloom filter device buffers Jan 17, 2025
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 17, 2025

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.

bdice
bdice approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks good to me. It surprises me that this segfaulted the compiler in certain cases. We will need to file an internal bug report for this.

@mhaseeb123
Copy link
Member Author

mhaseeb123 commented Jan 17, 2025 •
edited
Loading

Looks good to me. It surprises me that this segfaulted the compiler in certain cases. We will need to file an internal bug report for this.

This PR doesn't unfortunately fix the compiler segfault unfortunately, (NVIDIA/cuCollections#660) should do that instead. I will update this PR to use the new bloom_filter_ref constructors from NVIDIA/cuCollections#660 once the cuco is bumped rapidsai/rapids-cmake#744

@mhaseeb123 mhaseeb123 mentioned this pull request Jan 17, 2025
Open
@mhaseeb123 mhaseeb123 marked this pull request as ready for review January 17, 2025 23:44
@mhaseeb123 mhaseeb123 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jan 17, 2025
@mhaseeb123 mhaseeb123 changed the title 🚧 Use an aligned_resource_adaptor to allocate bloom filter device buffers Use the aligned_resource_adaptor to allocate bloom filter device buffers Jan 17, 2025
@mhaseeb123
Copy link
Member Author

Locally verified with test data that the bloom filter is working as expected

@mhaseeb123 mhaseeb123 added 4 - Needs Review Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Jan 18, 2025
PointKernel
PointKernel approved these changes Jan 18, 2025
View reviewed changes
Copy link
Member

@PointKernel PointKernel left a comment

Choose a reason for hiding this comment

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

I tested locally with the 12.0 pip unified container and the compilation finished without issues.

@mhaseeb123 great work! And thank you, @sleeepyjack for the prompt fix on the cuco end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PointKernel PointKernel PointKernel approved these changes

@bdice bdice bdice approved these changes

Assignees

@mhaseeb123 mhaseeb123

Labels
4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working cuco cuCollections related issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
libcudf
Status: Burndown
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mhaseeb123 @bdice @PointKernel