-
Notifications
You must be signed in to change notification settings - Fork 13.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
Support for claude v3 models. #18630
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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.
Looks good to me
Very NIce @3coins . Also +1 on the refactor |
Could e merge this please we urgently need this for the new Claude models |
@efriis is this something you could look at? |
) -> Dict[str, Any]: | ||
input_body = {**model_kwargs} | ||
if provider == "anthropic": | ||
input_body["prompt"] = _human_assistant_format(prompt) | ||
if messages: | ||
input_body["anthropic_version"] = "bedrock-2023-05-31" |
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.
input_body["anthropic_version"] = "bedrock-2023-05-31" | |
if "anthropic_version" not in input_body: | |
input_body["anthropic_version"] = "bedrock-2023-05-31" |
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.
@Barneyjm
One concern with this is that allowing another version would have to match the implementation of that version as well, in case it differs from what is currently here.
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.
One way to tackle this would be to have version specific implementations, and making sure they work consistently.
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.
yeah i agree it's not ideal, but providing some capability to pass this in will protect us in the future for whenever a new "anthropic_version" shows up.
@Adibuer-lab @Barneyjm Pros
Cons
@efriis |
I'd err on the side of compatibility and a nice deprecation warning to allow for the 'might be dropped at some point' use case. Nothing's worse than upgrading langchain and having all your tests break. Actually, scratch that, there is something worse: not having the right tests in the first place and having your application break :) |
I agree with @christopherwerner and like what @Barneyjm did there. A lot of people have probably invested a lot of time in developing code for prompts that use human-assistant format so it would be great for them to have the opportunity to use the v3 models while refactoring to use messages api. Being able to easily work with images in messages should motivate to migrate to the messages api On another note do you think the BedrockLLM could do with leveraging the BaseMessage class from langchain core similar to BedrockChat? |
I just spoke with @efriis and his recommendation is to migrate the code to One action on my end is to put a warning/error if the v3 model is used with Bedrock LLM, so that users migrate to the BedrockChat if they want to use the v3 model. I will add that shortly. |
Oh alright then BedrockLLM shouldn't need the BaseMessage class |
if prompt: | ||
input_body["prompt"] = _human_assistant_format(prompt) | ||
if "max_tokens_to_sample" not in input_body: | ||
input_body["max_tokens_to_sample"] = 256 |
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.
this one isn't necessary right? This was just a separate name for max_tokens
previously?
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.
No, this is required, it seems like Bedrock is still expecting this.
Error raised by bedrock service: An error occurred (ValidationException) when calling the InvokeModel operation: Malformed input request: #: required key [max_tokens_to_sample] not found#: extraneous key [max_tokens] is not permitted, please reformat your input and try again.
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.
Should we be using the value from the other here then?
1d208a7
to
cb0cb62
Compare
i'm jealous that the linters seem to prefer your commits over my commits. |
Tried with above changes seems like it not working with model claude2. claude 2.1 as well when using BedrockChat. Claude3 also facing the issue below: return BedrockChat( I found out that removing "max_tokens_to_sample" it works. Is it means that we are not allow to parse the max_tokens into kwargs anymore with this PR? |
@hteeyeoh |
Ya I have tested it out. It works seems this changes reflect to all other claude such as v2, v2.1 as well |
Hi @3coins, I'm one of the folks who are still using |
Works just fine with from langchain.chains.llm import LLMChain
from langchain_core.prompts import PromptTemplate
from langchain_community.chat_models import BedrockChat
chat = BedrockChat(
model_id="anthropic.claude-3-sonnet-20240229-v1:0",
model_kwargs={'temperature': 0.1}
)
prompt_template = "Tell me a {adjective} joke"
prompt = PromptTemplate(
input_variables=["adjective"], template=prompt_template
)
llm = LLMChain(llm=chat, prompt=prompt)
response=llm.invoke({'adjective': "funny"})
print(response['text']) Output:
|
As for import os
from urllib.request import urlretrieve
from langchain.chains import ConversationalRetrievalChain
from langchain_community.chat_models import BedrockChat
from langchain_community.document_loaders import TextLoader
from langchain_community.embeddings.bedrock import BedrockEmbeddings
from langchain_community.vectorstores import Chroma
from langchain_text_splitters import CharacterTextSplitter
if not os.path.isfile("state_of_the_union.txt"):
urlretrieve(
"https://raw.githubusercontent.com/hwchase17/chat-your-data/master/state_of_the_union.txt",
"state_of_the_union.txt"
)
# Vector store + retriever
raw_documents = TextLoader('state_of_the_union.txt').load()
text_splitter = CharacterTextSplitter(chunk_size=1000, chunk_overlap=0)
documents = text_splitter.split_documents(raw_documents)
vectorstore = Chroma.from_documents(
documents, BedrockEmbeddings(model_id="cohere.embed-english-v3")
)
retriever = vectorstore.as_retriever()
# Model
chat = BedrockChat(
model_id="anthropic.claude-3-sonnet-20240229-v1:0",
model_kwargs={'temperature': 0.0}
)
# Chain
chain = ConversationalRetrievalChain.from_llm(
llm=chat,
chain_type="stuff",
retriever=vectorstore.as_retriever(),
return_generated_question=True,
verbose=False,
)
# Request
response = chain.invoke({
'question': "What did the president say about Ketanji Brown Jackson",
'chat_history': []})
print(response['answer']) Output:
|
I wonder It can be complicated when we continue managing whole the models on Bedrock as all-in-one 🤔 |
Agree to this proposal. I think we should have the bedrock class now be split by model provider since each has their own nuances and prompt templates
From: みのるん ***@***.***>
Reply-To: langchain-ai/langchain ***@***.***>
Date: Saturday, March 9, 2024 at 10:41 PM
To: langchain-ai/langchain ***@***.***>
Cc: "Grewal, Rupinder" ***@***.***>, Comment ***@***.***>
Subject: Re: [langchain-ai/langchain] Support for claude v3 models. (PR #18630)
I wonder Bedrock LLM should be seperated by each providor models.
(e.g. BedrockClaude, etc.)
It can be complicated when we continue managing whole the models on Bedrock as all-in-one 🤔
—
Reply to this email directly, view it on GitHub<#18630 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AYMBZRQELYSEQVHGQ2G5EBLYXP56RAVCNFSM6AAAAABEIN5ND2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBXGEYDQNJYGA>.
You are receiving this because you commented.Message ID: ***@***.***>
|
I'm backporting these changes to langchain_community.chat_models.anthropic and I can't see the purpose of detecting the "image_url" type and converting it to the "image" type in _format_image. Is this for backwards compatibility with an earlier API version? Or is this something specific to Bedrock? |
It's not working for me in LLMChain as mentioned above. Running this code, I get an LLMChain error:
` |
@3coins When will this get tagged? I want to use this update. |
@ks233ever |
Fixes langchain-ai#18513. ## Description This PR attempts to fix the support for Anthropic Claude v3 models in BedrockChat LLM. The changes here has updated the payload to use the `messages` format instead of the formatted text prompt for all models; `messages` API is backwards compatible with all models in Anthropic, so this should not break the experience for any models. ## Notes The PR in the current form does not support the v3 models for the non-chat Bedrock LLM. This means, that with these changes, users won't be able to able to use the v3 models with the Bedrock LLM. I can open a separate PR to tackle this use-case, the intent here was to get this out quickly, so users can start using and test the chat LLM. The Bedrock LLM classes have also grown complex with a lot of conditions to support various providers and models, and is ripe for a refactor to make future changes more palatable. This refactor is likely to take longer, and requires more thorough testing from the community. Credit to PRs [18579](langchain-ai#18579) and [18548](langchain-ai#18548) for some of the code here. --------- Co-authored-by: Erick Friis <erick@langchain.dev>
Fixes langchain-ai#18513. ## Description This PR attempts to fix the support for Anthropic Claude v3 models in BedrockChat LLM. The changes here has updated the payload to use the `messages` format instead of the formatted text prompt for all models; `messages` API is backwards compatible with all models in Anthropic, so this should not break the experience for any models. ## Notes The PR in the current form does not support the v3 models for the non-chat Bedrock LLM. This means, that with these changes, users won't be able to able to use the v3 models with the Bedrock LLM. I can open a separate PR to tackle this use-case, the intent here was to get this out quickly, so users can start using and test the chat LLM. The Bedrock LLM classes have also grown complex with a lot of conditions to support various providers and models, and is ripe for a refactor to make future changes more palatable. This refactor is likely to take longer, and requires more thorough testing from the community. Credit to PRs [18579](langchain-ai#18579) and [18548](langchain-ai#18548) for some of the code here. --------- Co-authored-by: Erick Friis <erick@langchain.dev>
Quick note here, going off of the XML Agent documentation, "Use with regular LLMs, not with chat models." is stated as a tip for anthropic models. Should an exception be added for Claude V3 models as the Bedrock class doesn't support messages-based API? |
Fixes #18513.
Description
This PR attempts to fix the support for Anthropic Claude v3 models in BedrockChat LLM. The changes here has updated the payload to use the
messages
format instead of the formatted text prompt for all models;messages
API is backwards compatible with all models in Anthropic, so this should not break the experience for any models.Notes
The PR in the current form does not support the v3 models for the non-chat Bedrock LLM. This means, that with these changes, users won't be able to able to use the v3 models with the Bedrock LLM. I can open a separate PR to tackle this use-case, the intent here was to get this out quickly, so users can start using and test the chat LLM. The Bedrock LLM classes have also grown complex with a lot of conditions to support various providers and models, and is ripe for a refactor to make future changes more palatable. This refactor is likely to take longer, and requires more thorough testing from the community. Credit to PRs 18579 and 18548 for some of the code here.