[ENH] AmazonBedrockEmbeddingFunction for JS

[chezou]

Description of changes

Summarize the changes made by this PR.

• New functionality
  • Add AmazonBedrockEmbeddingFunction for JS. Follow up Add Amazon Bedrock Embedding function #1361

Test plan

• Tests pass locally with pytest for python, yarn test for js

Also, manually tested with node:

$ npx node
Welcome to Node.js v18.17.1.
Type ".help" for more information.
> const { fromSSO } = require('@aws-sdk/credential-providers');
> const { AmazonBedrockEmbeddingFunction} = require('chromadb')
> var c = await fromSSO({profile: "my-profile"})
> var ef = new AmazonBedrockEmbeddingFunction({config: {credentials: c, region: "us-east-1"}})
> var emb = await ef.generate(["foo"])
> emb
[
  [
     -0.033203125,  -0.15722656,  -0.21386719,   -0.080078125,
      0.061035156,  -0.26171875,      0.78125, -0.00090408325,
      0.119140625, -0.060058594,   0.96484375,        0.65625,
        0.3203125,  -0.24707031,  -0.49414062,     0.66796875,
             -0.5,   -1.0546875,   -0.7578125,        0.53125,
      -0.25585938,    0.5234375,   0.16601562,      1.2421875,
       0.34765625,   0.69140625,   0.32226562,     0.14550781,
         0.890625,  -0.46679688,   0.12011719,      -0.859375,
     -0.087890625,    -0.546875,   -0.1953125,     -1.3671875,
       0.34570312,   -1.3046875,  -0.13476562,    -0.47070312,
       0.43359375,  -0.47851562,    1.2578125,     0.44140625,
      -0.36914062,  -0.69921875,   0.16308594,     0.65234375,
      -0.40820312,  0.111328125, -0.026855469,    0.039794922,
       -0.8515625,  -0.08251953,   0.13574219,      0.7578125,
       0.21679688,   0.25585938,   -0.4296875,    -0.45898438,
               -1,  -0.51171875,   0.31054688,    -0.74609375,
      0.030151367,  -0.46484375,  -0.62109375,     0.58203125,
       -1.1015625,     0.515625, -0.111328125,   -0.076171875,
         0.796875,  -0.64453125,     0.921875,     0.27148438,
          0.71875,    0.1953125,   0.17578125,     0.90234375,
      -0.37695312,   0.33203125,  0.018066406,   -0.009094238,
       0.12890625,   0.34179688,    0.6796875,    -0.19335938,
    0.00012207031,   0.75390625,       -0.125,     0.94921875,
        1.2734375,   0.26757812,  -0.25976562,     0.83203125,
      -0.57421875,  -0.87109375, -0.020629883,        0.46875,
    ... 1436 more items
  ]
]

Documentation Changes

Will update chroma-core/docs later.

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[tazarov]

@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 -

chroma/clients/js/test/add.collections.test.ts

Line 40 in b5dc65f

if (!process.env.OPENAI_API_KEY) {

. The test should only run if AWS credentials necessary for running the EF are present as env vars.

[chezou]

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.

[chezou]

Added a test for the function 6ef9154

[chezou]

@tazarov Can you allow to run tests?

[jeffchuber]

@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

[chezou]

@jeffchuber Thanks! Created an empty commit b7425e8, but it looks approval for run is required

[chezou]

Fixed the PR title since "Check PR Title" was failed.

[jeffchuber]

tests will now re-run with the merge conflicts handled. sorry about the PR title issue

[chezou]

@tazarov @jeffchuber If there's no issue, could you approve and merge this PR?

[tazarov]

@chezou, do you mind resolving the conflict?

[chezou]

@tazarov Resolved

[tazarov]

One clarification and a nit about the test. Other than this it looks good.

[tazarov]

@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?

[chezou]

Yes, that's the correct understanding. If we need to preserve partial embeddings, I have to modify to use Promise.allSettled instead.

[chezou]

@tazarov Let me know if I need to do something additional.

[tazarov]

Do you mind moving this to a separate file under test/embeddings/aws.bedrock.test.ts? This file is getting very crowded.

[chezou]

Splitted 5a6b8ec

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
chroma	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 3, 2024 5:05am

[jeffchuber]

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.