Add support for O3-mini #930
Conversation
rorymalcolm
commented
Jan 31, 2025
rorymalcolm
commented
- Add support for the o3 mini set of models, including tests that match the constraints in OpenAI's API docs (https://platform.openai.com/docs/models#o3-mini).
- Add support for the o3 mini set of models, including tests that match the constraints in OpenAI's API docs (https://platform.openai.com/docs/models#o3-mini).
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #930 +/- ##
==========================================
+ Coverage 98.46% 98.87% +0.41%
==========================================
Files 24 27 +3
Lines 1364 1779 +415
==========================================
+ Hits 1343 1759 +416
+ Misses 15 14 -1
Partials 6 6 ☔ View full report in Codecov by Sentry. |
|
yes please! |
rorymalcolm
force-pushed
the
rorymalcolm/add-support-for-o3-mini
branch
5 times, most recently
from
9978505 to
25581e5
Compare
- Deprecate `ErrO1BetaLimitationsLogprobs` and `ErrO1BetaLimitationsOther` - Implement `validationRequestForReasoningModels`, which works on both o1 & o3, and has per-model-type restrictions on functionality (eg, o3 class are allowed function calls and system messages, o1 isn't)
rorymalcolm
force-pushed
the
rorymalcolm/add-support-for-o3-mini
branch
from
25581e5 to
1ad88a3
Compare
|
Thanks for that. @rorymalcolm I observed the full O1 model hasn't been added in the lib. Can you add it as well along with O3 ? I would appreciate it a lot. Thanks |
chat_test.go
Outdated
| @@ -64,15 +64,15 @@ func TestO1ModelsChatCompletionsDeprecatedFields(t *testing.T) { | |||
| MaxTokens: 5, | |||
| Model: openai.O1Preview, | |||
| }, | |||
| expectedError: openai.ErrO1MaxTokensDeprecated, | |||
| expectedError: openai.ErrO3MaxTokensDeprecated, | |||
There was a problem hiding this comment.
So this is a bit weird — we're requesting an o1 model but are getting an o3 error instead. I suggest we name these errors more generic like use ReasoningModel instead of O3 — this would future-proof us for o4, o5, etc.
There was a problem hiding this comment.
From what I know they're intending to merge the models in the near future, the normal and reasoning capabilities will be under the same model. Most likely we'll specify to use reasoning with an option param in the API.
Just fyi, if it changes anything about the implementation.
completion.go
Outdated
| var O3SeriesModels = map[string]struct{}{ | ||
| O3Mini: {}, | ||
| O3Mini20250131: {}, | ||
| } |
There was a problem hiding this comment.
We'll need to bookkeep this map forever and ever, could we use the strings.HasPrefix("o3-") instead?
completion.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func validateBasicParams(request ChatCompletionRequest) error { |
There was a problem hiding this comment.
The method name is pretty generic, let's make it's name clearly indicate that it is used only for reasoning models. Maybe even put all the logic related to reasoning models into a separate file (struct?)
|
Thank you for the updates! I left a few minor comments, but it generally looks good 🙌🏻 Ah, we also miss a test case in |
Now done |
rorymalcolm
force-pushed
the
rorymalcolm/add-support-for-o3-mini
branch
3 times, most recently
from
7424d67 to
ef7a4de
Compare
- Add a `NewReasoningValidator` which exposes a `Validate()` method for a given request - Also adds a test for chat streams
rorymalcolm
force-pushed
the
rorymalcolm/add-support-for-o3-mini
branch
from
ef7a4de to
e61a78d
Compare
|
@sashabaranov is this merge ready? i need |
rorymalcolm
force-pushed
the
rorymalcolm/add-support-for-o3-mini
branch
from
495c905 to
0c88b1c
Compare
