Add C-API method to fetch cuVS version from Java. by mythrocks · Pull Request #935 · rapidsai/cuvs

mythrocks · 2025-05-26T23:07:24Z

Fixes #930.

This commit adds a C API function to fetch the cuVS version.

copy-pr-bot · 2025-05-26T23:07:27Z

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.

ChrisHegarty · 2025-05-27T16:22:14Z

+ * @param[out] patch Patch version
+ * @return cuvsError_t
+ */
+cuvsError_t cuvsVersionGet(uint16_t *major, uint16_t *minor, uint16_t *patch);


Is the error return type by convention, or something else? Since this method can never fail, right ?

Agreed; this method can't fail, really.

This method currently follows the spec in #930, and the convention in the file.

I toyed with a different implementation:

std::tuple<uint16_t, uint16_t, uint16_t> cuvsVersionGet() { return {CUVS_VERSION_MAJOR, CUVS_VERSION_MINOR, CUVS_VERSION_PATCH}; }

I wasn't sure of the implications of using std::tuple with Panama/JNI.

Ah, alternatively, I could make this a void function, if that's preferable.

Use of std::tuple is fine in the C++ API, albeit, I would still caution against it in favor of using a specialized struct. Alternatively, the C API could have functions to compute the individual values like the following:

cuvsVersionGetMajor
cuvsVersionGetMajor
cuvsVersionGetPath.

But, I would still have these return through pointer ref so that we can still buffer in an error if needed. Please see the CUDA runtime C API for an example.

Use of std::tuple is fine in the C++ API, albeit...

The trick there is that I don't know what std::tuple<...> translates to in JNI. I considered the separate calls, and figured that the caller is unlikely to call one without calling the others. We might as well save the extra JNI round-trips.

Thank you, all. I'm inclined to leave this function in its current form, if it's agreeable to @ChrisHegarty.

The trick there is that I don't know what std::tuple<...> translates to in JNI.

What I'm saying is that std::tuple is not a C construct, and so we can't use it at all in the formal C-based API. We've built cuVS APIs (similar to much of RAPIDS) as a C++ core with a C wrapper around it. Everything we do in the C layer needs to be proper C (though it can call C++ underneath, obviously).

All this looks pretty complicated, and a simple string returned in "25.06.0" format should suffice too :-)
But, I'm not saying we can't handle it in Java, though. It should be possible, with the methods Corey suggested:

cuvsVersionGetMajor
cuvsVersionGetMajor
cuvsVersionGetPath.

In general, we would prefer to keep strings (and the need for string manipulation) out of C APIs. That's why we prefer concrete numbers for these values. Strings are fine in the Python/Java layers, but not in the C layer (unless it's for logging or relaying exceptions).

We could've handled the string in Java layer directly. But, if you think having those three pieces available separately makes more sense, then I'm okay with it :-)

if you think having those three pieces available separately makes more sense

Yes, centralizing this makes a ton of sense for the purposes of making sure the same symbol / underlying data is being given to everyone. The C layer is the place to do this.

chatman · 2025-05-27T18:11:27Z

LGTM, Thanks!
@narangvivek10 we should add a Java method to check this version (perhaps call it from the static initializer of CuVSResources) in a separate PR immediately.

cjnolet · 2025-05-27T22:24:30Z

/ok to test 76f40b0

cjnolet · 2025-05-28T15:16:16Z

@mythrocks,

Please set up git pre-commit as per the instructions here. After you run the pre-commit hooks, the style checker will run in this PR and we'll be able to approve and merge your changes.

ChrisHegarty

LGTM. Modelling this through Panama FFI in Java should be straightforward.

mythrocks · 2025-05-28T18:36:00Z

/ok to test 7b4525f

cjnolet · 2025-05-28T20:59:24Z

/ok to test 7b4525f

cjnolet · 2025-05-28T21:41:40Z

I know it seems redundant @mythrocks, but could we capture a quick test in here just for potential regression / maintainence purposes? You could literally verify that the value returned matches the constants.

mythrocks · 2025-05-28T23:07:57Z

could we capture a quick test in here...

I was debating that. I was wondering about having to move the version constants to the header, and why one would need the function any more. That reasoning wasn't right: We'd still need it for JNI.

I've updated with a test, and rejigged the rest accordingly.

cjnolet

LGTM!

copy-pr-bot · 2025-05-29T01:58:47Z

