-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Added support for models on AWS Bedrock and Sagemaker #772
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
Conversation
xiaochuan-du
left a comment
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.
do you think the lm.py need to be updated as well? otherwise, the llm_model.inspect_history(n=1) may throw errors
|
Good catch! |
dsp/modules/__init__.py
Outdated
| from .cache_utils import * | ||
| from .clarifai import * | ||
| from .cohere import * | ||
| from .cache_utils import * # noqa: F403 |
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.
@drawal1 can you remove all extra comments here?
dsp/modules/aws_models.py
Outdated
| from typing import Any | ||
|
|
||
| from dsp.modules.lm import LM | ||
| from shared.src.models.utils.aws_providers import AWSProvider, Bedrock, Sagemaker |
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.
where is this import coming from? can you wrap any external imports in a try/except block as done here?
dsp/modules/aws_models.py
Outdated
| right now. | ||
| """ | ||
| if not only_completed: | ||
| raise NotImplementedError("Error, only_completed not yet supported!") |
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.
minor styling but can you follow this here?
dsp/modules/aws_models.py
Outdated
|
|
||
|
|
||
| class AWSModel(LM): | ||
| """This class adds support for an AWS model.""" |
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.
can you add more clear documentation on how this class works with the other classes?
dsp/modules/aws_providers.py
Outdated
| profile_name (str, optional): boto3 credentials profile. | ||
| batch_n_enabled (bool): If False, call the LM N times rather than batching. | ||
| """ | ||
| import boto3 # pylint: disable=import-outside-toplevel |
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.
same comment as above - wrap external imports in a try/except block as done here?
dspy/__init__.py
Outdated
| from .primitives import * | ||
| from .retrieve import * | ||
| from .signatures import * | ||
| from .predict import * # noqa: F403 |
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.
same comment as above - remove any extra comments
| Google = dsp.Google | ||
|
|
||
| HFClientTGI = dsp.HFClientTGI | ||
| HFClientVLLM = HFClientVLLM |
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.
why is this deleted?
| pgvector = {version = "^0.2.5", optional = true} | ||
| structlog = "^24.1.0" | ||
|
|
||
|
|
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.
remove change
| from .aws_models import AWSAnthropic, AWSLlama2, AWSMistral, AWSModel | ||
| from .aws_providers import Bedrock, Sagemaker | ||
| from .azure_openai import AzureOpenAI | ||
| from .bedrock import * |
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.
why is importing bedrock removed from here?
|
Hi @drawal1, left some comments on the PR. It's also failing some of the CI tests. Please run It would be great if you could add both documentation and integration tests for the AWS model support as well. Feel free to follow these LM docs for reference. |
This check-in replaces the old Bedrock module which was incomplete and obsolete. The new code refactors the module into aws_providers and aws_models and allows extending to new models.
Supported aws providers - Bedrock and Sagemaker
Supported models - Mistral, Anthropic, Llama (extensible design to add custom models)