rapidsai / rmm

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

Create logger wrapper around spdlog that can be easily reused in other libraries #1722

Open vyasr opened 2 weeks ago

vyasr commented 2 weeks ago

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. This is a breaking change because consumers of rmm will need to link the new rmm_logger_impl target into their own libraries to get logging support. Once this gets merged, the plan is to move this implementation out of rmm into its own repository. 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). Once everything in RAPIDS is migrated to using the new logger, we will update the way that it uses spdlog to completely hide all spdlog symbols, which solves a half dozen different problems for us when it comes to packaging (symbol collision issues, ABI compatibility, conda environment conflicts, bundling of headers into conda packages, etc).

Resolves #1709

Checklist

vyasr commented 2 weeks ago

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.

vyasr commented 2 weeks ago

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.

vyasr commented 1 week ago

Since burndown starts tomorrow and this PR provides no material improvements in 24.12 while also introducing substantial extra complexity for the compatibility modes, I'm inclined to wait until we have 25.02 branches and go straight to that. As long as we're OK with breaking the user contract for logging in 25.02 (consumers now have to build the logger in a separate TU themselves, which is automated via CMake if they use it) I don't see any benefit to committing the compatibility code to the trunk. It was very useful for me to be able to switch back and forth transparently for debugging, but in the long run it's unneeded complexity.

harrism commented 1 week ago

I agree, @vyasr . Once the fmt removal PR merges a bunch of (but not all) the backwards compat ifdefs would go away anyway.

vyasr commented 1 week ago

I've merged in the latest. Once 25.02 is available I will remove the compatibility layers and then this will be ready to be taken out of draft.

vyasr commented 4 days ago

I'm concerned that this PR is making too many large changes (some deceptively so) to make sense to put in all at once. I'm going to split this into pieces, with the first just introducing the new logger class without changing anything about how spdlog is found/linked/etc. Then I'll work my way through the remaining changes in follow-ups.

harrism commented 3 days ago

I'm concerned that this PR is making too many large changes (some deceptively so) to make sense to put in all at once. I'm going to split this into pieces, with the first just introducing the new logger class without changing anything about how spdlog is found/linked/etc. Then I'll work my way through the remaining changes in follow-ups.

Agree. For example the new support for callbacks should definitely be separate. (What requirement is driving that feature addition, BTW?)

vyasr commented 1 day ago

It comes from cuml and raft, both of whose loggers currently support this functionality. My goal was to see what it would take to make drop-in replacements for all of them, and it looks like that's the only real gap. I'll do that in a separate PR though.