ronantakizawa/codereview-bench huggingface dataset:
Repository: ollama/ollama
File: server/routes.go (Go)

Code before:
					EvalCount:          r.EvalCount,
					EvalDuration:       r.EvalDuration,
				},
			}

			if r.Done {
				res.TotalDuration = time.Since(checkpointStart)
				res.LoadDuration = checkpointLoaded.Sub(checkpointStart)
			}

			// TODO: tool call checking and filtering should be moved outside of this callback once streaming
			// however this was a simple change for now without reworking streaming logic of this (and other)
			// handlers
			if req.Stream != nil && !*req.Stream || len(req.Tools) == 0 {
				ch <- res
				return
			}

			// Streaming tool calls:
			// If tools are recognized, use a flag to track the sending of a tool downstream
			// This ensures that content is cleared from the message on the last chunk sent
			sb.WriteString(r.Content)
			if toolCalls, ok := m.parseToolCalls(sb.String()); ok {
				res.Message.ToolCalls = toolCalls
				for i := range toolCalls {
					toolCallIndex++
					toolCalls[i].Function.Index = toolCallIndex
				}
				res.Message.Content = ""
				sb.Reset()
				ch <- res
				return
			}

			if r.Done {
				// Send any remaining content if no tool calls were detected
				if toolCallIndex == -1 {
					res.Message.Content = sb.String()
				}
				ch <- res
			}
		}); err != nil {
			ch <- gin.H{"error": err.Error()}
		}
	}()

	if req.Stream != nil && !*req.Stream {
		var resp api.ChatResponse
		var sb strings.Builder
		for rr := range ch {
			switch t := rr.(type) {

Review comment (suggestion):
```suggestion
					toolCalls[i].Function.Index = toolCallIndex
					toolCallIndex++
```

Code after:
					EvalCount:          r.EvalCount,
					EvalDuration:       r.EvalDuration,
				},
			}

			if r.Done {
				res.TotalDuration = time.Since(checkpointStart)
				res.LoadDuration = checkpointLoaded.Sub(checkpointStart)
			}

			// TODO: tool call checking and filtering should be moved outside of this callback once streaming
			// however this was a simple change for now without reworking streaming logic of this (and other)
			// handlers
			if req.Stream != nil && !*req.Stream || len(req.Tools) == 0 {
				ch <- res
				return
			}

			// Streaming tool calls:
			// If tools are recognized, use a flag to track the sending of a tool downstream
			// This ensures that content is cleared from the message on the last chunk sent
			sb.WriteString(r.Content)
			if toolCalls, ok := m.parseToolCalls(sb.String()); ok {
				res.Message.ToolCalls = toolCalls
				for i := range toolCalls {
					toolCalls[i].Function.Index = toolCallIndex
					toolCallIndex++
				}
				res.Message.Content = ""
				sb.Reset()
				ch <- res
				return
			}

			if r.Done {
				// Send any remaining content if no tool calls were detected
				if toolCallIndex == 0 {
					res.Message.Content = sb.String()
				}
				ch <- res
			}
		}); err != nil {
			ch <- gin.H{"error": err.Error()}
		}
	}()

	if req.Stream != nil && !*req.Stream {
		var resp api.ChatResponse
		var sb strings.Builder
		for rr := range ch {
			switch t := rr.(type) {
Repository: ollama/ollama
File: api/types.go (Go)

Code before:
	// following the request.
	KeepAlive *Duration `json:"keep_alive,omitempty"`

	// Tools is an optional list of tools the model has access to.
	Tools `json:"tools,omitempty"`

	// Options lists model-specific options.
	Options map[string]interface{} `json:"options"`
}

// GetOutputFormat returns either a string format or a JSON schema format.
// Returns (formatStr, nil) for string formats or (nil, schema) for JSON formats.
func (r *ChatRequest) GetOutputFormat() (string, map[string]interface{}) {
	if r.Format == nil {
		return "", nil
	}

	switch f := r.Format.(type) {
	case string:
		return f, nil
	case map[string]interface{}:
		return "", f
	default:
		slog.Warn("unknown format type", "type", fmt.Sprintf("%T", f))
		return "", nil
	}
}

type Tools []Tool

func (t Tools) String() string {
	bts, _ := json.Marshal(t)
	return string(bts)
}

func (t Tool) String() string {
	bts, _ := json.Marshal(t)
	return string(bts)
}

// Message is a single message in a chat sequence. The message contains the
// role ("system", "user", or "assistant"), the content and an optional list
// of images.
type Message struct {
	Role      string      `json:"role"`
	Content   string      `json:"content"`
	Images    []ImageData `json:"images,omitempty"`
	ToolCalls []ToolCall  `json:"tool_calls,omitempty"`
}

func (m *Message) UnmarshalJSON(b []byte) error {

Review comment (suggestion):
With `json.RawMessage` you should be able to do something like this:

```go
	var schema struct {
		Type       string         `json:"type"`
		Properties map[string]any `json:"properties"`
	}
	if json.Unmarshal(req.Format, &schema); err == nil {
		request["grammar"] = jsonSchemaToGrammar(schema)
	} else if strings.TrimSpace(strings.ToLower(string(req.Format))) == "json" {
		request["grammar"] = jsonGrammar
	} else {
		// handle error
	}
```

Code after:
	// following the request.
	KeepAlive *Duration `json:"keep_alive,omitempty"`

	// Tools is an optional list of tools the model has access to.
	Tools `json:"tools,omitempty"`

	// Options lists model-specific options.
	Options map[string]interface{} `json:"options"`
}

type Tools []Tool

func (t Tools) String() string {
	bts, _ := json.Marshal(t)
	return string(bts)
}

func (t Tool) String() string {
	bts, _ := json.Marshal(t)
	return string(bts)
}

// Message is a single message in a chat sequence. The message contains the
// role ("system", "user", or "assistant"), the content and an optional list
// of images.
type Message struct {
	Role      string      `json:"role"`
	Content   string      `json:"content"`
	Images    []ImageData `json:"images,omitempty"`
	ToolCalls []ToolCall  `json:"tool_calls,omitempty"`
}

func (m *Message) UnmarshalJSON(b []byte) error {
	type Alias Message
	var a Alias
	if err := json.Unmarshal(b, &a); err != nil {
		return err
	}

	*m = Message(a)
	m.Role = strings.ToLower(m.Role)
	return nil
}

type ToolCall struct {
	Function ToolCallFunction `json:"function"`
}

type ToolCallFunction struct {
	Index     int                       `json:"index,omitempty"`
	Name      string                    `json:"name"`
Repository: ollama/ollama
File: llama/llama.go (Go)

Code before:
	Defs       map[string]any `json:"$defs,omitempty"`
	Properties map[string]any `json:"properties,omitempty"`
	Required   []string       `json:"required,omitempty"`
	Title      string         `json:"title,omitempty"`
	Type       string         `json:"type,omitempty"`
}

func (js JsonSchema) AsGrammar() string {
	// Convert the JSON schema to a string representation
	jsonBytes, err := json.Marshal(js)
	if err != nil {
		return ""
	}

	cStr := C.CString(string(jsonBytes))
	defer C.free(unsafe.Pointer(cStr))

	// Allocate buffer for grammar output with reasonable size
	const maxLen = 32768 // 32KB
	buf := make([]byte, maxLen)

	// Call C function to convert schema to grammar
	C.schema_to_grammar(cStr, (*C.char)(unsafe.Pointer(&buf[0])), C.int32_t(maxLen))

	// Convert from C string to Go string - needs to be null-terminated
	return C.GoString((*C.char)(unsafe.Pointer(&buf[0])))
}


Review comment (suggestion):
You should call `C.GoBytes` instead which takes an unsafe.Pointer and length. If `schema_to_grammar` returns the number of bytes written, it can cleanly return the byte slice

Code after:

type JsonSchema struct {
	Defs       map[string]any `json:"$defs,omitempty"`
	Properties map[string]any `json:"properties,omitempty"`
	Required   []string       `json:"required,omitempty"`
	Title      string         `json:"title,omitempty"`
	Type       string         `json:"type,omitempty"`
}

func (js JsonSchema) AsGrammar() string {
	var b bytes.Buffer
	if err := json.NewEncoder(&b).Encode(js); err != nil {
		return ""
	}

	cStr := C.CString(b.String())
	defer C.free(unsafe.Pointer(cStr))

	// Allocate buffer for grammar output with reasonable size
	const maxLen = 32768 // 32KB
	buf := make([]byte, maxLen)

	// Call C function to convert schema to grammar
	length := C.schema_to_grammar(cStr, (*C.char)(unsafe.Pointer(&buf[0])), C.size_t(maxLen))
	if length == 0 {
		slog.Warn("unable to convert schema to grammar")
	}

	return string(buf[:length])
}
Repository: ollama/ollama
File: llama/llama.go (Go)

Code before:
	Title      string         `json:"title,omitempty"`
	Type       string         `json:"type,omitempty"`
}

func (js JsonSchema) AsGrammar() string {
	// Convert the JSON schema to a string representation
	jsonBytes, err := json.Marshal(js)
	if err != nil {
		return ""
	}

	cStr := C.CString(string(jsonBytes))
	defer C.free(unsafe.Pointer(cStr))

	// Allocate buffer for grammar output with reasonable size
	const maxLen = 32768 // 32KB
	buf := make([]byte, maxLen)

	// Call C function to convert schema to grammar
	length := C.schema_to_grammar(cStr, (*C.char)(unsafe.Pointer(&buf[0])), C.int32_t(maxLen))
	if length <= 0 {
		slog.Warn("unable to convert schema to grammar")
		return ""
	}

	return string(C.GoBytes(unsafe.Pointer(&buf[0]), length))
}


Review comment (suggestion):
This unnecessary since `buf` is `[]byte`

```suggestion
	return string(buf[:length])
```

Code after:
	Required   []string       `json:"required,omitempty"`
	Title      string         `json:"title,omitempty"`
	Type       string         `json:"type,omitempty"`
}

