Skip to content
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

Turn resource adaptors into alias templates #1457

Open
Tracked by #1445
miscco opened this issue Feb 5, 2024 · 5 comments
Open
Tracked by #1445

Turn resource adaptors into alias templates #1457

miscco opened this issue Feb 5, 2024 · 5 comments
Assignees
Labels
cpp Pertains to C++ code feature request New feature or request

Comments

@miscco
Copy link
Contributor

miscco commented Feb 5, 2024

Currently all resource adaptors are templated on their upstream resource.

Transitioning to resource_ref this is superfluous, as we do not need to know the upstream type anymore.

However, we cannot simply remove the template argument, as that would break user code. So we need to first introduce an indirection using an alias:

template<class>
struct resource_adaptor_impl{};

template<class Upstream>
[[deprecated("Use resource_adaptor_ref instead")]] using resource_adaptor = resource_adaptor_impl<Upstream>;

using resource_adaptor_ref = resource_adaptor_impl<int>;
@harrism
Copy link
Member

harrism commented Feb 6, 2024

I'm confused by the ref here, because I don't think this is intended to be a reference. Am I correct? Is the idea to make the alias match the name we intend the caller to eventually use?

@miscco
Copy link
Contributor Author

miscco commented Feb 6, 2024

The issue is how to transition something that is a template to something that is not. I am all ears on how to switch names

@harrism
Copy link
Member

harrism commented Feb 6, 2024

I don't have any ideas but answers to my questions might give me some. :) I still have the same questions I wrote above.

@harrism
Copy link
Member

harrism commented Jul 2, 2024

I think there's an easier way to make this change (though it will take at least 2 releases).

  1. Create new (duplicate) adaptor classes (and factory functions as necessary) that don't have the upstream template parameter.
  2. Convert downstream RAPIDS libs to use the new classes instead of the old ones.
  3. Deprecate the old classes.
  4. Remove the old classes.

@harrism harrism mentioned this issue Jul 22, 2024
3 tasks
@harrism
Copy link
Member

harrism commented Aug 29, 2024

My current plan is to:

  1. Deprecate adaptor factories (done)
  2. Add resource_ref constructors to the adaptors.
  3. Add a new alias for each adaptor class as @miscco described in this issue. I suggest just drop resource from the name. e.g.
using logging_adaptor = logging_resource_adaptor_impl<device_memory_resource>;
  1. Convert RAPIDS to use the new constructors and adaptor aliases.
  2. Deprecate the old constructors and adaptors.
  3. Rename the old adaptors to the new alias names. Remove the old constructors.

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
Projects
Status: Todo
Development

No branches or pull requests

2 participants