/ok to test 88993b6

@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/

cjnolet · 2025-05-29T01:59:03Z

/ok to test b074b76

cjnolet · 2025-05-29T01:59:18Z

/merge

mythrocks · 2025-05-29T15:59:29Z

I'm investigating the build failure now. The following is odd:

FAIL	github.com/rapidsai/cuvs/go [build failed]
/opt/conda/envs/go/bin/x86_64-conda-linux-gnu-ld: /tmp/go-link-550054738/000002.o:(.rodata+0x0): multiple definition of `CUVS_VERSION_MAJOR'; /tmp/go-link-550054738/000001.o:(.rodata+0x0): first defined here
/opt/conda/envs/go/bin/x86_64-conda-linux-gnu-ld: /tmp/go-link-550054738/000002.o:(.rodata+0x2): multiple definition of `CUVS_VERSION_MINOR'; /tmp/go-link-550054738/000001.o:(.rodata+0x2): first defined here
/opt/conda/envs/go/bin/x86_64-conda-linux-gnu-ld: /tmp/go-link-550054738/000002.o:(.rodata+0x4): multiple definition of `CUVS_VERSION_PATCH'; /tmp/go-link-550054738/000001.o:(.rodata+0x4): first defined here

The golang build seems to fail with multiple definitions. The symbols in question are const, and should have internal linkage. I'll try repro this locally.

mythrocks · 2025-05-29T16:51:01Z

I can't seem to get the Go builds going locally to debug.

I have switched the version constants to static const to force internal linkage, to see if the symbol clashes continue in the Go builds.

If this doesn't pass CI, I'll have to resort to using #define for now, and sorting this out after the release.

mythrocks · 2025-05-29T17:21:19Z

/ok to test e2a792d

mythrocks · 2025-05-30T16:39:01Z

@cjnolet, thank you for retargeting this PR to 25.08 already.

cjnolet · 2025-06-03T14:08:31Z

@mythrocks, the reason why CI is still not running in this PR is because there are unverified commits in the history.

cjnolet · 2025-06-03T14:08:47Z

/ok to test eaf9fbe

mythrocks · 2025-06-03T17:25:26Z

because there are unverified commits in the history.

Heard. I've tried squashing the commits in this PR and re-signing, but it's still turning up as unverified. Please bear with me while I sort this out.

Odd, since I have signed commits working with my other projects.

cjnolet · 2025-06-03T20:04:49Z

@mythrocks, I wouldn't worry about it. You could squash the commits in your branch and then open up a new PR, but it's really not a big deal. I'd honestly rather keep the history than have a force-push to this PR to change it.

You'll just need to manually do /ok to test <commit hash> each time a change is pushed to the branch. No biggie.

Fixes rapidsai#930. This commit adds a C API function to fetch the cuVS version.

mythrocks · 2025-06-03T20:16:25Z

You'll just need to manually do /ok to test each time...

Heard. Apologies for the forced push(es), but I think I've gotten it now. I figured if we were to lose history, it should probably be on a simple PR like this one.

It turns out that I wasn't verifying correctly so far. I think I have it going now.

cjnolet · 2025-06-03T20:24:43Z

In the future, I would prefer that you not force push to the PR. This can create a whole slew of other problems, especially when you have write access.

cjnolet · 2025-06-04T17:38:02Z

@mythrocks not sure if the failing test was flaky or not. We can try to rerun

mythrocks · 2025-06-04T17:44:37Z

/ok to test 303684e

mythrocks · 2025-06-04T22:54:40Z

This seems to be a failure in the C++ gtests:

      Start 19: NEIGHBORS_MG_TEST
19/32 Test #19: NEIGHBORS_MG_TEST .......................***Failed    6.11 sec
'./../../..//bin/gtests/libcuvs/NEIGHBORS_MG_TEST'
Running main() from /tmp/conda-bld-output/bld/rattler-build_libcuvs/work/cpp/build/_deps/gtest-src/googletest/src/gtest_main.cc
...
...
The following tests FAILED:
	 19 - NEIGHBORS_MG_TEST (Failed)

I'll check for whether this test is failing in other PRs.

chatman · 2025-06-04T23:25:39Z

@mythrocks I saw that same test failure in #916 as well. AFK at the moment, so don't know if it is still failing.

mythrocks · 2025-06-04T23:28:42Z

@mythrocks I saw that same test failure in #916 as well. AFK at the moment, so don't know if it is still failing.

ACK. The test passes locally on my workstation. I'll reach out to the libcuvs devs.

Fixes rapidsai#930. This commit adds a C API function to fetch the cuVS version. Authors: - MithunR (https://github.com/mythrocks) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Chris Hegarty (https://github.com/ChrisHegarty) - Corey J. Nolet (https://github.com/cjnolet) - James Lamb (https://github.com/jameslamb) URL: rapidsai#935

mythrocks requested review from a team as code owners May 26, 2025 23:07

mythrocks requested a review from KyleFromNVIDIA May 26, 2025 23:07

mythrocks self-assigned this May 26, 2025

github-actions Bot added ci cpp labels May 26, 2025

mythrocks added feature request New feature or request non-breaking Introduces a non-breaking change labels May 26, 2025

ChrisHegarty reviewed May 27, 2025

View reviewed changes

mythrocks requested a review from ChrisHegarty May 27, 2025 17:51

cjnolet added this to Unstructured Data Processing May 27, 2025

cjnolet moved this to In Progress in Unstructured Data Processing May 27, 2025

ChrisHegarty approved these changes May 28, 2025

View reviewed changes

mythrocks requested review from ChrisHegarty and cjnolet May 28, 2025 18:54

cjnolet approved these changes May 29, 2025

View reviewed changes

mythrocks force-pushed the get-version-jni-c-api branch 4 times, most recently from 2f955c1 to 108322b Compare June 3, 2025 17:22

mythrocks force-pushed the get-version-jni-c-api branch 3 times, most recently from 2ce9cff to 034ff66 Compare June 3, 2025 18:44

Add C-API method to fetch cuVS version from Java.

0ffd8b7

Fixes rapidsai#930. This commit adds a C API function to fetch the cuVS version.

mythrocks force-pushed the get-version-jni-c-api branch from 034ff66 to 0ffd8b7 Compare June 3, 2025 20:10

jameslamb approved these changes Jun 3, 2025

View reviewed changes

jameslamb removed request for ChrisHegarty and KyleFromNVIDIA June 3, 2025 21:03

Merge branch 'branch-25.08' into get-version-jni-c-api

303684e

cjnolet and others added 2 commits June 5, 2025 09:57

Merge branch 'branch-25.08' into get-version-jni-c-api

d423dfa

Merge branch 'branch-25.08' into get-version-jni-c-api

2d9866f

rapids-bot Bot merged commit 58735db into rapidsai:branch-25.08 Jun 7, 2025
53 checks passed

github-project-automation Bot moved this from In Progress to Done in Unstructured Data Processing Jun 7, 2025

Conversation

mythrocks commented May 26, 2025

Uh oh!

copy-pr-bot Bot commented May 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjnolet May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatman May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chatman commented May 27, 2025

Uh oh!

cjnolet commented May 27, 2025

Uh oh!

cjnolet commented May 28, 2025

Uh oh!

ChrisHegarty left a comment

Choose a reason for hiding this comment

Uh oh!

mythrocks commented May 28, 2025

Uh oh!

cjnolet commented May 28, 2025

Uh oh!

cjnolet commented May 28, 2025

Uh oh!

mythrocks commented May 28, 2025

Uh oh!

cjnolet left a comment

Choose a reason for hiding this comment

Uh oh!

copy-pr-bot Bot commented May 29, 2025

Uh oh!

cjnolet commented May 29, 2025

Uh oh!

cjnolet commented May 29, 2025

Uh oh!

mythrocks commented May 29, 2025

Uh oh!

mythrocks commented May 29, 2025

Uh oh!

mythrocks commented May 29, 2025

Uh oh!

mythrocks commented May 30, 2025

Uh oh!

cjnolet commented Jun 3, 2025

Uh oh!

cjnolet commented Jun 3, 2025

Uh oh!

mythrocks commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjnolet commented Jun 3, 2025

Uh oh!

mythrocks commented Jun 3, 2025

Uh oh!

cjnolet commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjnolet commented Jun 4, 2025

Uh oh!

mythrocks commented Jun 4, 2025

cjnolet May 27, 2025 •

edited

Loading

chatman May 27, 2025 •

edited

Loading

mythrocks commented Jun 3, 2025 •

edited

Loading

cjnolet commented Jun 3, 2025 •

edited

Loading

mythrocks commented Jun 4, 2025 •

edited

Loading