-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
community[minor]: Improve Azure Cosmos DB vector store support #5197
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -48,13 +48,18 @@ describe.skip("AzureCosmosDBVectorStore", () => { | |||
process.env.AZURE_COSMOSDB_CONNECTION_STRING! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there! I've reviewed the code and noticed that the recent changes explicitly access an environment variable using process.env
. I've flagged this for your review to ensure it aligns with our environment variable handling practices. Let me know if you have any questions or need further clarification.
similarity: AzureCosmosDBSimilarityType = AzureCosmosDBSimilarityType.COS | ||
): Promise<void> { | ||
await this.initPromise; | ||
await this.connectPromise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is hypothetically called before connectPromise
is initialized, could we throw instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah never mind, I see below. Could we always just rely on this.initPromise
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since init
calls createIndex
we need 2 different promise there otherwise there's an interlocking:
- connectPromise => create DB + collections clients
- initPromise => connectPromise + createIndex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the throw question, the issue is that the promise is created in the constructor and there's no way for the user to wait for it or know when the connect task is done.
857756b
to
4055993
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks solid, couple comments. Please request my review once implemented, these will be great to get released!
…azure-mongo-update
Thank you! |
* core[minor]: RunnableLambda should consume (async) iterator if the wrapped function returns one (#5342) * core: RunnableLambda should consume async iterator if the wrapped function returns one * Consume iterators too * Add tests * Dont interpret arrays/sets/etc as iterators * Implement in invoke too * Fix async storage propagation * Handle any async iterable * Add more tests * community[minor]: Improve Azure Cosmos DB vector store support (#5197) * feat: add delete by filter * feat: return added document ids * fix: delete by id * test: update integration tests * feat: add automatic index creation * Update azure_cosmosdb.ts * refactor: separate ids and filter params for delete() * refactor: use a single param for delete * test: fix unit tests * Address feedback --------- Co-authored-by: Jacob Lee <jacoblee93@gmail.com> * Revert "Merge branch 'v0.1' into main" (#5345) This reverts commit db5ab3f, reversing changes made to 2b3b194. --------- Co-authored-by: Nuno Campos <nuno@boringbits.io> Co-authored-by: Yohan Lasorsa <noda@free.fr> Co-authored-by: Jacob Lee <jacoblee93@gmail.com>
This PR improves the existing Azure Cosmos DB vector store in the following ways:
It does not include any breaking changes.