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

Introduces query parameter ef_search for NMSLIB #1732

Merged

Conversation

shatejas
Copy link
Contributor

@shatejas shatejas commented Jun 7, 2024

Description

Integrates NMSLIB with ef_search

curl -XGET "http://localhost:9200/nmslib-hnsw-index/_search" -H 'Content-Type: application/json' -d'
{
  "query": {
    "knn": {
      "nmslib_vector": {
        "vector": [2, 3, 5],
        "k": 2,
        "method_parameters" : {
            "ef_search" : 200
        }
        }
      }
    }
  }'
{
  "took": 6,
  "timed_out": false,
  "_shards": {
    "total": 1,
    "successful": 1,
    "skipped": 0,
    "failed": 0
  },
  "hits": {
    "total": {
      "value": 2,
      "relation": "eq"
    },
    "max_score": 0.11918951,
    "hits": [
      {
        "_index": "nmslib-hnsw-index",
        "_id": "3",
        "_score": 0.11918951,
        "_source": {
          "nmslib_vector": [
            3.5,
            4.5,
            3.3
          ],
          "price": 12.9
        }
      },
      {
        "_index": "nmslib-hnsw-index",
        "_id": "2",
        "_score": 0.10706638,
        "_source": {
          "nmslib_vector": [
            2.5,
            3.5,
            2.2
          ],
          "price": 7.1
        }
      }
    ]
  }
}

Issues Resolved

#1537

Check List - Pending

  • 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.

Date: Mon, 27 May 2024 22:02:12 -0700
Subject: [PATCH] Adds ability to pass ef parameter in the query for hnsw

It defaults to index ef_ value if its not type HNSWQuery
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the diff for this patch

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't there be a unit test for this change in nmslib code base?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no unit test in the entire lib. There is no way to test it out unless we end up writing it for the entire search which will become a heavy lift.

There is an integ test though and I tested it out using this code shatejas/nmslib@c5dd7cd. I did not add it in the patch because its not generic enough. There are some validations in the hnsw code which is making it difficult to add more parameters in the integ test

@heemin32

@navneet1v
Copy link
Collaborator

@shatejas please fix the failing GH actions

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.

lgtm - waiting on CI

@shatejas
Copy link
Contributor Author

@shatejas please fix the failing GH actions

The build in linux is failing with the same failure as main

jni/tests/nmslib_wrapper_unit_test.cpp Outdated Show resolved Hide resolved
Date: Mon, 27 May 2024 22:02:12 -0700
Subject: [PATCH] Adds ability to pass ef parameter in the query for hnsw

It defaults to index ef_ value if its not type HNSWQuery
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't there be a unit test for this change in nmslib code base?

jni/src/commons.cpp Outdated Show resolved Hide resolved
@heemin32
Copy link
Collaborator

Build failure will be fixed in #1748

@heemin32 heemin32 merged commit 79148f9 into opensearch-project:feature/ef-search Jun 13, 2024
41 of 52 checks passed
@shatejas shatejas deleted the ef-search-nmslib branch August 29, 2024 00:55
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.

5 participants