Skip to content
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

Connector for Amazon Bedrock #91

Merged

Conversation

athewsey
Copy link
Contributor

@athewsey athewsey commented Aug 30, 2024

Per issue #87

Description

Added a connector class and initial set of endpoint configuration JSONs for connecting to foundation models on Amazon Bedrock.

Motivation and Context

Bedrock is AWS' pay-as-you-go service for consuming foundation models from a range of popular providers: Including AI21, Amazon themselves, Anthropic, Cohere, Meta, Mistral, and Stability AI. This PR should enable AWS customers to use Moonshot to test models they're consuming through Bedrock.

Type of Change

feat: A new feature

How to Test

[Provide clear instructions on how to test and verify the changes introduced by this pull request, including any specific unit tests you have created to demonstrate your changes.]

Checklist

Please check all the boxes that apply to this pull request using "x":

  • I have tested the changes locally and verified that they work as expected.
  • I have added or updated the necessary documentation (README, API docs, etc.).
    • Updated connectors list in README (also added missing entry for Azure), and some links to clarify this table
    • Detailed authentication & setup in docstrings within the connector py file itself for now
    • ⚠️ But I'm not sure if there's anywhere else e.g. in the main moonshot repo where docs should also be added?
  • I have added appropriate unit tests or functional tests for the changes made.
    • ❓ I couldn't see any obvious place connector unit tests are stored in this repository... Is there something somewhere else that should be updated?
  • I have followed the project's coding conventions and style guidelines.
  • I have rebased my branch onto the latest commit of the main branch.
  • I have squashed or reorganized my commits into logical units.
  • I have added any necessary dependencies or packages to the project's build configuration.
    • ❓ The AWS SDK for Python, boto3 was already implicitly installed by requirements.txt but is now also an explicit dependency in pyproject.toml. Is anything else needed on e.g. the CI side?
  • I have performed a self-review of my own code.
  • I have read, understood and agree to the Developer Certificate of Origin below, which this project utilises.

Screenshots (if applicable)

N/A - no visual changes

Additional Notes

AWS API Authentication & Endpoints

Programmatic authentication to AWS APIs is a bit different from the basic API key pattern that Moonshot generally expects, and some specific suggested best-practices include avoiding storing API keys in code or files where possible.

Typical recommended practice (as documented in the connector file docstring) would be to configure the credentials via whatever environment you run Moonshot on: For example as per the CLI for running locally, or just by setting permissions on your execution role if deploying Moonshot to an AWS EC2 instance or ECS/EKS container, etc.

Since the API token field currently seems to be mandatory in Moonshot, this connector explicitly ignores short, placeholder-looking tokens and users are encouraged not to use the token configuration. However, it will try to pass longer-looking tokens through as an AWS_SESSION_TOKEN if provided.

Likewise, AWS users aren't typically expected to look up the exact endpoint URLs of the various services they use - because the SDK takes care of that in most cases.

As follow-up to this PR, I would actually suggest we start a discussion on the main Moonshot repo for:

  1. Making API token and endpoint URL non-mandatory fields for endpoint configuration
  2. Ideally, allowing different connector types to specify their configuration model through to the UI?

Wrapping synchronous SDK with asyncio.to_thread

AWS' official SDK for Python, boto3, is synchronous by design. There is a pretty popular 3rd-party alternative, aioboto3 - but today that explicitly requires service clients to be created via async with context managers, which doesn't sit well with Moonshot's class-based connector initialisation design. Re-building the client on every get_response would likely be pretty inefficient; hacking around with __aenter__() would risk leaking resources; and sticking with plain boto3 allowed us to not introduce any new dependencies (since boto3 was already present).

As a result, this PR sticks to classic boto3 and just wraps model invocations with asyncio.to_thread to try and maintain appropriate concurrency.

Endpoint naming

I did wonder whether to switch the naming of the example connector configurations around to e.g. "Llama 3.1 8B Instruct on Amazon Bedrock", but figured that users would probably rather see e.g. all the AWS/Bedrock-hosted models together rather than all the different hosting options for Llama together.

I notice that the current Moonshot UI doesn't scale super nicely for a large number of configured endpoints... Maybe it'd be nice to add a search box on the endpoint selection screen in future?

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
   have the right to submit it under the open source license
   indicated in the file; or

(b) The contribution is based upon previous work that, to the best
   of my knowledge, is covered under an appropriate open source
   license and I have the right under that license to submit that
   work with modifications, whether created in whole or in part
   by me, under the same open source license (unless I am
   permitted to submit under a different license), as indicated
   in the file; or

(c) The contribution was provided directly to me by some other
   person who certified (a), (b) or (c) and I have not modified
   it.

(d) I understand and agree that this project and the contribution
   are public and that a record of the contribution (including all
   personal information I submit with it, including my sign-off) is
   maintained indefinitely and may be redistributed consistent with
   this project or the open source license(s) involved.

@athewsey
Copy link
Contributor Author

Just re-based against latest, and everything still seems to work fine - any chance of a review on this? Thanks!

@imda-normanchia
Copy link
Collaborator

i have tested all the connector endpoints by running 1 benchmark test and 1 red team session and they are all working.

imda-lionelteo
imda-lionelteo previously approved these changes Sep 18, 2024
@imda-lionelteo
Copy link
Collaborator

@imda-normanchia lgtm

@imda-normanchia imda-normanchia dismissed stale reviews from imda-lionelteo and themself via 9f4d532 September 19, 2024 01:44
@imda-jacksonboey imda-jacksonboey merged commit a44c494 into aiverify-foundation:main Sep 23, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants