-
Notifications
You must be signed in to change notification settings - Fork 197
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
Switch over to rapids-logger #2530
base: branch-25.02
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 |
/ok to test |
/ok to test |
/ok to test |
I'll be on vacation next week and this PR has downstream consequences so I've marked it as "DO NOT MERGE" until I get back, but I would love to get it reviewed in advance so that I can safely merge when I'm ready. |
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.
The changes look fine overall. Definitely good to see consolidation. Just a question (though not sure if it’s important to merge this).
# Not using rapids-cmake since we never want to find, always download. | ||
CPMAddPackage( | ||
NAME rapids_logger GITHUB_REPOSITORY vyasr/rapids-logger GIT_SHALLOW FALSE GIT_TAG feat/cuml | ||
VERSION feat/cuml |
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.
Why feat/cuml here?
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 is only because I started work on rapidsai/cuml#6162 before this PR, so that's the branch name for rapidsai/rapids-logger#1. I'll merge that PR and update the commit hash here before this PR is merged.
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.
Pre-approving because the changes look fine to me
This PR removes raft's implementation of a logger in favor of the centralized one in rapids-logger. Consumers still get the benefits of a PImpl idiom, but now that is primarily handled by using the appropriate targets (if necessary the impl header is of course still available for direct inclusion). This change paves the way for ensuring consistent fmt/spdlog (lack of) linkage throughout RAPIDS conda and wheel packages.
This PR requires rapidsai/rapids-logger#1
Contributes to rapidsai/build-planning#104