From b79f3a0480f435ac9fda2c2ba700d1bd761b221b Mon Sep 17 00:00:00 2001 From: Dmytro Struk <13853051+dmytrostruk@users.noreply.github.com> Date: Thu, 20 Jul 2023 05:46:31 +0100 Subject: [PATCH] .Net: Fix for Chroma boolean values processing (#2072) ### Motivation and Context With latest updates in Chroma ([Release 0.4.0](https://github.com/chroma-core/chroma/releases/tag/0.4.0)), boolean values are automatically converted to numeric representation (False = 0, True = 1). This PR contains changes to handle this case and convert boolean values properly. Should fix #2049 ### Description 1. Implemented `ChromaBooleanConverter` to be used during Chroma response deserialization. 2. Added more fixes based on new Chroma version - changed error message when trying to delete non-existent collection. 3. Added integration test to verify boolean values processing. ### 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#dev-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone :smile: --------- Co-authored-by: Abby Harrison <54643756+awharrison-28@users.noreply.github.com> --- .../ChromaBooleanConverter.cs | 30 +++++++++++++ .../ChromaClientException.cs | 7 --- .../ChromaMemoryStore.cs | 19 +++++--- .../Memory/Chroma/ChromaMemoryStoreTests.cs | 12 +++-- .../Memory/Chroma/ChromaMemoryStoreTests.cs | 45 +++++++++++++++++++ 5 files changed, 98 insertions(+), 15 deletions(-) create mode 100644 dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaBooleanConverter.cs diff --git a/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaBooleanConverter.cs b/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaBooleanConverter.cs new file mode 100644 index 0000000000..6c4a9fc8fa --- /dev/null +++ b/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaBooleanConverter.cs @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace Microsoft.SemanticKernel.Connectors.Memory.Chroma; + +/// +/// JSON Converter for Chroma boolean values. +/// +public class ChromaBooleanConverter : JsonConverter +{ + /// + public override bool Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + if (!reader.TryGetInt16(out short value)) + { + return false; + } + + return Convert.ToBoolean(value); + } + + /// + public override void Write(Utf8JsonWriter writer, bool value, JsonSerializerOptions options) + { + writer.WriteNumberValue(Convert.ToDecimal(value)); + } +} diff --git a/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaClientException.cs b/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaClientException.cs index 7568718b51..535e7d3a3a 100644 --- a/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaClientException.cs +++ b/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaClientException.cs @@ -11,7 +11,6 @@ namespace Microsoft.SemanticKernel.Connectors.Memory.Chroma; public class ChromaClientException : Exception { private const string CollectionDoesNotExistErrorFormat = "Collection {0} does not exist"; - private const string DeleteNonExistentCollectionErrorMessage = "list index out of range"; /// /// Initializes a new instance of the class. @@ -43,10 +42,4 @@ public class ChromaClientException : Exception /// Collection name. public bool CollectionDoesNotExistException(string collectionName) => this.Message.Contains(string.Format(CultureInfo.InvariantCulture, CollectionDoesNotExistErrorFormat, collectionName)); - - /// - /// Checks if Chroma API error means that there was an attempt to delete non-existent collection. - /// - public bool DeleteNonExistentCollectionException() => - this.Message.Contains(DeleteNonExistentCollectionErrorMessage); } diff --git a/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaMemoryStore.cs b/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaMemoryStore.cs index 08d80d435e..723d543305 100644 --- a/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaMemoryStore.cs +++ b/dotnet/src/Connectors/Connectors.Memory.Chroma/ChromaMemoryStore.cs @@ -70,7 +70,7 @@ public class ChromaMemoryStore : IMemoryStore { await this._chromaClient.DeleteCollectionAsync(collectionName, cancellationToken).ConfigureAwait(false); } - catch (ChromaClientException e) when (e.DeleteNonExistentCollectionException()) + catch (ChromaClientException e) when (e.CollectionDoesNotExistException(collectionName)) { this._logger.LogError("Cannot delete non-existent collection {0}", collectionName); throw new ChromaMemoryStoreException($"Cannot delete non-existent collection {collectionName}", e); @@ -230,6 +230,11 @@ public class ChromaMemoryStore : IMemoryStore private readonly IChromaClient _chromaClient; private readonly List _defaultEmbeddingIncludeTypes = new() { IncludeMetadatas }; + private readonly JsonSerializerOptions _jsonSerializerOptions = new() + { + Converters = { new ChromaBooleanConverter() } + }; + private async Task GetCollectionOrThrowAsync(string collectionName, CancellationToken cancellationToken) { return @@ -292,15 +297,19 @@ public class ChromaMemoryStore : IMemoryStore var embeddingsVector = this.GetEmbeddingForMemoryRecord(embeddings, recordIndex); var key = ids?[recordIndex]; - return MemoryRecord.FromJsonMetadata( - json: metadata, + return MemoryRecord.FromMetadata( + metadata: metadata, embedding: embeddingsVector, key: key); } - private string GetMetadataForMemoryRecord(List>? metadatas, int recordIndex) + private MemoryRecordMetadata GetMetadataForMemoryRecord(List>? metadatas, int recordIndex) { - return metadatas != null ? JsonSerializer.Serialize(metadatas[recordIndex]) : string.Empty; + var serializedMetadata = metadatas != null ? JsonSerializer.Serialize(metadatas[recordIndex]) : string.Empty; + + return + JsonSerializer.Deserialize(serializedMetadata, this._jsonSerializerOptions) ?? + throw new ChromaMemoryStoreException("Unable to deserialize memory record metadata."); } private Embedding GetEmbeddingForMemoryRecord(List? embeddings, int recordIndex) diff --git a/dotnet/src/Connectors/Connectors.UnitTests/Memory/Chroma/ChromaMemoryStoreTests.cs b/dotnet/src/Connectors/Connectors.UnitTests/Memory/Chroma/ChromaMemoryStoreTests.cs index 79a88e3e0f..7c16caa580 100644 --- a/dotnet/src/Connectors/Connectors.UnitTests/Memory/Chroma/ChromaMemoryStoreTests.cs +++ b/dotnet/src/Connectors/Connectors.UnitTests/Memory/Chroma/ChromaMemoryStoreTests.cs @@ -27,6 +27,7 @@ public sealed class ChromaMemoryStoreTests : IDisposable private readonly HttpMessageHandlerStub _messageHandlerStub; private readonly HttpClient _httpClient; private readonly Mock _chromaClientMock; + private readonly JsonSerializerOptions _serializerOptions; public ChromaMemoryStoreTests() { @@ -37,6 +38,11 @@ public sealed class ChromaMemoryStoreTests : IDisposable this._chromaClientMock .Setup(client => client.GetCollectionAsync(CollectionName, CancellationToken.None)) .ReturnsAsync(new ChromaCollectionModel { Id = CollectionId, Name = CollectionName }); + + this._serializerOptions = new JsonSerializerOptions + { + Converters = { new ChromaBooleanConverter() } + }; } [Fact] @@ -102,12 +108,12 @@ public sealed class ChromaMemoryStoreTests : IDisposable { // Arrange const string collectionName = "non-existent-collection"; - const string deleteNonExistentCollectionErrorMessage = "list index out of range"; + const string collectionDoesNotExistErrorMessage = $"Collection {collectionName} does not exist"; const string expectedExceptionMessage = $"Cannot delete non-existent collection {collectionName}"; this._chromaClientMock .Setup(client => client.DeleteCollectionAsync(collectionName, CancellationToken.None)) - .Throws(new ChromaClientException(deleteNonExistentCollectionErrorMessage)); + .Throws(new ChromaClientException(collectionDoesNotExistErrorMessage)); var store = new ChromaMemoryStore(this._chromaClientMock.Object); @@ -310,7 +316,7 @@ public sealed class ChromaMemoryStoreTests : IDisposable private Dictionary GetEmbeddingMetadataFromMemoryRecord(MemoryRecord memoryRecord) { - var serialized = JsonSerializer.Serialize(memoryRecord.Metadata); + var serialized = JsonSerializer.Serialize(memoryRecord.Metadata, this._serializerOptions); return JsonSerializer.Deserialize>(serialized)!; } diff --git a/dotnet/src/IntegrationTests/Connectors/Memory/Chroma/ChromaMemoryStoreTests.cs b/dotnet/src/IntegrationTests/Connectors/Memory/Chroma/ChromaMemoryStoreTests.cs index 6ba48d4953..62b43eb1a1 100644 --- a/dotnet/src/IntegrationTests/Connectors/Memory/Chroma/ChromaMemoryStoreTests.cs +++ b/dotnet/src/IntegrationTests/Connectors/Memory/Chroma/ChromaMemoryStoreTests.cs @@ -379,6 +379,28 @@ public sealed class ChromaMemoryStoreTests : IDisposable this.AssertMemoryRecordEqual(expectedRecord2, actualRecord2); } + [Theory(Skip = SkipReason)] + [InlineData(true)] + [InlineData(false)] + public async Task ItProcessesBooleanValuesCorrectlyAsync(bool isReference) + { + // Arrange + var collectionName = this.GetRandomCollectionName(); + var metadata = this.GetRandomMemoryRecordMetadata(isReference: isReference); + var expectedRecord = this.GetRandomMemoryRecord(metadata: metadata); + + await this._chromaMemoryStore.CreateCollectionAsync(collectionName); + + // Act + var createdRecordKey = await this._chromaMemoryStore.UpsertAsync(collectionName, expectedRecord); + var actualRecord = await this._chromaMemoryStore.GetAsync(collectionName, createdRecordKey, true); + + // Assert + Assert.NotNull(actualRecord); + + Assert.Equal(expectedRecord.Metadata.IsReference, actualRecord.Metadata.IsReference); + } + public void Dispose() { this.Dispose(true); @@ -429,5 +451,28 @@ public sealed class ChromaMemoryStoreTests : IDisposable key: recordKey); } + private MemoryRecord GetRandomMemoryRecord(MemoryRecordMetadata metadata, Embedding? embedding = null) + { + var recordEmbedding = embedding ?? new Embedding(new[] { 1f, 3f, 5f }); + + return MemoryRecord.FromMetadata( + metadata: metadata, + embedding: recordEmbedding, + key: metadata.Id); + } + + private MemoryRecordMetadata GetRandomMemoryRecordMetadata(bool isReference = false, string? key = null) + { + var recordKey = key ?? Guid.NewGuid().ToString(); + + return new MemoryRecordMetadata( + isReference: isReference, + id: recordKey, + text: "text-" + Guid.NewGuid().ToString(), + description: "description-" + Guid.NewGuid().ToString(), + externalSourceName: "source-name-" + Guid.NewGuid().ToString(), + additionalMetadata: "metadata-" + Guid.NewGuid().ToString()); + } + #endregion }