-
Notifications
You must be signed in to change notification settings - Fork 243
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
Add openai/gpt-3.5-turbo-0301 model #1401
Conversation
cc @JoelNiklaus |
@teetone could you take a look? Multiple people are asking for this and it would be good to get this out soon. Thanks! |
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! I wasn't sure about the +1
for max_tokens
.
"n": request.num_completions, | ||
# TODO: Setting stop causes the server to return a HTTP 500 error. | ||
# "stop": request.stop_sequences or None, # API doesn't like empty list | ||
"max_tokens": request.max_tokens + 1, # Uhhh |
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.
What is this +1
for?
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.
Removing the +1 for now.
The special roles take up tokens, so if you want 1 token, you actually have to ask 2 tokens. But I'd rather handle this elsewhere.
@@ -79,6 +79,7 @@ def get_client(self, request: Request) -> Client: | |||
client = OpenAIClient( | |||
api_key=self.credentials["openaiApiKey"], | |||
cache_config=cache_config, | |||
tokenizer_client=self.get_tokenizer_client("huggingface"), | |||
chat_gpt_client=chat_gpt_client, |
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.
Do you mind removing chat_gpt_client
and deleting ChatGPTClient
? I don't think we need it now.
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.
OK, will do this in a new PR.
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.
Filed #1447 to track removing ChatGPTClient
.
Add window service Comments Fixes More fixes Review fixes
48d91a9
to
4113d59
Compare
Addresses #1218