-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[ENH] AmazonBedrockEmbeddingFunction for JS #1659
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
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.
@chezou, thank you for the great work. This looks good, but one important aspect of why Chroma is great is that we invest a lot of time crafting tests to ensure DX is smooth.
Will it be too much to ask you to add a test for the bedrock EF. You can find an example of a conditional test here for OpenAI here -
if (!process.env.OPENAI_API_KEY) { |
I added a change to ensure the default region, otherwise, the client returns error fa9746f I'll add a test for this embedding function later. |
Added a test for the function 6ef9154 |
@tazarov Can you allow to run tests? |
@chezou im not sure why tests aren't running 🤔 can you tack on an extra commit here to trigger them? thank you! my apologies for the delay here |
@jeffchuber Thanks! Created an empty commit b7425e8, but it looks approval for run is required |
Fixed the PR title since "Check PR Title" was failed. |
tests will now re-run with the merge conflicts handled. sorry about the PR title issue |
@tazarov @jeffchuber If there's no issue, could you approve and merge this PR? |
@chezou, do you mind resolving the conflict? |
@tazarov Resolved |
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.
One clarification and a nit about the test. Other than this it looks good.
await this.loadClient(); | ||
const td = new TextDecoder(); | ||
const embeddings: number[][] = []; | ||
return Promise.all(texts.map(async text => { |
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.
@chezou, just confirming my understanding here is that if the user provides several texts, the EF will embed each one individually, and if it hits a problem, then an error is raised. The existing embeddings are, in a way, lost, correct?
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.
Yes, that's the correct understanding. If we need to preserve partial embeddings, I have to modify to use Promise.allSettled
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.
@tazarov Let me know if I need to do something additional.
@@ -83,6 +86,34 @@ if (!process.env.COHERE_API_KEY) { | |||
}); | |||
} | |||
|
|||
if (!process.env.AWS_ACCESS_KEY_ID || !process.env.AWS_SECRET_ACCESS_KEY) { |
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.
Do you mind moving this to a separate file under test/embeddings/aws.bedrock.test.ts
? This file is getting very crowded.
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.
Splitted 5a6b8ec
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Our underlying impl has changed and so this PR is not landable as is. That being said - we'd still like to add this functionality and that is now tracked in this issue. |
Description of changes
Summarize the changes made by this PR.
Test plan
pytest
for python,yarn test
for jsAlso, manually tested with node:
Documentation Changes
Will update chroma-core/docs later.