-
Notifications
You must be signed in to change notification settings - Fork 138
feat: add support for anthropic to AWS bedrock for Anthropic translation #1418
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
feat: add support for anthropic to AWS bedrock for Anthropic translation #1418
Conversation
Signed-off-by: secustor <sebastian@poxhofer.at>
45c6171 to
82f752f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1418 +/- ##
==========================================
- Coverage 83.67% 83.67% -0.01%
==========================================
Files 138 139 +1
Lines 12268 12297 +29
==========================================
+ Hits 10265 10289 +24
- Misses 1395 1400 +5
Partials 608 608 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
site/docs/getting-started/connect-providers/aws-bedrock-anthropic.md
Outdated
Show resolved
Hide resolved
site/docs/getting-started/connect-providers/aws-bedrock-anthropic.md
Outdated
Show resolved
Hide resolved
site/docs/getting-started/connect-providers/aws-bedrock-anthropic.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Sebastian Poxhofer <secustor@users.noreply.github.com>
site/docs/getting-started/connect-providers/aws-bedrock-anthropic.md
Outdated
Show resolved
Hide resolved
Signed-off-by: secustor <sebastian@poxhofer.at>
Signed-off-by: secustor <sebastian@poxhofer.at>
Signed-off-by: secustor <sebastian@poxhofer.at>
# Conflicts: # site/docs/capabilities/llm-integrations/supported-endpoints.md # site/docs/getting-started/connect-providers/aws-bedrock.md
seluard
left a comment
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.
Latest refactor changes tested already at our setup 🧪 ✅
# Conflicts: # manifests/charts/ai-gateway-helm/crds/aigateway.envoyproxy.io_aiservicebackends.yaml
mathetake
left a comment
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.
Thanks, this is exciting and thank you for testing locally. can you add tests in here after you resolve my comments?
ai-gateway/tests/extproc/testupstream_test.go
Lines 821 to 887 in 1f8ca94
| { | |
| name: "gcp-anthropicai - /anthropic/v1/messages", | |
| backend: "gcp-anthropicai", | |
| path: "/anthropic/v1/messages", | |
| method: http.MethodPost, | |
| requestBody: `{"model":"claude-3-sonnet","max_tokens":100,"messages":[{"role":"user","content":[{"type":"text","text":"Hello, just a simple test message."}]}],"stream":false}`, | |
| expRequestBody: `{"anthropic_version":"vertex-2023-10-16","max_tokens":100,"messages":[{"content":[{"text":"Hello, just a simple test message.","type":"text"}],"role":"user"}],"stream":false}`, | |
| expHost: "gcp-region-aiplatform.googleapis.com", | |
| expPath: "/v1/projects/gcp-project-name/locations/gcp-region/publishers/anthropic/models/claude-3-sonnet:rawPredict", | |
| expRequestHeaders: map[string]string{"Authorization": "Bearer " + fakeGCPAuthToken}, | |
| responseStatus: strconv.Itoa(http.StatusOK), | |
| responseBody: `{"id":"msg_123","type":"message","role":"assistant","stop_reason": "end_turn", "content":[{"type":"text","text":"Hello from native Anthropic API!"}],"usage":{"input_tokens":8,"output_tokens":15}}`, | |
| expStatus: http.StatusOK, | |
| expResponseBody: `{"id":"msg_123","type":"message","role":"assistant","stop_reason": "end_turn", "content":[{"type":"text","text":"Hello from native Anthropic API!"}],"usage":{"input_tokens":8,"output_tokens":15}}`, | |
| }, | |
| { | |
| name: "gcp-anthropicai - /anthropic/v1/messages - streaming", | |
| backend: "gcp-anthropicai", | |
| path: "/anthropic/v1/messages", | |
| method: http.MethodPost, | |
| responseType: "sse", | |
| requestBody: `{"model":"claude-3-sonnet","max_tokens":100,"messages":[{"role":"user","content":[{"type":"text","text":"Tell me a short joke"}]}],"stream":true}`, | |
| expRequestBody: `{"anthropic_version":"vertex-2023-10-16","max_tokens":100,"messages":[{"content":[{"text":"Tell me a short joke","type":"text"}],"role":"user"}],"stream":true}`, | |
| expHost: "gcp-region-aiplatform.googleapis.com", | |
| expPath: "/v1/projects/gcp-project-name/locations/gcp-region/publishers/anthropic/models/claude-3-sonnet:streamRawPredict", | |
| expRequestHeaders: map[string]string{"Authorization": "Bearer " + fakeGCPAuthToken}, | |
| responseStatus: strconv.Itoa(http.StatusOK), | |
| responseBody: `event: message_start | |
| data: {"type": "message_start", "message": {"id": "msg_789", "usage": {"input_tokens": 8}}} | |
| event: content_block_start | |
| data: {"type": "content_block_start", "index": 0, "content_block": {"type": "text", "text": ""}} | |
| event: content_block_delta | |
| data: {"type": "content_block_delta", "index": 0, "delta": {"type": "text_delta", "text": "Why don't scientists trust atoms? Because they make up everything!"}} | |
| event: content_block_stop | |
| data: {"type": "content_block_stop", "index": 0} | |
| event: message_delta | |
| data: {"type": "message_delta", "delta": {"stop_reason": "end_turn"}, "usage": {"output_tokens": 15}} | |
| event: message_stop | |
| data: {"type": "message_stop"} | |
| `, | |
| expStatus: http.StatusOK, | |
| expResponseBody: `event: message_start | |
| data: {"type": "message_start", "message": {"id": "msg_789", "usage": {"input_tokens": 8}}} | |
| event: content_block_start | |
| data: {"type": "content_block_start", "index": 0, "content_block": {"type": "text", "text": ""}} | |
| event: content_block_delta | |
| data: {"type": "content_block_delta", "index": 0, "delta": {"type": "text_delta", "text": "Why don't scientists trust atoms? Because they make up everything!"}} | |
| event: content_block_stop | |
| data: {"type": "content_block_stop", "index": 0} | |
| event: message_delta | |
| data: {"type": "message_delta", "delta": {"stop_reason": "end_turn"}, "usage": {"output_tokens": 15}} | |
| event: message_stop | |
| data: {"type": "message_stop"} | |
| `, | |
| }, |
6cdd4eb to
7d6cd47
Compare
secustor
left a comment
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.
yet not validated e2e, but should work based on the tests
| require.Equal(t, "bearer token123", headers["authorization"]) | ||
| }) | ||
|
|
||
| t.Run("multiple header mutations with same key - last one wins", func(t *testing.T) { |
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.
My implementation relies on the ( current ) behaviour that when setting the same multiple headers with the same keys the last one wins.
I have added this test to ensure that behaviour in the future isn't broken by accident
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.
sorry can you remove this? it's irrelevant here in this PR. As I noted we do not want to mess up the commit trees with a conflated change. You wouldn't want to end up finding completely unrelated commit in a file that you are debugging
e2e failed in our side with the latest changes. The duplicated |
| } | ||
|
|
||
| // update content length after changing the body | ||
| setContentLength(headerMutation, preparedBody) |
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.
Signed-off-by: secustor <sebastian@poxhofer.at>
Signed-off-by: secustor <sebastian@poxhofer.at>
Signed-off-by: secustor <sebastian@poxhofer.at>
|
@mathetake Added the requested changes, but we couldn't test e2e so far because of unrelated constraints. Looking forward to v0.4.0 then. |
Signed-off-by: secustor <sebastian@poxhofer.at>
mathetake
left a comment
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.
**Description** The extproc test's envoy config was missing TLS config for AWS Bedrock sicne #1418. Since the test is using credentials, hence only running on the main branch, this was not caught before merging it. **Related Issues/PRs (if applicable)** Follow up on #1418 Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
…ion (envoyproxy#1418) **Description** This PR implement adds a translator from the Anthropic API to AWS Bedrock for Anthropic. **Related Issues/PRs (if applicable)** Closes envoyproxy#1371 --------- Signed-off-by: secustor <sebastian@poxhofer.at> Signed-off-by: yxia216 <yxia216@bloomberg.net>
**Description** The extproc test's envoy config was missing TLS config for AWS Bedrock sicne envoyproxy#1418. Since the test is using credentials, hence only running on the main branch, this was not caught before merging it. **Related Issues/PRs (if applicable)** Follow up on envoyproxy#1418 Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com> Signed-off-by: yxia216 <yxia216@bloomberg.net>
Description
This PR implement adds a translator from the Anthropic API to AWS Bedrock for Anthropic.
Related Issues/PRs (if applicable)
Closes #1371
Special notes for reviewers (if applicable)
Has been tested on our side together with #1394 successfully E2E using Claude Code
Using following setup
The implementation is more or less a copy of the Anthropic to GCP implementation, so it would make sense to include it in the planned refactor with the Anthropic to Anthropic translator