func (js JsonSchema) AsGrammar() string {
	var b bytes.Buffer
	if err := json.NewEncoder(&b).Encode(js); err != nil {
		return ""
	}

	cStr := C.CString(b.String())
	defer C.free(unsafe.Pointer(cStr))

	// Allocate buffer for grammar output with reasonable size
	const maxLen = 32768 // 32KB
	buf := make([]byte, maxLen)

	// Call C function to convert schema to grammar
	length := C.schema_to_grammar(cStr, (*C.char)(unsafe.Pointer(&buf[0])), C.size_t(maxLen))
	if length == 0 {
		slog.Warn("unable to convert schema to grammar")
	}

	return string(buf[:length])
}
Repository: ollama/ollama
File: openai/openai_test.go (Go)

Code before:

	gin.SetMode(gin.TestMode)
	router := gin.New()
	router.Use(ChatMiddleware(), captureRequestMiddleware(&capturedRequest))
	router.Handle(http.MethodPost, "/api/chat", endpoint)

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			req, _ := http.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(tc.body))
			req.Header.Set("Content-Type", "application/json")

			defer func() { capturedRequest = nil }()

			resp := httptest.NewRecorder()
			router.ServeHTTP(resp, req)

			var errResp ErrorResponse
			if resp.Code != http.StatusOK {
				if err := json.Unmarshal(resp.Body.Bytes(), &errResp); err != nil {
					t.Fatal(err)
				}
			}
			if capturedRequest != nil && !reflect.DeepEqual(tc.req, *capturedRequest) {
				t.Logf("  diff: %+v", cmp.Diff(tc.req, *capturedRequest))
				t.Fatal("requests did not match")
			}

			if !reflect.DeepEqual(tc.err, errResp) {
				t.Logf("tc.name: %s", tc.name)
				t.Logf("  diff: %+v", cmp.Diff(tc.err, errResp))
				t.Fatal("errors did not match")
			}
		})
	}
}

