-
Notifications
You must be signed in to change notification settings - Fork 750
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
feat: Integrate cohere models #932
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Thanks @NeilJohnson0930 , left some comments
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.
Thanks @NeilJohnson0930 ! Left some comments below
0bc66de
to
3ac127b
Compare
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.
Thanks @NeilJohnson0930 ! Left some comments
ecb1925
to
03cd8c9
Compare
f50c94c
to
c170ab5
Compare
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.
Thanks @NeilJohnson0930 , left some comments below, based on the review also added one more commit here: 3886e6e feel free to check~
camel/configs/cohere_config.py
Outdated
documents (dict, optional): A list of relevant documents that the | ||
model can cite to generate a more accurate reply. Each document is | ||
either a string or document object with content and metadata. | ||
(default: :obj:`None`) |
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.
from https://docs.cohere.com/reference/chat the type should be list of string or doc
camel/models/cohere_model.py
Outdated
def __init__( | ||
self, | ||
model_type: ModelType, | ||
model_config_dict: Dict[str, Any], |
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 should be optional
camel/models/cohere_model.py
Outdated
super().__init__( | ||
model_type, model_config_dict, api_key, url, token_counter | ||
) | ||
self.tool_call_id = None |
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.
we can set this in _to_cohere_chatmessage
pyproject.toml
Outdated
@@ -123,7 +123,7 @@ redis = { version = "^5.0.6", optional = true } | |||
|
|||
# retrievers | |||
rank-bm25 = { version = "^0.2.2", optional = true } | |||
cohere = { version = "^4.56", optional = true } | |||
cohere = { version = "^5.11.0", optional = true } |
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.
dependency position is not correct, refer to other models like mistralai
camel/models/cohere_model.py
Outdated
import ast | ||
import json | ||
import uuid |
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.
move to upper as they don't need additional install
COHERE_COMMAND_R = "command-r" | ||
COHERE_COMMAND_LIGHT = "command-light" | ||
COHERE_COMMAND = "command" | ||
COHERE_COMMAND_NIGHTLY = "command-nightly" |
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.
from https://docs.cohere.com/v2/docs/models more models like command-r-plus
could be supported
camel/types/enums.py
Outdated
elif self in { | ||
ModelType.COHERE_COMMAND_R, | ||
ModelType.COHERE_COMMAND_LIGHT, | ||
ModelType.COHERE_COMMAND, | ||
ModelType.COHERE_COMMAND_NIGHTLY, | ||
}: | ||
return 4_096 |
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.
we have one dict for 4_096, for COHERE_COMMAND_R it should be 128k
) | ||
|
||
# Define system message | ||
sys_msg = BaseMessage.make_assistant_message( |
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.
message setting could be simpified
camel/models/cohere_model.py
Outdated
role="assistant", | ||
content=content, # type: ignore[arg-type] | ||
) | ||
continue |
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.
I think here we should use else
statement instead of break the whole loop
camel/models/cohere_model.py
Outdated
arguments_dict = ast.literal_eval(arguments) | ||
arguments_json = json.dumps(arguments_dict) | ||
assis_tool_call_id = str(uuid.uuid4()) | ||
self.tool_call_id = assis_tool_call_id # type: ignore[assignment] |
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.
unnecessary type ignore
Description
Integrate cohere models
contributed by @NeilJohnson0930
Motivation and Context
Why is this change required? What problem does it solve?
If it fixes an open issue, please link to the issue here.
You can use the syntax `close #896'
Types of changes
What types of changes does your code introduce? Put an
x
in all the boxes that apply:Implemented Tasks
Checklist
Go over all the following points, and put an
x
in all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!