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.

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

Remove skip building graph check for quantization use case #2430

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Remove skip building graph check for quantization use case #2430

merged 1 commit into from
Jan 24, 2025

Conversation

VijayanB
Copy link
Member

@VijayanB VijayanB commented Jan 23, 2025 •
edited
Loading

Description

For on-disk quantization indices, we don't have to apply building graph check since it is already faster, and we found is causing latency spike during search. Hence, done check for threshold if quantization criteria is met.

Related Issues

part of #2215

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 remove-threshold-for-quants branch from 473c7db to ced2035 Compare January 23, 2025 20:54
@VijayanB VijayanB marked this pull request as ready for review January 23, 2025 21:02
@VijayanB
Copy link
Member Author

I am raising directly to 2.x since i am blocked because of lucene codec issue. Will backport this to main once it is fixed.

@VijayanB
Copy link
Member Author

Currently quantization state is not null only for on-disk mode. We don't support in-memory quantization at this moment. Hence, this check is good enough. However if we want to check for on-disk, we have to pass this as parameter and add it to check.

@jmazanec15 @shatejas @Vikasht34 @navneet1v What do you think?

@VijayanB
Remove skip building graph check for quantization use case
db017b6 
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 <[email protected]>
@VijayanB VijayanB force-pushed the remove-threshold-for-quants branch from ced2035 to db017b6 Compare January 23, 2025 21:25
@jmazanec15
Copy link
Member

In general, the "mode" is meant to select defaults for other parameters, so it should not be directly used to disable and enable certain features. Instead, it may resolve to a parameter that then makes this decision.

Anyway, in the PR, it appears that you actually disable if quantization is used. I think in general something like this is okay, but might be misleading/confusing to users who set the setting anticipating that graph builds will be skipped.

That being said, is this just a concern of performance, or is it a functional bug? If it is performance related only, Id recommend using mode to resolve a per-field value for skipping graph build. Then, users could override if need be. This would be consistent with current approach around mode.

shatejas
shatejas reviewed Jan 24, 2025
View reviewed changes
Copy link
Collaborator

@shatejas shatejas left a comment

Choose a reason for hiding this comment

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

Do we need to update documentation around this?

@shatejas shatejas mentioned this pull request Jan 24, 2025
Open
5 tasks
@VijayanB
Copy link
Member Author

Do we need to update documentation around this?

Yes, i will update that this parameter will be no-hop for lucene, and on-disk

