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.

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

Open
wants to merge 2 commits into
base: branch-24.12
Choose a base branch
from

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Oct 1, 2024

Description

Contributes to rapidsai/build-planning#104

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

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added breaking Breaking change improvement Improvement / enhancement to an existing function labels Oct 1, 2024
@vyasr vyasr self-assigned this Oct 1, 2024
@vyasr vyasr requested a review from a team as a code owner October 1, 2024 16:34
@vyasr vyasr requested review from harrism and bdice October 1, 2024 16:34
@github-actions github-actions bot added the cpp Pertains to C++ code label Oct 1, 2024
[[deprecated(
"Support for direct access to spdlog loggers in rmm is planned for "
"removal")]] RMM_EXPORT inline spdlog::logger&
logger()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this makes all the RMM_LOG_XXX macros produce warnings, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I can add a level of indirection to help with this.

@vyasr vyasr requested a review from wence- October 1, 2024 18:22
@wence- wence- added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Oct 2, 2024
@wence-
Copy link
Contributor

wence- commented Oct 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - DO NOT MERGE Hold off on merging; see PR for details breaking Breaking change cpp Pertains to C++ code improvement Improvement / enhancement to an existing function
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants