Remove duplicated memory_resource_tests #1451
Conversation
Durin the initial introduction of `async_resource_ref` we duplicated all tests that take a `device_memory_resource*`. Now that we have already some experience with running it in production and are moving more of the interfaces to `async resource_ref` remove those duplicated tests. closes rapidsai#1450
miscco
added
feature request
| @@ -33,6 +33,7 @@ INSTANTIATE_TEST_SUITE_P(ResourceTests, | |||
| #endif | |||
| mr_factory{"Managed", &make_managed}, | |||
| mr_factory{"Pool", &make_pool}, | |||
| mr_factory{"HostPinnedPool", &make_host_pinned_pool}, | |||
There was a problem hiding this comment.
💡 suggestion:
| mr_factory{"HostPinnedPool", &make_host_pinned_pool}, | |
| mr_factory{"HostPinned", &make_host_pinned}, | |
| mr_factory{"HostPinnedPool", &make_host_pinned_pool}, |
There was a problem hiding this comment.
This is actually not possible with the current test setup.
The tests machinery currently requires a std::function<std::shared_ptr<rmm::mr::device_memory_resource>>. However, pinned_host_memory_resource is not derived from device_memory_resource so that wont compile.
I am wary of completely rewriting the test infra in this PR.
There was a problem hiding this comment.
Oh I thought when you created device_mr_ref_tests that you had changed that machinery for the ref tests. OK, that can be a follow up.
There was a problem hiding this comment.
The issue is that we need to store the resource somewhere, as resource_ref is only a reference and now owning.
That is why I kept most of the base machinery unchanged
There was a problem hiding this comment.
OK. Seems we need to find a way to test both the (new) MR(s) that just implement the concepts and the (legacy) MRs that just derive from device_memory_resource.
There was a problem hiding this comment.
@miscco did you have further thoughts here?
Seems we need to find a way to test both the (new) MR(s) that just implement the concepts and the (legacy) MRs that just derive from device_memory_resource.
|
Ping for a second review |
|
@miscco I granted a second approval here -- do you need anything else before merging this? |
|
I was pondering whether we want to keep the old test, but we are indeed testing |
|
/merge |
During the initial introduction of
async_resource_refwe duplicated all tests that take adevice_memory_resource*.Now that we have already some experience with running it in production and are moving more of the interfaces to
async resource_refremove those duplicated tests.closes #1450
Checklist