@VijayanB VijayanB merged commit 8d13c74 into opensearch-project:2.x Jan 24, 2025
102 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 24, 2025
@VijayanB @github-actions
Remove skip building graph check for quantization use case (#2430)
f4c2c8d 
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 <[email protected]>
(cherry picked from commit 8d13c74)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jan 24, 2025
Open
VijayanB added a commit to VijayanB/k-NN-2 that referenced this pull request Feb 11, 2025
@VijayanB
Remove skip building graph check for quantization use case (opensearc…
33e3782 
…h-project#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 <[email protected]>
@VijayanB VijayanB mentioned this pull request Feb 11, 2025
Merged
5 tasks
VijayanB added a commit that referenced this pull request Feb 12, 2025
@VijayanB
Backport to main (#2520)
9baf3e4 
* 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 <[email protected]>

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

Signed-off-by: Balasubramanian <[email protected]>

* Update changelog

Signed-off-by: Balasubramanian <[email protected]>

---------

Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Balasubramanian <[email protected]>
navneet1v pushed a commit that referenced this pull request Feb 17, 2025
@rchitale7 @sam-herman
@VijayanB @opensearch-trigger-bot @peterzhuamazon @shatejas @kotwanikunal @jmazanec15
[Remote Vector Index Build] Initial implementation of Index Build Ser…
91d66eb 
…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 <[email protected]>

* fix make path

Signed-off-by: Samuel Herman <[email protected]>

* changelog update

Signed-off-by: Samuel Herman <[email protected]>

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

Signed-off-by: Samuel Herman <[email protected]>

* fix make discovery for gradle

Signed-off-by: Samuel Herman <[email protected]>

* fix cmake path for macOS

Signed-off-by: Samuel Herman <[email protected]>

---------

Signed-off-by: Samuel Herman <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

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

Signed-off-by: Peter Zhu <[email protected]>

* Remove test comments

Signed-off-by: Peter Zhu <[email protected]>

* Remove test comments

Signed-off-by: Peter Zhu <[email protected]>

* Remove test comments

Signed-off-by: Peter Zhu <[email protected]>

* Updating restart and rolling upgrade bwc test bundle.gradle

Signed-off-by: Peter Zhu <[email protected]>

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

Signed-off-by: Peter Zhu <[email protected]>

---------

Signed-off-by: Peter Zhu <[email protected]>
(cherry picked from commit 107c4f1)

Co-authored-by: Peter Zhu <[email protected]>

---------

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

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

* Adds debug logs for KNNQuery and KNNWeight

Signed-off-by: Tejas Shah <[email protected]>

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

Signed-off-by: Tejas Shah <[email protected]>

* Addressing comments on the PR

Signed-off-by: Tejas Shah <[email protected]>

* Adds shard and segment info in the logs

Signed-off-by: Tejas Shah <[email protected]>

* Removes unnecessary segment name param from exact search

Signed-off-by: Tejas Shah <[email protected]>

* Fixes the build

Signed-off-by: Tejas Shah <[email protected]>

---------

Signed-off-by: Tejas Shah <[email protected]>
(cherry picked from commit f322e27)

Co-authored-by: Tejas Shah <[email protected]>

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

* clean JNI artifacts with ./gradlew clean

Signed-off-by: Samuel Herman <[email protected]>

* nest release under build directory

Signed-off-by: Samuel Herman <[email protected]>

* adjust all references to the old release path

Signed-off-by: Samuel Herman <[email protected]>

* remove irrelevant paths from gitignore, add jni/build

Signed-off-by: Samuel Herman <[email protected]>

* fix logging on linux

Signed-off-by: Samuel Herman <[email protected]>

---------

Signed-off-by: Samuel Herman <[email protected]>

* 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 <[email protected]>

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

Signed-off-by: Balasubramanian <[email protected]>

* Update changelog

Signed-off-by: Balasubramanian <[email protected]>

---------

Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Balasubramanian <[email protected]>

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

* Add release notes for 2.19.0

Signed-off-by: Kunal Kotwani <[email protected]>

* Fix links for release notes

Co-authored-by: John Mazanec <[email protected]>
Signed-off-by: Kunal Kotwani <[email protected]>

---------

Signed-off-by: Kunal Kotwani <[email protected]>
Co-authored-by: John Mazanec <[email protected]>

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

Signed-off-by: Peter Zhu <[email protected]>

* Minor performance improvments in KNNQueryBuilder (#2528)

Signed-off-by: Tejas Shah <[email protected]>

* Initial implementation of control flow

Signed-off-by: Rohan Chitale <[email protected]>

---------

Signed-off-by: Samuel Herman <[email protected]>
Signed-off-by: Balasubramanian <[email protected]>
Signed-off-by: Vijayan Balasubramanian <[email protected]>
Signed-off-by: Kunal Kotwani <[email protected]>
Signed-off-by: Peter Zhu <[email protected]>
Signed-off-by: Tejas Shah <[email protected]>
Signed-off-by: Rohan Chitale <[email protected]>
Co-authored-by: sam-herman <[email protected]>
Co-authored-by: Vijayan Balasubramanian <[email protected]>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-by: Peter Zhu <[email protected]>
Co-authored-by: Tejas Shah <[email protected]>
Co-authored-by: Kunal Kotwani <[email protected]>
Co-authored-by: John Mazanec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shatejas shatejas shatejas left review comments

@navneet1v navneet1v navneet1v approved these changes

@jmazanec15 jmazanec15 jmazanec15 approved these changes

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@ryanbogan ryanbogan Awaiting requested review from ryanbogan ryanbogan is a code owner

@luyuncheng luyuncheng Awaiting requested review from luyuncheng luyuncheng is a code owner

@0ctopus13prime 0ctopus13prime Awaiting requested review from 0ctopus13prime 0ctopus13prime is a code owner

Assignees
No one assigned
Labels
backport main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VijayanB @jmazanec15 @navneet1v @shatejas