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

[FEA] Remove usage of fmt #1717

Closed
vyasr opened this issue Nov 5, 2024 · 2 comments · Fixed by #1724
Closed

[FEA] Remove usage of fmt #1717

vyasr opened this issue Nov 5, 2024 · 2 comments · Fixed by #1724
Labels
? - Needs Triage Need team to review and classify feature request New feature or request

Comments

@vyasr
Copy link
Contributor

vyasr commented Nov 5, 2024

Currently rmm has some usage of the fmt library. While fmt is convenient, it causes us significant headaches due to the need to keep our fmt dependency in sync with conda-forge, and because fmt's support for header-only usage makes it very easy to accidentally leak its symbols into public DSO symbol tables (which can cause serious runtime issues if multiple libraries do this with incompatible symbols, e.g. with wheels). Most of RAPIDS's fmt usage comes transitively via spdlog, so not exposing spdlog symbols in public APIs (see rapidsai/build-planning#104) is sufficient to remove a public fmt dependency, but rmm is a special case because it also has direct usage of fmt. Fortunately, fmt can be replaced relatively easily with std::snprintf in the short term, and with std::format in the future when we upgrade to building with C++20. We should replace all existing use cases of fmt so that we can remove our public reliance on the library.

@vyasr vyasr added ? - Needs Triage Need team to review and classify feature request New feature or request labels Nov 5, 2024
@harrism
Copy link
Member

harrism commented Nov 5, 2024

So, for example in this code:

logger_->info("allocate,{},{},{}", ptr, bytes, fmt::ptr(stream.value()));

I assume it is sufficient to just drop the usage of fmt::ptr(stream.value())) and instead manually format the pointer with snprintf?

I'm trying to remember what this is for:

rmm/include/rmm/logger.hpp

Lines 141 to 142 in dbae8c0

template <>
struct fmt::formatter<rmm::detail::bytes> : fmt::ostream_formatter {};

It was added by @bdice in #1177

@vyasr
Copy link
Contributor Author

vyasr commented Nov 6, 2024

Yup, pretty much that. I believe that the code you're referring to was added to adjust for fmtlib/fmt#3318.

@harrism harrism moved this from Todo to Review in RMM Project Board Nov 13, 2024
@rapids-bot rapids-bot bot closed this as completed in 52d61c5 Nov 14, 2024
@github-project-automation github-project-automation bot moved this from Review to Done in RMM Project Board Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
? - Needs Triage Need team to review and classify feature request New feature or request
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants