Use new rapids-logger library #1808

vyasr · 2025-02-01T00:03:16Z

Description

This change helps completely insulate rmm (and transitively) the rest of RAPIDS from fmt and spdlog as dependencies, thereby solving a large number of issues around ABI stability, symbol visibility, package clobbering, and more. See rapidsai/build-planning#104 for more information.

Checklist

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

copy-pr-bot · 2025-02-01T00:03:20Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

vyasr · 2025-02-01T00:04:37Z

/ok to test

vyasr · 2025-02-01T00:15:56Z

/ok to test

vyasr · 2025-02-01T00:30:43Z

/ok to test

vyasr · 2025-02-02T04:16:08Z

/ok to test

vyasr · 2025-02-02T05:14:02Z

/ok to test

vyasr · 2025-02-02T05:52:44Z

/ok to test

vyasr · 2025-02-02T05:53:54Z

/ok to test

vyasr · 2025-02-02T06:14:15Z

/ok to test

vyasr · 2025-02-02T06:44:32Z

/ok to test

vyasr · 2025-02-02T07:12:12Z

/ok to test

vyasr · 2025-02-04T20:13:56Z

rapids_config.cmake

+
+set(rapids-cmake-repo "vyasr/rapids-cmake")
+set(rapids-cmake-branch "feat/rapids_logger_library")
+


This suggestion should be applied as a final change on this PR before merging.

Suggested change

set(rapids-cmake-repo "vyasr/rapids-cmake")

set(rapids-cmake-branch "feat/rapids_logger_library")

Matt711 · 2025-02-05T21:30:42Z

python/rmm/rmm/__init__.py

+try:
+    import librmm
+except ModuleNotFoundError:
+    pass
+else:
+    del librmm


What does this do?

I think it's supposed to trigger the rapids_logger.load_library() code path? We need an explanatory comment if so (or if not).

That is what the import does, yes. The del is just good practice to not litter __init__.py with objects that are not actually part of the public API since anything that is here can be used by importing rmm. IOW I didn't want to enable

import rmm rmm.librmm.${do_something}

Thanks, a comment would be nice :)

bdice · 2025-02-05T22:26:45Z

CMakeLists.txt

+target_include_directories(
+  rmm
+  INTERFACE "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
+            "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>" "$<INSTALL_INTERFACE:include>")


Why did this change?

The logger macros like RMM_LOG_TRACE are generated via this CMake function and placed into the build directory. Therefore, when building against rmm's build directory, we need to include ${BUILD_DIR}/include. That was previously being done in the rmm_logger target by the old rapids-logger code, but that target no longer exists.

bdice · 2025-02-05T22:27:05Z

CMakeLists.txt

 target_link_libraries(rmm INTERFACE CCCL::CCCL)
 target_link_libraries(rmm INTERFACE dl)
 target_link_libraries(rmm INTERFACE nvtx3::nvtx3-cpp)
+target_link_libraries(rmm INTERFACE rapids_logger::rapids_logger)


Does the order of target_link_libraries matter here?

No, the link order here is not important.

bdice · 2025-02-05T22:28:09Z

benchmarks/replay/replay.cpp

@@ -403,7 +403,7 @@ int main(int argc, char** argv)
    auto const num_threads = per_thread_events.size();

    // Uncomment to enable / change default log level
-    // rmm::logger().set_level(rmm::level_enum::trace);
+    // rmm::logger().set_level(rapids_logger::level_enum::trace);


Would this require an #include of some rapids_logger header?

To be properly IWYU, yes, although in practice including the rmm/logger.hpp header will bring in the rapids_logger symbols transitively.

ci/build_wheel_python.sh

bdice · 2025-02-05T22:29:31Z

ci/check_symbols.sh

@@ -47,17 +47,6 @@ for dso_file in ${dso_files}; do
    echo "  * WEAK: $(grep --count -E '  WEAK '    < ${symbol_file})"
    echo "  * LOCAL: $(grep --count -E '  LOCAL '  < ${symbol_file})"

-    echo "checking for 'fmt::' symbols..."


Do we really want to remove this? It seems like a good safety net to ensure we don't have fmt or spdlog symbols.

+1, I wrote the same thing in my review: #1808 (comment)

(hadn't seen your comments yet while I was reviewing)

Removing this makes sense because spdlog and fmt are no longer libraries that rmm is linking to at all. If we want a check like this, we could include it in rapids-logger binaries, but it no longer makes sense here because there is no direct linkage. Put another way, there is no more reason for these checks to be here than in cugraph, for example. The dependency is entirely transitive, so unless we think it is important to sprinkle these checks through every RAPIDS repo they don't make sense to be here any more.

bdice · 2025-02-05T22:40:44Z

include/rmm/logger.hpp

I'm confused. Some files already have #include <rmm/logger.hpp> in their includes, but this file appears to be new. How is that possible? Am I reading the diff wrong?

You are reading it correctly. Previously the entire rmm/logger.hpp file was generated at CMake configure time. With the new code rmm/logger.hpp is a persistent header that is actually maintained in the rmm project. The only file that is generated now is the rmm/logger_macros.hpp header, which comes from here.

bdice · 2025-02-05T22:44:16Z

python/rmm/rmm/__init__.py

+try:
+    import librmm
+except ModuleNotFoundError:
+    pass
+else:
+    del librmm


I think it's supposed to trigger the rapids_logger.load_library() code path? We need an explanatory comment if so (or if not).

bdice · 2025-02-05T22:45:02Z

python/rmm/rmm/librmm/_logger.pxd

@@ -16,7 +16,7 @@ from libcpp cimport bool
 from libcpp.string cimport string


-cdef extern from "rmm/logger.hpp" namespace "rmm" nogil:
+cdef extern from "rapids_logger/logger.hpp" namespace "rapids_logger" nogil:


@vyasr Is this the part where you were concerned about re-implementing Cython pxd's for rapids_logger for multiple libraries? What was the concern again? Should we ship a pxd with rapids-logger, or is it C++-only?

Yes, exactly. Right now every repo that needs to export a logger to Python needs to redeclare rapids_logger types. It turns out that is only two places though, rmm and cuml. Neither cudf nor raft expose their logger type directly to Python, so I'm OK punting on this improvement until a later date. As I mentioned when we discussed before, changing it now would probably introduce more Python packaging scaffolding than we would save in reducing pxd duplication.

Is this worth creating an issue for?

jameslamb

I left a few comments I'd like answers on before approving. I especially want to be sure I understand why rmm is picking up a runtime dependency on librmm as part of this (I think I do, but left some questions about it).

jameslamb · 2025-02-05T22:12:08Z

ci/check_symbols.sh

-        | grep -v 'std\:\:_Destroy_aux'
-    then
-        raise-symbols-found-error 'spdlog::'
-    fi


IMO we should keep this test here, at least during this period of transition while rapids-logger is changing so rapidly.

That'd give us more confidence that fmt and spdlog symbols continue not to leak into rmm's public interface, and I don't think this test is causing much maintenance burden (correct me if I'm wrong).

conda/recipes/librmm/meta.yaml

jameslamb · 2025-02-05T22:48:22Z

python/librmm/librmm/__init__.py

+    pass
+else:
+    rapids_logger.load_library()
+    del rapids_logger


If I understand correctly, it's this loading right here that leads to this PR making librmm wheels a runtime dependency of rmm wheels, right?

And that's being done instead of having rmm hold the runtime dependency on rapids-logger + do this loading in preparation for #1779?

If I'm right about that, I think we should go slightly further and align this with the way library-loading looks in other RAPIDS wheels.

Like add a librmm/load.py with a load_library() function, except all that load_library() function would do is dlopen() rapids_logger like this.

def load_library(): try: import rapids_logger rapids_logger.load_library() except ModuleNotFoundError: pass

Then in rmm/__init__.py, it'd have a function call to librmm.load_library() instead of relying on just import librmm for this purpose.

To be clear, I'm only suggesting this for consistency and to reduce the size of the changeset required for #1779. I think what you have here is functionally equivalent and will work without issue.

Yup you have understood correctly, you just came down on a different side of this than I did. I figured it would make more sense for there to not be a load_library function in librmm until there actually was a library. Note that making this change now has an additional cost because it has the transitive effect that every RAPIDS C++ "Python" package (i.e. all our C++ wheels) will also need to add a call to librmm.load_library() or they will fail to function because they all inherit a dependency on librapids_logger.so from librmm and need the library loaded. Again, I didn't think that change made sense to include until there was a real librmm.so.

If you feel strongly the other way on either of these I can change this PR.

Matt711

Thanks, the python changes look good

Matt711 · 2025-02-06T01:58:35Z

python/rmm/rmm/__init__.py

+try:
+    import librmm
+except ModuleNotFoundError:
+    pass
+else:
+    del librmm


Thanks, a comment would be nice :)

Matt711 · 2025-02-06T02:00:16Z

python/rmm/rmm/librmm/_logger.pxd

@@ -16,7 +16,7 @@ from libcpp cimport bool
 from libcpp.string cimport string


-cdef extern from "rmm/logger.hpp" namespace "rmm" nogil:
+cdef extern from "rapids_logger/logger.hpp" namespace "rapids_logger" nogil:


Is this worth creating an issue for?

vyasr added breaking Breaking change improvement Improvement / enhancement to an existing function labels Feb 1, 2025

vyasr self-assigned this Feb 1, 2025

github-actions bot added CMake Python Related to RMM Python API conda cpp Pertains to C++ code ci labels Feb 1, 2025

vyasr added 12 commits February 2, 2025 07:11

Switch to using rapids-logger as a library

6861d01

Get Python package working upto runtime loading of the DSO

3a3ecd4

Fix comment

9078b01

Remove all packaging references to fmt and spdlog

beff663

Make dependencies on rapids-logger explicit

6ac4f95

Fix version in conda meta

e50f596

Simplify naming

91777ea

Add runtime requirement on rapids-logger

6058d09

Fix auditwheel

88bef31

Fix docs

40ee9b1

Load rapids-logger wheel

7047a1e

Move load to librmm

ef563a7

vyasr added 3 commits February 2, 2025 07:11

Make sure librmm is imported by rmm

c49963a

librmm is now a runtime requirement of rmm

fd1c46b

Also install downloaded librmm wheel when testing

255d96a

vyasr force-pushed the feat/rapids_logger_library branch from a7b1c85 to 255d96a Compare February 2, 2025 07:12

vyasr marked this pull request as ready for review February 4, 2025 20:12

vyasr requested review from a team as code owners February 4, 2025 20:12

vyasr requested review from KyleFromNVIDIA, wence- and bdice February 4, 2025 20:12

vyasr commented Feb 4, 2025

View reviewed changes

Merge branch 'branch-25.04' into feat/rapids_logger_library

c9bc4a3

Matt711 reviewed Feb 5, 2025

View reviewed changes

bdice reviewed Feb 5, 2025

View reviewed changes

jameslamb reviewed Feb 5, 2025

View reviewed changes

PR review

73b89b4

vyasr requested review from jameslamb, Matt711 and bdice February 6, 2025 00:32

typo

c0e934a

Matt711 approved these changes Feb 6, 2025

View reviewed changes


		set(rapids-cmake-repo "vyasr/rapids-cmake")
		set(rapids-cmake-branch "feat/rapids_logger_library")

Use new rapids-logger library #1808

Are you sure you want to change the base?

Use new rapids-logger library #1808

Conversation

vyasr commented Feb 1, 2025

Description

Checklist

copy-pr-bot bot commented Feb 1, 2025

vyasr commented Feb 1, 2025

vyasr commented Feb 1, 2025

vyasr commented Feb 1, 2025

vyasr commented Feb 2, 2025

vyasr commented Feb 2, 2025

vyasr commented Feb 2, 2025

vyasr commented Feb 2, 2025

vyasr commented Feb 2, 2025

vyasr commented Feb 2, 2025

vyasr commented Feb 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameslamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt711 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment