Cosmos DB No SQL Vector Search #103
Conversation
aayush3011
requested review from
vrdmr,
manvkaur,
aishwaryabh,
cgillum and
gavin-aguiar
as code owners
aayush3011
changed the title
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchConfigOptions.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchConfigOptions.cs
Outdated
Show resolved
Hide resolved
…/azure-functions-openai-extension into users/akataria/cosmosdb_nosql
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
...s.Extensions.OpenAI.CosmosDBNoSqlSearch/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch.csproj
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchConfigOptions.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
|
merge latest of main branch |
…/azure-functions-openai-extension into users/akataria/cosmosdb_nosql
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
| CosmosClient cosmosClient; | ||
| if ( | ||
| !string.IsNullOrEmpty(connectionStringName) | ||
| && this.cosmosDBClients.TryGetValue(connectionStringName, out cosmosClient) |
There was a problem hiding this comment.
this line can be moved inside the if block rather than keeping it empty
There was a problem hiding this comment.
Moving that will be wrong, because in this if condition we need to check if the connectionStringName is not null and if the dictionary already contains the value retrieve it.
I can update this this with a different approach, where I use containsKey in the if statement and then assign it in the if block.
But normally in multithreaded apps, it's better to use TryGetValue, as it combines both the operation. It avoids the race condition, if the dictionary gets updated between the two separate operations.
Let me know which approach to go with.
There was a problem hiding this comment.
I wanted to clarify my previous comment regarding line 119. I wasn’t suggesting any changes to lines 117 or 118; my focus was specifically on ensuring that the if block is not left blank.
I agree that using TryGetValue is a good approach, especially in multithreaded contexts. Please make sure to include logic within the if block to handle the value retrieval properly.
Let me know if you have any questions or need further clarification.
src/WebJobs.Extensions.OpenAI.CosmosDBNoSqlSearch/CosmosDBNoSqlSearchProvider.cs
Outdated
Show resolved
Hide resolved
| @@ -13,6 +13,24 @@ The sample is available in the following language stacks: | |||
|
|
|||
| Please refer to the [root README](../../README.md#requirements) for common prerequisites that apply to all samples. | |||
|
|
|||
| ### Chat Storage Configuration | |||
There was a problem hiding this comment.
also, isn't this meant for No SQL sample but has been added in mongo v core sample
| - Please refer to this two documents to assign permissions for cosmos db account to the managed identity. | ||
| - [Control Plane operations](https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/security/how-to-grant-control-plane-role-based-access?tabs=built-in-definition%2Ccsharp&pivots=azure-interface-cli) | ||
| - [Data Plane operations](https://learn.microsoft.com/en-us/azure/cosmos-db/nosql/security/how-to-grant-data-plane-role-based-access?tabs=built-in-definition%2Ccsharp&pivots=azure-interface-cli) |
There was a problem hiding this comment.
Could you please suggest what are the minimal role assignment requirements for this sample?
It will be confusing for the users to figure this out from all the roles available.
| 1. **Configure Cosmos URI**: | ||
|
|
||
| - Set the `cosmosUri` in the configuration as follows: | ||
|
|
||
| ```json | ||
| "<CONNECTION_NAME_PREFIX>__cosmosUri": "cosmosUri" | ||
| ``` |
There was a problem hiding this comment.
I have a few questions regarding the settings:
- Are these the only settings available?
- I noticed the following keywords in the code: tokenCredential, AccountEndpoint, and AccountKey. How are these intended to be used?
- Has the sample been verified on Azure using the following approaches?
Managed Identity
Connection String
| @@ -1,7 +1,7 @@ | |||
| <Project> | |||
| <PropertyGroup> | |||
| <TargetFramework>netstandard2.1</TargetFramework> | |||
| <LangVersion>11.0</LangVersion> | |||
| <LangVersion>12.0</LangVersion> | |||
There was a problem hiding this comment.
Is this intentional change?
Read more about CosmosDB NoSql Vector Search here