Skip to content

Fix RAFT_LOG_TRACE_VEC after the rapids-logger migration - #3105

Open
Wint3rNight wants to merge 3 commits into
NVIDIA:mainfrom
Wint3rNight:fix-2622-trace-vec-macro
Open

Fix RAFT_LOG_TRACE_VEC after the rapids-logger migration#3105
Wint3rNight wants to merge 3 commits into
NVIDIA:mainfrom
Wint3rNight:fix-2622-trace-vec-macro

Conversation

@Wint3rNight

@Wint3rNight Wint3rNight commented Aug 6, 2026

Copy link
Copy Markdown

Closes #2622.

Building with LIBRAFT_LOGGING_LEVEL=TRACE fails to compile. The issue reports one broken symbol; there are actually three, all left behind by #2566 when the logger moved to rapids-logger:

raft::detail::format was removed in that PR — replaced with direct stream insertion, which is all it was doing here
print_vector is unqualified so it doesn't resolve — now raft::print_vector
log() is passed RAPIDS_LOGGER_LOG_LEVEL_TRACE where it wants rapids_logger::level_enum::trace, which is what the sibling RAFT_LOG_TRACE already uses
Added cpp/tests/core/logger.cu covering the host and device pointer paths. The existing ext_headers test only does a bare #include <raft/core/logger.hpp> at the default log level, so the macro body expands to void(0) and is never compiled — which is why this went unnoticed since the migration. The new test raises the level for that translation unit only; reverting the fix makes it fail to build.

Callers still need raft/util/cudart_utils.hpp for print_vector. The macro only expands at the call site, so core/ doesn't take a dependency on util/.

@copy-pr-bot

copy-pr-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9ab6fdb8-f34d-4d72-a908-641ee060287a

📥 Commits

Reviewing files that changed from the base of the PR and between cea5405 and 1c9ef1f.

📒 Files selected for processing (1)
  • cpp/tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • cpp/tests/CMakeLists.txt

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved trace-level vector logging with more reliable formatting.
    • Enhanced logging support for both host and device data.
    • Reduced the likelihood of unexpected failures when logging vector contents.
  • Tests

    • Added coverage for trace logging with host and device pointers.
    • Verified logging across device memory allocation, transfer, synchronization, and cleanup workflows.

Walkthrough

Changes

The trace vector logging macro now uses typed trace logging and qualified vector formatting. CUDA tests cover host and device pointer logging, with the test source added to the core test target.

Trace Vector Logging

Layer / File(s) Summary
Trace macro implementation
cpp/include/raft/core/logger.hpp
RAFT_LOG_TRACE_VEC builds the source prefix with std::stringstream, calls raft::print_vector, and logs the resulting string at trace level.
Host and device logger tests
cpp/tests/core/logger.cu, cpp/tests/CMakeLists.txt
The core test target includes the new CUDA test. The test exercises RAFT_LOG_TRACE_VEC with host and device pointers, including device allocation, copying, logging, and cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 1c9ef

This change repairs TRACE-level vector logging compilation failures and adds host/device coverage; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: fixing RAFT_LOG_TRACE_VEC after the rapids-logger migration.
Description check ✅ Passed The description directly explains the compilation failures, fixes, and added TRACE logging tests.
Linked Issues check ✅ Passed The changes address issue #2622 by fixing all reported TRACE-level compilation errors and adding coverage for the affected macro paths.
Out of Scope Changes check ✅ Passed All changes support issue #2622 by fixing the macro and testing its host and device pointer paths.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/tests/core/logger.cu`:
- Around line 42-49: Replace the raw device_data allocation and manual cudaFree
with the project-standard RAII device container, preserving the existing size
and host-to-device copy behavior. Update the cudaMemcpy destination and
RAFT_LOG_TRACE_VEC call to use the container’s data() pointer, ensuring
ownership remains automatic if either operation fails.
🪄 Autofix

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: 8dc94097-fd60-4037-aad1-0eb7ce8364a4

📥 Commits

Reviewing files that changed from the base of the PR and between 07652ba and ee91565.

📒 Files selected for processing (3)
  • cpp/include/raft/core/logger.hpp
  • cpp/tests/CMakeLists.txt
  • cpp/tests/core/logger.cu

Comment thread cpp/tests/core/logger.cu Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] error: namespace "raft::detail" has no member "format" when RAFT_LOG_ACTIVE_LEVEL = RAPIDS_LOGGER_LOG_LEVEL_TRACE

1 participant