Adding option to allow spdlog to be compiled.

[cjnolet]

This change allows the spdlog cmake target to be configurable so the user can choose whether it should be spdlog_header_only or just spdlog to use the compiled library. Some investigation of RAFT's long compile times showed that we can get a speedup from using spdlog's compiled library instead of header-only. We're going to pass this option through RAFT's build as well but would like @robertmaynard's thoughts on whether this is a reasonable approach given RMM is used across RAPIDS.

[cjnolet]

@robertmaynard it's unclear to me whether

1. we should just turn off the header-only by default, and
2. whether this will be able to work for an RMM which has already been installed (and may have RMM_SPDLOG_COMPILED=OFF already set)

[harrism]

Does this affect install? Need any changes for that?

Interested to know the impact of setting this when compiling all RAPIDS libraries.

[harrism]

Please add a description with motivation for this change.

[robertmaynard]

@robertmaynard it's unclear to me whether

1. we should just turn off the header-only by default, and
2. whether this will be able to work for an RMM which has already been installed (and may have RMM_SPDLOG_COMPILED=OFF already set)

This approach won't work when a consumer of rmm gets it via conda or other means where rmm is built ahead of time.
What we need to do is move this logic into the string that is used with rapids_export( FINAL_CODE_BLOCK) so that this logic is executed by consumers.

[cjnolet]

@robertmaynard what would an example of the string that we'd use in FINAL_CODE_BLOCK look like? Are you saying we'd set the RMM_SPDLOG_COMPILED variable in the FINAL_CODE_BLOCK?

[harrism]

Would like @robertmaynard to approve before merging.

[cjnolet]

Would like @robertmaynard to approve before merging.

@harrism, yeah I think the problem with my current change is that the default is going to apply to all the rapids projects, so unless someone manually builds and installs it locally, they won't be able to change the value. Ideally, the consuming libraries would be able to have different settings for this (so, for example, RAFT users could prefer to use the pre-built binary while cudf might still use header-only). At least I think that's what @robertmaynard's previous response is addressing.

I've looked at the docs for the FINAL_CODE_BLOCK but I'm still unclear as to how to use it. I'm waiting on guidance there.

[robertmaynard]

To defer the selection of the spdlog to consumers we would want the following changes:

commit 0bc2a771eb5b009638dd2927166bb4bc52d68b03 (HEAD -> imp-2304-allow_spdlog_compiled)
Author: Robert Maynard <[email protected]>
Date:   Wed Mar 29 09:37:36 2023 -0400

    Allow consuming project to specify what spdlog to use

diff --git a/CMakeLists.txt b/CMakeLists.txt
index d8670576..4fbe8d6f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -39,8 +39,6 @@ rapids_cmake_build_type(Release)
 # build options
 option(BUILD_TESTS "Configure CMake to build tests" ON)
 option(BUILD_BENCHMARKS "Configure CMake to build (google) benchmarks" OFF)
-option(RMM_HEADER_ONLY_SPDLOG "Use spdlog header-only library. Compiled version is used otherwise"
-       ON)
 set(RMM_LOGGING_LEVEL
     "INFO"
     CACHE STRING "Choose the logging level.")
@@ -82,9 +80,6 @@ endif()
 message(STATUS "RMM: Using header-only SPDLOG: ${RMM_HEADER_ONLY_SPDLOG}")
 
 target_link_libraries(rmm INTERFACE rmm::Thrust)
-target_link_libraries(rmm INTERFACE fmt::fmt-header-only)
-target_link_libraries(rmm INTERFACE $<$<BOOL:NOT ${RMM_HEADER_ONLY_SPDLOG}>:spdlog::spdlog>)
-target_link_libraries(rmm INTERFACE $<$<BOOL:${RMM_HEADER_ONLY_SPDLOG}>:spdlog::spdlog_header_only>)
 target_link_libraries(rmm INTERFACE dl)
 target_compile_features(rmm INTERFACE cxx_std_17 $<BUILD_INTERFACE:cuda_std_17>)
 
@@ -142,6 +137,10 @@ set(code_string
 if(NOT TARGET rmm::Thrust)
   thrust_create_target(rmm::Thrust FROM_OPTIONS)
 endif()
+
+option(RMM_HEADER_ONLY_SPDLOG "Use spdlog header-only library. Compiled version is used otherwise" ON)
+mark_as_advanced(RMM_HEADER_ONLY_SPDLOG)
+target_link_libraries(rmm::rmm INTERFACE $<IF:$<BOOL:${RMM_HEADER_ONLY_SPDLOG}>,spdlog::spdlog_header_only,spdlog::spdlog>)
 ]=])
 
 rapids_export(
diff --git a/benchmarks/CMakeLists.txt b/benchmarks/CMakeLists.txt
index 9dfb2c53..0d77211b 100644
--- a/benchmarks/CMakeLists.txt
+++ b/benchmarks/CMakeLists.txt
@@ -34,7 +34,7 @@ function(ConfigureBench BENCH_NAME)
                RUNTIME_OUTPUT_DIRECTORY "$<BUILD_INTERFACE:${RMM_BINARY_DIR}/gbenchmarks>"
                CUDA_ARCHITECTURES "${CMAKE_CUDA_ARCHITECTURES}"
                INSTALL_RPATH "\$ORIGIN/../../../lib")
-  target_link_libraries(${BENCH_NAME} benchmark::benchmark pthread rmm)
+  target_link_libraries(${BENCH_NAME} benchmark::benchmark pthread rmm spdlog::spdlog)
   target_compile_definitions(${BENCH_NAME}
                              PUBLIC "SPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_${RMM_LOGGING_LEVEL}")
 
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index ecb44a49..4e2e559d 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -22,7 +22,7 @@ function(ConfigureTestInternal TEST_NAME)
   add_executable(${TEST_NAME} ${ARGN})
   target_include_directories(${TEST_NAME} PRIVATE "$<BUILD_INTERFACE:${RMM_SOURCE_DIR}>")
   target_link_libraries(${TEST_NAME} GTest::gmock GTest::gtest GTest::gmock_main GTest::gtest_main
-                        pthread rmm)
+                        pthread rmm spdlog::spdlog)
   set_target_properties(
     ${TEST_NAME}
     PROPERTIES POSITION_INDEPENDENT_CODE ON

This will allow consumers to do:

# Need to specify RMM_HEADER_ONLY_SPDLOG before `find_package` so that the rmm targets get the correct
# spdlog dependency
set(RMM_HEADER_ONLY_SPDLOG OFF)
find_package(rmm REQUIRED)

I was also curious if rmm tests would benifit from using a pre-compiled spdlog, and in testing on full clean builds with zero cache hits I see using a pre-built spdlog saving me 16sec, so I added those changes to the diff

build times:

spdlog::spdlog__________________________________________
Executed in   44.47 secs    fish           external
   usr time  693.39 secs  362.00 micros  693.39 secs
   sys time   33.57 secs   15.00 micros   33.57 secs


spdlog::spdlog_headeronly_______________________________
Executed in   60.92 secs    fish           external
   usr time   21.10 mins  286.00 micros   21.10 mins
   sys time    0.76 mins   17.00 micros    0.76 mins

[bdice]

Can this retarget 23.06? I would like to avoid merging changes to spdlog just before rmm’s code freeze.

[harrism]

Agree. Moving to 23.06 in the project board.

[cjnolet]

@bdice I'm certainly not against it