Skip to content

Chroma - fix for saving same memory record multiple times#1790

Merged
shawncal merged 2 commits intomicrosoft:mainfrom
dmytrostruk:chroma-upsert-fix
Jun 30, 2023
Merged

Chroma - fix for saving same memory record multiple times#1790
shawncal merged 2 commits intomicrosoft:mainfrom
dmytrostruk:chroma-upsert-fix

Conversation

@dmytrostruk
Copy link
Member

@dmytrostruk dmytrostruk commented Jun 30, 2023

Motivation and Context

Method SaveInformationAsync of kernel memory type should be idempotent to update memory record with same key multiple times. This PR contains changes to allow that behavior in Chroma connector and tests to verify it.

Description

  1. Replaced Chroma POST /collections/{collection_id}/add with POST collections/{collection_id}/upsert to allow idempotent behavior.
  2. Added integration tests to verify scenario with adding/updating memory record with same key multiple times.

Contribution Checklist

Fixes #1783

@dmytrostruk dmytrostruk requested a review from a team as a code owner June 30, 2023 09:56
@github-actions github-actions bot added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Jun 30, 2023
@dmytrostruk dmytrostruk added memory connector ai connector Anything related to AI connectors PR: ready for review All feedback addressed, ready for reviews labels Jun 30, 2023
@dmytrostruk dmytrostruk changed the title Chroma - fix for adding same memory record multiple times Chroma - fix for saving same memory record multiple times Jun 30, 2023
@shawncal shawncal added this pull request to the merge queue Jun 30, 2023
Merged via the queue into microsoft:main with commit 348c1c8 Jun 30, 2023
@dluc
Copy link
Contributor

dluc commented Jun 30, 2023

Should fix #1783 - Thanks!

shawncal pushed a commit to shawncal/semantic-kernel that referenced this pull request Jul 6, 2023
…1790)

### 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.
-->
Method `SaveInformationAsync` of kernel memory type should be idempotent
to update memory record with same key multiple times. This PR contains
changes to allow that behavior in Chroma connector and tests to verify
it.

### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
1. Replaced Chroma `POST /collections/{collection_id}/add` with `POST
collections/{collection_id}/upsert` to allow idempotent behavior.
2. Added integration tests to verify scenario with adding/updating
memory record with same key multiple times.

### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows SK Contribution Guidelines
(https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [x] The code follows the .NET coding conventions
(https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions)
verified with `dotnet format`
- [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

ai connector Anything related to AI connectors 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.

Chroma .NET SaveInformationAsync is not idempotent

4 participants