Skip to content
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: /v1/models/{model} compatibility #5028

Merged
merged 13 commits into from
Jul 2, 2024
Merged

OpenAI: /v1/models/{model} compatibility #5028

merged 13 commits into from
Jul 2, 2024

Conversation

royjhan
Copy link
Contributor

@royjhan royjhan commented Jun 13, 2024

Adds compatibility for /v1/models/{model}

E.g
curl http://localhost:11434/v1/models/llama3

{
    "id": "llama3",
    "object": "model",
    "created": 1718141294,
    "owned_by": "library"
}

Copy link

@JerrettDavis JerrettDavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Way better approach than what I was attempting in my branch.

@royjhan royjhan changed the title OpenAI: /v1/models/{model} compatability OpenAI: /v1/models/{model} compatibility Jun 13, 2024
@royjhan royjhan changed the title OpenAI: /v1/models/{model} compatibility OpenAI: /v1/models/{model} Retrieve and Delete compatibility Jun 13, 2024
openai/openai.go Outdated
func (w *DeleteWriter) writeResponse(data []byte) (int, error) {
// delete completion
w.ResponseWriter.Header().Set("Content-Type", "application/json")
err := json.NewEncoder(w.ResponseWriter).Encode(toDeleteCompletion(w.model))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We most likely don't need toDeleteCompletion and can just provide the struct inline here

openai/openai.go Outdated Show resolved Hide resolved
server/routes.go Outdated Show resolved Hide resolved
@royjhan royjhan changed the title OpenAI: /v1/models/{model} Retrieve and Delete compatibility OpenAI: /v1/models/{model} Retrieve compatibility Jun 14, 2024
@@ -233,6 +233,24 @@ func Test_Routes(t *testing.T) {
assert.Equal(t, expectedParams, params)
},
},
{
Name: "Retrieve Model Handler OpenAI",
Copy link
Member

@jmorganca jmorganca Jun 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, however what would be even better is some tests in openai.go that test the middleware in isolation. Here's an example I had an LLM generate me for a middleware that changes formats (not the same although I think you get the idea)

package main

import (
    "net/http"
    "net/http/httptest"
    "testing"

    "github.com/gin-gonic/gin"
)

func TestFormatMiddleware(t *testing.T) {
    // Create a test router
    r := gin.New()
    r.Use(FormatMiddleware())
    r.GET("/test", func(c *gin.Context) {
        format := c.GetString("format")
        if format == "json" {
            c.JSON(http.StatusOK, gin.H{"message": "Success"})
        } else if format == "xml" {
            c.XML(http.StatusOK, gin.H{"message": "Success"})
        } else {
            c.String(http.StatusOK, "Success")
        }
    })

    // Define test cases
    tests := []struct {
        name           string
        queryParam     string
        expectedStatus int
        expectedBody   string
    }{
        {"JSON Format", "json", http.StatusOK, `{"message":"Success"}`},
        {"XML Format", "xml", http.StatusOK, `<map><message>Success</message></map>`},
        {"Default Format", "", http.StatusOK, "Success"},
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            req, _ := http.NewRequest("GET", "/test?format="+tt.queryParam, nil)
            w := httptest.NewRecorder()
            r.ServeHTTP(w, req)

            if w.Code != tt.expectedStatus {
                t.Errorf("Expected status %d, got %d", tt.expectedStatus, w.Code)
            }

            if w.Body.String() != tt.expectedBody {
                t.Errorf("Expected body %s, got %s", tt.expectedBody, w.Body.String())
            }
        })
    }
}

openai/openai.go Outdated
@@ -175,7 +175,16 @@ func toListCompletion(r api.ListResponse) ListCompletion {
}
}

func fromRequest(r ChatCompletionRequest) api.ChatRequest {
func toRetrieveCompletion(r api.ShowResponse, model string) Model {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we change this to toModel?

Suggested change
func toRetrieveCompletion(r api.ShowResponse, model string) Model {
func toModel(r api.ShowResponse, model string) Model {

@royjhan royjhan changed the title OpenAI: /v1/models/{model} Retrieve compatibility OpenAI: /v1/models/{model} compatibility and v1/completions compatibility Jun 21, 2024
@royjhan royjhan changed the title OpenAI: /v1/models/{model} compatibility and v1/completions compatibility OpenAI: /v1/models/{model} compatibility Jun 22, 2024
openai/openai.go Outdated
Id: model,
Object: "model",
Created: r.ModifiedAt.Unix(),
OwnedBy: "ollama",
Copy link
Member

@jmorganca jmorganca Jun 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be the Ollama username similar to /v1/models

@@ -7,6 +7,7 @@ import (
"net/http"
"net/http/httptest"
"testing"
"time"

"github.com/gin-gonic/gin"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove Gin from this test file (in the PR this branches off of)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gin is needed to test the middleware, do you mind explaining what you mean?

t.Fatal(err)
}

assert.Equal(t, "model", retrieveResp.Object)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not use assert either – just t.Fatal t.Fatalf etc in the standard testing library is best

@@ -87,6 +88,27 @@ func TestMiddleware(t *testing.T) {
assert.Equal(t, "Test Model", listResp.Data[0].Id)
},
},
{
Name: "OpenAI Retrieve Handler",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

retrieve model

server/routes.go Outdated
@@ -660,6 +660,28 @@ func (s *Server) ShowModelHandler(c *gin.Context) {
c.JSON(http.StatusOK, resp)
}

func (s *Server) RetrieveModelHandler(c *gin.Context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used? I believe it's implemented in middleware now?

@royjhan royjhan merged commit 9bd9d39 into royh-openai Jul 2, 2024
12 checks passed
@royjhan royjhan deleted the royh-retrieve branch July 2, 2024 18:40
royjhan added a commit that referenced this pull request Jul 2, 2024
* OpenAI v1 models

* Refactor Writers

* Add Test

Co-Authored-By: Attila Kerekes

* Credit Co-Author

Co-Authored-By: Attila Kerekes <439392+keriati@users.noreply.github.com>

* Empty List Testing

* Use Namespace for Ownedby

* Update Test

* Add back envconfig

* v1/models docs

* Use ModelName Parser

* Test Names

* Remove Docs

* Clean Up

* Test name

Co-authored-by: Jeffrey Morgan <jmorganca@gmail.com>

* Add Middleware for Chat and List

* Testing Cleanup

* Test with Fatal

* Add functionality to chat test

* OpenAI: /v1/models/{model} compatibility (#5028)

* Retrieve Model

* OpenAI Delete Model

* Retrieve Middleware

* Remove Delete from Branch

* Update Test

* Middleware Test File

* Function name

* Cleanup

* Test Update

* Test Update

---------

Co-authored-by: Attila Kerekes <439392+keriati@users.noreply.github.com>
Co-authored-by: Jeffrey Morgan <jmorganca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants