-
-
Notifications
You must be signed in to change notification settings - Fork 111
16: Add support for Claude through Bedrock #50
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
- Add Bedrock provider with AWS Signature V4 authentication using Faraday - Implement chat functionality for Claude and Titan models - Add comprehensive test coverage - Support both streaming and non-streaming responses
- We only need Claude right now
Nice, thanks for putting in the work for this -- I can't wait to try this out at work tomorrow. Some thoughts/comments: I understand that avoiding dependencies was an intentional choice, but do you think that using the Also since library looks regularly maintained by AWS, it might sense to take the dependency here for security reasons. How do you feel about making the |
I'm certainly open to that. Probably supports more auth methods. Makes sense to me. Interested in what @crmne thinks.
This has already been expanded with the addition of event stream parser. |
I just implemented model aliases and provider selection at the global level. Now you should just be able to update the Here's some info:
|
Thanks for the PR and the discussion about AWS dependencies. After thinking about this, I'm leaning toward maintaining consistency with how we handle other providers in RubyLLM:
The question becomes: Is Bedrock fundamentally different from our other providers? All LLM providers could change their APIs at any time, and we handle those changes for OpenAI, Anthropic, etc. without taking dependencies on their SDKs. While AWS's auth mechanism (SigV4) is more complex than simple API keys, that's a one-time implementation cost. seuros on our Discord raised a good point about the challenges other gems have faced when they started supporting numerous provider SDKs - it often leads to maintainer burnout and creates dependency on provider release cycles. True integration with the AWS SDK would mean using their client directly instead of Faraday, which would be a significant architectural departure from how we handle other providers. I'm not making a final decision yet - I'd like to hear more perspectives. But I'm inclined to keep our implementation consistent across providers, which would mean sticking with your current Faraday-based approach. What do others think? Is there a compelling reason to treat Bedrock differently? |
Good question and I am not sure 😭 - but here are some thoughts: The AWS SDK handles credential complexity like region failover, credential rotation, and session management [1] so maintaining this complexity with a Faraday implementation might be more work than necessary? What if allowed optionally passing the client as a configuration option and supported its use if defined? RubyLLM.configure do |config|
config.bedrock_client = Aws::BedrockRuntime::Client.new(
region: 'us-west-2',
)
end As of right now, with this PR, you could do something like below ... client = Aws::BedrockRuntime::Client.new
RubyLLM.configure do |config|
config.bedrock_region = client.config.region
config.bedrock_access_key_id= client.config.access_key_id
config.bedrock_secret_access_key = client.config.secret_access_key
end but as seuros on our Discord mentioned this would not handle secrets being rotated at runtime. 1: https://docs.aws.amazon.com/sdk-for-ruby/v3/developer-guide/setup-config.html |
Where can I get an invite to the discord? |
@tpaulshippy - here's a linkI found posted on https://rubyweekly.com/issues/646 Discord link: https://discord.gg/k4Uc224xVD After that join the ruby_llm channel |
However, I'd like to highlight a few points regarding the configuration differences and potential implications: Configuration Differences:
AWS-Specific Complexities:
Credential Rotation:
|
I think I agree with @seuros that (at the very least) secret rotation should be supported here before release. But I'm torn because I want this out and I'm not sure if I will have time to implement that in the next few days. My first focus here today will be following @crmne's guidance on model IDs. |
On second thought, wouldn't this only be an issue in scenarios where the client instance is held in memory for longer than the session length? I suspect most users of this will have the AWS sdk in their app anyway and will use it to get the credentials to set up this call, right? That's certainly how I tested it. |
- Just supporting sigv4 for now
@crmne Do you want me to continue trying to add coverage for sad paths? Not sure how far you want me to go. |
Coverage is less important than meaningful tests. Don't worry about that! There are a few failing things in the CI, that's definitely more important. I'll then review the code tomorrow! Thank you so much for your effort! |
Ok I got overcommit back in place and removed the extra spec that violated rubocop a bunch. CI should pass now. |
- Needed for correct hash in Bedrock signing process
There seems to be some conflicts |
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.
Minor changes! Great great work so far. Love the spirit of clean code you've used. Looking forward to releasing that to the public.
Have you been using this code already?
We are in the testing phase of rolling this out, yes. Dealing with some tricky stuff around tools. I'll probably open an issue to ask about nested parameters. |
Looks like #76 covers the kind of issues I'm experiencing. |
Ah that's great then. We don't support nested parameters. I'll change the docs. Merging! |
One thing I just noticed: is the def configuration_requirements
%i[bedrock_api_key bedrock_secret_key bedrock_region]
end |
@tpaulshippy can you come in discord one sec? |
My understanding is that long term credentials can be used directly without a session token but it is not generally recommended for security reasons. |
Resolves #16 for the Claude models.
Purpose
Support Claude through Bedrock.
Approach
Follow pattern of adding providers. Avoid additional dependencies by bringing in the signing and decoding pieces from aws-sdk-ruby directly.
Testing
Ran with test script locally that had the following environment variables configured:
Used
anthropic.claude-3-5-sonnet-20240620-v1:0
model.Screenshot