Require explicit pool size in `pool_memory_resource` and move some things out of detail namespace #1417

harrism · 2023-12-20T01:18:48Z

Description

Fixes #1416.

~~Deprecates existing ctors of pool_memory_resource that provide optional parameter for the initial pool size.~~
Adds new ctors that require an explicit initial pool size.
We don't yet deprecate anything in this PR because that would break builds of some RAPIDS libraries. We will follow up with PRs to cuDF, cuGraph and anything else needed to remove deprecated usages after this PR is merged.
Adds a new utility fraction_of_available_device_memory that calculates the specified fraction of free memory on the current CUDA device. This is now used in tests to provide an explicit pool size and can be used to produce the previous behavior of pool_memory_resource for consumers of the library.
Moves available_device_memory from a detail header to cuda_device.hpp so it is now publicly usable, along with the above utility.
Temporarily adds detail::available_device_memory as an alias of the above in order to keep cudf and cugraph building until we can update them.
Duplicates commonly externally used alignment functions that are currently in rmm::detail to the public rmm namespace. The detail versions will be removed after cuDF and cuGraph are updated to not use them.

Checklist

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

…e_Device_memory utility.

… that require an initial pool size.

harrism · 2023-12-20T12:01:33Z

Discovering the cudf and cugraph depend on rmm::detail::available_device_memory so should probably add an alias for that rather than just moving it to rmm::available_device_memory. Then once the dependent libs are updated we can remove the detail version.

…e_device_memory

copy-pr-bot · 2024-01-09T03:15:09Z

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.

harrism · 2024-01-09T05:48:02Z

/ok to test

harrism · 2024-01-09T06:07:08Z

/ok to test

miscco

The changes look good to me.

The one thing that I find interesting is that this removes usage of get_upstream()->get_mem_info(cuda_stream_legacy)

I am wondering whether we should deprecate that facility too

harrism · 2024-01-09T09:39:48Z

Yes we should. :) See #1388 and #1389.

harrism · 2024-01-11T01:20:09Z

/ok to test

Note that cross-linking to top-level namespace global variables or functions (as in rmm/aligned.hpp) does not work without including a namespace directive. But, I can't figure out how to make breathe play nicely with that, so let's just do this for now.

wence- · 2024-01-11T10:57:54Z

/ok to test

When resolving the xref we must resolve relative to the _current_ document, not the document of the target we are trying to link to.

wence- · 2024-01-11T15:04:19Z

/ok to test

wence- · 2024-01-11T15:05:17Z

Figured out the problem with the cross-linking to objects defined in the utilities group (cc @vyars to check the conf.py changes), so now the docs build with correct linking.

So from my point of view this is definitely good to go.

vyasr · 2024-01-11T18:28:14Z

The doc fixes look correct to me, thanks @wence-!

harrism · 2024-01-15T20:49:23Z

/merge

This PR fixes up cuSpatial to avoid usage that will soon be deprecated in RMM. Depends on rapidsai/rmm#1417 Fixes #1318 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Wang (https://github.com/isVoid) URL: #1319

…ludes (#2088) This PR fixes up RAFT to avoid usage that will soon be deprecated in RMM. Depends on rapidsai/rmm#1417 Fixes #2087 Authors: - Mark Harris (https://github.com/harrism) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2088

This PR fixes up cuDF to avoid usage that will soon be deprecated in RMM. Depends on rapidsai/rmm#1417 Fixes #14658 Authors: - Mark Harris (https://github.com/harrism) - Yunsong Wang (https://github.com/PointKernel) - Nghia Truong (https://github.com/ttnghia) Approvers: - Nghia Truong (https://github.com/ttnghia) - Yunsong Wang (https://github.com/PointKernel) URL: #14741

This PR fixes up cuGraph to avoid usage that will soon be deprecated in RMM. Depends on rapidsai/rmm#1417 Fixes #4066 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Chuck Hastings (https://github.com/ChuckHastings) URL: #4086

…ilities, and optional pool_memory_resource initial size (#1424) Follow-on to #1417, this PR deprecates the following: - `rmm::detail::available_device_memory` in favor of rmm::available_device_memory - `rmm::detail::is_aligned`, `rmm::detail::align_up` and related alignment utility functions in favor of the `rmm::` top level namespace versions. - The `rmm::pool_memory_resource` constructors that take an optional initial size parameter. Should be merged after the following: - rapidsai/cugraph#4086 - rapidsai/cudf#14741 - rapidsai/raft#2088 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Michael Schellenberger Costa (https://github.com/miscco) - Rong Ou (https://github.com/rongou) URL: #1424

…ool_memory_resource`. (#1392) Depends on #1417 Adds a new `host_pinned_memory_resource` that implements the new `cuda::mr::memory_resource` and `cuda::mr::async_memory_resource` concepts which makes it usable as an upstream MR for `rmm::mr::device_memory_resource`. Also tests a pool made with this new MR as the upstream. Note that the tests explicitly set the initial and maximum pool sizes as using the defaults does not currently work. See #1388 . Closes #618 Authors: - Mark Harris (https://github.com/harrism) - Lawrence Mitchell (https://github.com/wence-) Approvers: - Michael Schellenberger Costa (https://github.com/miscco) - Alessandro Bellina (https://github.com/abellina) - Lawrence Mitchell (https://github.com/wence-) - Jake Hemstad (https://github.com/jrhemstad) - Bradley Dice (https://github.com/bdice) URL: #1392

`rmm::available_device_memory` was added and the former `rmm::detail::available_device_memory` was deprecated in #1417. This PR removes the deprecated function. Closes #1425 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Rong Ou (https://github.com/rongou) - Bradley Dice (https://github.com/bdice) URL: #1438

harrism added 3 commits December 19, 2023 01:07

Add new util to get a fraction of available device mem, move availabl…

c43a8c1

…e_Device_memory utility.

Deprecate old pool_mr ctors (optional initial size) and add new ctors…

d238daa

… that require an initial pool size.

Update all tests and resources to use new pool ctors and util

3d65d4c

harrism requested a review from a team as a code owner December 20, 2023 01:18

harrism requested review from wence- and jrhemstad December 20, 2023 01:18

github-actions bot added the cpp Pertains to C++ code label Dec 20, 2023

harrism added breaking Breaking change improvement Improvement / enhancement to an existing function and removed cpp Pertains to C++ code labels Dec 20, 2023

harrism self-assigned this Dec 20, 2023

harrism added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Dec 20, 2023

harrism added 2 commits December 20, 2023 03:13

Rename fraction_of_free_device_memory to percent_of_free_device_memory

66d85b4

clang-tidy Ignore 50 and 100 magic numbers

265de9b

github-actions bot added the cpp Pertains to C++ code label Dec 20, 2023

harrism added 3 commits December 20, 2023 04:01

Remove straggler includes of removed file.

0be364b

Merge branch 'branch-24.02' into fea-explicit-initial-pool-size

266afa9

Another missed include.

5d66f40

harrism added 2 commits January 8, 2024 17:22

Add detail::available_device_memory back as an alias of rmm::availabl…

fae5b73

…e_device_memory

merge branch 24.02

92c0653

copyright

2acf759

harrism removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jan 9, 2024

harrism mentioned this pull request Jan 9, 2024

Add a host-pinned memory resource that can be used as upstream for pool_memory_resource. #1392

Merged

3 tasks

document (and deprecate) available_device_memory alias

782ff55

miscco approved these changes Jan 9, 2024

View reviewed changes

docs: Fix custom handler for missing references

4ae13fc

When resolving the xref we must resolve relative to the _current_ document, not the document of the target we are trying to link to.

rapids-bot bot merged commit 64aa941 into rapidsai:branch-24.02 Jan 15, 2024
48 checks passed

This was referenced Jan 16, 2024

Deprecate detail::available_device_memory, most detail/aligned.hpp utilities, and optional pool_memory_resource initial size #1424

Merged

[FEA] Move error.hpp out of detail #1369

Closed

harrism mentioned this pull request Jan 24, 2024

Remove deprecated rmm::detail::available_device_memory #1438

Merged

3 tasks

This was referenced Jan 30, 2024

Update RAPIDS to use cuda::mr::async_resource_ref rapidsai/build-planning#16

Open

[FEA] Refactor RMM in terms of cuda::mr::memory_resource #1443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require explicit pool size in `pool_memory_resource` and move some things out of detail namespace #1417

Require explicit pool size in `pool_memory_resource` and move some things out of detail namespace #1417

harrism commented Dec 20, 2023 •

edited

Loading

harrism commented Dec 20, 2023

copy-pr-bot bot commented Jan 9, 2024

harrism commented Jan 9, 2024

harrism commented Jan 9, 2024

miscco left a comment

harrism commented Jan 9, 2024

harrism commented Jan 11, 2024

wence- commented Jan 11, 2024

wence- commented Jan 11, 2024

wence- commented Jan 11, 2024

vyasr commented Jan 11, 2024

harrism commented Jan 15, 2024

Require explicit pool size in pool_memory_resource and move some things out of detail namespace #1417

Require explicit pool size in pool_memory_resource and move some things out of detail namespace #1417

Conversation

harrism commented Dec 20, 2023 • edited Loading

Description

Checklist

harrism commented Dec 20, 2023

copy-pr-bot bot commented Jan 9, 2024

harrism commented Jan 9, 2024

harrism commented Jan 9, 2024

miscco left a comment

Choose a reason for hiding this comment

harrism commented Jan 9, 2024

harrism commented Jan 11, 2024

wence- commented Jan 11, 2024

wence- commented Jan 11, 2024

wence- commented Jan 11, 2024

vyasr commented Jan 11, 2024

harrism commented Jan 15, 2024

Require explicit pool size in `pool_memory_resource` and move some things out of detail namespace #1417

Require explicit pool size in `pool_memory_resource` and move some things out of detail namespace #1417

harrism commented Dec 20, 2023 •

edited

Loading