Upgrade to nvcomp 5.3.0.16 - #23294
Conversation
robertmaynard
left a comment
There was a problem hiding this comment.
Approving CMake changes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNVComp pins are updated to ChangesNVComp dependency update
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@robertmaynard Could I get your review of these final changes I made after your last commit? 8fead2a...5a80603 |
| target_link_libraries( | ||
| cudf PRIVATE "$<BUILD_INTERFACE:$<LINK_LIBRARY:WHOLE_ARCHIVE,${_dep_link}>>" | ||
| ) | ||
| # nvcomp_static already whole-archives rmm and rapids_logger. |
There was a problem hiding this comment.
nvcomp_static already whole-archives rmm and rapids_logger
There is no promise that nvcomp is going to whole archive the same version of rmm that cudf has built against. We need to whole archive rmm even when nvcomp_static is being used
There was a problem hiding this comment.
This was changed in 5a80603. Prior to that, CI failed in the linters job: https://github.com/rapidsai/cudf/actions/runs/30135081055/job/89617508579
Errors look like:
/opt/conda/envs/clang_tidy/bin/x86_64-conda-linux-gnu-ld: _deps/rapids_logger-build/librapids_logger.a(logger.cpp.o): in function `rapids_logger::detail::default_flush()':
logger.cpp:(.text._ZN13rapids_logger6detail13default_flushEv+0x0): multiple definition of `rapids_logger::detail::default_flush()'; _deps/nvcomp_proprietary_binary-src/lib/libnvcomp_static.a(logger.cpp.o):logger.cpp:(.text+0x0): first defined here
/opt/conda/envs/clang_tidy/bin/x86_64-conda-linux-gnu-ld: _deps/rapids_logger-build/librapids_logger.a(logger.cpp.o): in function `rapids_logger::sink::sink(std::unique_ptr<rapids_logger::detail::sink_impl, std::default_delete<rapids_logger::detail::sink_impl> >)':
logger.cpp:(.text._ZN13rapids_logger4sinkC2ESt10unique_ptrINS_6detail9sink_implESt14default_deleteIS3_EE+0x0): multiple definition of `rapids_logger::sink::sink(std::unique_ptr<rapids_logger::detail::sink_impl, std::default_delete<rapids_logger::detail::sink_impl> >)'; _deps/nvcomp_proprietary_binary-src/lib/libnvcomp_static.a(logger.cpp.o):logger.cpp:(.text+0x2e0): first defined here
There was a problem hiding this comment.
Yeah you have to disable or ignore those linter checks. We know we have duplicate symbols in our archives since rapids-logger is being embedded into nvcomp. We don't have a way to avoid that currently and are relying on the fact that rapids-logger is ABI stable.
| kvikio::kvikio ${CUDF_nanoarrow_TARGET} zstd $<BUILD_LOCAL_INTERFACE:xxhash> rtcx::rtcx | ||
| ) | ||
|
|
||
| if(TARGET spdlog::spdlog) |
There was a problem hiding this comment.
Why was spdlog different compared to rmm and rapids-logger?
There was a problem hiding this comment.
f404547 was needed because of this issue where fmt was being exported as a dependency. This made the cuDF CMake tests fail: https://github.com/rapidsai/cudf/actions/runs/30124560885/job/89585363258#step:13:1511
-- Found cudf: /__w/cudf/cudf/cpp/build/cmake-tests/cudf_export_consumer/cudf-build/cudf-config.cmake (found version "26.08.0")
CMake Error at CMakeLists.txt:16 (message):
cudf::cudf INTERFACE_LINK_LIBRARIES exposes private dependency fmt:::
CCCL::CCCL;dl;CUDA::cudart_static;Threads::Threads;fmt::fmt-header-only;CCCL::CCCL
-- Configuring incomplete, errors occurred!
ninja: build stopped: subcommand failed.
I don't know if this is the right fix, or if the test itself is flawed.
There was a problem hiding this comment.
Okay that makes sense, rapids-logger and rmm are part of the public API and have to be exported
|
Replaced by #23456. |
## Description Upgrades nvcomp to version 5.3.0.16. Depends on rapidsai/rmm#2462. Replaces #23294. ## Checklist - [x] I am familiar with the [Contributing Guidelines](https://github.com/rapidsai/cudf/blob/HEAD/CONTRIBUTING.md). - [x] New or existing tests cover these changes. - [x] The documentation is up to date with these changes. --------- Co-authored-by: Bradley Dice <bdice@bradleydice.com> Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
Description
Upgrades nvcomp to version 5.3.0.16.
Depends on rapidsai/rmm#2462.
Checklist