Skip to content

Conversation

@ldematte
Copy link
Contributor

CuVS uses raft underneath, which in turns uses rapids_logger. Users of the C++ API can access directly the logger used by raft via raft::default_logger(), and use it to get and set the log level.
This is not possible for users of the C API (and of any API based on it, like the C API).
This PR adds specific C functions to close this gap.

Closes #1310

@copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 25, 2025

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.

@ldematte ldematte requested a review from a team as a code owner September 26, 2025 08:53
@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Sep 26, 2025
@cjnolet
Copy link
Member

cjnolet commented Sep 26, 2025

/ok to test 7e7d49b

@ChrisHegarty
Copy link
Contributor

Do we need this in 25.10? How do we control the log level without it?

@mythrocks
Copy link
Contributor

/ok to test e91a6dc

@cjnolet
Copy link
Member

cjnolet commented Oct 1, 2025

/ok to test 1959150

@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 1, 2025

/ok to test 1959150

@cjnolet, there was an error processing your request: E2

See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/

@mythrocks
Copy link
Contributor

mythrocks commented Oct 3, 2025

@benfred, @cjnolet: Should this change be on branch-25.12? Or is it safe to go into the current release?

I would have thought the former, given that the ability to change log levels hasn't been available in previous releases either.

But I'm Johnny-come-lately.

@cjnolet
Copy link
Member

cjnolet commented Oct 3, 2025

Unfortunately now that the code is frozen we have to push it forward, so it'll need to be in 25.12.

@mythrocks
Copy link
Contributor

@cjnolet, thank you for confirming.

@ldematte: I'm 👍 on this change too. Let's retarget to branch-25.12.

@ldematte ldematte changed the base branch from branch-25.10 to branch-25.12 October 21, 2025 12:43
@ldematte
Copy link
Contributor Author

@mythrocks @benfred I have rebased this onto branch-25.12
I think it's ready to be merged.

@ldematte ldematte changed the base branch from branch-25.12 to main October 21, 2025 14:37
@mythrocks
Copy link
Contributor

/ok to test 0750e1c

@mythrocks
Copy link
Contributor

mythrocks commented Oct 21, 2025

Pardon me, but I don't think the changes are complete:

jextract downloaded to /__w/cuvs/cuvs/java/jextract-22
c_api.h:21:10: error: 'rapids_logger/log_levels.h' file not found
__stddef_max_align_t.h:22:15: warning: Skipping __clang_max_align_nonce2 (type LongDouble is not supported)

We'll need to change the Java build script, and change the jextract line to include the rapids_logger header directory.

@ldematte
Copy link
Contributor Author

I have that change in #1376, along with the Java changes, but it seems the way we use jextract breaks. I'll move the change to the scripts to this PR.

@ldematte ldematte requested a review from a team as a code owner October 22, 2025 11:13
@ldematte
Copy link
Contributor Author

@mythrocks In general, I find a bit odd that when you change the C api you need to update something related to the Java API. I understand the mechanics of it (jextract process a C header which includes headers from the C API, so in turn changing a C API header may have effects), but it's counterintuitive. Imagine if I was a developer purely focusing on the C side of things, with no knowledge of jextract etc.

I think we need a more robust fix for this; I think jextract should use the same include directories as the C compiler. No idea how to achieve that though, ATM.

@mythrocks
Copy link
Contributor

I think jextract should use the same include directories as the C compiler.

I agree in principle. But given that these are disparate build systems, I'm not sure how we'd unify these. :/

@mythrocks
Copy link
Contributor

/ok to test 97306fe

@mythrocks
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit c6b52fa into rapidsai:main Oct 24, 2025
239 of 244 checks passed
rapids-bot bot pushed a commit that referenced this pull request Oct 28, 2025
Java API for getting/setting the CuVS/raft logging

Depends on #1375

Authors:
  - Lorenzo Dematté (https://github.com/ldematte)
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)
  - Ben Frederickson (https://github.com/benfred)
  - MithunR (https://github.com/mythrocks)

URL: #1376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Development

Successfully merging this pull request may close these issues.

[BUG] Non-controllable logging from libcuvs/raft

5 participants