Skip to content

.Net: Weaviate .NET Connector: Added ReadOnlyMemoryConverter to fix deserialization#2777

Merged
dmytrostruk merged 13 commits intomicrosoft:mainfrom
jevgenigeurtsen:fix/weaviate-json-deserialization-issue
Sep 13, 2023
Merged

.Net: Weaviate .NET Connector: Added ReadOnlyMemoryConverter to fix deserialization#2777
dmytrostruk merged 13 commits intomicrosoft:mainfrom
jevgenigeurtsen:fix/weaviate-json-deserialization-issue

Conversation

@jevgenigeurtsen
Copy link
Contributor

Motivation and Context

All operations inside WeaviateMemoryStore whose response models contain any property of the type ReadOnlyMemory<>, will throw an exception whenever the JSON deserializer tries to deserialize that specific property.

I believe this is caused by the changes made in #2419

Description

I encountered the following exception while trying out the Weaviate connector:

  System.InvalidOperationException : The type 'System.ReadOnlySpan`1[System.Single]' of property 'Span' on type 'System.ReadOnlyMemory`1[System.Single]' is invalid for serialization or deserialization because it is a pointer type, is a ref struct, or contains generic parameters that have not been replaced by specific types.
  Stack Trace:
     at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(Type typeToConvert, Type declaringType, MemberInfo memberInfo)
   at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.CreateProperty(Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options, Boolean shouldCheckForRequiredKeyword)
   at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.CacheMember(Type typeToConvert, MemberInfo memberInfo, Boolean& propertyOrderSpecified, Dictionary`2& ignoredMembers, Boolean shouldCheckForRequiredKeyword)
   at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.LateAddProperties()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.InitializePropertyCache()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureLocked|143_0()
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.get_JsonTypeInfo()
   at System.Text.Json.ReadStack.Push()
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Microsoft.SemanticKernel.Connectors.Memory.Weaviate.WeaviateMemoryStore.UpsertBatchAsync(String collectionName, IEnumerable`1 records, CancellationToken cancellationToken)+MoveNext() in C:\dotnet\src\Connectors\Connectors.Memory.Weaviate\WeaviateMemoryStore.cs:line 266

The WeaviateMemoryStore statically instantiates a JsonSerializerOptions object, but without ReadOnlyMemoryConverter, which is required to (de)serialize properties of the ReadOnlyMemory type < .NET 8 (according to the comments inside the converter class).

The issue only occurs whilst deserializing. Serialization works properly because the responsibility of serialization is abstracted away into Microsoft.SemanticKernel.Connectors.Memory.Weaviate.Http.HttpRequest.CreateSerializerOptions() -- which does indeed instantiate and passes an instance of ReadOnlyMemoryConverter into the JsonSerializerOptions.

Contribution Checklist

@jevgenigeurtsen jevgenigeurtsen requested a review from a team as a code owner September 11, 2023 14:49
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory connector labels Sep 11, 2023
@github-actions github-actions bot changed the title Weaviate .NET Connector: Added ReadOnlyMemoryConverter to fix deserialization .Net: Weaviate .NET Connector: Added ReadOnlyMemoryConverter to fix deserialization Sep 11, 2023
@dmytrostruk dmytrostruk self-assigned this Sep 11, 2023
@shawncal shawncal added the docs and tests Improvements or additions to documentation label Sep 12, 2023
@dmytrostruk
Copy link
Member

@jevgenigeurtsen @rogerbarreto I updated the documentation how to test Weaviate manually with local Weaviate server.
I also tested the integration, and it appeared that integration is not working, because all endpoints require API version now:
https://weaviate.io/developers/weaviate/api/rest#references
image

I exposed ApiVersion field in WeaviateMemoryStore constructor, so users will be able to set API version on their side. I also made v1 version as default.

After my changes I verified that integration tests are passing successfully now. Please check my latest changes and let me know if it looks good to you. Thanks!

@dmytrostruk dmytrostruk added the PR: ready for review All feedback addressed, ready for reviews label Sep 12, 2023
@jevgenigeurtsen
Copy link
Contributor Author

@microsoft-github-policy-service agree

@jevgenigeurtsen
Copy link
Contributor Author

jevgenigeurtsen commented Sep 13, 2023

