Skip to content

fix: slow hnsw by caching num docs #1706

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.

Sign up for GitHub

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

Merged
jupyterjazz merged 5 commits into from
Jul 18, 2023
Merged

fix: slow hnsw by caching num docs #1706

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d31526
fix: slow hnsw by caching num docs
jupyterjazz Jul 18, 2023
232f6be
chore: remove unused line
jupyterjazz Jul 18, 2023
a411c5e
refactor: set num docs to 0
jupyterjazz Jul 18, 2023
916ed27
refactor: go back to the initial solution
jupyterjazz Jul 18, 2023
ace5e1a
Merge branch 'main' into fix-hnsw-performance
jupyterjazz Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docarray/index/backends/hnswlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ def __init__(self, db_config=None, **kwargs):
self._sqlite_cursor = self._sqlite_conn.cursor()
self._create_docs_table()
self._sqlite_conn.commit()
self._num_docs = self._get_num_docs_sqlite()
self._logger.info(f'{self.__class__.__name__} has been initialized')

@property
Expand Down Expand Up @@ -259,6 +260,7 @@ def index(self, docs: Union[BaseDoc, Sequence[BaseDoc]], **kwargs):

self._send_docs_to_sqlite(docs_validated)
self._sqlite_conn.commit()
self._num_docs = self._get_num_docs_sqlite()

def execute_query(self, query: List[Tuple[str, Dict]], *args, **kwargs) -> Any:
"""
Expand Down Expand Up @@ -379,6 +381,7 @@ def _del_items(self, doc_ids: Sequence[str]):

self._delete_docs_from_sqlite(doc_ids)
self._sqlite_conn.commit()
self._num_docs = self._get_num_docs_sqlite()

def _get_items(self, doc_ids: Sequence[str], out: bool = True) -> Sequence[TSchema]:
"""Get Documents from the hnswlib index, by `id`.
Expand All @@ -403,7 +406,7 @@ def num_docs(self) -> int:
"""
Get the number of documents.
"""
return self._get_num_docs_sqlite()
return self._num_docs

@JoanFM JoanFM Jul 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How do we update this value?

I think it may be better to handle it here totally: here do:

self._num_docs = self._num_docs or self._get_num_docs_sqlite()
return self._num_docs

And simply in all updates, deletes, index u set self._num_docs to 0.

@jupyterjazz jupyterjazz Jul 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we update the value in the end of index and del, I think this way is more intuitive rather than setting it to 0

@JoanFM JoanFM Jul 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

okey, and does it have update?

@jupyterjazz jupyterjazz Jul 18, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes there are sufficient tests for this and they all pass. I modified it to the way you suggested


###############################################
# Helpers #
Expand Down