Hi @dmytrostruk,
Thanks for your feedback (and let me know if you need a hand with Oobabooga)
I didn't expect this PR to be merged directly, so I went with quick fixes to remove the bugs and start this discussion.

But I'm not sure I understand the flow here: we try to search something in collection, which doesn't exist, so we create that collection, and the result will be always empty?
In this way we can notify developers that something is wrong in their searching logic.

Well I'm sorry I wasn't more precise in my description. The current issue is that with the current flow, with each Http request, the current chat session memory is searched for before the chat history memory items are added to it. It only crashes with chroma on first load (qdrant mutes the exception and then the collection is properly created). Accordingly, my fix was more some temporary patch before a better decision is made.
The thing is, we don't really want to test whether the collection exists each time we do a search: that involves an extra http call and that would be a significant performance drain in the long run. Accordingly, I made a synchronized dictionary to make sure we only ever test that once, and in the case of copilot chat, the collection is expected to exist when searched, thus my fix. I can happily change that for a muted try/catch, which would probably do the trick.

Regarding DoesCollectionExistAsync method in Chroma - I don't see an issue in handling exception in case if collection doesn't exist - it's just the way how Chroma API is designed and ChromaMemoryStore, as API consumer, handles that.

Sure, that was just a suggestion to prevent the crash. The thing is, as far as I can see, unlike QDrant, Chroma currently really does not like that use, and has a proper crash.
Here is the current stack trace from querying a missing collection, (I rebuilt my docker container from the latest code base to make sure it did not go away).

2023-07-13 17:23:29 chroma-server-1 | 2023-07-13 15:23:29 ERROR chromadb.server.fastapi Collection chat-documents-28763999-4538-4d90-918b-1ac90bbaa960 does not exist
2023-07-13 17:23:29 chroma-server-1 | Traceback (most recent call last):
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/anyio/streams/memory.py", line 98, in receive
2023-07-13 17:23:29 chroma-server-1 | return self.receive_nowait()
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/anyio/streams/memory.py", line 93, in receive_nowait
2023-07-13 17:23:29 chroma-server-1 | raise WouldBlock
2023-07-13 17:23:29 chroma-server-1 | anyio.WouldBlock
2023-07-13 17:23:29 chroma-server-1 |
2023-07-13 17:23:29 chroma-server-1 | During handling of the above exception, another exception occurred:
2023-07-13 17:23:29 chroma-server-1 |
2023-07-13 17:23:29 chroma-server-1 | Traceback (most recent call last):
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 43, in call_next
2023-07-13 17:23:29 chroma-server-1 | message = await recv_stream.receive()
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/anyio/streams/memory.py", line 118, in receive
2023-07-13 17:23:29 chroma-server-1 | raise EndOfStream
2023-07-13 17:23:29 chroma-server-1 | anyio.EndOfStream
2023-07-13 17:23:29 chroma-server-1 |
2023-07-13 17:23:29 chroma-server-1 | During handling of the above exception, another exception occurred:
2023-07-13 17:23:29 chroma-server-1 |
2023-07-13 17:23:29 chroma-server-1 | Traceback (most recent call last):
2023-07-13 17:23:29 chroma-server-1 | File "/chroma/./chromadb/server/fastapi/init.py", line 57, in catch_exceptions_middleware
2023-07-13 17:23:29 chroma-server-1 | return await call_next(request)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 46, in call_next
2023-07-13 17:23:29 chroma-server-1 | raise app_exc
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/middleware/base.py", line 36, in coro
2023-07-13 17:23:29 chroma-server-1 | await self.app(scope, request.receive, send_stream.send)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 75, in call
2023-07-13 17:23:29 chroma-server-1 | raise exc
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/middleware/exceptions.py", line 64, in call
2023-07-13 17:23:29 chroma-server-1 | await self.app(scope, receive, sender)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 21, in call
2023-07-13 17:23:29 chroma-server-1 | raise e
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/fastapi/middleware/asyncexitstack.py", line 18, in call
2023-07-13 17:23:29 chroma-server-1 | await self.app(scope, receive, send)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 680, in call
2023-07-13 17:23:29 chroma-server-1 | await route.handle(scope, receive, send)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 275, in handle
2023-07-13 17:23:29 chroma-server-1 | await self.app(scope, receive, send)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/routing.py", line 65, in app
2023-07-13 17:23:29 chroma-server-1 | response = await func(request)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 231, in app
2023-07-13 17:23:29 chroma-server-1 | raw_response = await run_endpoint_function(
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/fastapi/routing.py", line 162, in run_endpoint_function
2023-07-13 17:23:29 chroma-server-1 | return await run_in_threadpool(dependant.call, **values)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/starlette/concurrency.py", line 41, in run_in_threadpool
2023-07-13 17:23:29 chroma-server-1 | return await anyio.to_thread.run_sync(func, *args)
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/anyio/to_thread.py", line 33, in run_sync
2023-07-13 17:23:29 chroma-server-1 | return await get_asynclib().run_sync_in_worker_thread(
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 877, in run_sync_in_worker_thread
2023-07-13 17:23:29 chroma-server-1 | return await future
2023-07-13 17:23:29 chroma-server-1 | File "/usr/local/lib/python3.10/site-packages/anyio/_backends/_asyncio.py", line 807, in run
2023-07-13 17:23:29 chroma-server-1 | result = context.run(func, *args)
2023-07-13 17:23:29 chroma-server-1 | File "/chroma/./chromadb/server/fastapi/init.py", line 182, in get_collection
2023-07-13 17:23:29 chroma-server-1 | return self._api.get_collection(collection_name)
2023-07-13 17:23:29 chroma-server-1 | File "/chroma/./chromadb/api/local.py", line 180, in get_collection
2023-07-13 17:23:29 chroma-server-1 | raise ValueError(f"Collection {name} does not exist")
2023-07-13 17:23:29 chroma-server-1 | ValueError: Collection chat-documents-28763999-4538-4d90-918b-1ac90bbaa960 does not exist
2023-07-13 17:23:29 chroma-server-1 | 2023-07-13 15:23:29 INFO uvicorn.access 172.18.0.1:44546 - "GET /api/v1/collections/chat-documents-28763999-4538-4d90-918b-1ac90bbaa960 HTTP/1.1" 500

Accordingly, it felt wrong to me to trigger that crash "just" from calling "DoesCollectionExistAsync", whereas I would have been less uncomfortable with triggering that crash from searching a non existing collection.

and let me know if you need a hand with Oobabooga

I'm in progress :)

The thing is, we don't really want to test whether the collection exists each time we do a search: that involves an extra http call and that would be a significant performance drain in the long run. Accordingly, I made a synchronized dictionary to make sure we only ever test that once

My point here is that we shouldn't perform any check or creation action on Seach operation at all. If collection doesn't exist at point when you try to search something, it's not a business-logic error, it's development/architecture error, so you need to go and check manually why this is happening and fix it only once if needed. It's very similar case as in Entity Framework - you will receive an error if you try to search some results in non-existing table. If this is even the case - most probably something is wrong with application flow and should be re-visited.

• With approach proposed in this PR, Copilot Chat client will receive empty result set in both cases: when collection is empty, or collection doesn't exist. (In case it doesn't exist, server is going to create it, but it's not clear why).
• Ideally, Copilot Chat client should receive empty result set only when collection is empty, and if collection doesn't exist, it should receive 400 Bad Request or 404 Not Found if the logic is on client side, or 500 Internal Server Error if the logic is on server side. It's up to Copilot Chat client how to handle this case on UI.

Chroma currently really does not like that use, and has a proper crash.

As long as Chroma server is not stopped/crashed because of that error, it should be fine. I don't mind listing collections as well, because I'm not sure there will be a case with big number of collections, that could impact the performance (although theoretically it's still possible). I just don't see big advantage in making that change (yet). Maybe we can do some performance/stress testing and check which approach works faster/more reliable.

@dmytrostruk OK based on your last remarks I reverted my initial safeguards:

• The collection safe wrapper was removed and the copilotchat app now uses the Chroma memory store directly.
  Accordingly, the application crashes on first chat post when making use of SemanticTextMemory.SearchAsync on collections before they exist, that is before items are added to them.
• The Chroma memory store "DoesCollectionExistAsync" relies again on a try catch and logs an error.

To fix the first issue, we have to decide whether to:

• Update SemanticTextMemory to include a try/catch in the SearchAsync method

• Update the places in the copilotchat app where it is used to include try/catch, namely:

    Controllers
      BotController.cs
        (217,79) …lt> collectionMemoryRecords = await kernel.Memory.SearchAsync( ↵ collectionName, ↵ "abc", // dummy query since we don't care about relevance. An empty string will cause exception. …
    Skills
      ChatSkills
        DocumentMemorySkill.cs
          (64,38) var results = textMemory.SearchAsync( ↵ documentCollection, ↵ query, ↵ limit: 100, ↵ minRelevanceScore: this._promptOptions.DocumentMemoryMinRelevance)
        SemanticChatMemoryExtractor.cs
          (126,45) var memories = await context.Memory.SearchAsync( ↵ collection: memoryCollectionName, ↵ query: item.ToFormattedString(), ↵ limit: 1, ↵ minRelevanceScore: options.SemanticMemoryMinRelevance, ↵ cancellat…
        SemanticChatMemorySkill.cs
          (52,38) var results = textMemory.SearchAsync( ↵ SemanticChatMemoryExtractor.MemoryCollectionName(chatId, memoryName), ↵ query, ↵ limit: 100, ↵ minRelevanceScore: this._promptOptions.SemanticMemoryMinRelevance…```
  
  

• Update the Chroma memory store's GetNearestMatchesAsync method to align it with the way Qdrant or Volatile memory store were conceived, namely,

  • Volatile memory store has

if (this.TryGetCollection(collectionName, out var collectionDict))
        {
            embeddingCollection = collectionDict.Values;
        }

        if (embeddingCollection == null || embeddingCollection.Count == 0)
        {
            return AsyncEnumerable.Empty<(MemoryRecord, double)>();
        }

• Qdrant memory store has:

try
            {
                hasResult = await enumerator.MoveNextAsync().ConfigureAwait(false);
                if (hasResult)
                {
                    result = enumerator.Current;
                }
                else
                {
                    result = null;
                }
            }
            catch (HttpRequestException ex) when (ex.Message.Contains("404"))
            {
                this._logger?.LogWarning("NotFound when calling {0}::FindNearestInCollectionAsync - the collection '{1}' may not exist yet",
                    nameof(QdrantMemoryStore), collectionName);
                hasResult = false;
            }

• whereas Chroma memory store starts with:

Verify.NotNullOrWhiteSpace(collectionName);

var collection = await this.GetCollectionOrThrowAsync(collectionName, cancellationToken).ConfigureAwait(false);

Addressing the second issue seems out of scope of this PR, and would require further investigations about the best way to test for collection existence in Chroma.

Maybe change the existing error message though:

this._logger.LogError("Collection {0} does not exist", collectionName);

If this is to be used by "DoesCollectionExistAsync", it shouldn't probably log an error when collection does not exist. Since instrumentation was part of your last PRs, what do you suggest we use instead?

Note that as of the end of today, Copilot Chat will be renamed and moved to its own repo: https://github.com/microsoft/chat-copilot

You will want to close this PR and open a corresponding one in that repo instead.

Note that as of the end of today, Copilot Chat will be renamed and moved to its own repo: https://github.com/microsoft/chat-copilot

You will want to close this PR and open a corresponding one in that repo instead.

@glahaye Noted, I will make a new PR in the new repo.
In the mean time, can you guys make a decision about my last comment, so that the new PR include the appropriate fixes?

@glahaye Noted, I will make a new PR in the new repo. In the mean time, can you guys make a decision about my last comment, so that the new PR include the appropriate fixes?

@dmytrostruk Any thoughts on this?

@jsboige Sorry for delayed response.

Regarding first issue, I think Update the places in the copilotchat app where it is used to include try/catch approach should be good. If SearchAsync should be wrapped in try/catch block in multiple places, we can create common method and re-use it.

Regarding second issue about collection existence and logging, there are two cases:

1. When there is search action in collection that doesn't exist - we should throw an exception and log it as Error.
2. When there is a call DoesCollectionExistAsync and we try to fetch collection from DB. There is try/catch block based on Chroma API response. This is expected scenario and DoesCollectionExistAsync method will return false. In this case:
   • We can't log it as Error, since it's not throwing an exception to the caller and doesn't cause the application execution to stop.
   • We can't log it as Warning, since it's expected event and there is nothing to fix there.

So, we can log it as Debug just for additional observability, but in general that's just transient error.

Regarding alignment between different connectors - we should definitely do that at some point. And I think that for Search operation:

• If collection exists but it's empty - we should return empty collection.
• If collection doesn't exist - we should throw an exception to the caller, because it will clearly describe that something is wrong with memory connector setup.

In this case it will be easier to differentiate two scenarios and react on them appropriately.

Please let me know if that makes sense.

@jsboige thanks for all the hard work!
Are you okay with closing this PR since you created another one in new Copilot Chat repository?

I'm going ahead and closing this PR.

It's equivalent in the proper repo is microsoft/chat-copilot#62