microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.83k stars 3.25k forks source link

.Net: include parameter in QueryEmbeddingsAsync() in ChromaClient should not be null #6359

Open wooly905 opened 5 months ago

wooly905 commented 5 months ago

Describe the bug The signature of this function is below,

public async Task QueryEmbeddingsAsync(string collectionId, ReadOnlyMemory[] queryEmbeddings, int nResults, string[]? include = null, CancellationToken cancellationToken = default)

The last second parameter - include - should not be null as default; otherwise Chroma will throw exception in its python code.

The screenshot of exception will happen when "include" is null. image

The value of include should be "documents", "ids", and so on. Why not implement it as Enum array instead of string array to avoid typo.

dmytrostruk commented 5 months ago

Hi @wooly905 , thanks for reporting this issue.

The last second parameter - include - should not be null as default; otherwise Chroma will throw exception in its python code.

It's strange, since it worked previously, and I can see that this parameter has default value on Chroma server side: https://github.com/chroma-core/chroma/blob/cf6381383f03478800bad42895d736a469a1d137/chromadb/server/fastapi/types.py#L36

We will investigate it further and add default values on our side if needed.

Why not implement it as Enum array instead of string array to avoid typo.

During initial implementation, it was not clear whether this field will be dynamic or static. When it's dynamic, it's more flexible to define it as string collection to avoid updating enum every time when new value will be added to the list of possible values. But now it looks like this parameter can only accept limited set of values and it is not updated a lot, so we can rewrite it to enum on our side: https://github.com/chroma-core/chroma/blob/cf6381383f03478800bad42895d736a469a1d137/chromadb/api/types.py#L126

github-actions[bot] commented 2 months ago

This issue is stale because it has been open for 90 days with no activity.

moonbox3 commented 1 week ago

Hi @dmytrostruk, does this item still need to be open? Thanks.