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.

Sign up for GitHub

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

Jump to bottom

Update fmt (to 11.0.2) and spdlog (to 1.14.1). #16806

Merged
merged 21 commits into from
Sep 23, 2024
Merged

Update fmt (to 11.0.2) and spdlog (to 1.14.1). #16806

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1a3e4b7
Update fmt (to 11.0.2) and spdlog (to 1.14.1).
jameslamb Sep 16, 2024
6bc98d2
use RMM CI artifacts
jameslamb Sep 16, 2024
d3b87bc
Merge branch 'branch-24.10' into fmt-and-spdlog
jameslamb Sep 17, 2024
c9980ab
test if spdlog export is still needed
jameslamb Sep 18, 2024
b294193
Merge branch 'branch-24.10' into fmt-and-spdlog
jameslamb Sep 18, 2024
13fd917
move rapids-cmake overrides [skip ci]
jameslamb Sep 18, 2024
3a3e24a
stop exporting spdlog
jameslamb Sep 18, 2024
8d1bac6
test some builds in CI
jameslamb Sep 18, 2024
2235405
workflow branch
jameslamb Sep 18, 2024
b5ab6e5
merge branch-24.10, only run builds
jameslamb Sep 18, 2024
0a71521
comment out static-configure
jameslamb Sep 18, 2024
a1fce83
run all CI
jameslamb Sep 20, 2024
f962642
Merge branch 'branch-24.10' into fmt-and-spdlog
jameslamb Sep 20, 2024
9ba1063
cmakelint
jameslamb Sep 20, 2024
058b0d3
export spdlog and mark it as an INTERFACE dependency of cudf::cudf
jameslamb Sep 20, 2024
b9cdb5d
Merge branch 'branch-24.10' of github.com:rapidsai/cudf into fmt-and-…
jameslamb Sep 20, 2024
ab6d345
make spdlog::spdlog_header_only a PUBLIC dependency of cudf::cudf
jameslamb Sep 20, 2024
9568b98
Merge branch 'branch-24.10' into fmt-and-spdlog
jameslamb Sep 23, 2024
630ce51
remove testing-only changes [skip ci]
jameslamb Sep 23, 2024
c9a3395
Merge branch 'branch-24.10' of github.com:rapidsai/cudf into fmt-and-…
jameslamb Sep 23, 2024
1da86f7
remove testing-only changes [skip ci]
jameslamb Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ci/build_cpp.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ export CMAKE_GENERATOR=Ninja

rapids-print-env

LIBRMM_CHANNEL=$(rapids-get-pr-conda-artifact rmm 1678 cpp)

rapids-logger "Begin cpp build"

# With boa installed conda build forward to boa
RAPIDS_PACKAGE_VERSION=$(rapids-generate-version) rapids-conda-retry mambabuild \
--channel "${LIBRMM_CHANNEL}" \
conda/recipes/libcudf

rapids-upload-conda-to-s3 cpp
12 changes: 12 additions & 0 deletions ci/build_python.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ rapids-generate-version > ./VERSION
rapids-logger "Begin py build"

CPP_CHANNEL=$(rapids-download-conda-from-s3 cpp)
LIBRMM_CHANNEL=$(rapids-get-pr-conda-artifact rmm 1678 cpp)
RMM_CHANNEL=$(rapids-get-pr-conda-artifact rmm 1678 python)

# TODO: Remove `--no-test` flag once importing on a CPU
# node works correctly
Expand All @@ -26,30 +28,40 @@ CPP_CHANNEL=$(rapids-download-conda-from-s3 cpp)
RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
--no-test \
--channel "${CPP_CHANNEL}" \
--channel "${LIBRMM_CHANNEL}" \
--channel "${RMM_CHANNEL}" \
conda/recipes/pylibcudf

RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
--no-test \
--channel "${CPP_CHANNEL}" \
--channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" \
--channel "${LIBRMM_CHANNEL}" \
--channel "${RMM_CHANNEL}" \
conda/recipes/cudf

RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
--no-test \
--channel "${CPP_CHANNEL}" \
--channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" \
--channel "${LIBRMM_CHANNEL}" \
--channel "${RMM_CHANNEL}" \
conda/recipes/dask-cudf

RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
--no-test \
--channel "${CPP_CHANNEL}" \
--channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" \
--channel "${LIBRMM_CHANNEL}" \
--channel "${RMM_CHANNEL}" \
conda/recipes/cudf_kafka

RAPIDS_PACKAGE_VERSION=$(head -1 ./VERSION) rapids-conda-retry mambabuild \
--no-test \
--channel "${CPP_CHANNEL}" \
--channel "${RAPIDS_CONDA_BLD_OUTPUT_DIR}" \
--channel "${LIBRMM_CHANNEL}" \
--channel "${RMM_CHANNEL}" \
conda/recipes/custreamz


Expand Down
3 changes: 3 additions & 0 deletions ci/test_cpp_common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@ set -euo pipefail
rapids-logger "Generate C++ testing dependencies"

ENV_YAML_DIR="$(mktemp -d)"
LIBRMM_CHANNEL=$(rapids-get-pr-conda-artifact rmm 1678 cpp)

rapids-dependency-file-generator \
--output conda \
--file-key test_cpp \
--prepend-channel "${LIBRMM_CHANNEL}" \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch)" | tee "${ENV_YAML_DIR}/env.yaml"

rapids-mamba-retry env create --yes -f "${ENV_YAML_DIR}/env.yaml" -n test
Expand All @@ -31,6 +33,7 @@ rapids-print-env

rapids-mamba-retry install \
--channel "${CPP_CHANNEL}" \
--channel "${LIBRMM_CHANNEL}" \
libcudf libcudf_kafka libcudf-tests libcudf-example

rapids-logger "Check GPU usage"
Expand Down
7 changes: 7 additions & 0 deletions ci/test_python_common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@ rapids-logger "Generate Python testing dependencies"

ENV_YAML_DIR="$(mktemp -d)"

LIBRMM_CHANNEL=$(rapids-get-pr-conda-artifact rmm 1678 cpp)
RMM_CHANNEL=$(rapids-get-pr-conda-artifact rmm 1678 python)

rapids-dependency-file-generator \
--output conda \
--file-key test_python \
--prepend-channel "${LIBRMM_CHANNEL}" \
--prepend-channel "${RMM_CHANNEL}" \
--matrix "cuda=${RAPIDS_CUDA_VERSION%.*};arch=$(arch);py=${RAPIDS_PY_VERSION};dependencies=${RAPIDS_DEPENDENCIES}" \
| tee "${ENV_YAML_DIR}/env.yaml"

Expand All @@ -38,4 +43,6 @@ rapids-print-env
rapids-mamba-retry install \
--channel "${CPP_CHANNEL}" \
--channel "${PYTHON_CHANNEL}" \
--channel "${LIBRMM_CHANNEL}" \
--channel "${RMM_CHANNEL}" \
cudf libcudf
4 changes: 2 additions & 2 deletions conda/environments/all_cuda-118_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ dependencies:
- doxygen=1.9.1
- fastavro>=0.22.9
- flatbuffers==24.3.25
- fmt>=10.1.1,<11
- fmt>=11.0.2,<12
- fsspec>=0.6.0
- gcc_linux-64=11.*
- hypothesis
Expand Down Expand Up @@ -84,7 +84,7 @@ dependencies:
- s3fs>=2022.3.0
- scikit-build-core>=0.10.0
- scipy
- spdlog>=1.12.0,<1.13
- spdlog>=1.14.1,<1.15
- sphinx
- sphinx-autobuild
- sphinx-copybutton
Expand Down
4 changes: 2 additions & 2 deletions conda/environments/all_cuda-125_arch-x86_64.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ dependencies:
- doxygen=1.9.1
- fastavro>=0.22.9
- flatbuffers==24.3.25
- fmt>=10.1.1,<11
- fmt>=11.0.2,<12
- fsspec>=0.6.0
- gcc_linux-64=11.*
- hypothesis
Expand Down Expand Up @@ -82,7 +82,7 @@ dependencies:
- s3fs>=2022.3.0
- scikit-build-core>=0.10.0
- scipy
- spdlog>=1.12.0,<1.13
- spdlog>=1.14.1,<1.15
- sphinx
- sphinx-autobuild
- sphinx-copybutton
Expand Down
4 changes: 2 additions & 2 deletions conda/recipes/libcudf/conda_build_config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ librdkafka_version:
- ">=2.5.0,<2.6.0a0"

fmt_version:
- ">=10.1.1,<11"
- ">=11.0.2,<12"

flatbuffers_version:
- "=24.3.25"

spdlog_version:
- ">=1.12.0,<1.13"
- ">=1.14.1,<1.15"

nvcomp_version:
- "=4.0.1"
Expand Down
10 changes: 8 additions & 2 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,8 +798,14 @@ add_dependencies(cudf jitify_preprocess_run)
target_link_libraries(
cudf
PUBLIC CCCL::CCCL rmm::rmm $<BUILD_LOCAL_INTERFACE:BS::thread_pool>
PRIVATE $<BUILD_LOCAL_INTERFACE:nvtx3::nvtx3-cpp> cuco::cuco ZLIB::ZLIB nvcomp::nvcomp
kvikio::kvikio $<TARGET_NAME_IF_EXISTS:cuFile_interface> nanoarrow
PRIVATE $<BUILD_LOCAL_INTERFACE:nvtx3::nvtx3-cpp>
cuco::cuco
ZLIB::ZLIB
nvcomp::nvcomp
kvikio::kvikio
$<TARGET_NAME_IF_EXISTS:cuFile_interface>
nanoarrow
spdlog::spdlog_header_only
)

# Add Conda library, and include paths if specified
Expand Down
17 changes: 2 additions & 15 deletions cpp/cmake/thirdparty/get_spdlog.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# =============================================================================
# Copyright (c) 2023, NVIDIA CORPORATION.
# Copyright (c) 2023-2024, NVIDIA CORPORATION.
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
# in compliance with the License. You may obtain a copy of the License at
Expand All @@ -16,21 +16,8 @@
function(find_and_configure_spdlog)

include(${rapids-cmake-dir}/cpm/spdlog.cmake)
rapids_cpm_spdlog(FMT_OPTION "EXTERNAL_FMT_HO" INSTALL_EXPORT_SET cudf-exports)
rapids_export_package(BUILD spdlog cudf-exports)
rapids_cpm_spdlog(FMT_OPTION "EXTERNAL_FMT_HO")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've paired removing the spdlog export here with adding this over in cpp/CMakeLists.txt:

target_linked_libraries(cudf PRIVATE spdlog::spdlog_header_only)

Without that, the pip devcontainer build here fails like this while building the pylibcudf wheel:

CMake Error at /home/coder/.local/share/venvs/rapids/lib/python3.10/site-packages/cmake/data/share/cmake-3.30/Modules/CMakeFindDependencyMacro.cmake:76 (find_package):
    By not providing "Findspdlog.cmake" in CMAKE_MODULE_PATH this project has
    asked CMake to find a package configuration file provided by "spdlog", but
    CMake did not find one.
    
    Could not find a package configuration file provided by "spdlog" with any
    of the following names:
    
        spdlogConfig.cmake
        spdlog-config.cmake
    
    Add the installation prefix of "spdlog" to CMAKE_PREFIX_PATH or set
    "spdlog_DIR" to a directory containing one of the above files.  If "spdlog"
    provides a separate development package or SDK, be sure it has been
    installed.

I think because in devcontainers, libcudf.so is built outside of wheel builds, which means that this condition is hit:

cudf/python/libcudf/CMakeLists.txt

Lines 29 to 31 in a0c6fc8

if(cudf_FOUND)
return()
endif()

And then later code that makes spdlog available isn't run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems plausible to me but I'd like a confirmation from a CMake expert. @vyasr or @KyleFromNVIDIA?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If libcudf is only using spdlog privately, and its public headers don't depend on spdlog, then I think this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'd thought it was only using it privately, but now that I search around again I'm really not sure.

