feat(api): unify Bedrock provider using Runtime API #60
Conversation
Premshay
requested review from
ColemanRoo,
mrubens and
stea9499
as code owners
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 8280a80c93c12dd9776b8de5a8f4a40c43a05061 in 3 minutes and 7 seconds
More details
- Looked at
593lines of code in4files - Skipped
1files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. package.json:174
- Draft comment:
Remove the@anthropic-ai/bedrock-sdkdependency as it is no longer used. - Reason this comment was not posted:
Comment did not seem useful.
2. src/api/providers/bedrock.ts:69
- Draft comment:
RenameawsusePromptCachetoawsUsePromptCachefor consistency with camelCase convention. - Reason this comment was not posted:
Marked as duplicate.
3. src/api/providers/bedrock.ts:128
- Draft comment:
Avoid using 'any' type. Use 'unknown' instead to enforce type-checking before usage. This is from our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d5818051ae9cefd92c3aac2b - Reason this comment was not posted:
Marked as duplicate.
4. src/api/transform/bedrock-converse-format.ts:154
- Draft comment:
Avoid using 'any' type. Use 'unknown' instead to enforce type-checking before usage. This is from our Development Standards: https://www.notion.so/Development-Standards-59febcf8ead647fd9c2ec3f60c22f3df?pvs=4#11869ad2d5818051ae9cefd92c3aac2b - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_RW7qdTRZ8rfq7aqx
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
@Premshay Would you add some unit tests for your changes please? Specifically for changes in: |
Added unit tests. bedrock-converse-format tests all pass. bedrock.ts most tests fail, but real-life testing shows everything works - messages, cline tools are working, images are analyzed properly. All real-life tests pass, for all claude and nova models. So I think that the tests aren't so good and should be refactored. it would also be nice to a video analysis support (Nova support video, and the code already includes the basic use, now we need to get Cline to allow uploading videos like images). And after Amazon releases from preview the prompt caching an intelligent prompt routing features, we can add those quickly and allow cost savings for all. |
Sorry for the delay, just saw that the tests weren't committed. You will see them now, @mrubens, @ColemanRoo |
Hey @Premshay thank you for adding the tests! Can you please make sure to merge latest |
Done. Also added Meta 3, 3.1, 3.2 models available on bedrock api. |
package.json
Outdated
There was a problem hiding this comment.
For this PR to be able to merge the update published, you will need to update the version number in package.json
2.1.18 -> 2.1.19
Updated the tests and all are passing now. |
package-lock.json
Outdated
There was a problem hiding this comment.
Sorry I missed this @Premshay can you rebuild your package-lock.json too so it has the correct version number?
…icing and context windows
Problem: The current Bedrock implementation uses the Bedrock SDK, which requires separate handling for different model types and doesn't provide a unified streaming interface. Solution: Integrate the Bedrock Runtime API to provide a single, unified interface for all Bedrock models (Claude and Nova) using the ConverseStream API. This eliminates the need for separate handlers while maintaining all existing functionality. Key Changes: - Refactored AwsBedrockHandler to use @aws-sdk/client-bedrock-runtime - Enhanced bedrock-converse-format.ts to handle all content types and properly transform between Anthropic and Bedrock formats - Maintained cross-region inference support with proper region prefixing - Added support for prompt caching configuration - Improved AWS credentials handling to better support default providers - Added proper error handling and token tracking for all response types Dependencies: - Added @aws-sdk/client-bedrock-runtime for unified API access - Removed @anthropic-ai/bedrock-sdk dependency Testing: - Verified message format conversion for all content types - Tested cross-region inference functionality - Validated streaming responses for both Claude and Nova models This change simplifies the codebase by providing a single, consistent interface for all Bedrock models while maintaining full compatibility with existing features.
- Fix TypeScript error in ConverseStreamCommand payload - Add proper JSON parsing for test stream events - Improve error handling with proper Error objects - Add test-specific model info with required fields - Fix cross-region inference and prompt cache config
- Add tests for AWS Bedrock handler (stream handling, config, errors) - Add tests for message format conversion (text, images, tools) - Add tests for stream event parsing and transformation - Add tests for cross-region inference and prompt cache - Add tests for metadata and message lifecycle events
|
Had to redo the commits because of compounding unresolvable conflicts. It should now be ok. |
|
|
why is this PR not merged yet? |
Hi @Premshay Sorry for the delay; I am finally getting around to testing your PR and I noticed that when I ran through a simple task of having Roo-Cline write tests for me, the tokens and API costs in the screenshot Can you please take a look at why this might be happening? As of right now, I do not feel confident in merging this PR. |
What you are describing sounds like every time I have used openrouter or Bedrock before. In other words: This seems to be a Cline issue in general. That's the way Cline works on calculating the usage numbers it gets. |
In regards to the numbers, for the task I had your PR version do, it wrote ~400 lines of brand new code; it iterated over the code multiple times and ran tests and coverage reports each time. It was quick, but each time it accomplished a task, the |
|
Thanks for the added information. |
The key I use is an openRouter API Key, so if that routes differently in your PR, then that could be an issue as well. But my assumption is that bedrock is just the grouping of all the different models? But either way, using the same key I should get the same experience between existing marketplace version and your PR version. |
|
I'm sorry, I must be missing something here.
My PR is only for using the Bedrock service. It can only be used providing
aws credentials, and the only models that will work would be those that the
person with those credentials have access too. Otherwise you get specific
errors stating lack of credentials or non access to model. The Bedrock
provider can't be checked with anything but AWS credentials.
Could you further explain?
…On Mon, Dec 16, 2024, 22:41 Mike C ***@***.***> wrote:
Thanks for the added information. One point to be sure: you wrote "to
openrouter". You meant "to bedrock", right?
The key I use is an openRouter API Key, so if that routes differently in
your PR, then that could be an issue as well. But my assumption is that
bedrock is just the grouping of all the different models?
But either way, using the same key I should get the same experience
between existing marketplace version and your PR version.
—
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGWMILCZDELXJXE3PNLSSUT2F43G7AVCNFSM6AAAAABTLSYLP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWGY4TONRVG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
hmmm, ok Could you go into details exactly how one would go about using the bedrock service? We need to be able to test these changes... However, I am still concerned that my current experience differs between your PR and the currently available version when it comes to tracking tokens and cost as the task progresses. |
|
I can't really explain why you're seeing differences. Where do you see the
difference? I mean, which provider are you choosing? Openrouter?
Personally I came across that same behavior using openrouter in Cline (the
original marketplace Cline), for the past 2 months.
As for why you see difference between versions, I can't say, my PR only
Handles Bedrock, i.e. the changes to Bedrock.ts script in providers, adding
bedrock converse transform file, and adding some Bedrock data to the shared api.ts file. You can see in the files changed that no other provider
or src file have been touched.
As for how to test: you need to choose bedrock provider, input credentials,
and choose one of the models from the list (currently includes amazon nova,
claude and Meta Llama models).
…On Mon, Dec 16, 2024, 23:01 Mike C ***@***.***> wrote:
I'm sorry, I must be missing something here. My PR is only for using the
Bedrock service. It can only be used providing aws credentials, and the
only models that will work would be those that the person with those
credentials have access too. Otherwise you get specific errors stating lack
of credentials or non access to model. The Bedrock provider can't be
checked with anything but AWS credentials. Could you further explain?
… <#m_6693138867674315585_>
On Mon, Dec 16, 2024, 22:41 Mike C *@*.*> wrote: Thanks for the added
information. One point to be sure: you wrote "to openrouter". You meant "to
bedrock", right? The key I use is an openRouter API Key, so if that routes
differently in your PR, then that could be an issue as well. But my
assumption is that bedrock is just the grouping of all the different
models? But either way, using the same key I should get the same experience
between existing marketplace version and your PR version. — Reply to this
email directly, view it on GitHub <#60 (comment)
<#60 (comment)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AGWMILCZDELXJXE3PNLSSUT2F43G7AVCNFSM6AAAAABTLSYLP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWGY4TONRVG4
<https://github.com/notifications/unsubscribe-auth/AGWMILCZDELXJXE3PNLSSUT2F43G7AVCNFSM6AAAAABTLSYLP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWGY4TONRVG4>
. You are receiving this because you were mentioned.Message ID: @.*>
hmmm, ok
Could you go into details exactly how one would go about using the bedrock
service? We need to be able to test these changes...
However, I am still concerned that my current experience differs between
your PR and the currently available version when it comes to tracking
tokens and cost as the task progresses.
—
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGWMILE67QOWWA4WBLWJSOL2F45RNAVCNFSM6AAAAABTLSYLP6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBWG42TKMZRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| cacheWritesPrice: 3.75, // per million tokens | ||
| cacheReadsPrice: 0.3, // per million tokens |
There was a problem hiding this comment.
@Premshay out of curiosity where do you look up these values from?
There was a problem hiding this comment.
There was a problem hiding this comment.
BTW, I've prepared infrastructure but haven't fully connected the Bedrock prompt caching feature. Reason is that it's still in preview for limited number of users. When this is fully released it'll be easy to apply. I also hope to integrate the new intelligent prompt routing when it is fully released and tested.
3 participants
Problem:
The current Bedrock implementation uses the Bedrock SDK, which requires separate handling for different model types and doesn't provide a unified streaming interface.
Solution:
Integrate the Bedrock Runtime API to provide a single, unified interface for all Bedrock models (Claude and Nova) using the ConverseStream API. This eliminates the need for separate handlers while maintaining all existing functionality.
Key Changes:
Dependencies:
Testing:
This change simplifies the codebase by providing a single, consistent interface for all Bedrock models while maintaining full compatibility with existing features.
Important
Refactor Bedrock provider to use Bedrock Runtime API for unified model handling, improved error handling, and enhanced AWS credentials management.
AwsBedrockHandlerinbedrock.tsto use@aws-sdk/client-bedrock-runtimefor unified model handling.convertToBedrockConverseMessagesandconvertToAnthropicMessageinbedrock-converse-format.tsfor message format conversion.bedrockModelsinapi.tsto include new model configurations and pricing.@aws-sdk/client-bedrock-runtimeand removed@anthropic-ai/bedrock-sdkinpackage.json.This description was created by
for 8280a80c93c12dd9776b8de5a8f4a40c43a05061. It will automatically update as commits are pushed.