Skip to content
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.

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

Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. #1588

Conversation

navneet1v
Copy link
Collaborator

@navneet1v navneet1v commented Apr 3, 2024

Description

Integrating storeVectors interfaces with createIndex and createIndexTemplate functions.

This PR includes:

  1. I have changed the interface of createIndex and createIndexTemplate api to start using vectorAddress and dim, instead of vectors array.
  2. As of now all the vectors are still getting passed to native memory at once. In the upcoming PR I will make the change to send the vectors in chunks, after updating the RFC.
  3. I have remove the freeVector function from the code and started using JNICommons function.
  4. The transfer vector function is still there and will be removed in upcoming PRs after doing more testing.

Critical decision
As we have started sending vectorsAddress to Native layer as compared to vectordata, one of the critical code part here is who should free up the memory allocated to the vectors. To make sure that vector memory is freed once the index is created even in case there is exception to avoid possible memory leaks I have added try and finally block in addKNNBinaryField function. I don't want native code to free up this memory as

  1. The vector address generated by Java code and it becomes the responsibility of Java code to free up that memory.
  2. Going forward if this createIndex interface starts to get exposed to external libraries that lib may or may not free up the memory. Hence it makes logical sense for Java aka addKNNBinaryField function to free up that memory.

JNI tests

[==========] Running 22 tests from 20 test suites.
[----------] Global test environment set-up.
[----------] 1 test from FaissCreateIndexTest
[ RUN      ] FaissCreateIndexTest.BasicAssertions
[       OK ] FaissCreateIndexTest.BasicAssertions (5 ms)
[----------] 1 test from FaissCreateIndexTest (5 ms total)

[----------] 1 test from FaissCreateIndexFromTemplateTest
[ RUN      ] FaissCreateIndexFromTemplateTest.BasicAssertions
[       OK ] FaissCreateIndexFromTemplateTest.BasicAssertions (2 ms)
[----------] 1 test from FaissCreateIndexFromTemplateTest (2 ms total)

[----------] 3 tests from FaissLoadIndexTest
[ RUN      ] FaissLoadIndexTest.BasicAssertions
[       OK ] FaissLoadIndexTest.BasicAssertions (2 ms)
[ RUN      ] FaissLoadIndexTest.HNSWPQDisableSdcTable
WARNING clustering 256 points to 16 centroids: please provide at least 624 training points
[       OK ] FaissLoadIndexTest.HNSWPQDisableSdcTable (357 ms)
[ RUN      ] FaissLoadIndexTest.IVFPQDisablePrecomputeTable
WARNING clustering 256 points to 16 centroids: please provide at least 624 training points
[       OK ] FaissLoadIndexTest.IVFPQDisablePrecomputeTable (360 ms)
[----------] 3 tests from FaissLoadIndexTest (720 ms total)

[----------] 1 test from FaissQueryIndexTest
[ RUN      ] FaissQueryIndexTest.BasicAssertions
[       OK ] FaissQueryIndexTest.BasicAssertions (4 ms)
[----------] 1 test from FaissQueryIndexTest (4 ms total)

[----------] 1 test from FaissQueryIndexWithFilterTest1435
[ RUN      ] FaissQueryIndexWithFilterTest1435.BasicAssertions
[       OK ] FaissQueryIndexWithFilterTest1435.BasicAssertions (9 ms)
[----------] 1 test from FaissQueryIndexWithFilterTest1435 (9 ms total)

[----------] 1 test from FaissQueryIndexWithParentFilterTest
[ RUN      ] FaissQueryIndexWithParentFilterTest.BasicAssertions
[       OK ] FaissQueryIndexWithParentFilterTest.BasicAssertions (5 ms)
[----------] 1 test from FaissQueryIndexWithParentFilterTest (5 ms total)

[----------] 1 test from FaissFreeTest
[ RUN      ] FaissFreeTest.BasicAssertions
[       OK ] FaissFreeTest.BasicAssertions (0 ms)
[----------] 1 test from FaissFreeTest (0 ms total)

[----------] 1 test from FaissInitLibraryTest
[ RUN      ] FaissInitLibraryTest.BasicAssertions
[       OK ] FaissInitLibraryTest.BasicAssertions (0 ms)
[----------] 1 test from FaissInitLibraryTest (0 ms total)

[----------] 1 test from FaissTrainIndexTest
[ RUN      ] FaissTrainIndexTest.BasicAssertions
[       OK ] FaissTrainIndexTest.BasicAssertions (0 ms)
[----------] 1 test from FaissTrainIndexTest (0 ms total)

[----------] 1 test from FaissCreateHnswSQfp16IndexTest
[ RUN      ] FaissCreateHnswSQfp16IndexTest.BasicAssertions
[       OK ] FaissCreateHnswSQfp16IndexTest.BasicAssertions (5 ms)
[----------] 1 test from FaissCreateHnswSQfp16IndexTest (5 ms total)

[----------] 1 test from FaissIsSharedIndexStateRequired
[ RUN      ] FaissIsSharedIndexStateRequired.BasicAssertions
[       OK ] FaissIsSharedIndexStateRequired.BasicAssertions (0 ms)
[----------] 1 test from FaissIsSharedIndexStateRequired (0 ms total)

[----------] 1 test from FaissInitAndSetSharedIndexState
[ RUN      ] FaissInitAndSetSharedIndexState.BasicAssertions
WARNING clustering 256 points to 16 centroids: please provide at least 624 training points
[       OK ] FaissInitAndSetSharedIndexState.BasicAssertions (339 ms)
[----------] 1 test from FaissInitAndSetSharedIndexState (339 ms total)

