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

Improved OpenSearchKNN setup for doing benchmarks #412

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

navneet1v
Copy link
Contributor

@navneet1v navneet1v commented May 11, 2023

Description

Improved the OpenSearchKNN setup. The older setting of OpenSearch K-NN is not optimized for 1 CPU and 1 machine.

Changes involve:

  1. Use _id to store the id of the vector.
  2. Increase the timeouts for force_merge so that force_merge is actually performed.
  3. Setup the OpenSearch cluster with min distribution + K-NN plugin to remove all the background activities done by other plugins which are not required for doing vector search and indexing. The change ensure that plugin is added on top of core Opensearch.
  4. Updated the warmup to run after the index settings are changed for every query configuration, as warmup is useless if the index setting of ef_search is changed.
  5. While doing force_merge I set max segments to 5. This is to make sure that dataset with bigger dimensions can work. ideally whoever is running benchmarks they should reduce this value to 1 for better peformance.

@erikbern
Copy link
Owner

Is this ready to be merged?

@navneet1v
Copy link
Contributor Author

@erikbern Yes the code is ready for checked in. I resolved the conflicts which were present.

@navneet1v
Copy link
Contributor Author

Thanks @erikbern These checks were not running when I raised the PR. Let me see what is the issue.

@navneet1v
Copy link
Contributor Author

navneet1v commented Jun 13, 2023

@erikbern I looked into the run, and it is using 5.2GB of the memory. I was thinking we were running with around 16.5GB of memory. Can you please provide me info like what is the right set of memory which will be provided for containers?

As per documentation the tests are running on r6i.16xlarge and parallelization factor as 31. Which mean each container is getting ~16GB of Memory. The code in this PR is based on that.

Ref: https://github.com/erikbern/ann-benchmarks/blob/main/README.md#results

@navneet1v
Copy link
Contributor Author

On thinking over this more, I can see if there is a way I can get docker memory in the docker file and then take a call what should be the heap config. If there is no way I can add comments and switch to 3GB as memory.

@erikbern any thoughts

@erikbern
Copy link
Owner

It's the standard Github Actions runners right now: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners

It would be great if there's an easy way to configure it dynamically

@navneet1v
Copy link
Contributor Author

navneet1v commented Jun 13, 2023

It's the standard Github Actions runners right now: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners

It would be great if there's an easy way to configure it dynamically

sure I will start lookin into this, but in meanwhile I flip to 3GB so that this PR can be merged.

@navneet1v
Copy link
Contributor Author

@erikbern updated the PR, we run github actions to see if they succeeds.

@erikbern
Copy link
Owner

It's failing with a weird issue now: ValueError: invalid literal for int() with base 10: 'Ufj6togBCGDF5SIY3MlP'

@navneet1v
Copy link
Contributor Author

It's failing with a weird issue now: ValueError: invalid literal for int() with base 10: 'Ufj6togBCGDF5SIY3MlP'

taking a look.

@navneet1v
Copy link
Contributor Author

navneet1v commented Jun 13, 2023

@erikbern
Fixed the issue.

Seeing successful run on local.

['angular', 20, {'M': 36, 'efConstruction': 500}]
Trying to instantiate ann_benchmarks.algorithms.opensearchknn.OpenSearchKNN(['angular', 20, {'M': 36, 'efConstruction': 500}])
got a train set of size (9000 * 20)
got 1000 queries
Uploading data to the Index: os-m-36-efconstruction-500
Force Merge iteration 1...
Refreshing the Index...
Built index in 8.461687326431274
Index size:  592.0
Running query argument group 1 of 9...
Running Warmup API after setting query arguments...
{
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "failed" : 0
  }
}

Run 1/5...
Processed 1000/1000 queries...
Run 2/5...
Processed 1000/1000 queries...
Run 3/5...
Processed 1000/1000 queries...
Run 4/5...
100%|██████████| 9000/9000 [00:04<00:00, 2174.88it/s]
/usr/local/lib/python3.10/dist-packages/opensearchpy/connection/base.py:174: OpenSearchWarning: Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled
  warnings.warn(message, category=OpenSearchWarning)
Processed 1000/1000 queries...
Run 5/5...
Processed 1000/1000 queries...
Running query argument group 2 of 9...
Running Warmup API after setting query arguments...
{
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "failed" : 0
  }
}

Run 1/5...
Processed 1000/1000 queries...
Run 2/5...
Processed 1000/1000 queries...
Run 3/5...

@navneet1v
Copy link
Contributor Author

@erikbern can we merge the changes?

@erikbern erikbern merged commit 9184eac into erikbern:main Jun 14, 2023
@erikbern
Copy link
Owner

NIce – it's working now!

@vamshin
Copy link

vamshin commented Jun 14, 2023

@erikbern when would the benchmarks be trigged with the latest changes?

@erikbern
Copy link
Owner

I don't run it at any specific interval. Historically it's been 1y+ between runs. But given how fast the field is moving, I think it's probably worth running it again quite soon! Maybe in a few weeks?

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.

3 participants