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

[OAI]: Fix Anthropic Bedrock Client Bug #3200

Closed
wants to merge 5 commits into from

Conversation

Zizo-Vi
Copy link
Contributor

@Zizo-Vi Zizo-Vi commented Jul 24, 2024

Fix #3198

Checks

@Hk669
Copy link
Contributor

Hk669 commented Jul 24, 2024

thanks @wenngong , was about to make a PR, made changes locally.
can you fix the tests that are failing. for the code formatting use: pre-commit

@Zizo-Vi
Copy link
Contributor Author

Zizo-Vi commented Jul 24, 2024

thanks @wenngong , was about to make a PR, made changes locally.
can you fix the tests that are failing. for the code formatting use: pre-commit

Ok, I will fix it.

@Zizo-Vi
Copy link
Contributor Author

Zizo-Vi commented Jul 24, 2024

@microsoft-github-policy-service agree

setup.py Outdated Show resolved Hide resolved
@Zizo-Vi
Copy link
Contributor Author

Zizo-Vi commented Jul 24, 2024

thanks @wenngong , was about to make a PR, made changes locally.
can you fix the tests that are failing. for the code formatting use: pre-commit

Ok, I will fix it.

@Hk669 I have fixed the test failure. and pre-commit passed. Please review.

image image

@Hk669
Copy link
Contributor

Hk669 commented Jul 24, 2024

Ok, I will fix it.

@Hk669 I have fixed the test failure. and pre-commit passed. Please review.

image image

but the anthropic, shouldnt be in the required dependencies, can you please check on that.
cc @marklysze

@Zizo-Vi Zizo-Vi requested a review from Hk669 July 25, 2024 01:18
@Zizo-Vi
Copy link
Contributor Author

Zizo-Vi commented Jul 25, 2024

OAI_CONFIG_LIST config file example:

[
    {
        "model": "anthropic.claude-3-5-sonnet-20240620-v1:0",
        "aws_access_key":<accessKey>,
        "aws_secret_key":<secretKey>,
        "aws_region":"us-east-1",
        "api_type": "anthropic"
    }
]

@Hk669 Hk669 requested a review from yiranwu0 July 25, 2024 13:21
@Hk669
Copy link
Contributor

Hk669 commented Jul 25, 2024

@wenngong this seems to be a good PR, can you please check this #3210

@Zizo-Vi
Copy link
Contributor Author

Zizo-Vi commented Jul 25, 2024

@wenngong this seems to be a good PR, can you please check this #3210

@Hk669 I believe my PR is better. In my pr, the bedrock config just depends on AnthropicClient, just like what openai_kwargs did.

@Zizo-Vi
Copy link
Contributor Author

Zizo-Vi commented Jul 25, 2024

@wenngong this seems to be a good PR, can you please check this #3210

@Hk669 I believe my PR is better. In my pr, the bedrock config just depends on AnthropicClient, just like what openai_kwargs did.

@Hk669 I really think do not scatter bedrock related kwargs to client.py,they should be kept in AnthropicClient.

@@ -370,7 +370,8 @@ class OpenAIWrapper:

openai_kwargs = set(inspect.getfullargspec(OpenAI.__init__).kwonlyargs)
aopenai_kwargs = set(inspect.getfullargspec(AzureOpenAI.__init__).kwonlyargs)
openai_kwargs = openai_kwargs | aopenai_kwargs
anthropic_kwargs = set(inspect.getfullargspec(AnthropicClient.__init__).args)
openai_kwargs = openai_kwargs | aopenai_kwargs | anthropic_kwargs
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not intuitive: should openai_kwargs contain anthropic_kwargs?

Copy link
Contributor

Choose a reason for hiding this comment

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

i believe #3210 fixes this issue.

@Hk669
Copy link
Contributor

Hk669 commented Jul 26, 2024

@wenngong thanks for the efforts. this issue is fixed in the other PR.

@Hk669 Hk669 closed this Jul 26, 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.

[Bug]: Bug when using anthropic bedrock
4 participants