Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.Net: Fixes for Chroma connector to be compatible with Chroma 0.4.10 #2796

Merged
merged 5 commits into from
Sep 12, 2023

Conversation

dmytrostruk
Copy link
Member

Motivation and Context

Resolves: #2790

This PR contains changes to update processing of boolean values in order to be compatible with Chroma 0.4.10.
With new Chroma updates we need to update Chroma connector on regular basis to be compatible.

The note has been added to Chroma notebook and README file to show the latest version of Chroma, which was used to verify the connector.

In order to fix notebook to be compatible with latest version, the notebook should be updated as soon as this PR is released.

Description

  1. Removed ChromaBooleanConverter to be compatible with Chroma 0.4.10.
  2. Added note with latest tested version of Chroma in notebook and README file.
  3. Updated unit tests.

Contribution Checklist

@dmytrostruk dmytrostruk requested a review from a team as a code owner September 12, 2023 14:51
@shawncal shawncal added docs and tests Improvements or additions to documentation .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory connector labels Sep 12, 2023
@dmytrostruk dmytrostruk self-assigned this Sep 12, 2023
@dmytrostruk dmytrostruk added the PR: ready for review All feedback addressed, ready for reviews label Sep 12, 2023
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.

LGTM

@jamescarter-le
Copy link

I still get this error downloading main semantic-kernel and main chroma and executing the 09 tutorial notebook in C#.

It can write data into Chroma, and query them, looks like it fails to deserialize:

Error: Microsoft.SemanticKernel.Diagnostics.SKException: Function `recall` execution failed. System.Text.Json.JsonException: The JSON value could not be converted to Microsoft.SemanticKernel.Memory.MemoryRecordMetadata. Path: $.is_reference | LineNumber: 0 | BytePositionInLine: 102.
---> System.Text.Json.JsonException: The JSON value could not be converted to Microsoft.SemanticKernel.Memory.MemoryRecordMetadata. Path: $.is_reference | LineNumber: 0 | BytePositionInLine: 102.
---> System.InvalidOperationException: Cannot get the value of a token type 'False' as a number.
at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_ExpectedNumber(JsonTokenType tokenType)
at System.Text.Json.Utf8JsonReader.TryGetInt16(Int16& value)
at Microsoft.SemanticKernel.Connectors.Memory.Chroma.ChromaBooleanConverter.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)

@dmytrostruk
Copy link
Member Author

@jamescarter-le Thanks for reporting. As mentioned in notebook, current version of SK in notebook is compatible with Chroma version 0.4.0 at the moment and not above. We plan to upgrade SK version in all notebook examples soon, and after that it will be compatible with latest Chroma version. Meanwhile, you can test using Example15_TextMemoryPlugin example, which should be compatible with latest Chroma version:

// Chroma Memory Store
// store = CreateSampleChromaMemoryStore();

@jamescarter-le
Copy link

@dmytrostruk

Oh I see! The notebook is referencing a nuget package whilst the KernelSyntaxExamples reference the latest code and project.

I was able to run the same by pulling 0.4.0 chroma, my fault for not reading the notebook thoroughly.

Could change the chroma download instructions to include:

git fetch --all --tags
git checkout tags/0.4.0 -b sk-notebooks

Appreciate the support and enjoying SK so far.

SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
…icrosoft#2796)

### 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?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

Resolves: microsoft#2790

This PR contains changes to update processing of boolean values in order
to be compatible with Chroma 0.4.10.
With new Chroma updates we need to update Chroma connector on regular
basis to be compatible.

The note has been added to Chroma notebook and README file to show the
latest version of Chroma, which was used to verify the connector.

In order to fix notebook to be compatible with latest version, the
notebook should be updated as soon as this PR is released.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
1. Removed `ChromaBooleanConverter` to be compatible with Chroma 0.4.10.
2. Added note with latest tested version of Chroma in notebook and
README file.
3. Updated unit tests.

### 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 😄
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
None yet
Development

Successfully merging this pull request may close these issues.

.Net: Cannot get the value of a token type 'False' as a number when using Chroma Memory Store
5 participants