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

fix: add_texts method of Weaviate vector store creats wrong embeddings #4933

Merged
merged 4 commits into from
May 22, 2023
Merged

fix: add_texts method of Weaviate vector store creats wrong embeddings #4933

merged 4 commits into from
May 22, 2023

Conversation

Shawn91
Copy link
Contributor

@Shawn91 Shawn91 commented May 18, 2023

fix a bug in the add_texts method of Weaviate vector store that creats wrong embeddings

The following is the original code in the add_texts method of the Weaviate vector store, from line 131 to 153, which contains a bug. The code here includes some extra explanations in the form of comments and some omissions.

            for i, doc in enumerate(texts):

                # some code omitted

                if self._embedding is not None:
                    # variable texts is a list of string and doc here is just a string. 
                    # list(doc) actually breaks up the string into characters.
                    # so, embeddings[0] is just the embedding of the first character
                    embeddings = self._embedding.embed_documents(list(doc))
                    batch.add_data_object(
                        data_object=data_properties,
                        class_name=self._index_name,
                        uuid=_id,
                        vector=embeddings[0],
                    )

To fix this bug, I pulled the embedding operation out of the for loop and embed all texts at once.

According to the contributing.md doc, it seems I can have my twitter account mentioned? I'd love that very much. My twitter handler is ShawnDeveloping. HAHA.

Who can review?

Community members can review the PR once tests pass. Tag maintainers/contributors who might be interested:

@dev2049

if "uuids" in kwargs:
_id = kwargs["uuids"][i]
else:
_id = get_valid_uuid(uuid4())

if self._embedding is not None:
embeddings = self._embedding.embed_documents(list(doc))
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch! i wonder if we should just change list(doc) -> [doc] and leave the rest the same. reason being current implementation lets us lazily load in texts, whereas calling list(texts) up front would load all of them into memory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch! i wonder if we should just change list(doc) -> [doc] and leave the rest the same. reason being current implementation lets us lazily load in texts, whereas calling list(texts) up front would load all of them into memory

I checked the add_texts methods of all vectore stores. Here is what I found:

  1. Twelve vector stores turn the potential iterable texts into a list first and then embed them:
  • chroma
  • pgvector
  • qdrant
  • supabase
  • analyticdb
  • atlas
  • deeplake
  • elastic_vector_search
  • lancedb
  • milvus
  • opensearch_vector_search
  • tair
  1. Four vector stores that iterate over texts and embed text one by one lazily:
  • faiss
  • redis
  • pinecone
  • myscale

The behavious are not consistent. But most vector stores simply turn the texts variable into a list first.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea we definitely haven't been consistent about it, which is on us. and it's possible for a lot of workloads / embedding methods that embedding all the texts at once is more efficient. but think i'd prefer to not change behavior in an existing vector store until we've come up with a best practice that we apply everywhere. meaning in this case slight preference to keep lazy for now (otherwise somebody who's using Weaviate today could see their memory usage change next update for seemingly no reason)

@dev2049 dev2049 added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label May 22, 2023
@dev2049 dev2049 merged commit 9e64946 into langchain-ai:master May 22, 2023
@danielchalef danielchalef mentioned this pull request Jun 5, 2023
This was referenced Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PR looks good. Use to confirm that a PR is ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants