GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — feat(index): add epsilla connector by tonyyanga · Pull Request #1835 · docarray/docarray

This repository was archived by the owner on Sep 9, 2026. It is now read-only.

Conversation

tonyyanga commented Dec 4, 2023

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tonyyanga mentioned this pull request

Dec 4, 2023

Support Epsilla as a document store #1832

Closed

3 of 7 tasks


          feat(index): add epsilla connector

91e9351

Signed-off-by: Tony Yang <[email protected]>

tonyyanga force-pushed the tonyyang-add-epsilla-db-connector branch from de419f5 to 91e9351 Compare

December 4, 2023 19:06

codecov Bot commented Dec 4, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JoanFM suggested changes

Dec 4, 2023

View reviewed changes

JoanFM left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

docarray/index/backends/epsilla.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

docarray/index/backends/epsilla.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

docarray/index/backends/epsilla.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

docarray/index/backends/epsilla.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

docarray/index/backends/epsilla.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

tonyyanga added 2 commits

December 4, 2023 17:19


          fix: better input validation msgs + naming

bea7d88

Signed-off-by: Tony Yang <[email protected]>


          fix: better output handling

9fa77fd

Signed-off-by: Tony Yang <[email protected]>

tonyyanga commented Dec 5, 2023

Copy link

Copy Markdown

Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JoanFM reviewed

Dec 5, 2023

View reviewed changes

Comment thread

docarray/index/backends/epsilla.py Outdated

		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

JoanFM Dec 5, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JoanFM reviewed

Dec 5, 2023

View reviewed changes

Comment thread

docarray/index/backends/epsilla.py

		)

		def num_docs(self) -> int:
		raise NotImplementedError

JoanFM Dec 5, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tonyyanga Dec 5, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JoanFM Dec 5, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tonyyanga Dec 6, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JoanFM Dec 6, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tonyyanga Dec 6, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JoanFM commented Dec 5, 2023

Copy link

Copy Markdown

Member

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tonyyanga commented Dec 5, 2023

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

c6deb79

fix input validation logic

Signed-off-by: Tony Yang <[email protected]>

tonyyanga force-pushed the tonyyang-add-epsilla-db-connector branch from 744d7ee to c6deb79 Compare

December 5, 2023 20:00

tonyyanga commented Dec 5, 2023

Copy link

Copy Markdown

Contributor Author

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. :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          override is_index_empty property

1d3ff6d

Signed-off-by: Tony Yang <[email protected]>

tonyyanga force-pushed the tonyyang-add-epsilla-db-connector branch from a3923d0 to 1d3ff6d Compare

December 6, 2023 03:19

tonyyanga commented Dec 8, 2023

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JoanFM commented Dec 9, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JoanFM commented Dec 11, 2023

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tonyyanga added 2 commits

December 11, 2023 11:28


          fix: return false in _is_index_empty

3f1c181

Signed-off-by: Tony Yang <[email protected]>


          fix: fix test_find_empty_index

03c2b9c

Signed-off-by: Tony Yang <[email protected]>

tonyyanga commented Dec 11, 2023

Copy link

Copy Markdown

Contributor Author

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tonyyanga commented Dec 16, 2023

Copy link

Copy Markdown

Contributor Author

Hey @JoanFM

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JoanFM merged commit ff00b60 into docarray:main

Dec 19, 2023

JoanFM commented Dec 19, 2023

Copy link

Copy Markdown

Member

Hello @tonyyanga ,

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tonyyanga commented Dec 19, 2023

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

1 more reviewer

JoanFM

JoanFM requested changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Dec 4, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.