Create logger wrapper around spdlog that can be easily reused in other libraries #1722
Conversation
github-actions
bot
added
CMake
Python
vyasr
added
non-breaking
| { | ||
| std::lock_guard lock(mtx_); | ||
|
|
||
| logger->info(" Arena size: {}", rmm::detail::bytes{upstream_block_.size()}); |
There was a problem hiding this comment.
I didn't realize that we have to get rid of all uses of format placeholders. I would have thought that since these are inside of string literals they wouldn't expose anything from fmt publicly.
There was a problem hiding this comment.
Paraphrasing something I wrote offline: in principle I think it would be possible for us to keep this functionality and find a way to hide enough details of the fmt function calls to not leak anything, but it is tricky, especially given that rmm is header-only so its includes propagate to consumers, who then have similar responsibilities. The problem isn't these strings themselves, but the functions that ultimately process these strings (which must be variadic templates and therefore must be implemented, not just declared, in header files that expose the internals). I think we're better off losing some quality of debug print until we upgrade to C++20 to simplify package maintenance in the long run.
There was a problem hiding this comment.
Looks like spdlog / fmt require {} placeholders. I'd like to see an example of where a format string containing {} placeholders passed to spdlog exposes fmt symbols but a format string containing %p/%s/etc placeholders does not.
| struct bytes { | ||
| std::size_t value; ///< The size in bytes | ||
|
|
||
| #ifdef RMM_BACKWARDS_COMPATIBILITY |
There was a problem hiding this comment.
question: This code has nothing to do with spdlog or fmt. Why does it need to be ifdef'd?
There was a problem hiding this comment.
You're right that strictly speaking it doesn't, but the only reason that this was previously defined is so that you can pass these objects directly to an fmt formatter. We could leave it unconditionally defined too if we want, that would be fine.
| } | ||
| return os << size << ' ' << units.at(index); | ||
| } | ||
| #else |
There was a problem hiding this comment.
Why are these functions mutually exclusive? The implementation is the same so the operator<< should just return os << value.to_string();.
There was a problem hiding this comment.
Actually I'm experimenting with dropping this struct altogether and just having a format_bytes() function that returns a std::string.
There was a problem hiding this comment.
I think that would be quite clean, yes. I could reuse that in this PR.
| # https://cmake.org/cmake/help/latest/command/add_library.html#interface-with-sources | ||
| set(impl_target ${_RAPIDS_LOGGER_TARGET}_impl) | ||
| add_library(${impl_target} INTERFACE) | ||
| target_sources( |
There was a problem hiding this comment.
This target requires c++17 features and therefore needs to propagate that compiler requirement
There was a problem hiding this comment.
I think I've done this right, but let me know if I missed anything.
|
I should note that a lot of the complexity here is from supporting the backwards compatibility modes. I wanted to get this ready now so that we could easily test all cases and flip the switch as needed, but if it becomes too cumbersome we could just wait for 25.02 and delete the compatibility paths before merging anything. |
… flags are specific to each project embedding a logger
|
Sorry for force-pushing, I'm so used to doing that on my draft PRs that I forgot that this draft already had a few review comments. |
Description
This PR defines a new way to produce a logger wrapping spdlog. The logger's interface is declared in a template header file that can be processed by CMake to produce an interface that may be customized for placement into any project. The new implementation uses the PImpl idiom to isolate the spdlog (and transitively, fmt) dependency from the public API of the logger. The implementation is defined in an impl header. A corresponding source template file is provided that simply includes this header. All of these files are wrapped in some CMake logic for producing a custom target for a given project.
rmm leverages this new logger by requesting the creation of a logger target and a corresponding implementation. To provide a backwards-compatible usage path, a CMake option
RMM_BACKWARDS_COMPATIBILITYbreaks the PImpl by importing the implementation header into the public header. In this mode the logger module is entirely backwards-compatible with rmm's current logger module and exposes the same functions, in particular the direct access to the underlying spdlog logger. At the beginning of 25.02 we can turn off this option and instead turn on just theSUPPORTS_LOGGINGoption. This version of the code will support logging, but it will be a breaking change because consumers of rmm will need to link the newrmm_logger_impltarget into their own libraries to get logging support. As an alternative, they could compile against a version of rmm with withSUPPORTS_LOGGINGoff, in which case the public header defines a non-functional implementation. This feature is primarily provided to support usage of the new logger functionality with header-only libraries (like rmm) that do not wish to impose the compilation burden on their consumers (note that pre-built packages like conda packages will of course have this setting baked in since it is controlled by macros defined when the target is created).Assuming that this approach is acceptable, the new logger can be removed from rmm and placed either in its own repository or in rapids-cmake. At that point, the logger may also be used to completely replace logger implementations in cudf, raft, and cuml (as well as any other RAPIDS libraries that are aiming to provide their own logging implementation.
Resolves #1709
Checklist