Skip to content

Add managed option for RMM Pool memory resource to C API#305

Merged
rapids-bot[bot] merged 55 commits intorapidsai:branch-24.10from
ajit283:c-add-memory-pool
Sep 27, 2024
Merged

Add managed option for RMM Pool memory resource to C API#305
rapids-bot[bot] merged 55 commits intorapidsai:branch-24.10from
ajit283:c-add-memory-pool

Conversation

@ajit283
Copy link
Copy Markdown
Contributor

@ajit283 ajit283 commented Aug 27, 2024

No description provided.

raydouglass and others added 30 commits April 11, 2024 14:35
@raydouglass
Merge pull request rapidsai#79 from rapidsai/branch-24.04
decd895 
[RELEASE] v24.04
@raydouglass
REL v24.04.00 release
25385bb
@raydouglass
Merge pull request rapidsai#168 from rapidsai/branch-24.06
8a1b8b2 
[RELEASE] cuvs v24.06
@raydouglass
REL v24.06.00 release
2a2c433
@ajit283
wip
7961798
@ajit283
cleanup
764dc1e
@ajit283
Added support for different data types and device allocation in NewMa…
b0fd0b1 
…nagedTensor function.
@ajit283
bruteforce works
b47cf5b
@ajit283
refactor
1ec0c8c
@ajit283
improve resources
d5c0dc3
@ajit283
add ivf_flat, modules not working yet
cc6319e
@ajit283
sync
01af7c9
@ajit283
cleanup
ed9bf47
@ajit283
Merge branch 'rapidsai:branch-24.08' into go
4e79d8e
@ajit283
rename
d278abc
@ajit283
sync
b91721d
@ajit283
.
6b90861
@ajit283
Merge branch 'rapidsai:branch-24.08' into go
773fd94
@ajit283
sync
7cbc1f9
@ajit283
ivf_pq + cagra
46ec2f7
@ajit283
Merge branch 'branch-24.08' into go
9dcffbd
@ajit283
CI/CD attempt
f82216c
@ajit283
add distance
486d40a
@ajit283
replace string arguments
6f5c5a6
@ajit283
make library easier to use
bfdf3be
@ajit283
graph degrees
de3cea0
@ajit283
fix top-level tests
57e8dc0
@cjnolet
Merge branch 'branch-24.08' into go
ab173dc
@ajit283
renaming
2d5fb95
@ajit283
Merge branch 'go' of https://github.com/ajit283/cuvs into go
505d8dc
@ajit283 ajit283 requested review from a team as code owners August 28, 2024 13:13
@ajit283 ajit283 requested a review from raydouglass August 28, 2024 13:13
@ajit283
Copy link
Copy Markdown
Contributor Author

ajit283 commented Aug 28, 2024

Hi @cjnolet I think this implementation using make_owning_wrapper is more extensible and doesn't throw an error when the current memory resource is not a cuda_memory_resource

benfred
benfred reviewed Sep 6, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@benfred benfred left a comment

Choose a reason for hiding this comment

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

thanks for adding this!

This mostly looks good to me, left some more comments below:

Comment thread cpp/include/cuvs/core/c_api.h Outdated
Comment thread cpp/src/core/c_api.cpp Outdated
Comment thread cpp/src/core/c_api.cpp
Comment thread cpp/test/core/c_api.c Outdated
Comment thread cpp/test/core/c_api.c Outdated
@benfred
Copy link
Copy Markdown
Contributor

benfred commented Sep 6, 2024

/ok to test

@benfred
Copy link
Copy Markdown
Contributor

benfred commented Sep 9, 2024

/ok to test

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Sep 20, 2024

/ok to test

@bdice bdice removed request for a team and raydouglass September 20, 2024 17:33
bdice
bdice approved these changes Sep 20, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@bdice bdice left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I left two nonblocking comments. This PR did not require a packaging/CI review (which was requested). This PR had some complicated merge history and so it may have needed a review from some changes that were ultimately not in the final changeset. In the future you can try to deselect the packaging/CI review groups. If they are required, you won't be able to deselect them. If you can deselect them, the review wasn't really needed.

Comment thread cpp/include/cuvs/core/c_api.h
#pragma once

#include <cuda_runtime.h>
#include <stdbool.h>
Copy link
Copy Markdown
Contributor

@bdice bdice Sep 20, 2024

Choose a reason for hiding this comment

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

I'm not a C wizard -- but is this include really needed? It sounds like it's a compatibility shim for super old-school C code. https://stackoverflow.com/questions/47374356/why-use-stdbool-h-instead-of-bool

I'd be happy to learn more -- but this might have just been an include automatically added by an IDE?

Comment thread cpp/src/core/c_api.cpp
std::make_shared<rmm::mr::managed_memory_resource>());
} else {
mr = std::static_pointer_cast<rmm::mr::device_memory_resource>(
std::make_shared<rmm::mr::cuda_memory_resource>());
Copy link
Copy Markdown
Contributor

@bdice bdice Sep 20, 2024

Choose a reason for hiding this comment

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

Should we use an async memory resource by default?

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Sep 27, 2024

/ok to test

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Sep 27, 2024

/ok to test

@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Sep 27, 2024

/merge

@rapids-bot rapids-bot Bot merged commit 28ee7d2 into rapidsai:branch-24.10 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bdice bdice bdice approved these changes

+1 more reviewer

@benfred benfred benfred approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ajit283 ajit283

Labels

cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

Unstructured Data Processing
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ajit283 @cjnolet @benfred @bdice @raydouglass