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.

Sign up for GitHub

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

Jump to bottom

[FEA] Remove factory functions from resource adaptors #1616

Open
2 of 3 tasks
mhaseeb123 opened this issue Jul 18, 2024 · 3 comments
Open
2 of 3 tasks

[FEA] Remove factory functions from resource adaptors #1616

mhaseeb123 opened this issue Jul 18, 2024 · 3 comments
Assignees
@harrism
Labels
cpp Pertains to C++ code feature request New feature or request

Comments

@mhaseeb123
Copy link
Member

mhaseeb123 commented Jul 18, 2024 •
edited by harrism
Loading

Consider removing factory functions with constructors in prefetch_resource_adaptor, statistics_resource_adaptor, logging_resource_adaptor and others in rmm.

More context at:

         question: can't this just be a constructor? > Why do we need a factory function?

Originally posted by @jrhemstad in #1608 (comment)

Tasks
Beta Give feedback
  2. Deprecate adaptor factories #1641
    cpp feature request
@mhaseeb123 mhaseeb123 changed the title Consider refactoring prefetch, statistics and other resource adaptor to use constructors instead of factory functions. [FEA] Consider refactoring prefetch, statistics and other resource adaptor to use constructors instead of factory functions. Jul 18, 2024
@vyasr vyasr mentioned this issue Jul 19, 2024
Merged
3 tasks
@harrism
Copy link
Member

harrism commented Jul 22, 2024

All of these classes already have constructors. I think what you mean is we should remove the factory functions.

@mhaseeb123 mhaseeb123 changed the title [FEA] Consider refactoring prefetch, statistics and other resource adaptor to use constructors instead of factory functions. [FEA] Consider refactoring prefetch, statistics and other resource adaptor to use constructors if not and remove factory functions. Jul 22, 2024
@mhaseeb123 mhaseeb123 changed the title [FEA] Consider refactoring prefetch, statistics and other resource adaptor to use constructors if not and remove factory functions. [FEA] Consider refactoring prefetch, statistics and other resource adaptor to use constructors and remove factory functions. Jul 22, 2024
This was referenced Jul 23, 2024
Merged
Merged
@bdice
Copy link
Contributor

bdice commented Jul 23, 2024

I opened two PRs, #1625 and #1626, that should help with this.

@harrism
Copy link
Member

harrism commented Jul 23, 2024

Added tasklist to this issue.

@harrism harrism added feature request New feature or request cpp Pertains to C++ code labels Jul 23, 2024
@harrism harrism changed the title [FEA] Consider refactoring prefetch, statistics and other resource adaptor to use constructors and remove factory functions. [FEA] Remove factory functions from resource adaptors Jul 24, 2024
rapids-bot bot pushed a commit that referenced this issue Jul 25, 2024
@bdice
Remove prefetch factory. (#1625)
8c20e14 
PR #1608 added a prefetch resource adaptor. However, per issue #1616, we want to remove the adaptor factories like `make_prefetch_adaptor` in favor of constructors with CTAD. I am removing the prefetch adaptor factory because it has not yet been released, and thus can be deleted without deprecation.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Vyas Ramasubramani (https://github.com/vyasr)
  - Rong Ou (https://github.com/rongou)
  - Mark Harris (https://github.com/harrism)

URL: #1625
rapids-bot bot pushed a commit that referenced this issue Aug 7, 2024
@bdice
Deprecate adaptor factories. (#1626)
203c904 
This PR deprecates adaptor factory functions, per issue #1616.

After some deprecation cycle, these functions can be removed in a later release.

Authors:
  - Bradley Dice (https://github.com/bdice)
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Mark Harris (https://github.com/harrism)
  - Paul Mattione (https://github.com/pmattione-nvidia)

URL: #1626
@harrism harrism self-assigned this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@harrism harrism

Labels
cpp Pertains to C++ code feature request New feature or request
Projects
RMM Project Board
Status: In Progress
Milestone
No milestone
Development

No branches or pull requests
3 participants
@harrism @bdice @mhaseeb123