func TestCompletionsMiddleware(t *testing.T) {
	type testCase struct {
		name string
		body string
		req  api.GenerateRequest
		err  ErrorResponse
	}

	var capturedRequest *api.GenerateRequest

	testCases := []testCase{
		{
			name: "completions handler",
			body: `{
				"model": "test-model",

Review comment (suggestion):
nit: this can be simplified
```suggestion
			if diff := cmp.Diff(tc.req, *capturedRequest); diff != "" {
				t.Fatalf("requests did not match: %+v", diff)
			}
```

Code after:

	gin.SetMode(gin.TestMode)
	router := gin.New()
	router.Use(ChatMiddleware(), captureRequestMiddleware(&capturedRequest))
	router.Handle(http.MethodPost, "/api/chat", endpoint)

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			req, _ := http.NewRequest(http.MethodPost, "/api/chat", strings.NewReader(tc.body))
			req.Header.Set("Content-Type", "application/json")

			defer func() { capturedRequest = nil }()

			resp := httptest.NewRecorder()
			router.ServeHTTP(resp, req)

			var errResp ErrorResponse
			if resp.Code != http.StatusOK {
				if err := json.Unmarshal(resp.Body.Bytes(), &errResp); err != nil {
					t.Fatal(err)
				}
				return
			}
			if diff := cmp.Diff(&tc.req, capturedRequest); diff != "" {
				t.Fatalf("requests did not match: %+v", diff)
			}
			if diff := cmp.Diff(tc.err, errResp); diff != "" {
				t.Fatalf("errors did not match for %s:\n%s", tc.name, diff)
			}
		})
	}
}

func TestCompletionsMiddleware(t *testing.T) {
	type testCase struct {
		name string
		body string
		req  api.GenerateRequest
		err  ErrorResponse
	}

	var capturedRequest *api.GenerateRequest

	testCases := []testCase{
		{
			name: "completions handler",
			body: `{
				"model": "test-model",
				"prompt": "Hello",
				"temperature": 0.8,
				"stop": ["\n", "stop"],
Repository: ollama/ollama
File: openai/openai.go (Go)

Code before:
	}

	w.ResponseWriter.Header().Set("Content-Type", "application/json")
	err = json.NewEncoder(w.ResponseWriter).Encode(NewError(http.StatusInternalServerError, serr.Error()))
	if err != nil {
		return 0, err
	}

	return len(data), nil
}

func (w *ChatWriter) writeResponse(data []byte) (int, error) {
	var chatResponse api.ChatResponse
	err := json.Unmarshal(data, &chatResponse)
	if err != nil {
		return 0, err
	}

	// chat chunk
	if w.stream {
		c := toChunk(w.id, chatResponse, w.toolCallsSent)
		d, err := json.Marshal(c)
		if err != nil {
			return 0, err
		}
		if !w.toolCallsSent && len(c.Choices) > 0 && len(c.Choices[0].Delta.ToolCalls) > 0 {
			w.toolCallsSent = true
		}

		w.ResponseWriter.Header().Set("Content-Type", "text/event-stream")
		_, err = w.ResponseWriter.Write([]byte(fmt.Sprintf("data: %s\n\n", d)))
		if err != nil {
			return 0, err
		}

		if chatResponse.Done {
			if w.streamOptions != nil && w.streamOptions.IncludeUsage {
				u := toUsage(chatResponse)
				c.Usage = &u
				c.Choices = []ChunkChoice{}
				d, err := json.Marshal(c)
				if err != nil {
					return 0, err
				}
				_, err = w.ResponseWriter.Write([]byte(fmt.Sprintf("data: %s\n\n", d)))
				if err != nil {
					return 0, err
				}
			}
			_, err = w.ResponseWriter.Write([]byte("data: [DONE]\n\n"))
			if err != nil {

Review comment (suggestion):
I'm going to think about how to consolidate this behavior with the `toChatCompletion` since the signatures change by introducing this. Sorry for the wait

Code after:
	}

	w.ResponseWriter.Header().Set("Content-Type", "application/json")
	err = json.NewEncoder(w.ResponseWriter).Encode(NewError(http.StatusInternalServerError, serr.Error()))
	if err != nil {
		return 0, err
	}

	return len(data), nil
}

func (w *ChatWriter) writeResponse(data []byte) (int, error) {
	var chatResponse api.ChatResponse
	err := json.Unmarshal(data, &chatResponse)
	if err != nil {
		return 0, err
	}

	// chat chunk
	if w.stream {
		c := toChunk(w.id, chatResponse, w.toolCallSent)
		d, err := json.Marshal(c)
		if err != nil {
			return 0, err
		}
		if !w.toolCallSent && len(c.Choices) > 0 && len(c.Choices[0].Delta.ToolCalls) > 0 {
			w.toolCallSent = true
		}

		w.ResponseWriter.Header().Set("Content-Type", "text/event-stream")
		_, err = w.ResponseWriter.Write([]byte(fmt.Sprintf("data: %s\n\n", d)))
		if err != nil {
			return 0, err
		}

		if chatResponse.Done {
			if w.streamOptions != nil && w.streamOptions.IncludeUsage {
				u := toUsage(chatResponse)
				c.Usage = &u
				c.Choices = []ChunkChoice{}
				d, err := json.Marshal(c)
				if err != nil {
					return 0, err
				}
				_, err = w.ResponseWriter.Write([]byte(fmt.Sprintf("data: %s\n\n", d)))
				if err != nil {
					return 0, err
				}
			}
			_, err = w.ResponseWriter.Write([]byte("data: [DONE]\n\n"))
			if err != nil {
Repository: ollama/ollama
File: server/create.go (Go)

Code before:
		var allLayers []*layerGGML
		for _, v := range files {
			digest = v
			layers, err := ggufLayers(digest, fn)
			if err != nil {
				return nil, err
			}
			allLayers = append(allLayers, layers...)
		}
		return allLayers, nil
	default:
		return nil, errUnknownType
	}
}

func detectModelTypeFromFiles(files map[string]string) string {
	for fn := range files {
		if strings.HasSuffix(fn, ".safetensors") {
			return "safetensors"
		} else if strings.HasSuffix(fn, ".gguf") {
			return "gguf"
		} else {
			// try to see if we can find a gguf file even without the file extension
			blobPath, err := GetBlobsPath(files[fn])
			if err != nil {
				slog.Error("error gettings blobs path")
				return ""
			}

			f, err := os.Open(blobPath)
			if err != nil {
				slog.Error("error reading file", "error", err)
				return ""
			}
			buf := make([]byte, 4)
			_, err = f.Read(buf)
			if err != nil {
				slog.Error("error reading file", "error", err)
				return ""
			}

			ct := llm.DetectGGMLType(buf)
			if ct == "gguf" {
				return "gguf"
			}
		}
	}

	return ""
}


Review comment (bug):
Should we print the error here? 
```suggestion
				slog.Error("failed to get blobs path", "error", err)
```

Code after:
		var allLayers []*layerGGML
		for _, v := range files {
			digest = v
			layers, err := ggufLayers(digest, fn)
			if err != nil {
				return nil, err
			}
			allLayers = append(allLayers, layers...)
		}
		return allLayers, nil
	default:
		return nil, errUnknownType
	}
}

func detectModelTypeFromFiles(files map[string]string) string {
	for fn := range files {
		if strings.HasSuffix(fn, ".safetensors") {
			return "safetensors"
		} else if strings.HasSuffix(fn, ".gguf") {
			return "gguf"
		} else {
			// try to see if we can find a gguf file even without the file extension
			blobPath, err := GetBlobsPath(files[fn])
			if err != nil {
				slog.Error("error getting blobs path", "file", fn)
				return ""
			}

			f, err := os.Open(blobPath)
			if err != nil {
				slog.Error("error reading file", "error", err)
				return ""
			}
			defer f.Close()

			buf := make([]byte, 4)
			_, err = f.Read(buf)
			if err != nil {
				slog.Error("error reading file", "error", err)
				return ""
			}

			ct := llm.DetectGGMLType(buf)
			if ct == "gguf" {
				return "gguf"
			}
		}
	}

	return ""
Repository: ollama/ollama
File: benchmark/server_benchmark_test.go (Go)

Code before:
	}
	if _, err := client.Show(context.Background(), &api.ShowRequest{Model: getModel(b)}); err != nil {
		b.Fatalf("Model unavailable: %v", err)
	}

	return client
}

// warmup ensures the model is loaded and warmed up
func warmup(client *api.Client, model string, prompt string, b *testing.B) {
	for range 3 {
		err := client.Generate(
			context.Background(),
			&api.GenerateRequest{
				Model:   model,
				Prompt:  prompt,
				Options: map[string]interface{}{"num_predict": 50, "temperature": 0.1},
			},
			func(api.GenerateResponse) error { return nil },
		)
		if err != nil {
			b.Logf("Error during model warm-up: %v", err)
		}
	}
}

// unload forces model unloading using KeepAlive: 0 parameter
func unload(client *api.Client, model string, b *testing.B) {
	req := &api.GenerateRequest{
		Model:     model,
		KeepAlive: &api.Duration{Duration: 0},
	}
	if err := client.Generate(context.Background(), req, func(api.GenerateResponse) error { return nil }); err != nil {
		b.Logf("Unload error: %v", err)
	}
	time.Sleep(1 * time.Second)
}


Review comment (suggestion):
`mmap` may skew these numbers on macOS & CPU as the OS will transparently keep the model in memory. We may want to skip model load time in this PR until we figure it out

Code after:
		b.Fatalf("Model unavailable: %v", err)
	}

	return client
}

// warmup ensures the model is loaded and warmed up
func warmup(client *api.Client, model string, prompt string, b *testing.B) {
	for range 3 {
		err := client.Generate(
			context.Background(),
			&api.GenerateRequest{
				Model:   model,
				Prompt:  prompt,
				Options: map[string]interface{}{"num_predict": 50, "temperature": 0.1},
			},
			func(api.GenerateResponse) error { return nil },
		)
		if err != nil {
			b.Logf("Error during model warm-up: %v", err)
		}
	}
}

// unload forces model unloading using KeepAlive: 0 parameter
func unload(client *api.Client, model string, b *testing.B) {
	req := &api.GenerateRequest{
		Model:     model,
		KeepAlive: &api.Duration{Duration: 0},
	}
	if err := client.Generate(context.Background(), req, func(api.GenerateResponse) error { return nil }); err != nil {
		b.Logf("Unload error: %v", err)
	}
	time.Sleep(1 * time.Second)
}
Repository: ollama/ollama
File: benchmark/server_benchmark_test.go (Go)

Code before:
package benchmark

import (
	"context"
	"flag"
	"fmt"
	"testing"
	"time"

	"github.com/ollama/ollama/api"
)

// Command line flags
var modelFlag string

func init() {
	flag.StringVar(&modelFlag, "m", "", "Name of the model to benchmark")
	flag.Lookup("m").DefValue = "model"
}

// getModel returns the model name from flags, failing the test if not set
func getModel(b *testing.B) string {
	if modelFlag == "" {
		b.Fatal("Error: -m flag is required for benchmark tests")
	}
	return modelFlag
}

type TestCase struct {
	name      string
	prompt    string
	maxTokens int
}

// runGenerateBenchmark contains the common generate and metrics logic
func runGenerateBenchmark(b *testing.B, ctx context.Context, client *api.Client, req *api.GenerateRequest) {
	start := time.Now()
	var ttft time.Duration
	var metrics api.Metrics

	err := client.Generate(ctx, req, func(resp api.GenerateResponse) error {
		if ttft == 0 && resp.Response != "" {
			ttft = time.Since(start)
		}
		if resp.Done {
			metrics = resp.Metrics
		}

Review comment (suggestion):
```suggestion
func model(b *testing.B) string {
```

Code after:
package benchmark

import (
	"context"
	"flag"
	"fmt"
	"testing"
	"time"

	"github.com/ollama/ollama/api"
)

// Command line flags
var modelFlag string

func init() {
	flag.StringVar(&modelFlag, "m", "", "Name of the model to benchmark")
	flag.Lookup("m").DefValue = "model"
}

// modelName returns the model name from flags, failing the test if not set
func modelName(b *testing.B) string {
	if modelFlag == "" {
		b.Fatal("Error: -m flag is required for benchmark tests")
	}
	return modelFlag
}

type TestCase struct {
	name      string
	prompt    string
	maxTokens int
}

// runGenerateBenchmark contains the common generate and metrics logic
func runGenerateBenchmark(b *testing.B, ctx context.Context, client *api.Client, req *api.GenerateRequest) {
	start := time.Now()
	var ttft time.Duration
	var metrics api.Metrics

	err := client.Generate(ctx, req, func(resp api.GenerateResponse) error {
		if ttft == 0 && resp.Response != "" {
			ttft = time.Since(start)
		}
		if resp.Done {
			metrics = resp.Metrics
		}