add embedding && index embedding #86
Conversation
There was a problem hiding this comment.
Looks great @LittleLittleCloud. Added minor nit. Do we know if the OpenAI endpoint that's deployed by azd/bicep contains an embedding model or is this something we need to account for?
|
@LittleLittleCloud let's do this for this PR. We won't merge into main. Let's create a separate branch for it and update the READMEs to point to that branch if they're interested in trying out the vector support. |
| @@ -6,7 +6,8 @@ internal static class SearchClientExtensions | |||
| { | |||
| internal static async Task<string> QueryDocumentsAsync( | |||
| this SearchClient searchClient, | |||
| string query, | |||
| string? query = null, | |||
There was a problem hiding this comment.
Since now query can be null (e.g. pure vector search), you need to account for this below when creating search options that use semantic ranking. If query == null then it's an error to enable reranking (SearchQueryType.Semantic) since we need to have a text query for that.
| @@ -75,7 +75,7 @@ public class ReadRetrieveReadChatService | |||
|
|
|||
| // step 2 | |||
| // use query to search related docs | |||
| var documentContents = await _searchClient.QueryDocumentsAsync(query.Result, overrides, cancellationToken); | |||
| var documentContents = await _searchClient.QueryDocumentsAsync(query.Result, embedding: null, overrides: overrides, cancellationToken); | |||
There was a problem hiding this comment.
Why no vectors for the chat case? Seems like it's the most popular scenario, isn't it?
| InputType = "query", | ||
| }, cancellationToken); | ||
| var embedding = questionEmbeddingResponse.Value.Data.First().Embedding.ToArray(); | ||
| var text = await _searchClient.QueryDocumentsAsync(query: null, embedding: embedding, overrides: overrides, cancellationToken: cancellationToken); |
There was a problem hiding this comment.
Why null for query? We could do hybrid search using both text and vector, it'll yield better results.
Even better, though we may not have time for this right now, we could have an option in the "developer settings" part of the UX that let's the user choose between:
- Pure vector search
- Pure text search
- Hybrid search
- For 2 and 3, option to enable semantic
Separately, if the overrides enable semantic search, you can't have the text query == null.
| @@ -50,6 +50,8 @@ param gptDeploymentName string = 'davinci' | |||
| param gptModelName string = 'text-davinci-003' | |||
| param chatGptDeploymentName string = 'chat' | |||
| param chatGptModelName string = 'gpt-35-turbo' | |||
| param embeddingModelName string = 'text-embedding-ada-002' | |||
| param embeddingDeploymentName string = 'embedding' | |||
There was a problem hiding this comment.
I think you might have forgotten to wire up this name to the webapp settings in line ~140 below.
There was a problem hiding this comment.
Those values are saved in the key-vault now. So most of the parameters here are probably unnecessary to set.
LittleLittleCloud
merged commit 0a52a89
into
Azure-Samples:feature/embeddingSearch
Purpose
This PR enables
Azure Document vector searchsolution. It makes two major changetext-embedding-adamodel to generate embeddings for all sectionsTODO
text-embedding-adawith local embedding modelupdate .bicep to create embedding model during provision.fix issue
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test
What to Check
Verify that the following are valid
Other Information