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

Add mistral as supported provider for AWS Bedrock #4803

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

jarib
Copy link
Contributor

@jarib jarib commented Mar 17, 2024

Mistral is now supported in AWS Bedrock.

$ aws bedrock list-foundation-models --region us-east-1 --query 'modelSummaries[*].modelId' | grep mistral
    "mistral.mistral-7b-instruct-v0:2",
    "mistral.mixtral-8x7b-instruct-v0:1"
$

Langchain has a duplicated list of valid models. I'd argue that they both can be removed. Langchain doesn't need to validate the models before passing them on to Bedrock.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 17, 2024
Copy link

vercel bot commented Mar 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
langchainjs-api-refs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 18, 2024 9:51pm
langchainjs-docs ✅ Ready (Inspect) Visit Preview Mar 18, 2024 9:51pm

@jacoblee93
Copy link
Collaborator

The issue is in the input/output parsing - I think there might be some other changes needed in the adapter?

@jacoblee93 jacoblee93 added the question Further information is requested label Mar 18, 2024
@esoler-sage
Copy link
Contributor

esoler-sage commented Mar 18, 2024

@jacoblee93 You are 💯 right! I created a PR against his fork (https://github.dev/jarib/langchainjs/pull/1) with the changes needed to implement it properly.
@jarib if you want to take a look it'll solve 4683 ;)

Thanks!

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 18, 2024
@jarib
Copy link
Contributor Author

jarib commented Mar 18, 2024

Ah, I missed that. I've merged @esoler-sage's fix, thanks!

@jacoblee93
Copy link
Collaborator

Thank you! FYI @efriis

@jacoblee93 jacoblee93 added lgtm PRs that are ready to be merged as-is and removed question Further information is requested labels Mar 18, 2024
@@ -21,21 +21,21 @@ import { BedrockChat } from "../bedrock/web.js";
void testChatModel(
"Test Bedrock chat model: Claude-v2",
"us-east-1",
"anthropic.claude-v2",
"mistral.mistral-7b-instruct-v0:2",
"What is your name?"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this be added as separate tests? Claude support is pretty important too :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well we've already got Claude-3 below, which most people should be using now.

@efriis
Copy link
Member

efriis commented Mar 18, 2024

Good PR!

A lot of these if statements / double validation will be removed in an upcoming refactor of bedrock on the python side, and @jacoblee93 will probably tap in to do something similar in js. I believe the reason for validation is that Bedrock models don't have a standard api for their inputs and arguments, so checking and throwing an error fails quickly instead of bedrock introducing a new model that doesn't have a supported input/args format, and producing an output that doesn't make sense.

@jacoblee93 jacoblee93 merged commit 09db56a into langchain-ai:main Mar 18, 2024
17 checks passed
@esoler-sage
Copy link
Contributor

@efriis I was about to create 2 PR... token count on Bedrock Models and add different chat prompts to improve this line: https://github.com/langchain-ai/langchainjs/blob/main/libs/langchain-community/src/chat_models/bedrock/web.ts#L295.

For this second, probably it's better to wait until this refactoring is done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto:improvement Medium size change to existing code to handle new use-cases lgtm PRs that are ready to be merged as-is size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants