[Store][ChromaDB] Add documents for text context #1012
Conversation
xprojects-de
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
changed the title
xprojects-de
changed the title
OskarStark
changed the title
|
|
||
| /** | ||
| * @param array{where?: array<string, string>, whereDocument?: array<string, mixed>} $options | ||
| * @param array{where?: array<string, string>, whereDocument?: array<string, mixed>, include?: array<string>} $options |
There was a problem hiding this comment.
which expectations do we have here for the include option?
if i just want embeddings, metadatas and distances - how would i enable that?
There was a problem hiding this comment.
If you don't want to specify anything, simply leave it out. Then the default from the ChromaDB extension will be used. That's how it was before. However, as soon as something is passed by the user, we should ensure that at least the metadata and embeddings are included, which the array merge does (maybe we can remove distance)
There was a problem hiding this comment.
If you don't want to specify anything, simply leave it out.
yup, got that, that's good 👍
i understand that we basically treat ['embeddings', 'metadatas', 'distances'] as the default here, right?
what would i need to do, to enable that default? i'm questioning if that even works with that current implementation. adding a test here would be great
There was a problem hiding this comment.
Thanks for reviewing the code and for the comments.
I've added some tests.
The default $include = null should work since CodeWithKyrian/chromadb-php has the correct default (see link).
However, we can also define our own default if you like...
xprojects-de
force-pushed
the
feature/fixChromaDbStore
branch
from
cd847c1 to
88efb84
Compare
4 participants
To work properly with embeddings, the text context is also needed, otherwise LLM won't get the overall context. Therefore, it's possible to query the text context from the database too.