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.

Sign up for GitHub

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

Jump to bottom

allow embeddings vector to be used for mmr searching (#2620) #2625

Merged
merged 3 commits into from
Aug 13, 2024
Merged

allow embeddings vector to be used for mmr searching (#2620) #2625

merged 3 commits into from
Aug 13, 2024

Conversation

rishabh208gupta
Copy link
Contributor

Closes #2620
Provide an option to pass vector embeddings to search for in the max_marginal_relevance_search
This gives us the flexibility to perform mmr search with either a text query or an embeddings vector

@cla-checker-service CLA Checker Service
Copy link

cla-checker-service bot commented Aug 2, 2024 •
edited
Loading

💚 CLA has been signed

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 2, 2024

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@rishabh208gupta
Copy link
Contributor Author

rishabh208gupta commented Aug 2, 2024 •
edited
Loading

I have signed the document, I don't get it why its complaining
Please_DocuSign_Individual_Contributor_Licen (1).pdf

pquentin
pquentin reviewed Aug 2, 2024
View reviewed changes
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! Can you please add to the test a call with query_embedding passed as a parameter?

elasticsearch/helpers/vectorstore/_async/vectorstore.py
@@ -344,8 +344,8 @@ async def _create_index_if_not_exists(self) -> None:
async def max_marginal_relevance_search(
self,
*,
embedding_service: AsyncEmbeddingService,
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I did not realize this was not useful. Unfortunately, you can't remove it as the LangChain integration passes this parameter: https://github.com/langchain-ai/langchain-elastic/blob/9915f1e16d20e992896a651f7a3ec94db56a76f0/libs/elasticsearch/langchain_elasticsearch/vectorstores.py#L816-L825

Copy link
Contributor Author

@rishabh208gupta rishabh208gupta Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Okay I will add it as an optional param, my plan is to create another PR in the langchain repo.

@pquentin
Copy link
Member

pquentin commented Aug 6, 2024

Hey, the CLA is complaining because your patch is authored from "Rishabh Gupta [email protected]". You need to setup an user email and name in git, then git commit --amend --reset-author and force push.

@rishabh208gupta
Copy link
Contributor Author

Thanks @pquentin !

@pquentin
Copy link
Member

pquentin commented Aug 6, 2024

buildkite test this please

@pquentin pquentin requested a review from miguelgrinberg August 6, 2024 16:31
miguelgrinberg
miguelgrinberg approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@miguelgrinberg miguelgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM

@pquentin
Copy link
Member

buildkite test this please

@miguelgrinberg miguelgrinberg merged commit 3b1bce7 into elastic:main Aug 13, 2024
10 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 13, 2024
@rishabh208gupta @github-actions
allow embeddings vector to be used for mmr searching (#2620) (#2625)
1a0d7f7 
* allow embeddings vector to be used for mmr searching (#2620)

Signed-off-by: rishabh208gupta <[email protected]>

* Use embedding service if provided

---------

Signed-off-by: rishabh208gupta <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
(cherry picked from commit 3b1bce7)
@github-actions github-actions bot mentioned this pull request Aug 13, 2024
Merged
miguelgrinberg pushed a commit that referenced this pull request Aug 13, 2024
@github-actions @rishabh208gupta
allow embeddings vector to be used for mmr searching (#2620) (#2625) (#…
6905674 
…2639)

* allow embeddings vector to be used for mmr searching (#2620)

Signed-off-by: rishabh208gupta <[email protected]>

* Use embedding service if provided

---------

Signed-off-by: rishabh208gupta <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
(cherry picked from commit 3b1bce7)

Co-authored-by: Rishabh Gupta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miguelgrinberg miguelgrinberg miguelgrinberg approved these changes

@pquentin pquentin Awaiting requested review from pquentin

Assignees
No one assigned
Labels
backport 8.15
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Allow embedding vector search for max_marginal_relevance_search
3 participants
@rishabh208gupta @pquentin @miguelgrinberg