refactor(rtcx): decouple librtcx from cudf and add standalone CMake build - #22744
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
github-actions
Bot
added
libcudf
271214a to
01f71af
Compare
|
/ok to test 01f71af |
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. 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:
📝 WalkthroughWalkthroughThis pull request extracts librtcx from embedded cudf sources into a standalone CMake-built library, refactors its logging to remove external dependencies, enhances the embed function configuration, and reintegrates it into cudf as a linked library dependency. Changeslibrtcx extraction and build system refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| void log_warning(std::string_view msg) | ||
| { | ||
| CUDF_LOG_WARN("%.*s", static_cast<std::int32_t>(msg.size()), msg.data()); | ||
| std::fprintf(stderr, "[rtcx] warn: %.*s\n", static_cast<int>(msg.size()), msg.data()); |
There was a problem hiding this comment.
[Optional] Can/should this use nvtx_domain::name? Also in log_error?
There was a problem hiding this comment.
That's a good question. I will take note of both this and your other comment to open as new issues on the new repo for next steps in this work.
|
|
||
| struct nvtx_domain { | ||
| static constexpr char const* name = "rtcx"; | ||
| static constexpr char const* name [[maybe_unused]] = "rtcx"; |
There was a problem hiding this comment.
[Optional] Depending on the C++ standard you're targeting, this could be static constexpr std::string_view instead… But I guess this is potentially a more pervasive change.
There was a problem hiding this comment.
Yes, that is a potential change we could make. I think we'll be going through multiple rounds of iteration on librtcx once the code has been moved into its long-term home in a new repo.
01f71af to
575b282
Compare
vyasr
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/CMakeLists.txt`:
- Line 458: The embed(...) invocation in CMakeLists.txt is missing the required
OUTPUT_DIRECTORY argument which triggers a FATAL_ERROR in embed.cmake; update
the embed(cudf_cuda_embed COMPRESSION zstd) call to include OUTPUT_DIRECTORY
with a valid directory (for example a dedicated subdirectory in the build binary
dir) so that the embed macro can set cudf_cuda_embed_INCLUDE_DIRS and
cudf_cuda_embed_SOURCE_DIR; modify the embed call in CMakeLists.txt to pass
OUTPUT_DIRECTORY <your_chosen_output_dir> (ensure the chosen dir is writable and
consistent with downstream references to cudf_cuda_embed_*).
In `@cpp/librtcx/cmake/RAPIDS.cmake`:
- Line 11: The CMake minimum version is incorrectly set to 4.0 in the
cmake_minimum_required call; update the cmake_minimum_required(VERSION ...)
invocation to a valid 3.x release (for example 3.29.6 or the same version used
in the top-level/cpp/CMakeLists.txt) so configuration doesn't fail — locate the
cmake_minimum_required() line in RAPIDS.cmake and replace the invalid "4.0" with
the appropriate 3.x version used by the project.
In `@cpp/librtcx/CMakeLists.txt`:
- Line 8: The CMake minimum required version is incorrectly set to 4.0; update
the cmake_minimum_required invocation in CMakeLists.txt to a valid released
version per our guidelines (use 3.29.6 or higher) so configuration doesn't fail
— locate the cmake_minimum_required(VERSION ...) line and change the VERSION
value from 4.0 to 3.29.6 (or a compatible 3.29.x >= 3.29.6).
- Line 42: Fix the typo in the compile definition: change the incorrect value
for ZSTD_STATIC_LINKING_ONLY in the target_compile_definitions call for target
libzstd_static so it uses the valid CMake boolean "ON" instead of "0N" (i.e.,
update the definition used with target_compile_definitions(libzstd_static ...)
to set ZSTD_STATIC_LINKING_ONLY=ON).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75306ae7-d5d4-45ae-bc99-0471361c51c9
📒 Files selected for processing (6)
cpp/CMakeLists.txtcpp/librtcx/CMakeLists.txtcpp/librtcx/cmake/RAPIDS.cmakecpp/librtcx/cmake/rapids_config.cmakecpp/librtcx/embed.cmakecpp/librtcx/rtcx.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/librtcx/rtcx.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/CMakeLists.txt`:
- Line 458: The embed(...) invocation in CMakeLists.txt is missing the required
OUTPUT_DIRECTORY argument which triggers a FATAL_ERROR in embed.cmake; update
the embed(cudf_cuda_embed COMPRESSION zstd) call to include OUTPUT_DIRECTORY
with a valid directory (for example a dedicated subdirectory in the build binary
dir) so that the embed macro can set cudf_cuda_embed_INCLUDE_DIRS and
cudf_cuda_embed_SOURCE_DIR; modify the embed call in CMakeLists.txt to pass
OUTPUT_DIRECTORY <your_chosen_output_dir> (ensure the chosen dir is writable and
consistent with downstream references to cudf_cuda_embed_*).
In `@cpp/librtcx/cmake/RAPIDS.cmake`:
- Line 11: The CMake minimum version is incorrectly set to 4.0 in the
cmake_minimum_required call; update the cmake_minimum_required(VERSION ...)
invocation to a valid 3.x release (for example 3.29.6 or the same version used
in the top-level/cpp/CMakeLists.txt) so configuration doesn't fail — locate the
cmake_minimum_required() line in RAPIDS.cmake and replace the invalid "4.0" with
the appropriate 3.x version used by the project.
In `@cpp/librtcx/CMakeLists.txt`:
- Line 8: The CMake minimum required version is incorrectly set to 4.0; update
the cmake_minimum_required invocation in CMakeLists.txt to a valid released
version per our guidelines (use 3.29.6 or higher) so configuration doesn't fail
— locate the cmake_minimum_required(VERSION ...) line and change the VERSION
value from 4.0 to 3.29.6 (or a compatible 3.29.x >= 3.29.6).
- Line 42: Fix the typo in the compile definition: change the incorrect value
for ZSTD_STATIC_LINKING_ONLY in the target_compile_definitions call for target
libzstd_static so it uses the valid CMake boolean "ON" instead of "0N" (i.e.,
update the definition used with target_compile_definitions(libzstd_static ...)
to set ZSTD_STATIC_LINKING_ONLY=ON).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75306ae7-d5d4-45ae-bc99-0471361c51c9
📒 Files selected for processing (6)
cpp/CMakeLists.txtcpp/librtcx/CMakeLists.txtcpp/librtcx/cmake/RAPIDS.cmakecpp/librtcx/cmake/rapids_config.cmakecpp/librtcx/embed.cmakecpp/librtcx/rtcx.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/librtcx/rtcx.cpp
🛑 Comments failed to post (4)
cpp/CMakeLists.txt (1)
458-458:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Missing required OUTPUT_DIRECTORY argument
Issue: The
embed()call is missing the requiredOUTPUT_DIRECTORYargument
Why:cpp/librtcx/embed.cmakelines 209-211 enforceOUTPUT_DIRECTORYas a required parameter viaFATAL_ERROR. This call will cause CMake configuration to fail immediately.Impact: The variables
cudf_cuda_embed_INCLUDE_DIRS(used at line 1088) andcudf_cuda_embed_SOURCE_DIR(used at line 1015) will not be set, and the build will fail before reaching compilation.Suggested fix:
-embed(cudf_cuda_embed COMPRESSION zstd) +embed(cudf_cuda_embed COMPRESSION zstd OUTPUT_DIRECTORY "${CUDF_GENERATED_INCLUDE_DIR}")Or specify an appropriate output directory based on your build requirements.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/CMakeLists.txt` at line 458, The embed(...) invocation in CMakeLists.txt is missing the required OUTPUT_DIRECTORY argument which triggers a FATAL_ERROR in embed.cmake; update the embed(cudf_cuda_embed COMPRESSION zstd) call to include OUTPUT_DIRECTORY with a valid directory (for example a dedicated subdirectory in the build binary dir) so that the embed macro can set cudf_cuda_embed_INCLUDE_DIRS and cudf_cuda_embed_SOURCE_DIR; modify the embed call in CMakeLists.txt to pass OUTPUT_DIRECTORY <your_chosen_output_dir> (ensure the chosen dir is writable and consistent with downstream references to cudf_cuda_embed_*).cpp/librtcx/cmake/RAPIDS.cmake (1)
11-11:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Invalid CMake version requirement
Issue:
cmake_minimum_required(VERSION 4.0 FATAL_ERROR)specifies CMake 4.0, which does not exist
Why: CMake versioning is currently in the 3.x series (latest stable is ~3.30 as of 2026); version 4.0 has not been released, causing immediate configuration failure🔧 Proposed fix
Looking at the parent
cpp/CMakeLists.txt(context snippet 2), the correct requirement is likely:-cmake_minimum_required(VERSION 4.0 FATAL_ERROR) +cmake_minimum_required(VERSION 3.29.6 FATAL_ERROR)Or match the top-level requirement shown in coding guidelines (General requirements section:
cmakeversion 3.29.6+).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/librtcx/cmake/RAPIDS.cmake` at line 11, The CMake minimum version is incorrectly set to 4.0 in the cmake_minimum_required call; update the cmake_minimum_required(VERSION ...) invocation to a valid 3.x release (for example 3.29.6 or the same version used in the top-level/cpp/CMakeLists.txt) so configuration doesn't fail — locate the cmake_minimum_required() line in RAPIDS.cmake and replace the invalid "4.0" with the appropriate 3.x version used by the project.cpp/librtcx/CMakeLists.txt (2)
8-8:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Invalid CMake version requirement
Issue:
cmake_minimum_required(VERSION 4.0 FATAL_ERROR)specifies CMake 4.0, which does not exist
Why: CMake versioning is currently in the 3.x series; version 4.0 has not been released, causing immediate configuration failure🔧 Proposed fix
-cmake_minimum_required(VERSION 4.0 FATAL_ERROR) +cmake_minimum_required(VERSION 3.29.6 FATAL_ERROR)Match the requirement from coding guidelines (General requirements:
cmakeversion 3.29.6+).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/librtcx/CMakeLists.txt` at line 8, The CMake minimum required version is incorrectly set to 4.0; update the cmake_minimum_required invocation in CMakeLists.txt to a valid released version per our guidelines (use 3.29.6 or higher) so configuration doesn't fail — locate the cmake_minimum_required(VERSION ...) line and change the VERSION value from 4.0 to 3.29.6 (or a compatible 3.29.x >= 3.29.6).
42-42:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Typo in compile definition
Issue:
ZSTD_STATIC_LINKING_ONLY=0Nshould beZSTD_STATIC_LINKING_ONLY=ON
Why:0Nis not a valid CMake boolean value; this will cause the macro to be incorrectly defined, potentially breaking zstd's experimental API visibility🔧 Proposed fix
- target_compile_definitions(libzstd_static PUBLIC ZSTD_STATIC_LINKING_ONLY=0N) + target_compile_definitions(libzstd_static PUBLIC ZSTD_STATIC_LINKING_ONLY=ON)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.target_compile_definitions(libzstd_static PUBLIC ZSTD_STATIC_LINKING_ONLY=ON)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/librtcx/CMakeLists.txt` at line 42, Fix the typo in the compile definition: change the incorrect value for ZSTD_STATIC_LINKING_ONLY in the target_compile_definitions call for target libzstd_static so it uses the valid CMake boolean "ON" instead of "0N" (i.e., update the definition used with target_compile_definitions(libzstd_static ...) to set ZSTD_STATIC_LINKING_ONLY=ON).
575b282 to
52756fe
Compare
| void log_warning(std::string_view msg) | ||
| { | ||
| CUDF_LOG_WARN("%.*s", static_cast<std::int32_t>(msg.size()), msg.data()); | ||
| std::fprintf(stderr, "[rtcx] warn: %.*s\n", static_cast<int>(msg.size()), msg.data()); | ||
| } |
There was a problem hiding this comment.
Optionally, in the future, we might want to turn this into a function/virtual object hook that can be set at runtime/compile time.
There was a problem hiding this comment.
@lamarrr Just to clarify, by "this" are you referring to the logging destination, a part of the log message, or something else?
There was a problem hiding this comment.
Agreed — a pluggable logging backend (e.g. a function pointer or virtual interface that consumers can set at init time) would be the right long-term approach. For this PR the goal was just to sever the cudf header dependency with the simplest possible replacement. I'll open an issue on the new repo to track making the logging destination configurable.
| void log_warning(std::string_view msg) | ||
| { | ||
| CUDF_LOG_WARN("%.*s", static_cast<std::int32_t>(msg.size()), msg.data()); | ||
| std::fprintf(stderr, "[rtcx] warn: %.*s\n", static_cast<int>(msg.size()), msg.data()); | ||
| } |
There was a problem hiding this comment.
@lamarrr Just to clarify, by "this" are you referring to the logging destination, a part of the log message, or something else?
| void log_warning(std::string_view msg) | ||
| { | ||
| CUDF_LOG_WARN("%.*s", static_cast<std::int32_t>(msg.size()), msg.data()); | ||
| std::fprintf(stderr, "[rtcx] warn: %.*s\n", static_cast<int>(msg.size()), msg.data()); |
There was a problem hiding this comment.
[Optional] Actually, just realized, if we're on a newer C++ standard, could this use std::ostream operators?
| std::fprintf(stderr, "[rtcx] warn: %.*s\n", static_cast<int>(msg.size()), msg.data()); | |
| std::cerr << "[rtcx] warn: " << msg << std::endl; |
Also in log_error…
There was a problem hiding this comment.
Good suggestion. For this PR I kept fprintf to minimize the diff (just swapping out the cudf logger macro for a direct stderr call), but switching to std::cerr << or even a pluggable logging hook (per @lamarrr's comment below) are natural follow-ups once librtcx lands in its own repo. I'll track both as issues there.
|
@coderabbitai All four findings from your review are false positives — the review was run against an intermediate commit (
All CI checks (131/131) pass on the final commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Resolve all open review comments made by coderabbitai on PR `#22744`
gh api graphql -f query='
{
repository(owner: "rapidsai", name: "cudf") {
pullRequest(number: 22744) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | python3 -c "
import json, sys
data = json.load(sys.stdin)
threads = data['data']['repository']['pullRequest']['reviewThreads']['nodes']
for t in threads:
author = t['comments']['nodes'][0]['author']['login'] if t['comments']['nodes'] else ''
resolved = t['isResolved']
if author == 'coderabbitai' and not resolved:
print(t['id'])
"Length of output: 1027
🐇 |
|
/merge |
6 participants
Description
Decouples librtcx from cudf by removing source-level dependencies on cudf functions/macros (e.g.
cudf/logger.hpp,CUDF_LOG_WARN) and providing librtcx with its own standalone CMake build system. This prepares librtcx to be extracted into a separate repository.After this PR,
cmake -S cpp/librtcx -B buildconfigures and buildslibrtcx.aindependently of cudf, while cudf continues consuming it viaadd_subdirectory(librtcx).Checklist