8000 [Java] Exposing merge API for multiple CAGRA indices by punAhuja · Pull Request #891 · rapidsai/cuvs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Java] Exposing merge API for multiple CAGRA indices #891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
May 29, 2025
Merged

[Java] Exposing merge API for multiple CAGRA indices #891

merged 15 commits into from
May 29, 2025

Conversation

punAhuja
Copy link
Contributor
@punAhuja punAhuja commented May 13, 2025 •
edited
Loading

Exposing the C merge API in the Java layer.

This is work in progress.

Pull request for merge API in C is not merged yet:
#860

@punAhuja punAhuja requested a review from a team as a code owner May 13, 2025 13:59
@copy-pr-bot copy-pr-bot
8000 Copy link
copy-pr-bot bot commented May 13, 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.

@github-actions github-actions bot added the cpp label May 13, 2025
@punAhuja punAhuja closed this May 13, 2025
@punAhuja punAhuja changed the title [Java] Suppor for merge API for multiple CAGRA indices [Java] Support for merge API for multiple CAGRA indices May 13, 2025
@punAhuja punAhuja changed the title [Java] Support for merge API for multiple CAGRA indices [Java][WIP] Exposing merge API for multiple CAGRA indices May 13, 2025
@punAhuja punAhuja reopened this May 13, 2025
@cjnolet cjnolet added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 19, 2025
@punAhuja
Copy link
Contributor Author

#860 is merged now, will rebase my branch with branch-25.06 and update this pull request.

@punAhuja punAhuja force-pushed the merge-api-java-support branch from 0afbed8 to 7cac5d2 Compare May 21, 2025 18:20
@github-actions github-actions bot removed the cpp label May 21, 2025
@punAhuja punAhuja changed the title [Java][WIP] Exposing merge API for multiple CAGRA indices [Java] Exposing merge API for multiple CAGRA indices May 23, 2025
narangvivek10
narangvivek10 reviewed May 26, 2025
View reviewed changes
@punAhuja
Resolved review comments
447e8fc 
-Added description for function merge_cagra_indexes
-passing parameters instead of builder instance for consistency
-Initialized outputIndexParams in builder class of CagraMegeParams
-Added license header in CagramergeParams
-Commented out Logical Merge test as it is not yet implemented in C
-Using File and InputStream instead of java.io.File
narangvivek10
narangvivek10 reviewed May 27, 2025
View reviewed changes
java/cuvs-java/src/main/java22/com/nvidia/cuvs/internal/CagraIndexImpl.java Outdated
@@ -433,6 +455,48 @@ private MemorySegment segmentFromIndexParams(CagraIndexParams params) {
return seg;
}

private static MemorySegment segmentFromIndexParams(CagraIndexParams params, CuVSResourcesImpl resources) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is almost identical (except for being static) to the one above. Should be just one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done, please review.

@cjnolet
Copy link
Member
cjnolet commented May 27, 2025

/ok to test e682d01

@chatman
Copy link
Contributor
chatman commented May 27, 2025

@narangvivek10 I incorporated your review feedback on behalf of @punAhuja.
@cjnolet I added a commit just a couple of minutes after your "ok to test", can you please trigger it again?

@cjnolet
Copy link
Member
cjnolet commented May 27, 2025

/ok to test 6b65b70

@chatman
Copy link
Contributor
chatman commented May 27, 2025

Java tests passed here :-) I think this is ready for merge now.

cjnolet
cjnolet reviewed May 27, 2025
View reviewed changes
java/cuvs-java/src/test/java/com/nvidia/cuvs/CagraBuildAndSearchIT.java Outdated
CagraIndex physicalMergedIndex = CagraIndex.merge(new CagraIndex[]{index1, index2}, physicalMergeParams);
log.info("Physical merge completed successfully");

// CagraMergeParams logicalMergeParams = new CagraMergeParams.Builder()
Copy link
Member

Choose a reason for hiding this comment

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

Please don't leave commented out code in here

8000

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed, thanks!

@chatman
Copy link
Contributor
chatman commented May 27, 2025

@cjnolet fixed the uncommented code, and also added more repetitions to the randomized cagra test (to catch bugs, if any). It is a good to have test fix regardless of this feature. If you have any objections, I can revert that change.

If it looks good, please trigger another CI and merge. Thanks!

@chatman
Copy link
Contributor
chatman commented May 28, 2025

@cjnolet Can you please review and merge this?

@cjnolet
Copy link
Member
cjnolet commented May 28, 2025

@narangvivek10 @chatman please allow CI to fail the Recently updated checker before hitting the Update branch button. The branch doesn't have to be lockstep with the commits in main- this was done by design. But every time you hit the "Update branch" button, it's guaranteed to have to rerun CI.

@cjnolet
Copy link
Member
cjnolet commented May 28, 2025

/ok to test 3a749e9

@narangvivek10
Copy link
Contributor

d9a7089 commit is for conflict resolution caused due to the merging of #902

FYI @cjnolet

@cjnolet
Copy link
Member
cjnolet commented May 29, 2025

/ok to test 6f855d4

@cjnolet
Copy link
Member
cjnolet commented May 29, 2025

/merge

@rapids-bot rapids-bot bot merged commit c7a1b57 into rapidsai:branch-25.06 May 29, 2025
75 checks passed
mythrocks pushed a commit to mythrocks/cuvs that referenced this pull request Jun 3, 2025
@punAhuja @mythrocks
[Java] Exposing merge API for multiple CAGRA indices (rapidsai#891)
451af8b 
Exposing the C merge API in the Java layer.

This is work in progress.

Pull request for merge API in C is not merged yet:
rapidsai#860

Authors:
  - https://github.com/punAhuja
  - Vivek Narang (https://github.com/narangvivek10)

Approvers:
  - Ishan Chattopadhyaya (https://github.com/chatman)
  - Corey J. Nolet (https://github.com/cjnolet)

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

@narangvivek10 narangvivek10 narangvivek10 left review comments

@cjnolet cjnolet cjnolet approved these changes

@chatman chatman chatman approved these changes

Assignees

@punAhuja punAhuja

Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
Vector Search, ML, & Data Mining Release Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@punAhuja @cjnolet @chatman @narangvivek10
0