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 load index #36

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Fix load index #36

merged 1 commit into from
Feb 14, 2024

Conversation

buhe
Copy link
Contributor

@buhe buhe commented Feb 14, 2024

Hi, @ZachNagengast

The loadIndex method has already loaded the embedding, and there is no need to call addItem. The original method is very slow in some cases. FIY https://github.com/buhe/langchain-swift/blob/main/Sources/LangChain/vectorstores/SimilaritySearchKit.swift #L35 with openaiembedding

BR
buhe

…ndexItems and print the count and path of the loaded items."
@ZachNagengast
Copy link
Owner

Nice! Do you think we can still add these to the index object after calling it? This function kind of does two things, one is loading the embeddings from the store and one is adding them to the parent object for searches. Definitely support splitting these apart, but curious how you think it could be handled if someone wants to load the items and then run a search of them?

@buhe
Copy link
Contributor Author

buhe commented Feb 14, 2024

I I've added it and tested it :)
https://github.com/ZachNagengast/similarity-search-kit/pull/36/files
Please look carefully, the results of assignment are different. : )

@ZachNagengast
Copy link
Owner

I think addItems is necessary to make sure the index has access to all the embeddings from the vector store. When using this function it should be possible to do this:

        let _ = try! similarityIndex.loadIndex(name: "TestIndex")
        let results = similarityIndex.search("search query for TestIndex items")

@buhe
Copy link
Contributor Author

buhe commented Feb 14, 2024

The modified code works, I tested it, after assigning it to indexItems, addItem is not needed.

@ZachNagengast
Copy link
Owner

Ah, I understand now, great!

@buhe
Copy link
Contributor Author

buhe commented Feb 14, 2024

👍

@ZachNagengast
Copy link
Owner

Looks good, nice find and thanks for submitting, this should help speed things up 🙌

@ZachNagengast ZachNagengast merged commit ef737c4 into ZachNagengast:main Feb 14, 2024
1 check passed
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.

2 participants