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

Update default to 0 to always build graph as default behavior #2452

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Jan 27, 2025

Description

Update default to 0 to always build graph as default behavior

Conclusion

We performed following experiment to analyze the increase in latency (p90)
Data set: Cohere
Vector Count: 10M
Dimension: 768
Force merge to 1 segment: No
Concurrent Segment Enabled: yes
No of runs per limit: 5

Results

Comparison
P90 Metrics (ms) for Search

Metric LIMIT = 0 LIMIT = 5K LIMIT = 10K LIMIT = 15K
MIN 21.56 22.8 25.4 27.1
MAX 22.5 23.4 26.2 28.1
AVG 21.56 22.94 25.6 27.78

Indexing

LIMIT Total Indexing Time (secs)
0 2023
5K 1306
10K 1210
15K 1016

In order to avoid any regression from vector search, we are falling back to 0 as limit. However, users can configure this if they want to improve indexing performance by considering how that will impact the search latency.

The drop in latency is due to exact search was preferred over Approx search for segments where limit is not met.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@VijayanB VijayanB force-pushed the update-default branch 2 times, most recently from fea471c to 1ece3d8 Compare January 27, 2025 22:48
Signed-off-by: Balasubramanian <balasvij@amazon.com>
@VijayanB
Copy link
Member Author

We performed following experiment to analyze the drop in latency (p90)
Data set: Cohere
Vector Count: 10M
Dimension: 768
Force merge to 1 segment: No
Concurrent Segment Enabled: yes
No of runs per limit: 5

Results

Comparison
P90 Metrics

Metric LIMIT = 0 LIMIT = 5K LIMIT = 10K LIMIT = 15K
MIN 21.56 22.8 25.4 27.1
MAX 22.5 23.4 26.2 28.1
AVG 21.56 22.94 25.6 27.78

In order to avoid any regression from approximate search, we are falling back to 0 as limit. However, users can configure this if they want to improve indexing performance by considering how that will impact the search latency.

The drop in latency is due to exact search was preferred over Approx search for segments where limit is not met.

@navneet1v
Copy link
Collaborator

We performed following experiment to analyze the drop in latency (p90) Data set: Cohere Vector Count: 10M Dimension: 768 Force merge to 1 segment: No Concurrent Segment Enabled: yes No of runs per limit: 5

Results

Comparison P90 Metrics

Metric LIMIT = 0 LIMIT = 5K LIMIT = 10K LIMIT = 15K
MIN 21.56 22.8 25.4 27.1
MAX 22.5 23.4 26.2 28.1
AVG 21.56 22.94 25.6 27.78
In order to avoid any regression from approximate search, we are falling back to 0 as limit. However, users can configure this if they want to improve indexing performance by considering how that will impact the search latency.

The drop in latency is due to exact search was preferred over Approx search for segments where limit is not met.

Thanks for the details. Please add the indexing time also for these runs.

@navneet1v
Copy link
Collaborator

@VijayanB please update the documentation too with details on how to set these numbers.

@navneet1v
Copy link
Collaborator

Indexing

LIMIT Total Indexing Time (ms)
0 2023
5K 1306
10K 1210
15K 1016

is this ms or sec?

@VijayanB
Copy link
Member Author

VijayanB commented Jan 28, 2025

Indexing
LIMIT Total Indexing Time (ms)
0 2023
5K 1306
10K 1210
15K 1016

is this ms or sec?
You are right. It is seconds. I fixed it. Thanks

@VijayanB VijayanB merged commit 3885256 into opensearch-project:2.x Jan 28, 2025
120 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to main failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-main main
# Navigate to the new working tree
cd .worktrees/backport-main
# Create a new branch
git switch --create backport/backport-2452-to-main
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 388525617e7cb6740821c575650fdb7a55fbcc4b
# Push it to GitHub
git push --set-upstream origin backport/backport-2452-to-main
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-main

Then, create a pull request where the base branch is main and the compare/head branch is backport/backport-2452-to-main.

VijayanB added a commit to VijayanB/k-NN-2 that referenced this pull request Feb 11, 2025
@VijayanB VijayanB mentioned this pull request Feb 11, 2025
5 tasks
VijayanB added a commit that referenced this pull request Feb 12, 2025
* Remove skip building graph check for quantization use case (#2430)

For quantization indices, we don't have to apply building graph check
since it is already faster, this is now only applied for fp32/16 indices
and where threshold is configured.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Update default to 0 to always build graph as default behavior (#2452)

Signed-off-by: Balasubramanian <balasvij@amazon.com>

* Update changelog

Signed-off-by: Balasubramanian <balasvij@amazon.com>

---------

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Balasubramanian <balasvij@amazon.com>
navneet1v pushed a commit that referenced this pull request Feb 17, 2025
…vice (#2532)

* fix broken build flag, move build to one directory (#2442)

* move build to one directory, fix broken flag

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* fix make path

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* changelog update

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* add fix for classpath change and for cmake discovery on macos

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* fix make discovery for gradle

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* fix cmake path for macOS

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

---------

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* Update package name to fix compilation issue (#2513)

* Update package name to fix compilation issue

Core renamed this package in opensearch-project/OpenSearch#17272
This commit renames package accordingly

Signed-off-by: Balasubramanian <balasvij@amazon.com>

* Update build.gradle and build.sh to separate x64 linux nmslib build with different gcc versions (#2506) (#2508)

* Update build gradle to separate nmslib / faiss generation

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Update scripts/build.sh to separate enable gcc switch

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove test comments

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove test comments

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Remove test comments

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Updating restart and rolling upgrade bwc test bundle.gradle

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Enforce gcc10 for nmslib to compile and avx512_spr have no-op

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
(cherry picked from commit 107c4f1)

Co-authored-by: Peter Zhu <zhujiaxi@amazon.com>

---------

Signed-off-by: Balasubramanian <balasvij@amazon.com>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Peter Zhu <zhujiaxi@amazon.com>

* Adds debug logs for KNNQuery and KNNWeight (#2466) (#2470)

* Adds debug logs for KNNQuery and KNNWeight

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Adds check to see if log is enabled to start and stop StopWatch

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Addressing comments on the PR

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Adds shard and segment info in the logs

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Removes unnecessary segment name param from exact search

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Fixes the build

Signed-off-by: Tejas Shah <shatejas@amazon.com>

---------

Signed-off-by: Tejas Shah <shatejas@amazon.com>
(cherry picked from commit f322e27)

Co-authored-by: Tejas Shah <shatejas@amazon.com>

* Clean JNI artifacts with ./gradlew clean (#2516)

* clean JNI artifacts with ./gradlew clean

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* nest release under build directory

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* adjust all references to the old release path

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* remove irrelevant paths from gitignore, add jni/build

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* fix logging on linux

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

---------

Signed-off-by: Samuel Herman <sherman8915@gmail.com>

* Backport to main (#2520)

* Remove skip building graph check for quantization use case (#2430)

For quantization indices, we don't have to apply building graph check
since it is already faster, this is now only applied for fp32/16 indices
and where threshold is configured.

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>

* Update default to 0 to always build graph as default behavior (#2452)

Signed-off-by: Balasubramanian <balasvij@amazon.com>

* Update changelog

Signed-off-by: Balasubramanian <balasvij@amazon.com>

---------

Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Balasubramanian <balasvij@amazon.com>

* [Backport main] Add release notes for 2.19.0 (#2503)

* Add release notes for 2.19.0

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>

* Fix links for release notes

Co-authored-by: John Mazanec <jmazane@amazon.com>
Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>

---------

Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
Co-authored-by: John Mazanec <jmazane@amazon.com>

* Fix main knnlib dir in build script based on #2442 (#2526)

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>

* Minor performance improvments in KNNQueryBuilder (#2528)

Signed-off-by: Tejas Shah <shatejas@amazon.com>

* Initial implementation of control flow

Signed-off-by: Rohan Chitale <r.chitale7@gmail.com>

---------

Signed-off-by: Samuel Herman <sherman8915@gmail.com>
Signed-off-by: Balasubramanian <balasvij@amazon.com>
Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
Signed-off-by: Kunal Kotwani <kkotwani@amazon.com>
Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
Signed-off-by: Tejas Shah <shatejas@amazon.com>
Signed-off-by: Rohan Chitale <r.chitale7@gmail.com>
Co-authored-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
Co-authored-by: Vijayan Balasubramanian <balasvij@amazon.com>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Peter Zhu <zhujiaxi@amazon.com>
Co-authored-by: Tejas Shah <shatejas@amazon.com>
Co-authored-by: Kunal Kotwani <kkotwani@amazon.com>
Co-authored-by: John Mazanec <jmazane@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants