Adding for Cohere models in BedrockChat

[ksaegusa]

BedrockChat did not have a Cohere model, so we will add one

The stream and tool calls are non-overlapping

[ksaegusa]

Hi @3coins,
I have fixed the lint error. Could you please run the workflow again at your earliest convenience?

Thank you!

[ksaegusa]

I apologize, but I have discovered there are still some errors. I will fix them promptly.

[ksaegusa]

I realized that there were some areas lacking consideration, so I have revised the code. Could you please review the updated code?

To successfully pass linting locally, I had to make changes to multiple files, but the only files with added code are and .chat_models/bedrock.pyllms/bedrock.py

Thank you for your assistance.

[ksaegusa]

Hi @3coins,

I'm investigating an issue with the CI tests but haven't been able to determine the cause. Could you provide some advice if possible?

When I run make lint in my local environment, the same error does not appear.

Thank you for your help.

[revsystem]

Hi @ksaegusa,
What about adding model=self._get_model() argument to "convert_messages_to_prompt" of "ChatPromptAdapter"?

            elif provider == "cohere":
                if "command-r" in self.model_id:
                    prompt, chat_history = ChatPromptAdapter.format_cohere_message(
                        provider, messages
                    )
                else:
                    prompt = ChatPromptAdapter.convert_messages_to_prompt(
                        provider=provider, messages=messages, model=self._get_model()
                    )

[ksaegusa]

Hi @revsystem

Thank you for your comments!
Upon reviewing the code, I realized that the state of the main branch had changed since I forked it, and I did not notice that the model was not applied correctly.

I have made the necessary corrections based on your advice!

[arm-diaz]

@3coins when should we expect this PR to get approved?

[supreetkt]

@ksaegusa - seems like there are merge conflicts with the base branch. Can those be resolved so this functionality be added in?

[ksaegusa]

@supreetkt
Thank you for pointing that out. I have resolved the merge conflicts!

@3coins
Could you please review the updated PR at your convenience and let me know if there is anything further that needs to be addressed?

[Anirudh31415926535]

May I check why it's not supported here?

[ksaegusa]

command-r has been excluded from BedrockLLM for the same reason as claude-3. Specifically, command-r differs from the standard command model in terms of output format and behavior. Due to these differences, it requires specialized handling, which is why it has been excluded from the general BedrockLLM class.

[Anirudh31415926535]

@ksaegusa Thanks so much for the help here! May I confirm that this PR doesn't add tool calling support for cohere in bedrock? May I check if this is something down the line that you'd want to add support to as well?

[ksaegusa]

Hi @Anirudh31415926535
Thank you for your comment!

Yes, tool calling for Cohere has not been added in this PR. I would like to add it in the future, but currently, I do not fully understand the tool calling mechanism.

[supreetkt]

@3coins: can we get a review on this PR?

[3coins]

@ksaegusa
Can you add some integration/unit tests to verify these changes, this will help us merge this feature.

[ksaegusa]

Hi, @3coins

Thank you for your comment!
Following your feedback, I have added the following tests:

• Unit Test:
  • For the _format_cohere_messages method to ensure that the message formatting is performed correctly.
• Integration Tests:
  • For cohere.command and cohere.command-r-plus to verify that these methods work together as expected.

Please let me know if there are any additional aspects you think should be tested or if you have any further suggestions.

Thank you!

[Anirudh31415926535]

Hi @Anirudh31415926535 Thank you for your comment!

Yes, tool calling for Cohere has not been added in this PR. I would like to add it in the future, but currently, I do not fully understand the tool calling mechanism.

I see, yeah it could be a little confusing haha... But if you're keen to add support for this, I'd highly recommend taking a look at this page here: https://docs.cohere.com/docs/tools for single-step and multi-step tool use!

Additionally, you could also take a look at how it's implemented in ChatCohere in langchain-cohere.

But, as a first step, I think adding support for multi-step tool use would be highly beneficial, more so than the single step!
You could raise a PR if you're keen, and I can help review!

[Anirudh31415926535]

PR looks good imo!
Just a call out here that tool calling isn't supported for this integration as of now with the cohere models!

[ksaegusa]

Thank you for your review!

While I’m unable to implement tool support in this PR, I will definitely consider it for future improvements after reviewing the documentation you mentioned!

[ksaegusa]

@3coins
There were some conflicts due to updates in the main branch, and I have resolved them. Could you please review the changes? If there is a reason why this PR hasn’t been merged, could you kindly let me know? Thank you!

[ksaegusa]

Hi @ccurme

Apologies for the sudden mention. I understand everyone is busy, but the PR (#42) I submitted hasn't made much progress. The code changes and tests are already complete. If you have time, could you kindly check if there's anything additional required?

Thank you for your time and help!

[ksaegusa]

Since it seems that this pull request hasn't garnered any interest, I will proceed to close it. I understand that the changes I proposed might not align with your project's direction, or perhaps the timing just wasn’t right to review this contribution.

I had hoped this PR could add some value to the project, but unfortunately, it appears that wasn't meant to be. In any case, I wish the project continued success and all the best moving forward.

[supreetkt]

@3coins, @ccurme - can we understand why this PR is not being looked into? Is the idea that these models won't be supported for ChatBedrock?