-
Notifications
You must be signed in to change notification settings - Fork 660
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
Integrate Open-source Models #245
Conversation
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 is an awesome change and we can invoke any model using openAI API after merging it!
There are some suggestions about code structure.
However, I think the biggest problem is that I should set up a entirely new environment to launch a LLM server in my computer to use open source models, which is diffcult and time wasting. If I am a user who want to use a opensource model in camel, it is possibly because I don't have an openai key but I want to try camel for free. These complicated set up processes may prevent me from trying camel. We should either provide detailed instructions of how to set up a llm server or using a wrapper to do that in the code.
Anyway, this PR is very valuable for invoking other models, thank you so much!
camel/models/model_factory.py
Outdated
def create( | ||
model_type: ModelType, | ||
model_config_dict: Dict, | ||
model_path: Optional[str] = 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.
Is it possible to not contain model_path
parameter in this? I think it is only used to create the tokenizer. So can we assign it inside the factory by model_type
instead of passing it in?
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.
The thing is the current model_type
has only general information of the model but without the specific model name/path (e.g. lm-sys/vicuna-7b-v1.5
).
I might get what you mean by modifying ModelType
to be a data class? Should we implement another class called like ModelCard
, which has a field being ModelType
in limited range and can have other information like model path?
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.
todo
Thanks for these valuable suggestions! Yeah I totally agree with the problem for the inconvenience launching the server, which I have discussed with Guohao. I am now trying to find a way to automatically launching a |
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.
Great feature you are adding! Thanks! Let's polish the code.
camel/models/open_source_model.py
Outdated
class OpenSourceModel(BaseModelBackend): | ||
r"""OpenAI API in a unified BaseModelBackend interface.""" | ||
|
||
def __init__(self, model_type: ModelType, model_config_dict: Dict[str, |
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 need a test for this newly introduced class.
camel/utils/token_counting.py
Outdated
return num_tokens | ||
|
||
|
||
class TokenCounterFactory: |
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 need a test for TokenCounterFactory with all possible models types.
Have just modified the current implementation but has not completed the documentation and tests. Will do in tomorrow morning as it is quite late (UTC+8) |
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.
Awesome! Left some comments.
camel/models/model_factory.py
Outdated
def create( | ||
model_type: ModelType, | ||
model_config_dict: Dict, | ||
model_path: Optional[str] = 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.
todo
Hi @Obs01ete @lightaime I write the sample code following Tianqi's suggestion and am wondering whether such an implementation of |
@MorphlingEd Why do we need a factory for configs? Can't we make model backends in the factory and that's it? |
Yeah that's a illed design shown here using a redundant factory. Later I tried to introduce a unified ...
if model_type is ModelType.GPT_4:
model_config = ModelConfig(
...
token_limit = 8192,
...
)
return OpenAIModel(model_config, ...)
elif model_type is ModelType.GPT_3_5_TURBO:
model_config = ModelConfig(
...
token_limit = 4096
...
)
return OpenAIModel(model_config, ...)
... Though I could use a dictionary like In addition, by introducing the new |
I just found there are some authentication problems due to the HuggingFace repo for LLaMA2 is gated. If we do not authenticate by the following commands in the check environment like below: export HUGGINGFACE_TOKEN="<huggingface_token>"
huggingface-cli login --token $HUGGINGFACE_TOKEN when using HuggingFace's |
Sorry just closed the branch by mistake |
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.
Great tests in test_open_source_model.py. 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.
Good progress. Left comments.
@Obs01ete I have reverted the python version back to 3.8. But I failed trying to implement an equivalent functionality to |
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 see, now all comments are resolved, and the code looks nice. Approved.
LGTM. Only the config part is a little redundant now. We can consider refactoring it later. Anyway, good job! Feel free to merge it. |
Yeah, this does not look natural... Okay, let's leave it like this and rework later. Go ahead with merging this PR. |
Could you wait for my final check a bit?
…On Thu 24. Aug 2023 at 14:08, Dmitrii Khizbullin ***@***.***> wrote:
I changed the OpenSourceConfig to be composed of an instance of
ChatGPTConfig
Yeah, this does not look natural... Okay, let's leave it like this and
rework later. Go ahead with merging this PR.
—
Reply to this email directly, view it on GitHub
<#245 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFUJTYHKLVWV6CYEHSIGHXDXW475VANCNFSM6AAAAAA3MPOFIA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes of course :D |
@lightaime Hi Guohao, could I ask whether there is still something to be improved further? :) |
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.
Look great!! Thanks @MorphlingEd. Left some small comments. Please free feel to merge it.
@CodiumAI-Agent /review |
PR Analysis
PR Feedback
How to use
|
PR Analysis
PR Feedback
How to use
|
Description
ModelType
elements defined.http://localhost:8000/v1
model_path
forBaseModelBackend
class, though this should only be applicable for open-source models. It can either be a model id in HuggingFace such aslmsys/vicuna-13b-v1.5-16k
or a path to the local folder containing the tokenizer (necessary) and models.OPENAI_API_BASE
, which will be set to alteropenai.api_base
.utils/token_counting.py
and equips each model object with a specific token counter.BaseMessage.token_len
as this function seems not useful. Counting number of tokens cannot be decoupled from the specific model and especially the open-source models need to load specific tokenizers. Each time of counting tokens here needs to load the tokenizer, which is quite costly. We can just count the tokens after models are defined with their token counters.The functionality structure may need to be further optimized but Guohao told me to create this PR for more convenient reviewing.
Motivation and Context
Why is this change required? What problem does it solve?
close #225
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!