This makes me think libcudf's logger is considered part of its public API:

cudf/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md

Lines 1084 to 1087 in 2676924

* Global logger object exposed via `cudf::logger()` - sets the minimum logging level at runtime.
For example, calling `cudf::logger().set_level(spdlog::level::err)`, will exclude any messages that
are not errors or critical errors. This API should not be used within libcudf to manipulate logging,
its purpose is to allow upstream users to configure libcudf logging to fit their application.

Here's the one non-test header directly #include-ing spdlog:

cudf/cpp/include/cudf/utilities/logger.hpp

Line 21 in 2676924

#include <spdlog/spdlog.h>

And there I can see that cudf::logger is a spdlog::logger.

cudf/cpp/include/cudf/utilities/logger.hpp

Line 46 in 2676924

spdlog::logger& logger();

So maybe it does need to be exported? But if so then I don't understand how the pylibcudf and cudf builds and tests could be succeeding with it marked private like this.

One other helpful piece of context that might help explain why this is even coming up now... we were previously patching spdlog in rapids-cmake, which took it down the "always download with CPM and patch" code paths there. Now that we're not patching, different rapids-cmake codepaths are being followed. Something related to exporting must be different between those cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're using the header-only version of spdlog, if the downstream targets already have include directories that contain spdlog, they don't need to link against the spdlog target to get those headers. So perhaps it is currently just working by accident.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yeah the pip devcontainers job (where I'd originally observed the issue at the top of this thread) is passing with these changes: https://github.com/rapidsai/cudf/actions/runs/10962944541/job/30443351446?pr=16806

I feel much better about this state, INTERFACE seems right to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds right to me. Apologies for not catching this earlier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't see this earlier. Yes, the spdlog logger is currently part of the public API for cudf via the logger. That is why we also wound up with symbol issues around spdlog before we went through and hid all the necessary symbols.

The current code looks close, but I don't think INTERFACE is right. It should be a PUBLIC dependency because it is required both by consumers of libcudf (because of the spdlog bits in the logger's public interface) and by libcudf internals (to compiled logger.cpp). I suspect that right now INTERFACE is working because we transitively inherit the spdlog INTERFACE include from rmm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, ok that makes sense! Let me test switching it to PUBLIC, shouldn't take too long.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yeah I tested that in devcontainers and it worked great (yes really that fast... yay sccache).

And looks like it's working here in CI too: https://github.com/rapidsai/cudf/actions/runs/10963768468/job/30445951463?pr=16806


if(spdlog_ADDED)
rapids_export(
BUILD spdlog
EXPORT_SET spdlog
GLOBAL_TARGETS spdlog spdlog_header_only
NAMESPACE spdlog::
)
include("${rapids-cmake-dir}/export/find_package_root.cmake")
rapids_export_find_package_root(
BUILD spdlog [=[${CMAKE_CURRENT_LIST_DIR}]=] EXPORT_SET cudf-exports
)
endif()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context for this change: rapidsai/rapids-cmake#387 (comment)

endfunction()

find_and_configure_spdlog()
4 changes: 2 additions & 2 deletions dependencies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,12 +350,12 @@ dependencies:
common:
- output_types: conda
packages:
- fmt>=10.1.1,<11
- fmt>=11.0.2,<12
- flatbuffers==24.3.25
- librdkafka>=2.5.0,<2.6.0a0
# Align nvcomp version with rapids-cmake
- nvcomp==4.0.1
- spdlog>=1.12.0,<1.13
- spdlog>=1.14.1,<1.15
rapids_build_skbuild:
common:
- output_types: [conda, requirements, pyproject]
Expand Down
2 changes: 2 additions & 0 deletions rapids_config.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ else()
)
endif()

set(rapids-cmake-repo jameslamb/rapids-cmake)
set(rapids-cmake-branch fmt-and-spdlog)
if(NOT EXISTS "${CMAKE_CURRENT_BINARY_DIR}/CUDF_RAPIDS-${RAPIDS_VERSION_MAJOR_MINOR}.cmake")
file(
DOWNLOAD
Expand Down
Loading