-
Notifications
You must be signed in to change notification settings - Fork 105
Support for AnthropicBedrock models #307
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -553,3 +553,43 @@ def make_model(self): | |
| temperature=self.temperature, | ||
| max_tokens=self.max_new_tokens, | ||
| ) | ||
|
|
||
|
|
||
| class BedrockChatModel(AnthropicChatModel): | ||
| def __init__( | ||
| self, | ||
| model_name, | ||
| api_key=None, | ||
| temperature=0.5, | ||
| max_tokens=100, | ||
| max_retry=4, | ||
| ): | ||
| self.model_name = model_name | ||
| self.temperature = temperature | ||
| self.max_tokens = max_tokens | ||
| self.max_retry = max_retry | ||
|
Comment on lines
+558
to
+570
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improper inheritance implementation
Tell me moreWhat is the issue?BedrockChatModel inherits from AnthropicChatModel but completely overrides the parent's init method without calling super().init(). This breaks the inheritance chain and duplicates code. Why this mattersThis violates the DRY principle and could lead to maintenance issues if the parent class initialization changes. It also makes the inheritance relationship misleading since the child isn't actually using the parent's initialization logic. Suggested change ∙ Feature Previewclass BedrockChatModel(AnthropicChatModel):
def __init__(self, *args, **kwargs):
if not all(os.getenv(key) for key in ['AWS_REGION', 'AWS_ACCESS_KEY', 'AWS_SECRET_KEY']):
raise ValueError('AWS credentials must be set in environment')
super().__init__(*args, **kwargs)
self.client = anthropic.AnthropicBedrock(
aws_region=os.getenv('AWS_REGION'),
aws_access_key=os.getenv('AWS_ACCESS_KEY'),
aws_secret_key=os.getenv('AWS_SECRET_KEY')
)Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
|
||
| if ( | ||
| not os.getenv("AWS_REGION") | ||
| or not os.getenv("AWS_ACCESS_KEY") | ||
| or not os.getenv("AWS_SECRET_KEY") | ||
|
Comment on lines
+574
to
+575
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Incorrect AWS environment variable names
Tell me moreWhat is the issue?The AWS environment variable names used are non-standard. AWS SDK typically expects AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY, not AWS_ACCESS_KEY and AWS_SECRET_KEY. Why this mattersUsing incorrect environment variable names will cause authentication failures even when users have properly configured their AWS credentials using standard AWS conventions. Suggested change ∙ Feature PreviewUse the standard AWS environment variable names: if (
not os.getenv("AWS_REGION")
or not os.getenv("AWS_ACCESS_KEY_ID")
or not os.getenv("AWS_SECRET_ACCESS_KEY")
):
raise ValueError(
"AWS_REGION, AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY must be set in the environment when using BedrockChatModel"
)
self.client = anthropic.AnthropicBedrock(
aws_region=os.getenv("AWS_REGION"),
aws_access_key=os.getenv("AWS_ACCESS_KEY_ID"),
aws_secret_key=os.getenv("AWS_SECRET_ACCESS_KEY"),
)Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
| ): | ||
| raise ValueError( | ||
| "AWS_REGION, AWS_ACCESS_KEY and AWS_SECRET_KEY must be set in the environment when using BedrockChatModel" | ||
| ) | ||
|
|
||
| self.client = anthropic.AnthropicBedrock( | ||
| aws_region=os.getenv("AWS_REGION"), | ||
| aws_access_key=os.getenv("AWS_ACCESS_KEY"), | ||
| aws_secret_key=os.getenv("AWS_SECRET_KEY"), | ||
| ) | ||
|
Comment on lines
+572
to
+585
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Insecure storage of AWS credentials
Tell me moreWhat is the issue?The code is using environment variables to store and retrieve AWS credentials, which is not the most secure method for handling sensitive information. Why this mattersStoring credentials in environment variables can lead to accidental exposure through system logs or environment dumps. If the system is compromised, an attacker could potentially access these credentials. Suggested change ∙ Feature PreviewUse AWS IAM roles for EC2 instances or AWS SDK credential providers instead of environment variables. If environment variables must be used, consider using a secure secrets management system like AWS Secrets Manager or HashiCorp Vault. Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
|
||
|
|
||
| @dataclass | ||
| class BedrockModelArgs(BaseModelArgs): | ||
| def make_model(self): | ||
| return BedrockChatModel( | ||
| model_name=self.model_name, | ||
| temperature=self.temperature, | ||
| max_tokens=self.max_new_tokens, | ||
| ) | ||
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.
Misleading Parameter Inheritance
Tell me more
What is the issue?
The BedrockChatModel accepts an unused api_key parameter inherited from AnthropicChatModel.
Why this matters
Including unused parameters creates confusion about the actual requirements of the class and its relationship with the parent class.
Suggested change ∙ Feature Preview
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.