initial implementation for vectordb #3879
Conversation
|
@jackgerrits @ekzhu , wondering if storage should be an abstraction in the core package and only the implementation should go into autogen-ext. Any thoughts? Still an early draft, just looking for some feedback on the design. I think libraries like langchain and llamaindex have similar abstractions which are useful for rag but could have other uses for for agents that do few-shot prompting |
There was a problem hiding this comment.
Thanks @lspinheiro for the PR! I've just minor suggestions, others look good to me.
lspinheiro
force-pushed
the
lpinheiro/feat/add-vectordb-chroma-store
branch
from
22cb336 to
7da80e9
Compare
lspinheiro
force-pushed
the
lpinheiro/feat/add-vectordb-chroma-store
branch
2 times, most recently
from
22cb336 to
4b886b0
Compare
|
@ekzhu , small nudge. This should be ready for review now. |
lspinheiro
force-pushed
the
lpinheiro/feat/add-vectordb-chroma-store
branch
from
8bb13f0 to
b3f0672
Compare
| """Define Document according to autogen 0.4 specifications.""" | ||
|
|
||
| id: ItemID | ||
| content: Optional[str] = None |
There was a problem hiding this comment.
This is the same representation used in 0.2 and I believe it assumes the content has already been preprocessed to a string format. More complex data types can lead to more complex operations in general. Would you suggest something different?
There was a problem hiding this comment.
the vector store can have a payload object, which is usually a json. Unless you know the schema mapping returning string is a problem, maybe we can use dictionary or some other model?
There was a problem hiding this comment.
I don't think this is meant to capture the full payload but only the main text content of the associated document and the transformation between autogen doc and vectordb doc is handled in each implementation. The pydantic model is defined with arbitrary_types_allowed which should allow implementations to unpack additional attributes as needed at the document level instead of the content. Would that work for the scenario you have in mind?
| type: str = "" | ||
| embedding_function: Optional[Callable[..., Any]] = None # embeddings = embedding_function(sentences) | ||
|
|
||
| async def create_collection(self, collection_name: str, overwrite: bool = False, get_or_create: bool = True) -> Any: |
There was a problem hiding this comment.
some vector store requires schema to create collection and some other parameters, may using variable args here can help generalise
There was a problem hiding this comment.
That is a great callout. Checked the old code and those additional arguments were being passed in through the class constructor. Not the best design. Pushing a change as soon as the checks complete.
There was a problem hiding this comment.
Should be updated now
| """ | ||
| ... | ||
|
|
||
| def update_docs(self, docs: List[Document], collection_name: Optional[str] = None, **kwargs: Any) -> None: |
There was a problem hiding this comment.
the most used method is upsert
There was a problem hiding this comment.
For the protocol, my understanding is that the idea is to keep it closer in design the DB theory and enforce CRUD like operations. Implementations can add upsert but they probably shouldn't be forced to
MohMaz
added
the
awaiting-op-response
Why are these changes needed?
Related issue number
Checks