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

[ENH] Dynamic ef_search #15

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

atroyn
Copy link

@atroyn atroyn commented Jun 28, 2024

ef_search is a parameter consumed at query time which determines how many edges of the HNSW graph are traversed to find approximate nearest neighbors. Setting this hyperparameter allows HNSW to trade recall for speed at query time.

The way this was implemented by HNSWlib had some bad design decisions. It was a property of each index, changed by calling set_ef on the index; besides being cumbersome, it also creates a data race in concurrent execution where multiple threads want to execute queries with different ef_search.

Additionally, it was not written out with the index, and when the index was loaded again, it was set to 10, creating an unncessary footgun.

This PR fixes all of the above. It:

  • changes the index parameter name to ef_search_default_ to make it clear what this is for, and renames the function signatures setting it accordingly.

  • adds a shared mutex which is locked when writing, but allows parallel reading.

  • stores the default when the index is written out, and reads it when it's loaded

  • adds an argument to the query path allowing each query to set it independently - this is passed by value so it's on the stack of the function call, rather than on the heap. if it's not passed, we read the index default

  • updates all tests and examples

Making this work required us to go up to C++ 17 and change the mac OS target to be 10.12 or later. These are both ancient, and we compile this for our users anyway.

@atroyn atroyn requested a review from HammadB August 22, 2024 03:07
@atroyn
Copy link
Author

atroyn commented Aug 22, 2024

This will require additional work to integrate into Chroma, before we release, since it does incur an API change. I think the right thing to do is to pull this into the monorepo, under a cpp directory. That can then also house our inference execution environment if we use cpp for it.

Alternatively, we can leave the original API calls and redirect them to the new ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant