Python: feat: add chroma memory store

[joowon-dm-snu]

Motivation and Context

solve issue #403
this is reopened PR #426

Description

I've added one quick E2E test example without adding any unit tests.
I tried to make it as identical to the original classes(VolatileDataStore & VolatileMemoryStore) as possible.

This PR has 3 issues.

• collection name does not allow upper cases(Collection name validation error message should mention lowercase or permit uppercase chroma-core/chroma#237). I've written a snake case to replace it inside the current class.
• get_nearest_matches_async function can be more complicated if your team wants to calculate relevance score like langchain.
• I didn't add requirements (pip install chromadb)

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
• The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
• All unit tests pass, and I have added new tests where possible
• I didn't break anyone 😄

[awharrison-28]

Hi @joowon-dm-snu

Tried this out with the memory notebook and I have a couple questions. By default, if you replace the VolatileMemoryStore() with ChromaMemoryStore(), the memory store uses embedded DuckDB and I'm pretty confident that it's downloading an embedding model from sentence-transformers - ie the embeddings are not coming from the Kernel, but being generated by chroma infrastructure. Presumably this behavior can be changed using a chromadb settings file?

At minimum, we should make sure to document the default behavior, but ideally the base ChromaMemoryStore() without additional settings should use embeddings from the kernel.

[joowon-dm-snu]

@awharrison-28

At minimum, we should make sure to document the default behavior, but ideally the base ChromaMemoryStore() without additional settings should use embeddings from the kernel.

Agree with documentation!
However, I didn't write the documentation because the semantic-kernel is a fully managed by MS, and I was worried that it wouldn't be adopted even if I wrote the official documentation. this is same for unit-tests

If you don't mind me writing it, and tell me what you want it to contain, I can write it :)

[awharrison-28]

• Can you please add *.chroma/ to the .gitignore under # Persistant storage (which you could rename from # Persistant storage -> # Persistent storage while you're at it :)
• See Code Comment - Found the Root Issue ---There's some incorrect logic in this PR that I'm investigating. I originally thought it might be the embeddings not being generated by the Kernel's registered AI service, but your code is consistent with ChromaDB docs for 'bringing your own embeddings." Here's a printout of the difference between memory stores (they should be the same). I will comment further if I find the issue.

With the VolatileMemoryStore:

======================
Query: I love Jupyter notebooks, how should I get started?

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/blob/main/samples/notebooks/dotnet/00-getting-started.ipynb
Title : Jupyter notebook describing how to get started with the Semantic Kernel
Relevance: 0.8677540305183273

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/blob/main/samples/notebooks/dotnet/02-running-prompts-from-file.ipynb
Title : Jupyter notebook describing how to pass prompts from a file to a semantic skill or function
Relevance: 0.8164925932277455

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/blob/main/README.md
Title : README: Installation, getting started, and how to contribute
Relevance: 0.8086237941419581

With ChromaMemoryStore:

===========================
Query: I love Jupyter notebooks, how should I get started?

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/blob/main/samples/notebooks/dotnet/00-getting-started.ipynb
Title : Jupyter notebook describing how to get started with the Semantic Kernel
Relevance: 0.2644919753074646

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/blob/main/samples/notebooks/dotnet/02-running-prompts-from-file.ipynb
Title : Jupyter notebook describing how to pass prompts from a file to a semantic skill or function
Relevance: 0.3668723404407501

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/blob/main/README.md
Title : README: Installation, getting started, and how to contribute
Relevance: 0.38269340991973877

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/tree/main/samples/apps/chat-summary-webapp-react/README.md
Title : README: README associated with a sample starter react-based chat summary webapp
Relevance: 0.47094565629959106

Result {++i}:
URL: : https://github.com/microsoft/semantic-kernel/tree/main/samples/skills/ChatSkill/ChatGPT
Title : Sample demonstrating how to create a chat skill interfacing with ChatGPT
Relevance: 0.5258410573005676

[awharrison-28]

@awharrison-28

At minimum, we should make sure to document the default behavior, but ideally the base ChromaMemoryStore() without additional settings should use embeddings from the kernel.

Agree with documentation! However, I didn't write the documentation because the semantic-kernel is a fully managed by MS, and I was worried that it wouldn't be adopted even if I wrote the official documentation. this is same for unit-tests

If you don't mind me writing it, and tell me what you want it to contain, I can write it :)

We are pro-documentation!

Can you add a description of how ChromaMemoryStore works at the top of ChromaMemoryStore.py. Mentioning things like the kernel generates the embeddings and the decision to custom calculate similarity instead of using ChromaDB nearest neighbor implementation are great things to add. @mkarle @alexchaomander would you agree that this is a good place to add implementation detail comments?

[alexchaomander]

Yup +1 to putting directly in the comments the implementation details if we have to make certain choices on how to use Chroma with the SK.

[alexchaomander]

Couple grammar changes but overall looks good to me!

[awharrison-28]

@joowon-dm-snu heads up, Python SK memory is getting a refactor -> major simplification of classes. If this PR goes through first, I'll take point on updating Chroma memory store.

#684

[joowon-dm-snu]

@joowon-dm-snu

Would you be willing to start the test_integration dependencies group in poetry?

okay, i think splitting dependencies should be merged first. I'll take a look when I get to work

@joowon-dm-snu heads up, Python SK memory is getting a refactor -> major simplification of classes. If this PR goes through first, I'll take point on updating Chroma memory store.

No problem!

[joowon-dm-snu]

@awharrison-28 i do some searches to fix installation of chromadb for macos-latest, python3.11 but couldn't find good solution

changing semantic-kernel/.github/workflows/python-test.yml like below raise other clang errors

- name: Setup envs
  if: matrix.os == 'macos-latest' && matrix.python-version == '3.11'
  run: |
    export POETRY_HNSWLIB_NO_NATIVE=1
    python -m pip wheel --use-pep517 "hnswlib (==0.7.0)"
    cd python && poetry install --with test_integration

[tawalke]

Currently reviewing to ensure in line with both SK versions.

[joowon-dm-snu]

@tawalke

Currently reviewing to ensure in line with both SK versions.

this PR might need change as memory store changed by #684.
If your team wants to me to done with this PR, i can take it.
But installation of chromadb for macos-latest, python3.11 should be solved first to pass Python tests / build (3.11, macos-latest) (pull_request)

[tawalke]

@tawalke

Currently reviewing to ensure in line with both SK versions.

this PR might need change as memory store changed by #684. If your team wants to me to done with this PR, i can take it. But installation of chromadb for macos-latest, python3.11 should be solved first to pass Python tests / build (3.11, macos-latest) (pull_request)

@awharrison-28 Per your last comment re; your update of chroma memory based upon memory_base update.; should the current review be paused?

[awharrison-28]

@joowon-dm-snu would you be open to me updating this PR directly to update it to be consistent with #684.

I'll also have a PR out shortly to solidify how we handle conditional dependencies.

[joowon-dm-snu]

@joowon-dm-snu would you be open to me updating this PR directly to update it to be consistent with #684.

I'll also have a PR out shortly to solidify how we handle conditional dependencies.

Okay I get it, I'll handle it this week :)

[awharrison-28]

reopening and updating

[joowon-dm-snu]

@awharrison-28 ah my forked branch was so twisted so I rebranched it.