-
Notifications
You must be signed in to change notification settings - Fork 200
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
Deprecate support for directly accessing logger #1690
Deprecate support for directly accessing logger #1690
Conversation
Setting to do not merge, I think the Cython bindings in |
@vyasr am I right in my belief that the cython bindings now also want to use the detail API call? |
You are correct. In fact we probably need to do more than that. This PR can probably just do that, but we will need to add some public rmm APIs to do things that were previously being done by accessing the spdlog logger's APIs directly in the Cython layer. I'll work on that in a follow-up where I'll try to isolate the core APIs that rmm will need to support in order to not expose spdlog directly. I suspect that we're going to have to ask users to do some sort of unique symbol generation on their end so as to be able to generate a private set of spdlog symbols that they don't leak to consumers. That will also end up updating the tests, which for now I'm having call the detail API. I think that's a consistent approach here because in the Python layer we are exposing these other functions so we can look into providing replacements, and then exposing them in C++. For C++ users, for the moment they'll lose the direct access functionality but gain whatever replacements we add. |
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!
/merge |
Reminder we need an issue for the required Cython/Python changes. |
Description
Contributes to rapidsai/build-planning#104
This PR removes support for accessing rmm's underlying spdlog logger directly.
Checklist