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] Move error.hpp out of detail #1369

Closed
vyasr opened this issue Nov 1, 2023 · 7 comments · Fixed by #1439
Closed

[FEA] Move error.hpp out of detail #1369

vyasr opened this issue Nov 1, 2023 · 7 comments · Fixed by #1439
Assignees
@lamarrr
Labels
cpp Pertains to C++ code feature request New feature or request

Comments

@vyasr
Copy link
Contributor

vyasr commented Nov 1, 2023

Is your feature request related to a problem? Please describe.
Currently the header error.hpp is in the detail directory. However, it contains user-facing error definitions, which should not be coming from a detail header.

Describe the solution you'd like
The header should be moved out of the detail directory.

Describe alternatives you've considered
None

Additional context
There are some pieces of the header (such as the macros) that may not belong in a publicly visible header. If so, we may need to refactor a bit to avoid exposing those.
@vyasr vyasr added feature request New feature or request ? - Needs Triage Need team to review and classify labels Nov 1, 2023
@lamarrr
Copy link
Contributor

lamarrr commented Jan 16, 2024

i'd like to take on this once triaged

@harrism harrism added cpp Pertains to C++ code and removed ? - Needs Triage Need team to review and classify labels Jan 16, 2024
@harrism
Copy link
Member

harrism commented Jan 16, 2024

@lamarrr go for it, your help is welcome! I have been making related changes lately (part of #1417). Note that step one is to search all of RAPIDS repos for uses of the detail functions and if any exist, then we have to do a first PR like 1417 that removes nothing and adds public API versions of the functions. Then PRs to update the RAPIDS repos that use the detail functions / types to the new public functions / types should be submitted and merged before a final PR that removes the old detail functions / types. This is the only way to avoid breaking downstream builds.

@lamarrr
Copy link
Contributor

lamarrr commented Jan 17, 2024

@harrism there is no detail namespaced type/function in the error.hpp header. so I suppose it should be okay to move them out to a public header and leave the internal macros in the detail folder.

@harrism
Copy link
Member

harrism commented Jan 17, 2024

That's good. There are 5 files across cuDF, cuGraph and RAFT that #include <rmm/detail/error.hpp> that will need to be fixed quickly once the file is moved.

@harrism
Copy link
Member

harrism commented Jan 17, 2024

@lamarrr I think you can consider this triaged. If you open your PR after today, you should target branch-24.04 as we are entering burndown for 24.02 tonight (no new non-urgent PRs once burndown starts). See https://docs.rapids.ai/maintainers

@lamarrr
Copy link
Contributor

lamarrr commented Jan 19, 2024

@lamarrr I think you can consider this triaged. If you open your PR after today, you should target branch-24.04 as we are entering burndown for 24.02 tonight (no new non-urgent PRs once burndown starts). See https://docs.rapids.ai/maintainers

noted!

@lamarrr lamarrr mentioned this issue Jan 26, 2024
Merged
@lamarrr
Copy link
Contributor

lamarrr commented Jan 29, 2024

Once merged, I'll refractor the upstream repost to use the public API

rapids-bot bot pushed a commit that referenced this issue Jan 29, 2024
@lamarrr
Refactor error.hpp out of detail (#1439)
f259b88 
I've moved the public part of error.hpp out of the detail header as described in #1369

Fixes #1369

Authors:
  - Basit Ayantunde (https://github.com/lamarrr)
  - Lawrence Mitchell (https://github.com/wence-)
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Michael Schellenberger Costa (https://github.com/miscco)
  - Lawrence Mitchell (https://github.com/wence-)
  - Mark Harris (https://github.com/harrism)

URL: #1439
@github-project-automation github-project-automation bot moved this from Todo to Done in RMM Project Board Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lamarrr lamarrr

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

Successfully merging a pull request may close this issue.

Refactor error.hpp out of detail lamarrr/rmm
3 participants
@harrism @vyasr @lamarrr