.Net: Support records with no key, fix issue with Relevance #6393
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markwallace-microsoft
added
.NET
github-actions
bot
changed the title
Pilchie
commented
May 24, 2024
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
Pilchie
commented
May 24, 2024
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Show resolved
Hide resolved
Pilchie
force-pushed
the
6379-SaveReferenceAsync
branch
from
629bf78 to
af4248e
Compare
|
@dmytrostruk - can you take another look here? I restricted the provider to only use |
dmytrostruk
reviewed
Jun 10, 2024
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Show resolved
Hide resolved
…reCosmosDBNoSQLMemoryStore.cs Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com>
westey-m
approved these changes
Jun 11, 2024
westey-m
approved these changes
Jun 12, 2024
dmytrostruk
approved these changes
Jun 12, 2024
There was a problem hiding this comment.
Small updates on constant naming. We use PascalCase for constants:
https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names#naming-conventions
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/AzureCosmosDBNoSQLMemoryStore.cs
Outdated
Show resolved
Hide resolved
dmytrostruk
approved these changes
Jun 12, 2024
LudoCorporateShark
pushed a commit
to LudoCorporateShark/semantic-kernel
that referenced
this pull request
Aug 25, 2024
…t#6393) ### Motivation and Context When using SemanticTextMemory.SaveReferenceAsync, no value is passed for the `Key` to use. In the Cosmos DB Memory Connector, we treat the "Key" as the Id and PartitionKey, and so it has to have a value. Address this by generating a `GUID` to use if none are provided. ### Description * Assign a GUID key when the passed in `MemoryRecord.Key` is null or empty so that CosmosDB always has a valid PartitionKey. * While adding test coverage for this I noticed two things: 1. `withEmbeddings` is ignored in `AzureCosmosDBNoSQLMemoryStore.GetAsync`. It's not clear whether it's worth changing this, as point reads like the current one tend to be less expensive than queries in Cosmos DB. 2. The SimilarityScore returned from `GetNearestAsync` wasn't actually a similarity, it was a cosine distance. Normalize that. Fixes microsoft#6379. ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄 --------- Co-authored-by: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
When using SemanticTextMemory.SaveReferenceAsync, no value is passed for the
Keyto use. In the Cosmos DB Memory Connector, we treat the "Key" as the Id and PartitionKey, and so it has to have a value. Address this by generating aGUIDto use if none are provided.Description
MemoryRecord.Keyis null or empty so that CosmosDB always has a valid PartitionKey.withEmbeddingsis ignored inAzureCosmosDBNoSQLMemoryStore.GetAsync. It's not clear whether it's worth changing this, as point reads like the current one tend to be less expensive than queries in Cosmos DB.GetNearestAsyncwasn't actually a similarity, it was a cosine distance. Normalize that.Fixes #6379.
Contribution Checklist