-
Notifications
You must be signed in to change notification settings - Fork 200
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.
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
Deprecate and remove get_mem_info
methods from memory resources.
#1388
Closed
4 tasks done
Labels
cpp
Pertains to C++ code
feature request
New feature or request
tech debt
debt Internal clean up and improvements to reduce maintenance and technical debt in general
Comments
harrism
added
feature request
New feature or request
tech debt
debt Internal clean up and improvements to reduce maintenance and technical debt in general
cpp
Pertains to C++ code
labels
Nov 23, 2023
Merged
3 tasks
3 tasks
This was referenced Jan 18, 2024
rapids-bot bot
pushed a commit
that referenced
this issue
Jan 18, 2024
…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
This was referenced Jan 22, 2024
rapids-bot bot
pushed a commit
that referenced
this issue
Jan 23, 2024
…nfo() nonvirtual. Remove derived implementations and calls in RMM (#1430) Closes #1426 As part of #1388, this PR contributes to deprecating and removing all `get_mem_info` functionality from memory resources. This first PR makes these methods optional without deprecating them. - Makes `rmm::mr::device_memory_resource::supports_get_mem_info()` nonvirtual (and always return false) - Makes `rmm::mr::device_memory_resource::do_get_mem_info()` nonvirtual (and always return `{0, 0}`). - Removes all derived implementations of the above. - Removes all calls to the above. Authors: - Mark Harris (https://github.com/harrism) Approvers: - Bradley Dice (https://github.com/bdice) - Vyas Ramasubramani (https://github.com/vyasr) URL: #1430
rapids-bot bot
pushed a commit
to rapidsai/cudf
that referenced
this issue
Jan 24, 2024
Part of rapidsai/rmm#1388. This removes now-optional and soon-to-be deprecated functions from cuDF's custom device_memory_resource implementations: * `supports_get_mem_info()` * `do_get_mem_info()` Authors: - Mark Harris (https://github.com/harrism) Approvers: - Jason Lowe (https://github.com/jlowe) - Nghia Truong (https://github.com/ttnghia) - Vukasin Milovanovic (https://github.com/vuule) URL: #14832
3 tasks
rapids-bot bot
pushed a commit
to rapidsai/raft
that referenced
this issue
Jan 24, 2024
Part of rapidsai/rmm#1388. This removes now-optional and soon-to-be deprecated functions from cuDF's custom device_memory_resource implementations: - `supports_get_mem_info()` - `do_get_mem_info()` Authors: - Mark Harris (https://github.com/harrism) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #2108
PointKernel
pushed a commit
to PointKernel/cudf
that referenced
this issue
Jan 25, 2024
…14832) Part of rapidsai/rmm#1388. This removes now-optional and soon-to-be deprecated functions from cuDF's custom device_memory_resource implementations: * `supports_get_mem_info()` * `do_get_mem_info()` Authors: - Mark Harris (https://github.com/harrism) Approvers: - Jason Lowe (https://github.com/jlowe) - Nghia Truong (https://github.com/ttnghia) - Vukasin Milovanovic (https://github.com/vuule) URL: rapidsai#14832
rapids-bot bot
pushed a commit
that referenced
this issue
Jan 26, 2024
…s_get_mem_info(). (#1436) Closes #1427 . Part of #1388. #1430 made these functions non-virtual and removed them from all MRs and tests. This PR completes the next step of deprecating them. Merge after - rapidsai/raft#2108 - rapidsai/cudf#14832 Authors: - Mark Harris (https://github.com/harrism) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Michael Schellenberger Costa (https://github.com/miscco) - Bradley Dice (https://github.com/bdice) URL: #1436
This was referenced Jan 30, 2024
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
cpp
Pertains to C++ code
feature request
New feature or request
tech debt
debt Internal clean up and improvements to reduce maintenance and technical debt in general
device_memory_resource::get_mem_info
has always been hard to support for arbitrary memory resources. Sometimes it is impossible to calculate and so MRs have another methodsupports_get_mem_info()
that returns true ifget_mem_info()
returns useful values.We are now in the process of adopting
cuda::memory_resource
andresource_ref
concepts into RMM, and we would like to embrace the flexibility and composability they enable.cuda::memory_resource
does not provide anything likeget_mem_info()
, just like its inspirationstd::pmr::memory_resource
.I am now experimenting with defining a
host_pinned_memory_resource
using thecuda::memory_resource
concept and not deriving fromrmm::mr::device_memory_resource
. This works as an upstream for other resources, such aspool_memory_resource
, except for one detail:pool_memory_resource
relies oncudaMemGetInfo
if the initial and maximum pool sizes are not specified, but that function cannot be used for pinned host memory!I propose that we deprecate and remove the use of
supports_get_mem_info()
andget_mem_info()
. For resources likepool
, I recommend that we change them to NOT query memory sizes and make initial and maximum pool sizes required constructor parameters. While this may seem to make them harder to use, we can exposermm::detail::available_device_memory()
(which callscudaMemGetInfo
) as a utility in the public interface to use when the user knows the pool is device memory. We may also be able to provide a utility function that estimates host memory available.Once we move entirely to the
cuda::memory_resource
concept, we will be able to drop thedevice_memory_resource
base class and in that way remove the deprecatedget_mem_info
methods.See also #305
Tasks
pool_memory_resource
to require explicit initial and maximum pool sizes. Possibly allow a default of zero for initial size. #1416rmm::mr::device_memory_resource::get_mem_info()
andrmm::mr::device_memory_resource::supports_get_mem_info()
not pure virtual, remove derived implementations and all calls in RMM code including tests. #1426rmm::mr::device_memory_resource::get_mem_info() and rmm::mr::device_memory_resource::supports_get_mem_info()
. #1427rmm::mr::device_memory_resource_get_mem_info()
andrmm::mr::device_memory_resource::supports_get_mem_info()
. #1428The text was updated successfully, but these errors were encountered: