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

feat: add support in vector_store for server-side embeddings #22

Merged
merged 18 commits into from
Apr 29, 2024

Conversation

jordanrfrazier
Copy link
Collaborator

@jordanrfrazier jordanrfrazier commented Apr 17, 2024

Adds the $vectorize functionality to the vector store. Still uses the legacy astrapy paths.

Note: does not add functionality to the cache.

Copy link
Collaborator

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

Great work!!
I've left some comments

libs/astradb/langchain_astradb/vectorstores.py Outdated Show resolved Hide resolved
libs/astradb/langchain_astradb/vectorstores.py Outdated Show resolved Hide resolved
libs/astradb/langchain_astradb/vectorstores.py Outdated Show resolved Hide resolved
@hemidactylus
Copy link
Collaborator

Very nice and comprehensive work!

My only suggestion concerns the fixtures around testing. With the vectorize feature eventually landing in prod, it could be better to prepare for that (something like a twin set of fixtures, vector- and vectorize-based) and add a small amount of skipifs to momentarily block the latter in prod? For ways to combine this with the pytest parametrize, I found a nice way to condition items of a parametrize (see here).

@jordanrfrazier
Copy link
Collaborator Author

My only suggestion concerns the fixtures around testing. With the vectorize feature eventually landing in prod, it could be better to prepare for that (something like a twin set of fixtures, vector- and vectorize-based) and add a small amount of skipifs to momentarily block the latter in prod? For ways to combine this with the pytest parametrize, I found a nice way to condition items of a parametrize (see here).

Cool, this is a neat technique to know. I did look into using this pattern, but since we have several tests that use one or the other of the StoreEmbedding vs. ParseEmbedding, it was easier to just put a single skipif on the vectorize_store, which can be removed once vectorize is in prod.

We could probably refactor the tests into separate modules for the different embeddings, then use the linked pattern in a further revision.

@jordanrfrazier jordanrfrazier marked this pull request as ready for review April 29, 2024 01:56
@jordanrfrazier jordanrfrazier merged commit 3149f79 into main Apr 29, 2024
13 checks passed
@hemidactylus hemidactylus deleted the add-vectorize-support branch May 30, 2024 12:19
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