[REVIEW] Remove C++ Wrappers in memory_resource_adaptors.hpp Needed by Cython

[mdemoret-nv]

Depends on #661

While working on PR #661, it looked like it was possible to remove the "owning" C++ memory resource adaptors in memory_resource_adaptors.hpp. This PR is a quick implementation of what that would look like to run through CI and get feedback.

The main driving factor of this PR is to eliminate the need for 2 layers of wrappers around every memory resource in the library. When adding new memory resources, C++ wrappers must be created in memory_resource_adaptors.hpp and Cython wrappers must be created in memory_resource.pyx, for any property/function that needs to be exposed at the python level. This removes the C++ wrappers in favor of using pythons reference counting for lifetime management.

A few notes:

1. MemoryResource was renamed DeviceMemoryResource to more closely match the C++ class names. Easily can be changed back
2. Upstream MR are kept alive by a base class UpstreamResourceAdaptor that stores a single property upstream_mr. Any MR that has an upstream, needs to inherit from this class.
3. Once the UpstreamResourceAdaptor was created, most of the work/changes were updating the Cython imports to use the C++ classes instead of the C++ wrappers.
4. This should make it easier to expose more methods/properties at the python layer in the future.

Would appreciate any feedback.

[harrism]

I think the problem, as @shwina explained, is with upstream resources.

[mdemoret-nv]

I think the problem, as @shwina explained, is with upstream resources.

I did run into a similar issue where the order of calls to __dealloc__ could get messed up in downstream MRs but was able to work around that with this line. Would be interested to see if this is what @shwina was referring too.

Ultimately this PR is just a nice to have. It wasn't very much work and removes a pain point I ran into with previous PRs: having multiple layers of wrappers for each MR. Figured I would submit it before the holiday break since we're early in the release cycle.

[shwina]

I think the approach of using __dealloc__ to ensure the destruction of the downstream resource before upstream resources will work. I think this is a wonderful improvement over the current design 😄Thanks @mdemoret-nv!

If I understand correctly, this approach assumes that a memory resource can have only a single upstream memory resource. That was not the case previously when we had HybridMemoryResource that could be composed from two upstreams. HybridMemoryResource has since been replaced by BinningMemoryResource.

If multiple upstreams is something that we might want to support, then we might have to tweak the approach a bit. In particular, we may have to eliminate UpstreamResourceAdaptor, and instead write a __dealloc__ for each DeviceMemoryResource that can have upstreams.

[shwina]

Side note: our previous hesitation with relying on __dealloc__ was that we weren't sure when exactly it would be invoked. It turns out we can safely rely on it being invoked before C/C++ attributes are destructed in Cython: cython/cython#3946

[shwina]

@mdemoret-nv According to @jrhemstad, it may be possible in the future to have multiple upstream memory resources. So we should either:

1. Rewrite UpstreamResourceAdaptor to accept a list (or some kind of sequence) of memory resources as its first argument. I'm not a fan of this approach because it needs each class to explicitly document how to pass the upstream resources.

2. Remove UpstreamResourceAdaptor entirely. Instead, each MemoryResource defines a constructor with explicit DeviceMemoryResource arguments for the upstreams. Personally, I'd prefer this as it leads to a self-documenting API.

In either case, I think we can get away with moving the definition of __dealloc__ to DeviceMemoryResource. It's guaranteed to fire before the class attributes are destroyed. In other words, the downstream memory resource is guaranteed to be destroyed before the upstream ones.

[jrhemstad]

fwiw, it's probably safe to leave handling multiple upstreams for future work. We don't have any short term plans to add resources with multiple upstreams, but we definitely shouldn't exclude ever supporting multiple upstreams.

[mdemoret-nv]

I think the approach of using __dealloc__ to ensure the destruction of the downstream resource before upstream resources will work. I think this is a wonderful improvement over the current design Thanks @mdemoret-nv!

Excellent. Glad you like it.

If I understand correctly, this approach assumes that a memory resource can have only a single upstream memory resource. That was not the case previously when we had HybridMemoryResource that could be composed from two upstreams. HybridMemoryResource has since been replaced by BinningMemoryResource.

Yes, you are correct. The current implementation of UpstreamResourceAdaptor assumes just a single upstream. The implementation of BinningMemoryResource handles multiple MRs for each bin with an additional list of MR references (see here). This was very simple to implement and mimicked the previous wrapper design using a std::vector<std::shared_ptr<device_memory_resource>>.

If multiple upstreams is something that we might want to support, then we might have to tweak the approach a bit. In particular, we may have to eliminate UpstreamResourceAdaptor, and instead write a __dealloc__ for each DeviceMemoryResource that can have upstreams.

If multiple upstreams are needed, I don't think scrapping the UpstreamResourceAdaptor completely is necessary. You could possibly have a second class MultiUpstreamResourceAdaptor that handles multiple upstreams and leave UpstreamResourceAdaptor for MRs that only require 1. In the end, I think it really depends on the functionality of how the multiple upstream MRs would be used. For example, the current MRs that use an upstream simply forward all calls to the upstream and add some additional logic on the side (logging, tracking, fixed_size). I would be interested to hear how you plan to use multiple upstream MRs in the future.

[mdemoret-nv]

Took too long writing my reply and didn't see your comments.

fwiw, it's probably safe to leave handling multiple upstreams for future work. We don't have any short term plans to add resources with multiple upstreams, but we definitely shouldn't exclude ever supporting multiple upstreams.

Sounds good to me. Adding support in the future wouldn't be hard and I think the current design with UpstreamResourceAdaptor doesn't prevent it either.

[harrism]

So happy that you found a way to manage lifetimes without that extra C++ layer! Adding MR types was such a pain before.

[cwharris]

cool.

[harrism]

Moving to 0.19...

[mdemoret-nv]

This PR is ready for a re-review. It depends on #661 and contains the changes in that PR as well for easy merging (and required for the added tests to pass)

[mdemoret-nv]

rerun tests

[mdemoret-nv]

@kkraus14 Any idea why the ubuntu 18.04 tests are failing but 16.04 and centos is passing? I tried to recreate this locally with the docker build but havent had any success.

[kkraus14]

@kkraus14 Any idea why the ubuntu 18.04 tests are failing but 16.04 and centos is passing? I tried to recreate this locally with the docker build but havent had any success.

Looks like something in one of these two tests is causing a bad CUDA state that doesn't get cleared:

rmm/tests/test_rmm.py::test_rmm_modes[False-True-numba.cuda-1-int8] PASSED [ 14%]
rmm/tests/test_rmm.py::test_rmm_modes[False-True-numba.cuda-1-int16] FAILED [ 15%]

Possibly related to the change in using a fixture to automatically reinitialize? EDIT: That's introduced in #661

[shwina]

rerun tests

[shwina]

@mdemoret-nv can we undo this change in this PR now? :)

[kkraus14]

@gpucibot merge

[mdemoret-nv]

@mdemoret-nv can we undo this change in this PR now? :)

Yup. That file no longer exists so I think it's safe to undo that change.