rapidsai / rmm

RAPIDS Memory Manager
https://docs.rapids.ai/api/rmm/stable/
Apache License 2.0
492 stars 198 forks source link

Deprecate support for directly accessing logger #1690

Closed vyasr closed 1 month ago

vyasr commented 1 month ago

Description

Contributes to https://github.com/rapidsai/build-planning/issues/104

This PR removes support for accessing rmm's underlying spdlog logger directly.

Checklist

wence- commented 1 month ago

Setting to do not merge, I think the Cython bindings in rmm/_lib/logger.pyx also want updated.

wence- commented 1 month ago

@vyasr am I right in my belief that the cython bindings now also want to use the detail API call?

vyasr commented 1 month ago

@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.

wence- commented 1 month ago

/merge

harrism commented 1 month ago

Reminder we need an issue for the required Cython/Python changes.

vyasr commented 1 month ago

https://github.com/rapidsai/rmm/issues/1709