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

Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests #926

Conversation

navneet1v
Copy link
Collaborator

Description

Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests

Issues Resolved

#903

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.

@codecov
Copy link

codecov bot commented Jun 4, 2023

Codecov Report

Merging #926 (6fe46e9) into feature/faiss-filtering (119b8d6) will not change coverage.
The diff coverage is n/a.

❗ Current head 6fe46e9 differs from pull request most recent head 5bb4a0f. Consider uploading reports for the commit 5bb4a0f to get more accurate results

@@                    Coverage Diff                     @@
##             feature/faiss-filtering     #926   +/-   ##
==========================================================
  Coverage                      83.92%   83.92%           
  Complexity                      1092     1092           
==========================================================
  Files                            152      152           
  Lines                           4492     4492           
  Branches                         400      400           
==========================================================
  Hits                            3770     3770           
  Misses                           538      538           
  Partials                         184      184           

index_name: target_index
- name: create_index
index_name: target_index
index_spec: /home/ec2-user/[PATH]/index.json
Copy link
Member

Choose a reason for hiding this comment

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

Shall we use /home/ec2-user by a root path parameter instead of hardcoded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will require changes in the core perf tool. We can take this as a backlog. It was already like this on the test.yml so I am using it as it is.

@@ -23,6 +23,7 @@ class TestConfig:
test_name: str
test_id: str
endpoint: str
port: int
Copy link
Member

Choose a reason for hiding this comment

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

If port is configurable we need to update description of test params in benchmarks/perf-tool/README

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack.

@@ -0,0 +1,35 @@
endpoint: "navneev.aka.corp.amazon.com"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to change this to endpoint: [ENDPOINT]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,35 @@
endpoint: "navneev.aka.corp.amazon.com"
port: 9900
Copy link
Member

Choose a reason for hiding this comment

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

Also, the port

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

port I will remove

Copy link
Member

Choose a reason for hiding this comment

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

If we are using ELB endpoint and if I send the port(80) with endpoint, it doesn't work right ? We need to set it separately like you added before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we need to set separately.

Copy link
Member

Choose a reason for hiding this comment

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

So, shall we add the placeholder to set port in all .yml files ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's fine. not sure how many times we use ELB. Plus I have added things in documentation so we should be good.

@@ -0,0 +1,37 @@
endpoint: [ENDPOINT]
test_name: "Faiss HNSW Restrictive Filter Test"
Copy link
Member

Choose a reason for hiding this comment

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

We need to add port: here and in other .yml files as well. Else, it will pick default port as 80 instead of 9200 for localhost and will fail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

making default value as 9200

…ated the perf-tool to include Faiss HNSW tests

Signed-off-by: Navneet Verma <[email protected]>
@navneet1v navneet1v force-pushed the feature/faiss-filtering branch from e53070f to 5bb4a0f Compare June 5, 2023 22:38
@navneet1v navneet1v merged commit 1135d79 into opensearch-project:feature/faiss-filtering Jun 5, 2023
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…ated the perf-tool to include Faiss HNSW tests (opensearch-project#926)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…ated the perf-tool to include Faiss HNSW tests (opensearch-project#926)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…es include

 * Enabled the efficient filtering support for Faiss Engine (opensearch-project#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (opensearch-project#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (opensearch-project#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (opensearch-project#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (opensearch-project#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…es include

 * Enabled the efficient filtering support for Faiss Engine (opensearch-project#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (opensearch-project#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (opensearch-project#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (opensearch-project#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (opensearch-project#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…es include

 * Enabled the efficient filtering support for Faiss Engine (opensearch-project#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (opensearch-project#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (opensearch-project#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (opensearch-project#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (opensearch-project#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…es include

 * Enabled the efficient filtering support for Faiss Engine (opensearch-project#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (opensearch-project#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (opensearch-project#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (opensearch-project#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (opensearch-project#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit that referenced this pull request Jun 14, 2023
…es include (#936)

* Enabled the efficient filtering support for Faiss Engine (#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jun 14, 2023
…es include (opensearch-project#936)

* Enabled the efficient filtering support for Faiss Engine (opensearch-project#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (opensearch-project#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (opensearch-project#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (opensearch-project#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (opensearch-project#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit to navneet1v/k-NN that referenced this pull request Jul 10, 2023
…es include (opensearch-project#936)

* Enabled the efficient filtering support for Faiss Engine (opensearch-project#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (opensearch-project#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (opensearch-project#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (opensearch-project#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (opensearch-project#934)

Signed-off-by: Navneet Verma <[email protected]>
navneet1v added a commit that referenced this pull request Jul 10, 2023
…es include (#936)

* Enabled the efficient filtering support for Faiss Engine (#907)
 * Fixed the ef_search default value for faiss HNSW with filters and updated the perf-tool to include Faiss HNSW tests (#926)
 * Added exact search for cases when filteredIds < k to improve the recall for exact search (#928)
 * Improved Exact Search to return only K results and added client side latency metric for query Benchmarks (#933)
 * Added Integration Tests and Unit test for Efficient Filtering for Faiss Engine (#934)

Signed-off-by: Navneet Verma <[email protected]>
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.

4 participants