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[patch]: fix Azure AI search delete and improve documentation #5272

Merged
merged 7 commits into from
May 6, 2024

Conversation

sinedied
Copy link
Contributor

@sinedied sinedied commented May 2, 2024

This PRs includes the following changes:

  • Fix missing await initPromise when doing a delete with filter, causing race conditions
  • Set default search mode to hybrid to improve search results relevance
  • Add documentation on hybrid search and semantic ranking

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label May 2, 2024
Copy link

vercel bot commented May 2, 2024

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

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2024 9:37pm
langchainjs-docs ✅ Ready (Inspect) Visit Preview May 3, 2024 9:37pm

@dosubot dosubot bot added auto:bug Related to a bug, vulnerability, unexpected error with an existing feature auto:documentation Changes to documentation and examples, like .md, .rst, .ipynb files. Changes to the docs/ folder labels May 2, 2024
Copy link
Collaborator

@bracesproul bracesproul left a comment

Choose a reason for hiding this comment

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

Lgtm, just had one comment

Comment on lines +372 to +373
const searchType =
this.options.type ?? AzureAISearchQueryType.SimilarityHybrid;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for this change? Technically this is "breaking" to users in the sense that the functionality will be different.

Choose a reason for hiding this comment

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

Hybrid search is the default retreival mode for other vector stores that support it. We'd like to align to that pattern as hybrid proves better retrevial quality vs pure vector search.

Copy link
Contributor Author

@sinedied sinedied May 3, 2024

Choose a reason for hiding this comment

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

We had a discussion regarding which search mode would bring the best relevance in most scenarios as a default value, and concluded that hybrid search should be the best for most use cases.
This is technically breaking, but people relying on defaults should get slightly improved relevance without any other impact on their code.

@bracesproul bracesproul added question Further information is requested close PRs that need one or two touch-ups to be ready labels May 3, 2024
Copy link

@chuwik chuwik left a comment

Choose a reason for hiding this comment

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

LGTM, left some nits.

Comment on lines +372 to +373
const searchType =
this.options.type ?? AzureAISearchQueryType.SimilarityHybrid;

Choose a reason for hiding this comment

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

Hybrid search is the default retreival mode for other vector stores that support it. We'd like to align to that pattern as hybrid proves better retrevial quality vs pure vector search.

@sinedied
Copy link
Contributor Author

sinedied commented May 6, 2024

@bracesproul We've answered your comment, is there anything else you need from our side?

@bracesproul
Copy link
Collaborator

@bracesproul We've answered your comment, is there anything else you need from our side?

Nope, lgtm and will get a release asap!

@bracesproul bracesproul added lgtm PRs that are ready to be merged as-is and removed question Further information is requested close PRs that need one or two touch-ups to be ready labels May 6, 2024
@bracesproul bracesproul merged commit 51b7721 into langchain-ai:main May 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:bug Related to a bug, vulnerability, unexpected error with an existing feature auto:documentation Changes to documentation and examples, like .md, .rst, .ipynb files. Changes to the docs/ folder lgtm PRs that are ready to be merged as-is size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants