-
Notifications
You must be signed in to change notification settings - Fork 20
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
OpenAI o1 model support #43
base: main
Are you sure you want to change the base?
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.
LGTM. I cannot test o1 yet because of limited access. However, the implementation looks great.
A lint question: should we rename "map_params" to "_map_params" for readability?
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 'num_tokens' change and test updates are good to have.
…ms to _map_params
@marklysze could you check the build error? It indicates some problem with the token counting. |
This should be related to the change in count_token function. |
All good, committing correction shortly Update: committed |
@qingyun-wu could you make sure there's an o1 endpoint in the config list used by the openai CI? |
Why are these changes needed?
OpenAI has just released o1-mini and o1-preview, these changes add support for the model.
I don't have level 5 API access to test against the API, if anyone can test with the models that would be great.
Notes:
max_tokens
andmax_completion_tokens
should be used. Code mapsmax_tokens
tomax_completion_tokens
.Tests still TODO.
Related issue number
Checks