-
Notifications
You must be signed in to change notification settings - Fork 209
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
Use new rapids-logger library #1808
base: branch-25.04
Are you sure you want to change the base?
Conversation
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
/ok to test |
8 similar comments
/ok to test |
/ok to test |
/ok to test |
/ok to test |
/ok to test |
/ok to test |
/ok to test |
/ok to test |
a7b1c85
to
255d96a
Compare
/ok to test |
|
||
set(rapids-cmake-repo "vyasr/rapids-cmake") | ||
set(rapids-cmake-branch "feat/rapids_logger_library") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion should be applied as a final change on this PR before merging.
set(rapids-cmake-repo "vyasr/rapids-cmake") | |
set(rapids-cmake-branch "feat/rapids_logger_library") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments I'd like answers on before approving. I especially want to be sure I understand why rmm
is picking up a runtime dependency on librmm
as part of this (I think I do, but left some questions about it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the python changes look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few small comments, mostly replies to existing threads. I have nothing else significant to add here. @vyasr Please take a pass or two through the open threads and get an approval from @jameslamb before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any additional comments, changes look good to me! I'll help review the downstream PRs next week.
Description
This change helps completely insulate rmm (and transitively) the rest of RAPIDS from fmt and spdlog as dependencies, thereby solving a large number of issues around ABI stability, symbol visibility, package clobbering, and more. See rapidsai/build-planning#104 for more information.
Checklist