feat(index): add epsilla connector by tonyyanga · Pull Request #1835 · docarray/docarray · GitHub

This PR adds the EpsillaDocumentIndex. It allows docarray users to use Epsilla vector database for queries.

It is the 1st PR for issue #1832. I intend to make a follow-up PR to add documentation for the connector, but want to limit the size of the PR to make it easier to review.

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (522811f) 85.05% compared to head (03c2b9c) 85.07%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
docarray/index/backends/epsilla.py	87.44%	27 Missing ⚠️
docarray/index/__init__.py	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
+ Coverage   85.05%   85.07%   +0.02%     
==========================================
  Files         135      136       +1     
  Lines        9031     9254     +223     
==========================================
+ Hits         7681     7873     +192     
- Misses       1350     1381      +31

Flag	Coverage Δ
docarray	85.07% <86.42%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thanks for the nice contribution, I have some comments about the PR.

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

@JoanFM Thanks for the review!

I have updated the input validation logic to report specific parameters missing.

It is unfortunate that Epsilla's python library does not include pythonic handling of error (throw instead of status codes). It directly returns HTTP status codes. For now I replaced it with from http import HTTPStatus.

When there is a conflict on loading the db, Epsilla returns HTTP 500 when it should return HTTP 409. I have filed an issue, but parsing the string error message is the best workaround for now.

What do you think? Please let me know if there are other issues we should address.

if the quick is fix on Epsilla side, would it be possible to wait for a patch on epsille side?

isn't there a way to know the number of docs?

There is currently no public API for counting the number of docs. One workaround is to run filter query without any condition, but I worry that is quite inefficient. Would you prefer to implement num_docs with that rather than throwing here?

If there is no API, okey. But I will double check if some functionality actually depends on this

I double checked. num_docs impacts these two methods:

__len__
_is_index_empty

I pushed a _filter based implementation for _is_index_empty. Confirmed there is nothing that depends on num_docs is limited (nothing in abstract.py depends on this). But there is no easy way to check all the len() locations.

$ grep -rn -l --exclude-dir=./tests "num_docs()"
./docarray/index/abstract.py
./docarray/index/backends/milvus.py
./docarray/index/backends/redis.py
./docarray/index/backends/hnswlib.py
./docs/user_guide/storing/index_hnswlib.md
./docs/user_guide/storing/index_elastic.md
./docs/user_guide/storing/index_in_memory.md
./docs/user_guide/storing/index_qdrant.md
./docs/user_guide/storing/index_redis.md

and _is_index_empty, I think it was checked in many queries. I am not sure if the result is kind of cached. I have little access to check properly now the code

For _is_index_empty, it should work because I have overriden the default implementation. See https://github.com/docarray/docarray/pull/1835/files#diff-2584da09c45f6eeaca7247dcdfd1a1f1cb1da5752f9a00289cb3f9a52e228969R361-R368

@JoanFM Thanks for the review!

I have updated the input validation logic to report specific parameters missing.

It is unfortunate that Epsilla's python library does not include pythonic handling of error (throw instead of status codes). It directly returns HTTP status codes. For now I replaced it with from http import HTTPStatus.

When there is a conflict on loading the db, Epsilla returns HTTP 500 when it should return HTTP 409. I have filed an issue, but parsing the string error message is the best workaround for now.

What do you think? Please let me know if there are other issues we should address.

Do you plan to have the fix for that behavior anytime soon on epsilla side?

Epsilla is going to merge a quick fix the load_db behavior: epsilla-cloud/vectordb#93 Will wait for them.

fix input validation logic Signed-off-by: Tony Yang <tonyyanga@gmail.com>

Epsilla fixed on their end. I've pushed a change to use HTTP 409 status codes rather than relying on error messages.

Love to know what you think about the num_docs implementation. :-)

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

@JoanFM Any other issues to address before we can merge this change?

I will review the implications of num_docs in detail on Monday.

Is it possible for you to have an API that decides whether the table is empty or not? Otherwise, u can try to override _is_index_empty to say always False. And make sure that all the methods work when the table is empty returning empty results properly.

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

@JoanFM

Unfortunately, there is no API for that. I have pushed a change to override _is_index_empty to return False, and added test cases with the index being empty.

Please let me know if there are other things to address.

Hey @JoanFM

Gently bumping this - please let me know if there are other things to address.

Hello @tonyyanga ,

Thanks for the contribution, we will be waiting for the documentation PR

Thank you! will follow up with it, hopefully later this week.

tonyyanga mentioned this pull request Dec 4, 2023

Support Epsilla as a document store #1832

Closed

7 tasks

feat(index): add epsilla connector

91e9351

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

tonyyanga force-pushed the tonyyang-add-epsilla-db-connector branch from de419f5 to 91e9351 Compare December 4, 2023 19:06

JoanFM requested changes Dec 4, 2023

View reviewed changes

Comment thread docarray/index/backends/epsilla.py Outdated

Comment thread docarray/index/backends/epsilla.py Outdated

Comment thread docarray/index/backends/epsilla.py Outdated

Comment thread docarray/index/backends/epsilla.py Outdated

Comment thread docarray/index/backends/epsilla.py Outdated

tonyyanga added 2 commits December 4, 2023 17:19

fix: better input validation msgs + naming

bea7d88

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

fix: better output handling

9fa77fd

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

JoanFM reviewed Dec 5, 2023

View reviewed changes

fix: use HTTP 409 to check if db is already loaded

c6deb79

fix input validation logic Signed-off-by: Tony Yang <tonyyanga@gmail.com>

tonyyanga force-pushed the tonyyang-add-epsilla-db-connector branch from 744d7ee to c6deb79 Compare December 5, 2023 20:00

override is_index_empty property

1d3ff6d

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

tonyyanga force-pushed the tonyyang-add-epsilla-db-connector branch from a3923d0 to 1d3ff6d Compare December 6, 2023 03:19

tonyyanga added 2 commits December 11, 2023 11:28

fix: return false in _is_index_empty

3f1c181

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

fix: fix test_find_empty_index

03c2b9c

Signed-off-by: Tony Yang <tonyyanga@gmail.com>

JoanFM merged commit ff00b60 into docarray:main Dec 19, 2023

tonyyanga mentioned this pull request Dec 21, 2023

docs(epsilla): add epsilla integration guide #1838

Merged

JoanFM mentioned this pull request Dec 22, 2023

Release Note #1842

Closed

-                                  or "already exists" in response["message"]
+                          if status_code != HTTPStatus.OK:
+                              # Epsilla returns HTTP 500 when multiple clients connect to the same db
+                              # Bug filed at https://github.com/epsilla-cloud/vectordb/issues/93

+                          )
+                  def num_docs(self) -> int:
+                      raise NotImplementedError

feat(index): add epsilla connector by tonyyanga · Pull Request #1835 · docarray/docarray · GitHub

Uh oh!

feat(index): add epsilla connector - #1835

feat(index): add epsilla connector#1835JoanFM merged 7 commits intodocarray:maindocarray/docarray:mainfrom tonyyanga:tonyyang-add-epsilla-db-connectortonyyanga/docarray:tonyyang-add-epsilla-db-connectorCopy head branch name to clipboard

Conversation

tonyyanga commented Dec 4, 2023

Uh oh!

codecov Bot commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoanFM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonyyanga commented Dec 5, 2023

Uh oh!

JoanFM Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

JoanFM Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

tonyyanga Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

JoanFM Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

tonyyanga Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

JoanFM Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

tonyyanga Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

JoanFM commented Dec 5, 2023

Uh oh!

tonyyanga commented Dec 5, 2023

Uh oh!

tonyyanga commented Dec 5, 2023

Uh oh!

tonyyanga commented Dec 8, 2023

Uh oh!

JoanFM commented Dec 9, 2023

Uh oh!

JoanFM commented Dec 11, 2023

Uh oh!

tonyyanga commented Dec 11, 2023

Uh oh!

tonyyanga commented Dec 16, 2023

Uh oh!

JoanFM commented Dec 19, 2023

Uh oh!

tonyyanga commented Dec 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat(index): add epsilla connector#1835
JoanFM merged 7 commits into
docarray:mainfrom
tonyyanga:tonyyang-add-epsilla-db-connector

codecov Bot commented Dec 4, 2023 •
edited
Loading