@jevgenigeurtsen @rogerbarreto I updated the documentation how to test Weaviate manually with local Weaviate server. I also tested the integration, and it appeared that integration is not working, because all endpoints require API version now: https://weaviate.io/developers/weaviate/api/rest#references image

I exposed ApiVersion field in WeaviateMemoryStore constructor, so users will be able to set API version on their side. I also made v1 version as default.

After my changes I verified that integration tests are passing successfully now. Please check my latest changes and let me know if it looks good to you. Thanks!

Looks good to me, nice. Can we maybe bump the version of the Weaviate container to the latest version? This woud be version 1.21.2. Let me know if I need to do this.

Also (though this might be a different issue/PR): it would be nice if the CI runs the .NET integration tests, like it does for Python. This would've raised the issue regarding deserializing in a much earlier phase. I'm not that familiar with CI/CD on GitHub, but I'd be willing to contribute to this if needed.

@dmytrostruk
Copy link
Member

Can we maybe bump the version of the Weaviate container to the latest version? This woud be version 1.21.2.

Done, I've updated the version and verified using integration tests.

it would be nice if the CI runs the .NET integration tests

We are going to work on it in the nearest future. Most probably this will require authorized access to configure GitHub Actions.

Copy link
Member

@rogerbarreto rogerbarreto left a comment

Choose a reason for hiding this comment

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

Thank you both @dmytrostruk for @jevgenigeurtsen for looking into this!

@dmytrostruk dmytrostruk added this pull request to the merge queue Sep 13, 2023
Merged via the queue into microsoft:main with commit 97b8a67 Sep 13, 2023
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
…eserialization (microsoft#2777)

### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?

  3. What problem does it solve?
  4. What scenario does it contribute to?
  5. If it fixes an open issue, please link to the issue here.
-->

All operations inside `WeaviateMemoryStore` whose response models
contain any property of the type `ReadOnlyMemory<>`, will throw an
exception whenever the JSON deserializer tries to deserialize that
specific property.

I believe this is caused by the changes made in
microsoft#2419

### Description

I encountered the following exception while trying out the Weaviate
connector:

```
  System.InvalidOperationException : The type 'System.ReadOnlySpan`1[System.Single]' of property 'Span' on type 'System.ReadOnlyMemory`1[System.Single]' is invalid for serialization or deserialization because it is a pointer type, is a ref struct, or contains generic parameters that have not been replaced by specific types.
  Stack Trace:
     at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(Type typeToConvert, Type declaringType, MemberInfo memberInfo)
   at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.CreateProperty(Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options, Boolean shouldCheckForRequiredKeyword)
   at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.CacheMember(Type typeToConvert, MemberInfo memberInfo, Boolean& propertyOrderSpecified, Dictionary`2& ignoredMembers, Boolean shouldCheckForRequiredKeyword)
   at System.Text.Json.Serialization.Metadata.ReflectionJsonTypeInfo`1.LateAddProperties()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.InitializePropertyCache()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.Configure()
   at System.Text.Json.Serialization.Metadata.JsonTypeInfo.<EnsureConfigured>g__ConfigureLocked|143_0()
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo.get_JsonTypeInfo()
   at System.Text.Json.ReadStack.Push()
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Microsoft.SemanticKernel.Connectors.Memory.Weaviate.WeaviateMemoryStore.UpsertBatchAsync(String collectionName, IEnumerable`1 records, CancellationToken cancellationToken)+MoveNext() in C:\dotnet\src\Connectors\Connectors.Memory.Weaviate\WeaviateMemoryStore.cs:line 266
```

The `WeaviateMemoryStore` statically instantiates a
`JsonSerializerOptions` object, but without `ReadOnlyMemoryConverter`,
which is required to (de)serialize properties of the `ReadOnlyMemory`
type < .NET 8 (according to the comments inside the converter class).

The issue only occurs whilst deserializing. Serialization works properly
because the responsibility of serialization is abstracted away into
`Microsoft.SemanticKernel.Connectors.Memory.Weaviate.Http.HttpRequest.CreateSerializerOptions()`
-- which does indeed instantiate and passes an instance of
`ReadOnlyMemoryConverter` into the `JsonSerializerOptions`.

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [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: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com>
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

Labels

docs and tests Improvements or additions to documentation kernel Issues or pull requests impacting the core kernel memory connector .NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants