Chore: Add openai o1 support #366

StrongMonkey · 2025-01-23T22:46:11Z

Add openai o1 support in proxy to modify request body.

thedadams · 2025-01-24T11:27:36Z

openai-model-provider/proxy/proxy.go

@@ -96,6 +101,41 @@ func (s *server) proxyDirector(req *http.Request) {
 	if s.cfg.PathPrefix != "" && !strings.HasPrefix(req.URL.Path, s.cfg.PathPrefix) {
 		req.URL.Path = s.cfg.PathPrefix + req.URL.Path
 	}
+
+	if req.Body == nil || req.Method != http.MethodPost {


Does it make sense to do this?

Suggested change

if req.Body == nil || req.Method != http.MethodPost {

if req.Body == nil || s.cfg.UpstreamHost != "api.openai.com" || req.URL.Path != "/v1/chat/completion" {

I like this, because it means that we can look for errors when we try to unmarshal the body later.

yep, I will change this.

StrongMonkey · 2025-01-24T23:09:57Z

@thedadams Pushed a change that make the response compatiable with stream client in gptscript.

StrongMonkey · 2025-01-24T23:10:58Z

openai-model-provider/proxy/proxy.go

+	return nil
+}
+
+func isModelO1(model string) bool {


The current PR only supports o1, as o1-mini doesn't seem to support system message which could screw a lot of things. We could investigate in a different issue.

thedadams · 2025-01-29T12:03:10Z

openai-model-provider/proxy/proxy.go

+}
+
+func (s *server) modifyResponse(resp *http.Response) error {
+	if resp.StatusCode != http.StatusOK || resp.Body == nil || resp.Request.URL.Path != chatCompletionsPath || resp.Request.URL.Host != openaiBaseHostName {


nit: resp.Body is always non-nil.

Suggested change

if resp.StatusCode != http.StatusOK || resp.Body == nil || resp.Request.URL.Path != chatCompletionsPath || resp.Request.URL.Host != openaiBaseHostName {

if resp.StatusCode != http.StatusOK || resp.Request.URL.Path != chatCompletionsPath || resp.Request.URL.Host != openaiBaseHostName {

thedadams · 2025-01-29T12:04:21Z

openai-model-provider/proxy/proxy.go

+		rawBody, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return fmt.Errorf("failed to read response body: %w", err)
+		}
+		if err := json.Unmarshal(rawBody, &respBody); err == nil && isModelO1(respBody.Model) {


So we only have to process the body once.

Suggested change

rawBody, err := io.ReadAll(resp.Body)

if err != nil {

return fmt.Errorf("failed to read response body: %w", err)

}

if err := json.Unmarshal(rawBody, &respBody); err == nil && isModelO1(respBody.Model) {

if err := json.Decode(resp.Body).Decode(&respBody); err == nil && isModelO1(respBody.Model) {

It needs to save the original response so that in case it is not a O1 model, restore the resp.Body to the original data.

thedadams · 2025-01-29T12:06:57Z

openai-model-provider/proxy/proxy.go

+	}
+
+	if resp.Header.Get("Content-Type") == "application/json" {
+		var respBody openai.ChatCompletionResponse


Have to close the body if we are reading and replacing it. I think you need to save the original body in a variable so you don't close the new body.

Suggested change

var respBody openai.ChatCompletionResponse

originalBody := resp.Body

defer originalBody.Close()

var respBody openai.ChatCompletionResponse

yeah, probably better to just close the original body without using defer

thedadams · 2025-01-29T12:07:59Z

openai-model-provider/proxy/proxy.go

+				Choices: func() []openai.ChatCompletionStreamChoice {
+					var choices []openai.ChatCompletionStreamChoice
+					for _, choice := range respBody.Choices {
+						choices = append(choices, openai.ChatCompletionStreamChoice{
+							Index: choice.Index,
+							Delta: openai.ChatCompletionStreamChoiceDelta{
+								Content:      choice.Message.Content,
+								Role:         choice.Message.Role,
+								FunctionCall: choice.Message.FunctionCall,
+								ToolCalls:    choice.Message.ToolCalls,
+							},
+							FinishReason: choice.FinishReason,
+						})
+					}
+					return choices
+				}(),


Whoa! I've never seen this before. I think I like it.

thedadams · 2025-01-29T12:09:50Z

openai-model-provider/proxy/proxy.go

@@ -63,7 +74,8 @@ func Run(cfg *Config) error {
 		ModifyResponse: cfg.RewriteModelsFn,
 	})
 	mux.Handle("/v1/", &httputil.ReverseProxy{
-		Director: s.proxyDirector,
+		Director:       s.proxyDirector,
+		ModifyResponse: s.modifyResponse,


I think it would be good if we could find a way to not put this on every model provider that uses this proxy code, and only put it on the OpenAI model provider.

yes, it would need us to put the common code into a seperate folder/repo with its own go modules. Right now if I can update dep for openai-model-provider it pulls other tools in.

wait, actually, yeah I didn't realize the proxy code is shared. Let me think..

ok, thorsten's custom path comes right on time. This switches to use that.

Signed-off-by: Daishan Peng <daishan@acorn.io>

StrongMonkey · 2025-01-29T17:44:26Z

openai-model-provider/proxy/proxy.go

 	// Register default handlers only if they are not already registered
 	if _, exists := cfg.CustomPathHandleFuncs["/{$}"]; !exists {
 		mux.HandleFunc("/{$}", s.healthz)
 	}
-	if _, exists := cfg.CustomPathHandleFuncs["/v1/models"]; !exists {
+	if handler, exists := cfg.CustomPathHandleFuncs["/v1/models"]; !exists {


@iwilltry42 can you review this?

LGTM!
I don't like all those if-else statements, but have no cleaner solution right now (unfortunately, mux doesn't take care of the path sorting and neither allows overrides nor looking up what paths are already handled.

thedadams · 2025-01-30T17:45:57Z

openai-model-provider/proxy/proxy.go

 type Config struct {
-	url *url.URL
+	Url *url.URL


nit:

Suggested change

Url *url.URL

URL *url.URL

Signed-off-by: Daishan Peng <daishan@acorn.io>

pixeye33 · 2025-01-31T10:13:35Z

Hello,

since this PR has been opened #378 has been merged.

while not being certain it needs to, but should generic-openai-model-provider also be changed ?

iwilltry42 · 2025-01-31T10:47:02Z

@pixeye33 that would only make sense, if one were to use the generic provider to connect to OpenAI directly instead of using the OpenAI provider, right?
Or is there an o1 variant with the same limitations that you can run in e.g. Ollama?

Now that I think about it - it may be needed for Azure OpenAI which has access to o1 as well.

pixeye33 · 2025-01-31T12:09:35Z

For this PR, at least Ollama and generic-openai-model-provider should be identical (or even be just one).

But, for my specific case : i'm using https://www.litellm.ai/ in front of groq/OpenAI for all the AI tools i'm playing with, and i'd like to use generic-openai-model-provider to use it for obot too (Same logic applies when using https://openrouter.ai/).

However, i'm not even sure that a o1 specific fix is necessary/done on litellm : i have not tried yet.

StrongMonkey requested review from thedadams, njhale, sanjay920, iwilltry42 and g-linville January 23, 2025 22:46

StrongMonkey force-pushed the openai-o1 branch 2 times, most recently from 4a4025b to f0ab964 Compare January 23, 2025 23:06

thedadams reviewed Jan 24, 2025

View reviewed changes

StrongMonkey force-pushed the openai-o1 branch 3 times, most recently from aa67dc3 to bd15449 Compare January 24, 2025 22:57

StrongMonkey commented Jan 24, 2025

View reviewed changes

StrongMonkey force-pushed the openai-o1 branch from bd15449 to 5a2759d Compare January 24, 2025 23:11

g-linville approved these changes Jan 28, 2025

View reviewed changes

StrongMonkey requested a review from thedadams January 28, 2025 17:07

thedadams requested changes Jan 29, 2025

View reviewed changes

StrongMonkey force-pushed the openai-o1 branch from 1d3f905 to 162d589 Compare January 29, 2025 17:05

StrongMonkey added 5 commits January 29, 2025 10:07

Chore: Add openai o1 support

f09a61f

Signed-off-by: Daishan Peng <daishan@acorn.io>

Update go.mod tidy

91dc14d

Signed-off-by: Daishan Peng <daishan@acorn.io>

Add proxy change to return o1 response as SSE events

ad24333

Address donnie's comment

96e4ba3

Signed-off-by: Daishan Peng <daishan@acorn.io>

Use custom path

46eee43

Signed-off-by: Daishan Peng <daishan@acorn.io>

StrongMonkey force-pushed the openai-o1 branch from 162d589 to 46eee43 Compare January 29, 2025 17:43

StrongMonkey commented Jan 29, 2025

View reviewed changes

StrongMonkey requested a review from thedadams January 29, 2025 17:44

iwilltry42 approved these changes Jan 29, 2025

View reviewed changes

thedadams approved these changes Jan 30, 2025

View reviewed changes

Address comment

27cf37c

Signed-off-by: Daishan Peng <daishan@acorn.io>

StrongMonkey merged commit 6cd817a into obot-platform:main Feb 4, 2025
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Add openai o1 support #366

Chore: Add openai o1 support #366

StrongMonkey commented Jan 23, 2025 •

edited

Loading

thedadams Jan 24, 2025

g-linville Jan 24, 2025 •

edited

Loading

StrongMonkey Jan 24, 2025

StrongMonkey commented Jan 24, 2025

StrongMonkey Jan 24, 2025

thedadams Jan 29, 2025

thedadams Jan 29, 2025

StrongMonkey Jan 29, 2025

thedadams Jan 29, 2025

StrongMonkey Jan 29, 2025

thedadams Jan 29, 2025

thedadams Jan 29, 2025

StrongMonkey Jan 29, 2025 •

edited

Loading

StrongMonkey Jan 29, 2025

StrongMonkey Jan 29, 2025

StrongMonkey Jan 29, 2025

iwilltry42 Jan 29, 2025

thedadams Jan 30, 2025

pixeye33 commented Jan 31, 2025

iwilltry42 commented Jan 31, 2025

pixeye33 commented Jan 31, 2025 •

edited

Loading

	if req.Body == nil \|\| req.Method != http.MethodPost {
	if req.Body == nil \|\| s.cfg.UpstreamHost != "api.openai.com" \|\| req.URL.Path != "/v1/chat/completion" {

	if resp.StatusCode != http.StatusOK \|\| resp.Body == nil \|\| resp.Request.URL.Path != chatCompletionsPath \|\| resp.Request.URL.Host != openaiBaseHostName {
	if resp.StatusCode != http.StatusOK \|\| resp.Request.URL.Path != chatCompletionsPath \|\| resp.Request.URL.Host != openaiBaseHostName {

Chore: Add openai o1 support #366

Chore: Add openai o1 support #366

Conversation

StrongMonkey commented Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

g-linville Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrongMonkey commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StrongMonkey Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixeye33 commented Jan 31, 2025

iwilltry42 commented Jan 31, 2025

pixeye33 commented Jan 31, 2025 • edited Loading

StrongMonkey commented Jan 23, 2025 •

edited

Loading

g-linville Jan 24, 2025 •

edited

Loading

StrongMonkey Jan 29, 2025 •

edited

Loading

pixeye33 commented Jan 31, 2025 •

edited

Loading