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.

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

Fix some logger issues #1739

Merged
merged 6 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ rapids_cpm_init()

add_subdirectory(rapids_logger)
rapids_make_logger(rmm EXPORT_SET rmm-exports)
add_library(rmm::rmm_logger ALIAS rmm_logger)
add_library(rmm::rmm_logger_impl ALIAS rmm_logger_impl)

include(cmake/thirdparty/get_cccl.cmake)
include(cmake/thirdparty/get_nvtx.cmake)
Expand Down
4 changes: 3 additions & 1 deletion ci/check_symbols.sh
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ for dso_file in ${dso_files}; do
fi

echo "checking for 'spdlog::' symbols..."
if grep -E 'spdlog\:\:' < "${symbol_file}"; then
if grep -E 'spdlog\:\:' < "${symbol_file}" \
| grep -v 'std\:\:_Destroy_aux'
then
raise-symbols-found-error 'spdlog::'
fi
echo "No symbol visibility issues found"
Expand Down
8 changes: 8 additions & 0 deletions cmake/thirdparty/get_spdlog.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ function(find_and_configure_spdlog)

include(${rapids-cmake-dir}/cpm/spdlog.cmake)
rapids_cpm_spdlog(
# The conda package for fmt is hard-coded to assume that we use a preexisting fmt library. This
Copy link
Contributor

@bdice bdice Nov 27, 2024

Choose a reason for hiding this comment

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

Is it viable to remove conda package dependencies and always get spdlog/fmt as header-only deps via CPM? That way we should never interact with a local spdlog or local/shared fmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it viable to remove conda package dependencies

No. The problem is that the logger is compiled not by rmm itself but by packages that build against rmm, and we cannot guarantee that spdlog or fmt will never exist in the environment in which that build is taking place. For example, if cuml has some other dependency that requires spdlog, then during cuml's conda build spdlog will be in the environment when its copy of the rmm logger is built. You can think of how the new build works as similar to what we do with gtest/gmock, with the additional complexity that the build is done by the user of rmm rather than rmm itself.

always get spdlog/fmt as header-only deps via CPM

Yes, with caveats. This is what I was alluding to in my message above where I said

We could make the first somewhat more palatable by having the download be controlled by some flag that is set to on by default (since that's what we'll want for everything in RAPIDS). I will prototype that after this PR is merged.

The big caveat is that we do not want to require this all the time, otherwise it will make offline builds of any package built against rmm impossible because you will have CPMAddPackage calls embedded in a dependency (the only workarounds would be to manually modify CPM-cloned code inside the build directory). We need to write the CMake so as to make this behavior possible to opt out of.

# is why we have always had a libfmt linkage despite choosing to specify the header-only version
# of fmt. We need a more robust way of modifying this to support fully self-contained build and
# usage even in environments where fmt and/or spdlog are already present. The crudest solution
# would be to modify the interface compile definitions and link libraries of the spdlog target,
# if necessary. For now I'm specifying EXTERNAL_FMT_HO here so that in environments where spdlog
# is cloned and built from source we wind up with the behavior that we expect, but we'll have to
# resolve this properly eventually.
FMT_OPTION "EXTERNAL_FMT_HO"
INSTALL_EXPORT_SET rmm-exports
BUILD_EXPORT_SET rmm-exports)
Expand Down
1 change: 0 additions & 1 deletion conda/environments/all_cuda-118_arch-x86_64.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ dependencies:
- python>=3.10,<3.13
- rapids-build-backend>=0.3.0,<0.4.0.dev0
- scikit-build-core >=0.10.0
- spdlog>=1.14.1,<1.15
- sphinx
- sphinx-copybutton
- sphinx-markdown-tables
Expand Down
1 change: 0 additions & 1 deletion conda/environments/all_cuda-125_arch-x86_64.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ dependencies:
- python>=3.10,<3.13
- rapids-build-backend>=0.3.0,<0.4.0.dev0
- scikit-build-core >=0.10.0
- spdlog>=1.14.1,<1.15
- sphinx
- sphinx-copybutton
- sphinx-markdown-tables
Expand Down
14 changes: 7 additions & 7 deletions conda/recipes/librmm/meta.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,13 @@ requirements:
- {{ stdlib("c") }}
host:
- cuda-version ={{ cuda_version }}
# We require spdlog and fmt (which was de-vendored from spdlog
# conda-forge packages in 1.11.0) so that the spdlog headers are not
# pulled by CPM and installed as a part of the rmm packages. However,
# building against librmm still requires these headers. They are also
# added as a run requirement via the packages' run_exports.
# We need fmt here for now because the conda spdlog package is hard-coded
Copy link
Contributor

Choose a reason for hiding this comment

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

But we removed the spdlog package dependencies…? So this is an “ABI constraint” that is meant to align downstream packages that depend on spdlog, even though fmt is not used/linked as a shared dependency in librmm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your comment isn't correct, but I'm afraid I don't fully understand your explanation so I may not be able to address your confusion correctly. Basically what is happening is that (because rmm is an interface target) it becomes part of downstream packages build to find spdlog. When that package, say cudf, finds a conda-provided spdlog in the environment, that version of spdlog says to use the fmt shared library even if you use the spdlog_header_only target. Therefore, we actually do have an fmt shared library dependency. That is what I tried to explain in the PR description.

Does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I'm sorry I just realized that there is a typo here. The first sentence should read "The conda package for spdlog is hard-coded to assume that we use a preexisting fmt library"

Copy link
Contributor

Choose a reason for hiding this comment

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

But the spdlog conda package already has a dependency on fmt. Why not just let that dependency suffice? Why add it to rmm?

Copy link
Contributor

@bdice bdice Nov 27, 2024

Choose a reason for hiding this comment

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

And spdlog run-exports itself so two different consumers of spdlog will only be usable at runtime with compatible spdlog and fmt versions — and that’s enforced by the solver / dependency graph. https://github.com/conda-forge/spdlog-feedstock/blob/97ea289e6733b60e4c619e9cb1e0ebc329358936/recipe/meta.yaml#L18

I’m not convinced we need a fmt dependency here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized I never answered this question. The problem is that spdlog could be in the build environment as a transitive dependency even though we no longer have it as a dependency at all, resulting in linkage to fmt without spdlog or fmt being part of the runtime dependency list of our conda package (run exports aren't transitive). But yes, you could also fix this problem by simply putting spdlog back into the dependency list right now.

# to use fmt as a compiled library, not header-only, so we must ensure that
# the library is present for now so that if a downstream library tries to
# build against rmm and some other package in its build environment uses
# fmt (or spdlog) that the default rmm build is consistent with such
# environments.
- fmt {{ fmt_version }}
- spdlog {{ spdlog_version }}

build:
script_env:
Expand Down Expand Up @@ -77,8 +77,8 @@ outputs:
{% if cuda_major == "11" %}
- cudatoolkit
{% endif %}
# See comment about fmt in the build section above.
- fmt {{ fmt_version }}
- spdlog {{ spdlog_version }}
test:
commands:
- test -d "${PREFIX}/include/rmm"
Expand Down
1 change: 0 additions & 1 deletion dependencies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ dependencies:
- c-compiler
- cxx-compiler
- fmt>=11.0.2,<12
- spdlog>=1.14.1,<1.15
specific:
- output_types: conda
matrices:
Expand Down
2 changes: 1 addition & 1 deletion include/rmm/detail/logging_assert.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
if (!success) { \
RMM_LOG_CRITICAL( \
"[" __FILE__ ":" RMM_STRINGIFY(__LINE__) "] Assertion " RMM_STRINGIFY(_expr) " failed."); \
rmm::detail::logger().flush(); \
rmm::default_logger().flush(); \
/* NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) */ \
assert(success); \
} \
Expand Down
6 changes: 6 additions & 0 deletions rapids_logger/logger_impl.hpp.in
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,16 @@

// Start hiding before including spdlog headers.
#pragma GCC visibility push(hidden)
// This issue claims to have been resolved in gcc 8, but we still seem to encounter it here.
// The code compiles and links and all tests pass, and nm shows symbols resolved as expected.
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80947
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wattributes"

#include <spdlog/sinks/basic_file_sink.h>
#include <spdlog/sinks/ostream_sink.h>
#include <spdlog/spdlog.h>
#pragma GCC diagnostic pop

#include <memory>
#include <sstream>
Expand Down