-
Notifications
You must be signed in to change notification settings - Fork 691
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
Refactoring client and providers. #27
Conversation
Added Azure support & refactored code. Refactoring includes - - ProviderFactory provides a specific provider impl. - Lazily import the provider based on config passed to Client. - Reduced the number of files.
c44c25d
to
d9ede71
Compare
Added back few older files. Replaced test_client with newer test code. Will need to port the older provider files to the new format. Till then keeping the older provider interface related tests.
Changed client.ipynb
aisuite/client.py
Outdated
""" | ||
Initialize the client with provider configurations. | ||
Use the ProviderFactory to create provider instances. | ||
""" |
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.
Since the provider_configs
is a simple dict, it could be helpful to add the args to doc string to explain what type of data is expected.
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.
Done, thanks!
# Used to call the AWS Bedrock converse API | ||
# Converse API provides consistent API, that works with all Amazon Bedrock models that support messages. | ||
# Eg: anthropic.claude-v2, | ||
# meta.llama3-70b-instruct-v1:0, | ||
# mistral.mixtral-8x7b-instruct-v0:1 | ||
# The model value can be a baseModelId or provisionedModelArn. | ||
# Using a base model id gives on-demand throughput. | ||
# Use CreateProvisionedModelThroughput API to get provisionedModelArn for higher throughput. | ||
# https://docs.aws.amazon.com/bedrock/latest/userguide/model-ids.html |
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.
Suggestion: Make this a doc string for the module.
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.
done, thanks!
headers = {"Content-Type": "application/json", "Authorization": self.api_key} | ||
|
||
try: | ||
req = urllib.request.Request(url, body, headers) |
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.
I recommend using httpx
for the requests.
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.
agreed. will send a follow up PR for this.
from aisuite.provider import Provider | ||
|
||
|
||
class GcpProvider(Provider): |
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.
I think Andrew wanted this named Google instead of GCP. I'm fine either way.
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.
agreed. i will sync up with him change this. i am divided between glcoud and google, similar to how "aws" is preferred over amazon.
assert isinstance(interface, AnthropicInterface) | ||
assert model_name == "some-model:v1" | ||
assert client.all_interfaces["anthropic"] == interface | ||
class TestClient(unittest.TestCase): |
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.
We were using pytest
before are you open to that or would you prefer the unittest
?
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.
i saw later that we already use pytest. I will migrate it in the follow-up PR.
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 for your review. I have addressed the suggestions.
There are few things I will address in follow up PR - like moving to pytest instead of unittest, and chaning the gcp_provider file name.
aisuite/client.py
Outdated
""" | ||
Initialize the client with provider configurations. | ||
Use the ProviderFactory to create provider instances. | ||
""" |
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.
Done, thanks!
# Used to call the AWS Bedrock converse API | ||
# Converse API provides consistent API, that works with all Amazon Bedrock models that support messages. | ||
# Eg: anthropic.claude-v2, | ||
# meta.llama3-70b-instruct-v1:0, | ||
# mistral.mixtral-8x7b-instruct-v0:1 | ||
# The model value can be a baseModelId or provisionedModelArn. | ||
# Using a base model id gives on-demand throughput. | ||
# Use CreateProvisionedModelThroughput API to get provisionedModelArn for higher throughput. | ||
# https://docs.aws.amazon.com/bedrock/latest/userguide/model-ids.html |
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.
done, thanks!
headers = {"Content-Type": "application/json", "Authorization": self.api_key} | ||
|
||
try: | ||
req = urllib.request.Request(url, body, headers) |
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.
agreed. will send a follow up PR for this.
from aisuite.provider import Provider | ||
|
||
|
||
class GcpProvider(Provider): |
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.
agreed. i will sync up with him change this. i am divided between glcoud and google, similar to how "aws" is preferred over amazon.
assert isinstance(interface, AnthropicInterface) | ||
assert model_name == "some-model:v1" | ||
assert client.all_interfaces["anthropic"] == interface | ||
class TestClient(unittest.TestCase): |
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.
i saw later that we already use pytest. I will migrate it in the follow-up PR.
Added Azure support & refactored code.
Refactoring includes -