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

community: Enable Claude v3 #18548

Closed
wants to merge 26 commits into from
Closed

Conversation

Barneyjm
Copy link
Contributor

@Barneyjm Barneyjm commented Mar 5, 2024

Description: implements changes for Claude V3 models through bedrock
Note: i'm not sure how to best handle the many breaking changes that Anthropic introduced into v3 of the model.

I'd suggest we do something like model_id = 'anthropic_v3' to distinguish old model input formats or make some serious changes to the other input models. But that will also introduce it's own headaches.

Currently this works by stripping out the "_v3" dynamically where needed to make boto3 happy but also make it possible to respect the two different input formats between v1/2 Claude and v3 Claude. Obviously this isn't ideal since "anthropic_v3" isn't a thing anywhere in Anthropic's documentation or Amazon's documentation. But I don't know how else we could make it work.

Open to any feedback

Issue: #18513

Copy link

vercel bot commented Mar 5, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Mar 6, 2024 1:47am

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. Ɑ: models Related to LLMs or chat model modules 🤖:improvement Medium size change to existing code to handle new use-cases labels Mar 5, 2024
@Barneyjm Barneyjm marked this pull request as draft March 5, 2024 01:47
@Barneyjm Barneyjm changed the title Draft for Issue #18513 community: Enable Claude v3 Mar 5, 2024
@Barneyjm Barneyjm marked this pull request as ready for review March 5, 2024 03:00
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. 🔌: anthropic Primarily related to Anthropic integrations and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 5, 2024
@Barneyjm
Copy link
Contributor Author

Barneyjm commented Mar 5, 2024

lint hates me. no idea why openai's file can have a near identical function and not get caught in this linter

@deepend-dev
Copy link

Great initiative. Does it also support vision api?

@@ -77,6 +77,22 @@ def _human_assistant_format(input_text: str) -> str:
return input_text


def _generate_messages_format(input_text: str) -> list:
Copy link
Contributor

@jonathancaevans jonathancaevans Mar 5, 2024

Choose a reason for hiding this comment

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

What about multiple turns for word in mouth of assistant turn?

It might be best to convert the old prompt formatting to the new messages by adding some logic to break things up Assistant Human turns into messages. This will give backwards compatibility for Claude 2 text completion prompts.

ie

def convert_prompt_to_messages(prompt):
    messages = []
    role_regex = re.compile(r'(Human:|Assistant:)\s?(.*?)(?=Human:|Assistant:|$)', re.DOTALL)

    for match in role_regex.finditer(prompt):
        role, content = match.groups()
        role = role.strip(':').lower()
        if role == "human" or role == "Human":
            role = "user"
        else:
            role = "assistant"
        messages.append({"role": role, "content": content.strip()})

    return messages

Copy link
Contributor Author

@Barneyjm Barneyjm Mar 5, 2024

Choose a reason for hiding this comment

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

the problem is that the Bedrock API itself doesn't allow for that flexibility -- for Claude2 it expects "prompt" and a string as the input.
"prompt": "\n\nHuman: hello claude \n\nAssistant:"
, not a list of message objects.

I might be misunderstanding your suggestion though, can you expand on it if I am?

multi-turn is simple enough, i just forgot to add the list comprehension. Effectively this will make it a chat_model though, so do we just modify those files instead? or do we keep it separate? Note the List[BaseMessages] here https://github.com/langchain-ai/langchain/blob/master/libs/community/langchain_community/chat_models/bedrock.py#L100

I'm not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea would be for Cluade 2 prompts such as,
"prompt": "\n\nHuman: Write me a play. \n\nAssistant: Here is a play about cats, "

would be translated to,

    "messages": [
        {
            "content": [
                {
                    "text": "Write me a play.",
                    "type": "text"
                }
            ],
            "role": "user"
        },
        {
            "content": [
                {
                    "text": "Sure, here is a play about cats,",
                    "type": "text"
                }
            ],
            "role": "assistant"
        }
    ]

By using the regex, this function could offer full breadth backwards compatibility to folks migrating their prompts from Claude 2, to Claude 3 models without having to migrate completion string prompts to Langchain messages.

When it comes to BedrockChat, I have an implementation on hand, let me see if I can merge that with your branch...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key thing here is that Bedrock itself has the model_id and input format "hardcoded" to expect certain inputs.

That regex approach would work if Bedrock wasn't validating the input format -- we almost need to un-regex it and put it into the format that Bedrock expects for each Claude model.

and then the trouble there is each Claude model has different input/output structures that you can't sniff out without differentiating as a "new" provider, anthropic_v3.

the parameters for prepare_input_stream and prepare_output_stream has no model information in it, only provider -- so we can't programmatically say "for claude v2, do this kind of regex operation but for claude v3, leave it alone".

to the LLMInputOutputAdapter class, it all just looks like "anthropic" without this new v3 moniker.

Choose a reason for hiding this comment

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

The messages API also supports system property for a system prompt, so it can be more sophisticated than just Human/Assistant sequence.

While playing with it, I found that Claude 3 and Claude 2 both work fine with messages API (i.e. no errors, and reasonable response), when you feed them a message that includes Human/Assistant sequence. I.e. w/o having a need to have a separate anthropic_v3 provider, you can feed the exact same "prompt": "\n\nHuman: Write me a play. \n\nAssistant: Here is a play about cats, "

I made a change locally that looks like this

      if provider == "anthropic":
            if input_body.get("use_completion_api") is True:
                input_body["prompt"] = _human_assistant_format(prompt)
                if "max_tokens_to_sample" not in input_body:
                    input_body["max_tokens_to_sample"] = 256
                del input_body["use_completion_api"]
            else:
                input_body["messages"] = _generate_messages_format(prompt)

And for Claude 2 it returns very similar results for the same prompt in both cases, when use_completion_api is set, and when it's not.

Though, to use messages API with Claude 2, model_kwargs need to be something like this:

model_kwargs_claude = {
                "temperature": 0,
                "top_k": 10,
                "max_tokens": 4000,
                "anthropic_version": "bedrock-2023-05-31",
            }

@jonathancaevans
Copy link
Contributor

@Barneyjm Can we add support for ChatBedrock here too? I like that core llm implementation is added for supporting easy migration from Claude 2 Bedrock, but there should be native support for Langchain messages added to ChatBedrock too.

Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

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

Hey there! Given claude v3 models only support the messages api in anthropic, would it make sense to add this compatibility to BedrockChat instead? https://docs.aws.amazon.com/bedrock/latest/userguide/model-parameters-anthropic-claude-messages.html

@Barneyjm
Copy link
Contributor Author

Barneyjm commented Mar 6, 2024

Hey there! Given claude v3 models only support the messages api in anthropic, would it make sense to add this compatibility to BedrockChat instead? https://docs.aws.amazon.com/bedrock/latest/userguide/model-parameters-anthropic-claude-messages.html

from my understanding, the BedrockChat class still uses this LLM input/output class under the covers, so it'll need to be added here

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 6, 2024
@efriis efriis self-assigned this Mar 6, 2024
@efriis
Copy link
Member

efriis commented Mar 6, 2024

Hey @Barneyjm ! I think it uses that for some other models, but the class inherits from BaseChatModel which will allow people interfacing with it to pass messages instead of raw text.

Because the new claude 3 models are only chat models, and aren't compatible with text completions, I would prefer to implement the logic over there.

I'm less familiar with the bedrock integrations and just starting to dig so might have some stupid questions / need some education :) - are you saying that there's an existing bedrock claude messages api integration on this Bedrock LLM and not BedrockChat Chat Model that this hooks into?

@efriis
Copy link
Member

efriis commented Mar 6, 2024

cc @Adibuer-lab - would love your take on the above too (just easier to discuss all of us on one thread rather than splitting between the two)

@Barneyjm
Copy link
Contributor Author

Barneyjm commented Mar 6, 2024

@efriis I've only spent a few hours on this so i'm by no means the expert! i am now painfully familiar with how these things interact though :P

in bedrock chat, i see it imports BedrockBase

the BedrockBase class leverages the LLMInputOutputParser which is, for better or worse, in that same bedrock.py file

so by adding these changes here, i believe we are adding it to the chat models. while the new models are indeed chat-only, i think the way this is currently structured makes us put it here? of course we could always move it over to bedrock chat! my thinking was by adding it here, we'd maintain the old prediction api while it's still around and add the new stuff transparently

@Adibuer-lab
Copy link

Adibuer-lab commented Mar 6, 2024

@efriis I've only spent a few hours on this so i'm by no means the expert! i am now painfully familiar with how these things interact though :P

in bedrock chat, i see it imports BedrockBase

the BedrockBase class leverages the LLMInputOutputParser which is, for better or worse, in that same bedrock.py file

so by adding these changes here, i believe we are adding it to the chat models. while the new models are indeed chat-only, i think the way this is currently structured makes us put it here? of course we could always move it over to bedrock chat! my thinking was by adding it here, we'd maintain the old prediction api while it's still around and add the new stuff transparently

For now I'm seeing it as

  1. BedrockChat and BedrockBase need to both be able to handle inputs(whether human assistant or messages api format) since people may be using BedrockBase on its own
  2. BedrockChat can potentially move over to messages api completely since anthropic.py in chat_models uses BaseMessage class from langchain core.
  3. For BedrockChat to move completely or dual run both apis BedrockBase _prepare_input_and_invoke_stream needs to be able to handle messages api format

Why I think so:

looks like BedrockBase is using LLMInputOutputAdapterwhereas BedrockChat is using a ChatPromptAdapter to handle that conversion. The ChatPromptAdapter uses functions from anthropic.py in chat_models to handle the conversion.

BedrockChat then uses _prepare_input_and_invoke_stream from BedrockBase but it's already done the conversion to human-assistant format. Making the change in BedrockChat will involve this ChatPromptAdapter or better, anthropic.py in chat_models but that'll leave BedrockBase without messages api compatibility, and anyone using only BedrockBase directly won't get the claude 3 features.
So ideally BedrockBase LLMInputOutputAdapter should be able handle messages api format and text completion api format that devs directly send to BedrockBase and also using _prepare_input_and_invoke_stream, handle prompts in messages api format that BedrockChat sends to i.

Doing it this way will require separate handling of BedrockChat and anthropic.py in chat_models where maybe the model_id is injected to get the conversion to messages api if using Claude 3(if we want to dual run both apis, BedrockChat could move over completely). But as long as they're doing their conversions separately, doing one and ignoring the other will leave some people without claude v3 compatibility and that'll need to be addressed at some point

Maybe there should be another issue to streamline where this conversion is happening so its only in one place.

@Barneyjm
Copy link
Contributor Author

Barneyjm commented Mar 6, 2024

@3coins with the slam dunk PR! Nice!

@Barneyjm
Copy link
Contributor Author

Barneyjm commented Mar 6, 2024

closing this in place of @3coins PR: #18630

@Barneyjm Barneyjm closed this Mar 6, 2024
efriis added a commit that referenced this pull request Mar 6, 2024
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](#18579) and
[18548](#18548) for some
of the code here.

---------

Co-authored-by: Erick Friis <[email protected]>
@marfago
Copy link

marfago commented Mar 7, 2024

Is there a PR for Bedrock LLM to support Claude 3?

gkorland pushed a commit to FalkorDB/langchain that referenced this pull request Mar 30, 2024
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔌: anthropic Primarily related to Anthropic integrations 🤖:improvement Medium size change to existing code to handle new use-cases Ɑ: models Related to LLMs or chat model modules size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants