Python bindings for `cuda_async_memory_resource` #718

shwina · 2021-03-01T21:52:41Z

Closes #701.

…d-async-mr-python

python/rmm/_cuda/11.2/gpu.pxi

shwina · 2021-03-01T21:55:02Z

python/rmm/_lib/memory_resource.pyx

+cdef class CudaAsyncMemoryResource(DeviceMemoryResource):
+    def __cinit__(self, device=None):
+        self.c_obj.reset(
+            new cuda_async_memory_resource()


What should failure look like here?

Should we just let the C++ error propagate up and expose that directly?

Do we want to wrap this call in a try..except and re-raise with more information?

Do we want to call driverGetVersion() and duplicate the check for 11.2 in C++ and Python?

What does the C++ error look like if someone tries to create this on CUDA 11.0?

https://github.com/rapidsai/rmm/blob/branch-0.19/include/rmm/mr/device/cuda_async_memory_resource.hpp#L44-L54

We may want to improve the message here: https://github.com/rapidsai/rmm/blob/branch-0.19/include/rmm/mr/device/cuda_async_memory_resource.hpp#L53 to say that it was compiled without support instead of just the generic error message

I think this part of the macro deals specifically with CUDA version < 11.2 -- @harrism any thoughts here on a possibly more informative error message? This will directly be propagated up to Python users.

"cudaMallocAsync not supported by the version of the CUDA Toolkit used for compilation"? I don't want to say "... used to compile RMM" since RMM is header-only.

Improved the error message based on your suggestion.

You changed the wrong error message. :)

python/rmm/tests/test_rmm.py

Co-authored-by: Keith Kraus <[email protected]>

…async-mr-python

python/rmm/tests/test_rmm.py

jakirkham

Thanks Ashwin! 😄 Had a couple of questions below 🙂

python/rmm/_lib/memory_resource.pyx

python/setup.py

jakirkham

LGTM. Thanks Ashwin! 😄

python/rmm/_cuda/11.0/gpu.pxi

kkraus14 · 2021-03-02T22:02:38Z

python/rmm/tests/test_rmm.py

+
+
+@pytest.mark.skipif(
+    rmm._cuda.gpu.runtimeGetVersion() < 11020,


I think technically we need to check both the runtime and driver version here. Someone could use a newer runtime with an older driver for example. Where the call would exist but would error at runtime.

I'm a bit confused but happy to make the change.

cudaMallocAsync depends on having both libcudart >= 11.2 and libcuda >= 11.2. If say you have libcudart == 11.2 and libcuda == 11.0, then https://github.com/rapidsai/rmm/blob/branch-0.19/include/rmm/mr/device/cuda_async_memory_resource.hpp#L49 would error at runtime that there isn't a new enough driver for the feature. If you had libcudart == 11.0 and libcuda == 11.2, then https://github.com/rapidsai/rmm/blob/branch-0.19/include/rmm/mr/device/cuda_async_memory_resource.hpp#L49 would error with an invalid DeviceAttribute since it doesn't exist in libcudart 11.0.

include/rmm/mr/device/cuda_async_memory_resource.hpp

harrism · 2021-03-02T22:04:04Z

python/rmm/_lib/memory_resource.pyx

+cdef class CudaAsyncMemoryResource(DeviceMemoryResource):
+    def __cinit__(self, device=None):
+        self.c_obj.reset(
+            new cuda_async_memory_resource()


You changed the wrong error message. :)

python/setup.py

Co-authored-by: Mark Harris <[email protected]>

…async-mr-python

kkraus14 · 2021-03-03T00:42:39Z

rerun tests

kkraus14 · 2021-03-03T00:42:52Z

@gpucibot merge

mike-wendt · 2021-03-03T15:53:44Z

rerun tests

cwharris · 2021-03-03T16:33:43Z

python/setup.py

+for pxd_basename in files_to_preprocess:
+    pxi_basename = os.path.splitext(pxd_basename)[0] + ".pxi"
+    if CUDA_VERSION in cuda_version_to_pxi_dir:
+        pxi_pathname = os.path.join(
+            cwd,
+            "rmm/_cuda",
+            cuda_version_to_pxi_dir[CUDA_VERSION],
+            pxi_basename,
        )
+        pxd_pathname = os.path.join(cwd, "rmm/_cuda", pxd_basename)
+        try:
+            if filecmp.cmp(pxi_pathname, pxd_pathname):
+                # files are the same, no need to copy
+                continue
+        except FileNotFoundError:
+            # pxd_pathname doesn't exist yet
+            pass
+        shutil.copyfile(pxi_pathname, pxd_pathname)


Can we move the cuda version check outside of the loop and invert it to reduce nesting?

if CUDA_VERSION not in cuda_version_to_pxi_dir: raise TypeError(f"{CUDA_VERSION} is not supported.")

That would mean we always check, regardless of how many files we have to preprocess, so that might need to be accounted for. example: if len(files_to_preprocess) and CUDA_VERSION not in cuda_version_to_pxi_dir

Agreed that this is low hanging fruit to fix and we may as well tackle it now.

Woops, this merged before fixing this. Will raise an issue to tackle it in a follow up.

Ah sorry. I'll put in one tomorrow.

kkraus14 · 2021-03-03T17:08:44Z

rerun tests

kkraus14 · 2021-03-03T22:39:58Z

rerun tests

shwina added 3 commits March 1, 2021 16:17

Add 11.2 CUDA bindings

4ce3db0

Merge branch 'branch-0.19' of https://github.com/rapidsai/rmm into ad…

5f9e13d

…d-async-mr-python

Add bindings for CudaAsyncMemoryResource

3fe8da4

shwina requested a review from a team as a code owner March 1, 2021 21:52

github-actions bot added the Python Related to RMM Python API label Mar 1, 2021

kkraus14 reviewed Mar 1, 2021

View reviewed changes

python/rmm/_cuda/11.2/gpu.pxi Outdated Show resolved Hide resolved

shwina commented Mar 1, 2021

View reviewed changes

kkraus14 reviewed Mar 1, 2021

View reviewed changes

python/rmm/tests/test_rmm.py Outdated Show resolved Hide resolved

Update python/rmm/_cuda/11.2/gpu.pxi

710de0c

Co-authored-by: Keith Kraus <[email protected]>

kkraus14 added feature request New feature or request non-breaking Non-breaking change labels Mar 1, 2021

shwina added 2 commits March 1, 2021 17:11

test using non-default stream

d7abce8

Merge branch 'add-async-mr-python' of github.com:shwina/rmm into add-…

93ee155

…async-mr-python

shwina commented Mar 1, 2021

View reviewed changes

python/rmm/tests/test_rmm.py Show resolved Hide resolved

Add skipif

9870383

jakirkham reviewed Mar 1, 2021

View reviewed changes

python/rmm/_lib/memory_resource.pyx Outdated Show resolved Hide resolved

python/setup.py Outdated Show resolved Hide resolved

shwina added 6 commits March 2, 2021 14:43

Move to a class docstring and remove __init__

6321a7d

Maintain a single pxi for 11.0/11.1/11.2

c31688e

Use runtime, not driver version to skip test

09f1798

breakpoint

9e8d5be

Redundant join

74dd4fe

Better error message?

216b6ba

github-actions bot added the cpp Pertains to C++ code label Mar 2, 2021

clang-format

1d9032b

shwina requested a review from a team as a code owner March 2, 2021 21:14

shwina requested review from rongou and cwharris March 2, 2021 21:14

jakirkham approved these changes Mar 2, 2021

View reviewed changes

kkraus14 reviewed Mar 2, 2021

View reviewed changes

python/rmm/_cuda/11.0/gpu.pxi Show resolved Hide resolved

kkraus14 reviewed Mar 2, 2021

View reviewed changes

harrism requested changes Mar 2, 2021

View reviewed changes

kkraus14 reviewed Mar 2, 2021

View reviewed changes

python/setup.py Show resolved Hide resolved

shwina and others added 6 commits March 2, 2021 17:19

Update include/rmm/mr/device/cuda_async_memory_resource.hpp

11bec76

Co-authored-by: Mark Harris <[email protected]>

Rename dir

e967899

Rename dir

b7d607b

Only copyfile if we need to

ae1765d

Merge branch 'add-async-mr-python' of github.com:shwina/rmm into add-…

1f8cdac

…async-mr-python

Check both driver and runtime versions

57f5d1b

kkraus14 approved these changes Mar 2, 2021

View reviewed changes

Fix error message

f59cb92

harrism approved these changes Mar 3, 2021

View reviewed changes

cwharris suggested changes Mar 3, 2021

View reviewed changes

cwharris approved these changes Mar 3, 2021

View reviewed changes

rapids-bot bot merged commit 3b4a555 into rapidsai:branch-0.19 Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python bindings for `cuda_async_memory_resource` #718

Python bindings for `cuda_async_memory_resource` #718

shwina commented Mar 1, 2021

shwina Mar 1, 2021 •

edited

Loading

kkraus14 Mar 1, 2021

shwina Mar 1, 2021

kkraus14 Mar 1, 2021

shwina Mar 1, 2021

harrism Mar 2, 2021

shwina Mar 2, 2021

harrism Mar 2, 2021

jakirkham left a comment

jakirkham left a comment

kkraus14 Mar 2, 2021

shwina Mar 2, 2021

kkraus14 Mar 3, 2021

harrism Mar 2, 2021

kkraus14 commented Mar 3, 2021

kkraus14 commented Mar 3, 2021

mike-wendt commented Mar 3, 2021

cwharris Mar 3, 2021

cwharris Mar 3, 2021 •

edited

Loading

kkraus14 Mar 3, 2021

kkraus14 Mar 3, 2021

shwina Mar 3, 2021

kkraus14 commented Mar 3, 2021

kkraus14 commented Mar 3, 2021



		@pytest.mark.skipif(
		rmm._cuda.gpu.runtimeGetVersion() < 11020,

Python bindings for cuda_async_memory_resource #718

Python bindings for cuda_async_memory_resource #718

Conversation

shwina commented Mar 1, 2021

shwina Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham left a comment

Choose a reason for hiding this comment

jakirkham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkraus14 commented Mar 3, 2021

kkraus14 commented Mar 3, 2021

mike-wendt commented Mar 3, 2021

Choose a reason for hiding this comment

cwharris Mar 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkraus14 commented Mar 3, 2021

kkraus14 commented Mar 3, 2021

Python bindings for `cuda_async_memory_resource` #718

Python bindings for `cuda_async_memory_resource` #718

shwina Mar 1, 2021 •

edited

Loading

cwharris Mar 3, 2021 •

edited

Loading