-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: support aws proxy_url #1901
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
return cls( | ||
model_id=model_name, | ||
region_name=model_credential['region_name'], | ||
credentials_profile_name=model_credential['credentials_profile_name'], | ||
streaming=model_kwargs.pop('streaming', True), | ||
model_kwargs=optional_params | ||
model_kwargs=optional_params, | ||
config=config | ||
) |
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.
The provided code seems to be a custom implementation of a Bedrock model provider for use with LangChain community's BedrockChat
class. Here are some checks and suggestions:
Regularity Check:
- Imports: The necessary imports appear correct.
- Class Definition: The
BedrockModel
class inherits fromMaxKBBaseModel
. - Initialization Method: The constructor initializes the
super()
call correctly.
Potential Issues:
-
Missing Required Parameters:
- In the
__init__
method, there are no required parameters (likeendpoint_url
) mentioned but not included in your example initialization (model_kwargs
). Ensure that all required parameters are handled appropriately.
- In the
-
Proxy Configuration:
- You should ensure that
proxy_url
is properly set if it exists inmodel_credentials
. If proxy handling is required, it should be documented clearly.
- You should ensure that
-
Optional Keyword Arguments:
- Consider how
model_kwargs
is being used. It might be beneficial to explicitly list what arguments can be passed through and explain their usage, especially since they override base model parameters.
- Consider how
Optimization Suggestions:
-
Default Proxy Handling:
- Implement default proxy handling using
None
if no proxy URL is provided inmodel_credential
.
- Implement default proxy handling using
-
Configuration Class:
- Use
config = Config(**kwargs)
instead of manually defining each parameter to keep configuration more flexible.
- Use
Here’s an optimized version of the function incorporating these considerations:
from typing import List, Dict
import time
from botocore.config import Config
from langchain_community.chat_models import BedrockChat
from setting.models_provider.base_model_provider import MaxKBBaseModel
class BedrockModel(MaxKBBaseModel):
def is_cache_model(self):
return False
def __init__(self, model_id: str, region_name: str, credentials_profile_name: str,
streaming: bool = False, config: Config = None, **kwargs):
# Default proxy or none if not specified
proxy_url = kwargs.get('proxy_url') or 'http://none.proxy.com'
# Optionally add other configurations based on kwargs
additional_config_items = {
"connect_timeout": 60,
"read_timeout": 60,
}
config.update(additional_config_items)
# Initialize with merged configuration
super().__init__(
model_id=model_id,
region_name=region_name,
credentials_profile_name=credentials_profile_name,
stream_mode='async' if streaming else 'sync',
session=None, # Ensure you have access to create sessions if needed
boto_config=config
)
@classmethod
def new_instance(cls, model_type: str, model_name: str, model_credential: Dict[str, str],
**model_kwargs) -> 'BedrockModel':
optional_params = cls._filter_optional_params(model_kwargs)
# Create a Config object based on model credential
base_url = model_credential.get('base_url')
connect_timeout = model_credential.get('connect_timeout', 60)
read_timeout = model_credential.get('read_timeout', 60)
proxies = base_url and {'http': base_url, 'https': base_url}
config = Config(proxies=proxies, connection_timeout=connect_timeout, read_timeout=read_timeout)
return cls(
model_name=model_name,
region_name=model_credential['region_name'],
credentials_profile_name=model_credential['credentials_profile_name'],
streaming=model_kwargs.pop('streaming', True),
model_kwargs={k: v for k, v in model_kwargs.items() if k not in ['base_url']},
boto_config=config
)
Key Changes:
- Defaults and Optional Handling: Added defaults where appropriate and ensured flexibility with keyword arguments.
- Config Initialization: Created a
Config
object with dynamic values based on input. - Documentation: Improved documentation around expected behavior and usage of optional parameters.
This revised function maintains clarity, flexibility, and handles common edge cases related to proxied connections and configurable timeouts.
@@ -79,6 +79,7 @@ def encryption_dict(self, model: Dict[str, object]): | |||
region_name = forms.TextInputField('Region Name', required=True) | |||
access_key_id = forms.TextInputField('Access Key ID', required=True) | |||
secret_access_key = forms.PasswordInputField('Secret Access Key', required=True) | |||
base_url = forms.TextInputField('Proxy URL', required=False) | |||
|
|||
def get_model_params_setting_form(self, model_name): | |||
return BedrockLLMModelParams() |
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.
The provided code is syntactically correct. However, there are a few areas where you might consider making improvements or addressing potential issues:
Potential Improvement Suggestions
-
Comments: Add comments to explain the purpose of each method and variables if they aren't already documented.
-
Type Annotations: Although not strictly necessary in Python versions prior to 3.5, using type annotations (e.g.,
Dict[str, object]
) can improve readability and help static analysis tools understand your code better. -
Form Naming Convention: Ensure that form fields use a consistent naming convention, particularly when working with frameworks like Flask-WTF or Django's forms.
-
Security Practices:
- Consider adding validation on
secret_access_key
to ensure it meets minimum security standards such as being at least 32 characters long. - Implement rate limiting and throttling for requests related to AWS credentials to prevent abuse.
- Consider adding validation on
-
Error Handling: If dealing with API calls, add error handling to manage exceptions gracefully. For example, handle cases where retrieving the user from the database fails.
-
URL Validation: Since HTTP/HTTPS URLs need to be valid, you might want to validate them before processing.
-
Configuration Management: If
model
,forms
,BedrockLLMModelParams
, and other modules/classes are part of a larger application architecture, ensure they are properly imported and configured. -
Code Formatting: Ensure consistent coding style rules are followed, which improves maintainability and collaboration among developers.
By considering these points, you can make your codebase more robust, maintainable, and secure while still meeting its current requirements.
feat: support aws proxy_url