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

[Community]: AzureSearchVectorStoreRetriever Fixed to account for search_kwargs #21572

Merged
merged 16 commits into from
May 22, 2024

Conversation

keenborder786
Copy link
Contributor

Copy link

vercel bot commented May 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview May 22, 2024 9:10pm

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. Ɑ: vector store Related to vector store module 🤖:improvement Medium size change to existing code to handle new use-cases labels May 12, 2024
@keenborder786
Copy link
Contributor Author

@baskaryan please review

elif self.search_type == "similarity_score_threshold":
docs = [
doc
for doc, _ in self.vectorstore.similarity_search_with_relevance_scores(
query, k=self.k, **kwargs
query, k=self.k, **self.search_kwargs, **kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above here (and below). Defining params before the if clause would be much cleaner.

@keenborder786
Copy link
Contributor Author

@mspronesti I have made your changes. Please check

@mspronesti
Copy link
Contributor

@keenborder786 Thanks for taking my suggestion into consideration -- guess you forgot to apply these changes in the last 3 elif branches.

@keenborder786
Copy link
Contributor Author

@baskaryan, @efriis, @eyurtsev, @hwchase17 please review

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels May 14, 2024
@mspronesti
Copy link
Contributor

@keenborder786 There's a small conflict which arose from my latest PR being merged. Resolving it should only require replacing kwargs with params in the new search type elif branch.

@ccurme Could you please review this as well? :)

@keenborder786
Copy link
Contributor Author

@keenborder786 There's a small conflict which arose from my latest PR being merged. Resolving it should only require replacing kwargs with params in the new search type elif branch.

@ccurme Could you please review this as well? :)

okay, I will resolve it.

@keenborder786
Copy link
Contributor Author

@mspronesti fixed

@keenborder786
Copy link
Contributor Author

@eyurtsev please see

baskaryan
baskaryan previously approved these changes May 21, 2024
@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label May 21, 2024
@@ -740,6 +739,18 @@ class AzureSearchVectorStoreRetriever(BaseRetriever):
or "semantic_hybrid_score_threshold"."""
k: int = 4
"""Number of documents to return."""
search_kwargs: dict = Field(
default_factory=lambda: {"fetch_k": 20, "lambda_mult": 0.5}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just default to empty dict here and let vecstore set defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baskaryan done

@baskaryan baskaryan merged commit 16617dd into langchain-ai:master May 22, 2024
42 checks passed
JuHyung-Son pushed a commit to JuHyung-Son/langchain that referenced this pull request May 23, 2024
…r search_kwargs (langchain-ai#21572)

- **Description:** Fixed `AzureSearchVectorStoreRetriever` to account
for search_kwargs. More explanation is in the mentioned issue.
- **Issue:** langchain-ai#21492

---------

Co-authored-by: MAC <mac@MACs-MacBook-Pro.local>
Co-authored-by: Massimiliano Pronesti <massimiliano.pronesti@gmail.com>
Co-authored-by: Bagatur <baskaryan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases lgtm PR looks good. Use to confirm that a PR is ready for merging. size:S This PR changes 10-29 lines, ignoring generated files. Ɑ: vector store Related to vector store module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants