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

fix(bedrock): correct URL encoding for model params #759

Conversation

moritalous
Copy link
Contributor

This pull request fixes an issue #740 by using Bedrock's Application Profile.

@moritalous moritalous requested a review from a team as a code owner November 22, 2024 10:46
@RobertCraigie
Copy link
Collaborator

Ah thanks for tracking this down and opening a PR! Would you be able to add a unit test for this case to tests/lib/test_bedrock.py?

@moritalous
Copy link
Contributor Author

The ARN of the Application Profile contains a unique inference_profile_id that is automatically assigned, but how should I write it?

I have the following ARN in my environment, but it will be a different ID in other AWS environments.

arn:aws:bedrock:us-east-1:637423213562:application-inference-profile/jf2sje1c0jnb

@RobertCraigie
Copy link
Collaborator

ah the test shouldn't hit the live API, all of our tests hit mocks.

here's an example from the test_bedrock.py file

@pytest.mark.respx()
def test_messages_retries(respx_mock: MockRouter) -> None:
    respx_mock.post(re.compile(r"https://bedrock-runtime\.us-east-1\.amazonaws\.com/model/.*/invoke")).mock(
        side_effect=[
            httpx.Response(500, json={"error": "server error"}, headers={"retry-after-ms": "10"}),
            httpx.Response(200, json={"foo": "bar"}),
        ]
    )
    sync_client.messages.create(
        max_tokens=1024,
        messages=[
            {
                "role": "user",
                "content": "Say hello there!",
            }
        ],
        model="anthropic.claude-3-sonnet-20240229-v1:0",
    )

    calls = cast("list[MockRequestCall]", respx_mock.calls)

    assert len(calls) == 2

    assert (
        calls[0].request.url
        == "https://bedrock-runtime.us-east-1.amazonaws.com/model/anthropic.claude-3-sonnet-20240229-v1:0/invoke"
    )
    assert (
        calls[1].request.url
        == "https://bedrock-runtime.us-east-1.amazonaws.com/model/anthropic.claude-3-sonnet-20240229-v1:0/invoke"
    )

@moritalous
Copy link
Contributor Author

Thanks, I've added the test code.

Copy link

@JCuomo JCuomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested and working!

@RobertCraigie RobertCraigie changed the base branch from main to next November 27, 2024 10:27
Copy link
Collaborator

@RobertCraigie RobertCraigie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@RobertCraigie RobertCraigie changed the title Fix #740 Added URL encoding for model names that contain "/" when using application inference profile. fix(bedrock): correct URL encoding for model params Nov 27, 2024
@RobertCraigie RobertCraigie merged commit be4e73a into anthropics:next Nov 27, 2024
2 checks passed
@stainless-app stainless-app bot mentioned this pull request Nov 27, 2024
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.

3 participants