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

pgvector improvements #98

Merged
merged 5 commits into from
Apr 11, 2024
Merged

pgvector improvements #98

merged 5 commits into from
Apr 11, 2024

Conversation

ankane
Copy link
Contributor

@ankane ankane commented Jan 31, 2024

Hi, thanks for creating this framework! This PR makes a number of improvements to pgvector performance.

  1. Separates uploading and indexing
  2. Removes an extra query from each search
  3. Sets parameters for the Postgres server
  4. Updates pgvector to 0.6.0

raise IncompatibilityError(f"Unsupported distance metric: {distance}")

cls.conn.execute("SET max_parallel_workers = 128")
cls.conn.execute("SET max_parallel_maintenance_workers = 128")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will use all available cores to build the index (let me know if it should do something different)

Copy link
Member

Choose a reason for hiding this comment

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

If you want to utilize all cores, would not it be better to get the number of cores from OS rather than to hardcode it to 128?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting a high upper limit should be simpler than trying to get the exact number of cores from the server (and won't make a difference in the number of cores used).

# maintenance_work_mem should be ~65%
command: postgres -c shared_buffers=2GB -c maintenance_work_mem=5GB -c max_connections=200
# shm_size should be shared_buffers + maintenance_work_mem
shm_size: 7g
Copy link
Contributor Author

@ankane ankane Jan 31, 2024

Choose a reason for hiding this comment

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

These settings should be updated based on the hardware used to run the benchmark

Edit: Updated the parameters for 25 GB of memory in a follow-up commit

"search_params": [
{ "parallel": 1, "search_params": { "hnsw_ef": 128 } }
{ "parallel": 8, "search_params": { "hnsw_ef": 128 } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the parallel options for pgvector-default to be the same as qdrant-default in qdrant-single-node.json

Copy link
Member

Choose a reason for hiding this comment

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

It's not used for running the experiments. But okay :)

@@ -3,13 +3,17 @@ version: '3.7'
services:
pgvector:
container_name: pgvector
image: ankane/pgvector:v0.5.1
image: pgvector/pgvector:pg16
Copy link
Contributor Author

@ankane ankane Jan 31, 2024

Choose a reason for hiding this comment

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

New Docker image as of v0.6.0

Edit: Updated to use the versioned tag 0.6.0-pg16

with cls.cur.copy(
"COPY items (id, embedding) FROM STDIN WITH (FORMAT BINARY)"
) as copy:
copy.set_types(["integer", "vector"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Binary format requires set_types in psycopg 3

@@ -7,6 +7,7 @@
def get_db_config(host, connection_params):
return {
"host": host or "localhost",
"port": PGVECTOR_PORT,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PGVECTOR_PORT was previously unused

Comment on lines +51 to +53
cls.conn.execute(
f"CREATE INDEX ON items USING hnsw (embedding {hnsw_distance_type}) WITH (m = {cls.upload_params['hnsw_config']['m']}, ef_construction = {cls.upload_params['hnsw_config']['ef_construct']})"
)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, might be a bit unfair to the other competitors, which also support loading in a "split-mode", however everyone is welcome to come and improve the setup if they think it is not optimised 🤷‍♂️

@joein
Copy link
Member

joein commented Feb 4, 2024

@KShivendu could you make a couple of test runs to make sure that everything works fine?

Copy link
Member

@KShivendu KShivendu left a comment

Choose a reason for hiding this comment

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

It worked as expected. Thanks for the contribution!

Please make a few small changes as I suggested and I'd be happy to approve :)

@@ -3,13 +3,17 @@ version: '3.7'
services:
pgvector:
container_name: pgvector
image: ankane/pgvector:v0.5.1
image: pgvector/pgvector:0.6.0-pg16
Copy link
Member

@KShivendu KShivendu Mar 7, 2024

Choose a reason for hiding this comment

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

Can you please use the official image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the official image starting with 0.6.0. https://github.com/pgvector/pgvector#docker-1

"search_params": [
{ "parallel": 1, "search_params": { "hnsw_ef": 128 } }
{ "parallel": 8, "search_params": { "hnsw_ef": 128 } }
Copy link
Member

Choose a reason for hiding this comment

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

It's not used for running the experiments. But okay :)

@ankane
Copy link
Contributor Author

ankane commented Mar 7, 2024

Rebased to fix merge conflict from #100

Copy link
Member

@KShivendu KShivendu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for contributing!

@KShivendu KShivendu merged commit f4436e4 into qdrant:master Apr 11, 2024
1 check passed
KShivendu pushed a commit that referenced this pull request Apr 11, 2024
* pgvector improvements

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Updated Postgres parameters

* Use versioned Docker image

* Updated pgvector to 0.6.2

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@ankane
Copy link
Contributor Author

ankane commented Apr 11, 2024

Thanks

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