[----------] 1 test from IDGrouperBitMapTest
[ RUN      ] IDGrouperBitMapTest.BasicAssertions
[       OK ] IDGrouperBitMapTest.BasicAssertions (0 ms)
[----------] 1 test from IDGrouperBitMapTest (0 ms total)

[----------] 1 test from NmslibIndexWrapperSearchTest
[ RUN      ] NmslibIndexWrapperSearchTest.BasicAssertions
[       OK ] NmslibIndexWrapperSearchTest.BasicAssertions (0 ms)
[----------] 1 test from NmslibIndexWrapperSearchTest (0 ms total)

[----------] 1 test from NmslibCreateIndexTest
[ RUN      ] NmslibCreateIndexTest.BasicAssertions
[       OK ] NmslibCreateIndexTest.BasicAssertions (2 ms)
[----------] 1 test from NmslibCreateIndexTest (2 ms total)

[----------] 1 test from NmslibLoadIndexTest
[ RUN      ] NmslibLoadIndexTest.BasicAssertions
[       OK ] NmslibLoadIndexTest.BasicAssertions (1 ms)
[----------] 1 test from NmslibLoadIndexTest (1 ms total)

[----------] 1 test from NmslibQueryIndexTest
[ RUN      ] NmslibQueryIndexTest.BasicAssertions
[       OK ] NmslibQueryIndexTest.BasicAssertions (2 ms)
[----------] 1 test from NmslibQueryIndexTest (2 ms total)

[----------] 1 test from NmslibFreeTest
[ RUN      ] NmslibFreeTest.BasicAssertions
[       OK ] NmslibFreeTest.BasicAssertions (0 ms)
[----------] 1 test from NmslibFreeTest (0 ms total)

[----------] 1 test from NmslibInitLibraryTest
[ RUN      ] NmslibInitLibraryTest.BasicAssertions
[       OK ] NmslibInitLibraryTest.BasicAssertions (0 ms)
[----------] 1 test from NmslibInitLibraryTest (0 ms total)

[----------] 1 test from CommonsTests
[ RUN      ] CommonsTests.BasicAssertions
[       OK ] CommonsTests.BasicAssertions (0 ms)
[----------] 1 test from CommonsTests (0 ms total)

[----------] Global test environment tear-down
[==========] 22 tests from 20 test suites ran. (1101 ms total)
[  PASSED  ] 22 tests.

Issues Resolved

#1506

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@navneet1v navneet1v added skip-changelog v2.14.0 Enhancements Increases software capabilities beyond original client specifications labels Apr 3, 2024
@navneet1v navneet1v force-pushed the stream-vectors branch 3 times, most recently from 729a487 to 7fedcca Compare April 3, 2024 01:22
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 84.88%. Comparing base (fccc5a9) to head (673a62c).

Files Patch % Lines
...index/codec/KNN80Codec/KNN80DocValuesConsumer.java 90.00% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##             feature/stream-vectors    #1588      +/-   ##
============================================================
+ Coverage                     84.86%   84.88%   +0.02%     
- Complexity                     1375     1377       +2     
============================================================
  Files                           173      173              
  Lines                          5609     5611       +2     
  Branches                        553      554       +1     
============================================================
+ Hits                           4760     4763       +3     
+ Misses                          616      614       -2     
- Partials                        233      234       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

Took a first pass on cpp side. Overall looks good - a few minor comments. Will take another pass on java side next

jni/src/faiss_wrapper.cpp Outdated Show resolved Hide resolved
jni/src/faiss_wrapper.cpp Show resolved Hide resolved
Copy link
Member

@martin-gaievski martin-gaievski left a comment

Choose a reason for hiding this comment

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

Overall looks good, left minor comments

…emplate functions.

Signed-off-by: Navneet Verma <navneev@amazon.com>
Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

This LGTM. I agree that Java code should free up the vector data.

@navneet1v
Copy link
Collaborator Author

@martin-gaievski , @jmazanec15 added a new rev where the JNI tests are fixed and the comments are resolved.

@navneet1v navneet1v merged commit badbb1d into opensearch-project:feature/stream-vectors Apr 4, 2024
47 of 52 checks passed
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 9, 2024
…layer to enable creation of larger segments for vector indices

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 9, 2024
…layer to enable creation of larger segments for vector indices

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 9, 2024
…layer to enable creation of larger segments for vector indices

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (opensearch-project#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 9, 2024
…layer to enable creation of larger segments for vector indices

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (opensearch-project#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 9, 2024
…layer to enable creation of larger segments for vector indices

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (opensearch-project#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit that referenced this pull request Apr 10, 2024
…layer to enable creation of larger segments for vector indices (#1604)

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 10, 2024
…layer to enable creation of larger segments for vector indices (opensearch-project#1604)

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (opensearch-project#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit that referenced this pull request Apr 10, 2024
…layer to enable creation of larger segments for vector indices (#1604) (#1608)

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Apr 11, 2024
…layer to enable creation of larger segments for vector indices (opensearch-project#1604) (opensearch-project#1608)

Changes include:
1. Add the interface for streaming the vectors from java to jni layer with initial capacity (opensearch-project#1586)
2. Integrating storeVectors interfaces with createIndex and createIndexTemplate functions. (opensearch-project#1588)
3. Update KNN80BinaryDocValues reader count live docs and use live docs as initial capacity to initialize vector address(opensearch-project#1595)
4. Move free vectorAddress from Java to JNI layer to reduce the memory footprint for Nmslib (opensearch-project#1602)

Signed-off-by: Navneet Verma <navneev@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancements Increases software capabilities beyond original client specifications skip-changelog v2.14.0
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants