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

Code before:
	return msgs
}

// handleToolParsing handles the interaction with the tool parser.
// It ensures that intermediate chunks containing logprobs are not dropped while buffering.
// Note: This function takes ownership of sending the response to the channel (ch).
// The caller should return immediately after calling this function to avoid double-sending.
func handleToolParsing(
	r llm.CompletionResponse,
	res api.ChatResponse,
	toolParser *tools.Parser,
	ch chan any,
) {
	toolCalls, content := toolParser.Add(res.Message.Content)
	if len(content) > 0 {
		res.Message.Content = content
	} else if len(toolCalls) > 0 {
		for i := range toolCalls {
			toolCalls[i].ID = toolCallId()
		}
		res.Message.ToolCalls = toolCalls
		res.Message.Content = ""
	} else if res.Message.Thinking != "" {
		// don't return, fall through to send
	} else {
		// If content is buffered by the parser but we have logprobs, send them.
		if len(res.Logprobs) > 0 && !r.Done {
			logprobRes := res
			logprobRes.Message.Content = ""
			logprobRes.Message.ToolCalls = nil
			// Create a copy of logprobs to avoid race conditions if needed,
			// though here we are just passing value/slice header.
			ch <- logprobRes
		}

		if r.Done {
			res.Message.Content = toolParser.Content()
			ch <- res
		}
		return
	}

	ch <- res
}


Review comment (suggestion):
```suggestion
		// Send logprobs while content is being buffered by the parser for tool calls
```

Code after:
	if m.Config.ModelFamily == "qwen3" || model.ParseName(m.Name).Model == "deepseek-r1" {
		finalUserIndex := -1
		for i, msg := range msgs {
			if msg.Role == "user" {
				finalUserIndex = i
			}
		}

		for i, msg := range msgs {
			if msg.Role == "assistant" && i < finalUserIndex {
				// TODO(drifkin): this is from before we added proper thinking support.
				// However, even if thinking is not enabled (and therefore we shouldn't
				// change the user output), we should probably perform this filtering
				// for all thinking models (not just qwen3 & deepseek-r1) since it tends
				// to save tokens and improve quality.
				thinkingState := &thinking.Parser{
					OpeningTag: "<think>",
					ClosingTag: "</think>",
				}
				_, content := thinkingState.AddContent(msg.Content)
				msgs[i].Content = content
			}
		}
	}
	return msgs
}
Repository: ollama/ollama
File: model/models/mistral3/model_text.go (Go)

Code before:
package mistral3

import (
	"cmp"
	"math"

	"github.com/ollama/ollama/fs"
	"github.com/ollama/ollama/kvcache"
	"github.com/ollama/ollama/ml"
	"github.com/ollama/ollama/ml/nn"
	"github.com/ollama/ollama/ml/nn/fast"
	"github.com/ollama/ollama/model/input"
)

type TextOptions struct {
	hiddenSize, numHeads, numKVHeads int
	headDim, ropeDim                 int
	eps, ropeBase, ropeScale         float32
	llama4Scaling                    bool
	ropeOrigPosEmbeddings            int
	ropeScalingBeta                  float32
}

type TextModel struct {
	TokenEmbedding *nn.Embedding `gguf:"token_embd"`
	Layers         []Layer       `gguf:"blk"`
	OutputNorm     *nn.RMSNorm   `gguf:"output_norm"`
	Output         *nn.Linear    `gguf:"output,alt:token_embd"`

	*TextOptions
}

type SelfAttention struct {
	Query  *nn.Linear `gguf:"attn_q"`
	Key    *nn.Linear `gguf:"attn_k"`
	Value  *nn.Linear `gguf:"attn_v"`
	Output *nn.Linear `gguf:"attn_output"`
}

func (sa *SelfAttention) Forward(ctx ml.Context, hiddenState, positionIDs, positionsScale ml.Tensor, cache kvcache.Cache, opts *TextOptions) ml.Tensor {
	batchSize := hiddenState.Dim(1)
	headDim := cmp.Or(opts.headDim, opts.hiddenSize/opts.numHeads)

	q := sa.Query.Forward(ctx, hiddenState)

Review comment (suggestion):
you don't really need this additional flag. if `originalMaxPositionEmbeddings` (or `scalingBeta`) is zero, `getScale` should return nil. then you simply need to check if `positionScale` is nil

Code after:
package mistral3

import (
	"cmp"
	"math"

	"github.com/ollama/ollama/fs"
	"github.com/ollama/ollama/kvcache"
	"github.com/ollama/ollama/ml"
	"github.com/ollama/ollama/ml/nn"
	"github.com/ollama/ollama/ml/nn/fast"
	"github.com/ollama/ollama/model/input"
)

type TextOptions struct {
	hiddenSize, numHeads, numKVHeads int
	headDim, ropeDim                 int
	eps, ropeBase, ropeScale         float32
	ropeOrigPosEmbeddings            int
	ropeScalingBeta                  float32
}

type TextModel struct {
	TokenEmbedding *nn.Embedding `gguf:"token_embd"`
	Layers         []Layer       `gguf:"blk"`
	OutputNorm     *nn.RMSNorm   `gguf:"output_norm"`
	Output         *nn.Linear    `gguf:"output,alt:token_embd"`

	*TextOptions
}

type SelfAttention struct {
	Query  *nn.Linear `gguf:"attn_q"`
	Key    *nn.Linear `gguf:"attn_k"`
	Value  *nn.Linear `gguf:"attn_v"`
	Output *nn.Linear `gguf:"attn_output"`
}

func (sa *SelfAttention) Forward(ctx ml.Context, hiddenState, positionIDs, positionsScale ml.Tensor, cache kvcache.Cache, opts *TextOptions) ml.Tensor {
	batchSize := hiddenState.Dim(1)
	headDim := cmp.Or(opts.headDim, opts.hiddenSize/opts.numHeads)

	q := sa.Query.Forward(ctx, hiddenState)
	q = q.Reshape(ctx, headDim, opts.numHeads, batchSize)
Repository: ollama/ollama
File: runner/ollamarunner/runner.go (Go)

Code before:
					panic(r)
				}
			} else {
				panic(r)
			}
		}
	}()

	var err error
	s.model, err = model.New(mpath, params)
	if err != nil {
		return err
	}

	// TODO(jessegross): LoRA loading
	if len(loraPath) > 0 {
		return errors.New("loras are not yet implemented")
	}

	s.cache, err = NewInputCache(s.model, kvCacheType, int32(kvSize), parallel, s.batchSize, multiUserCache)
	if err != nil {
		return err
	}

	if !s.cache.enabled {
		if parallel > 1 {
			parallel = 1
			slog.Warn("model does not support caching, disabling parallel processing")
		}
		if s.batchSize < kvSize {
			s.batchSize = kvSize
			slog.Warn("model does not support caching, setting batch size to context length")
		}
	}

	s.parallel = parallel
	s.seqs = make([]*Sequence, s.parallel)
	s.seqsSem = semaphore.NewWeighted(int64(s.parallel))

	err = s.reserveWorstCaseGraph(true)
	if err != nil {
		return nil
	}

	return s.reserveWorstCaseGraph(false)
}

// closeModel frees all memory associated with a model
func (s *Server) closeModel() {
	s.cache.Close()
	s.cache = nil

Review comment (performance):
We actually use both parallel and batchSize in the NewInputCache function, so we should not change their values after it has already been initialized. To avoid this, we should probably move this whole `if !s.cache.enabled` block before NewInputCache and check `model.Config().Cache != nil` as the condition instead.

Code after:
					panic(r)
				}
			} else {
				panic(r)
			}
		}
	}()

	var err error
	s.model, err = model.New(mpath, params)
	if err != nil {
		return err
	}

	// TODO(jessegross): LoRA loading
	if len(loraPath) > 0 {
		return errors.New("loras are not yet implemented")
	}

	if s.model.Config().Cache == nil {
		if parallel > 1 {
			parallel = 1
			slog.Warn("model does not support caching, disabling parallel processing")
		}
		if s.batchSize < kvSize {
			s.batchSize = kvSize
			slog.Warn("model does not support caching, setting batch size to context length", "batch_size", kvSize)
		}
	}

	s.cache, err = NewInputCache(s.model, kvCacheType, int32(kvSize), parallel, s.batchSize, multiUserCache)
	if err != nil {
		return err
	}

	s.parallel = parallel
	s.seqs = make([]*Sequence, s.parallel)
	s.seqsSem = semaphore.NewWeighted(int64(s.parallel))

	err = s.reserveWorstCaseGraph(true)
	if err != nil {
		return nil
	}

	return s.reserveWorstCaseGraph(false)
}

// closeModel frees all memory associated with a model
func (s *Server) closeModel() {
	s.cache.Close()
	s.cache = nil
Repository: ollama/ollama
File: app/server/server.go (Go)

Code before:
		select {
		case <-ctx.Done():
			return nil, fmt.Errorf("timeout scanning server log for inference compute details")
		default:
		}
		file, err := os.Open(serverLogPath)
		if err != nil {
			slog.Debug("failed to open server log", "log", serverLogPath, "error", err)
			time.Sleep(time.Second)
			continue
		}
		defer file.Close()
		scanner := bufio.NewScanner(file)
		for scanner.Scan() {
			line := scanner.Text()
			// Check for inference compute lines
			if computeMarker.MatchString(line) {
				ic := InferenceCompute{
					Library: get("library", line),
					Variant: get("variant", line),
					Compute: get("compute", line),
					Driver:  get("driver", line),
					Name:    get("name", line),
					VRAM:    get("total", line),
				}

				slog.Info("Matched", "inference compute", ic)
				info.Computes = append(info.Computes, ic)
				continue
			}
			// Check for default context length line
			if defaultCtxMarker.MatchString(line) {
				match := defaultCtxRegex.FindStringSubmatch(line)
				if len(match) > 1 {
					numCtx, err := strconv.Atoi(match[1])
					if err == nil {
						info.DefaultContextLength = numCtx
						slog.Info("Matched default context length", "default_num_ctx", numCtx)
					}
				}
				return info, nil
			}
			// If we've found compute info but hit a non-matching line, return what we have
			// This handles older server versions that don't log the default context line
			if len(info.Computes) > 0 {
				return info, nil
			}
		}
		time.Sleep(100 * time.Millisecond)
	}
}

Review comment (suggestion):
```suggestion
			// Check for inference compute logs
```

Code after:
		if len(match) > 1 {
			return match[1]
		}
		match = regex.nq.FindStringSubmatch(line)
		if len(match) > 1 {
			return match[1]
		}
		return ""
	}
	for {
		select {
		case <-ctx.Done():
			return nil, fmt.Errorf("timeout scanning server log for inference compute details")
		default:
		}
		file, err := os.Open(serverLogPath)
		if err != nil {
			slog.Debug("failed to open server log", "log", serverLogPath, "error", err)
			time.Sleep(time.Second)
			continue
		}
		defer file.Close()
		scanner := bufio.NewScanner(file)
		for scanner.Scan() {
			line := scanner.Text()
			// Check for inference compute lines
			if computeMarker.MatchString(line) {
				ic := InferenceCompute{
					Library: get("library", line),
					Variant: get("variant", line),
					Compute: get("compute", line),
					Driver:  get("driver", line),
					Name:    get("name", line),
					VRAM:    get("total", line),
				}

				slog.Info("Matched", "inference compute", ic)
				info.Computes = append(info.Computes, ic)
				continue
			}
			// Check for default context length line
			if defaultCtxMarker.MatchString(line) {
				match := defaultCtxRegex.FindStringSubmatch(line)
				if len(match) > 1 {
					numCtx, err := strconv.Atoi(match[1])
					if err == nil {
						info.DefaultContextLength = numCtx
						slog.Info("Matched default context length", "default_num_ctx", numCtx)
					}
				}
				return info, nil
Repository: ollama/ollama
File: server/routes_generate_test.go (Go)

Code before:
			pendingReqCh:  make(chan *LlmRequest, 1),
			finishedReqCh: make(chan *LlmRequest, 1),
			expiredCh:     make(chan *runnerRef, 1),
			unloadedCh:    make(chan any, 1),
			loaded: map[string]*runnerRef{
				"": {
					llama:       &mock,
					Options:     &opts,
					model:       &Model{Config: model.ConfigV2{Capabilities: []string{"image"}}},
					isImagegen:  true,
					numParallel: 1,
				},
			},
			newServerFn:     newMockServer(&mock),
			getGpuFn:        getGpuFn,
			getSystemInfoFn: getSystemInfoFn,
		},
	}

	go s.sched.Run(t.Context())

	// Create model manifest with image capability
	n := model.ParseName("test-image")
	cfg := model.ConfigV2{Capabilities: []string{"image"}}
	var b bytes.Buffer
	if err := json.NewEncoder(&b).Encode(&cfg); err != nil {
		t.Fatal(err)
	}
	configLayer, err := manifest.NewLayer(&b, "application/vnd.docker.container.image.v1+json")
	if err != nil {
		t.Fatal(err)
	}
	if err := manifest.WriteManifest(n, configLayer, nil); err != nil {
		t.Fatal(err)
	}

	streamFalse := false
	w := createRequest(t, s.GenerateHandler, api.GenerateRequest{
		Model:  "test-image",
		Prompt: "test prompt",
		Stream: &streamFalse,
	})

	if w.Code != http.StatusOK {
		t.Fatalf("expected status 200, got %d: %s", w.Code, w.Body.String())
	}

	if ct := w.Header().Get("Content-Type"); ct != "application/json; charset=utf-8" {
		t.Errorf("expected Content-Type 'application/json; charset=utf-8', got %q", ct)
	}


Review comment (suggestion):
This was causing a conflict with the mlx based model names which don't have the same ModelPath name assigned to them, so the test ends up not starting.

Instead we just create the model manifest upfront and give it its name first.

Code after:
	var b bytes.Buffer
	if err := json.NewEncoder(&b).Encode(&cfg); err != nil {
		t.Fatal(err)
	}
	configLayer, err := manifest.NewLayer(&b, "application/vnd.docker.container.image.v1+json")
	if err != nil {
		t.Fatal(err)
	}
	if err := manifest.WriteManifest(n, configLayer, nil); err != nil {
		t.Fatal(err)
	}

	loadedModel, err := GetModel("test-image")
	if err != nil {
		t.Fatal(err)
	}

	opts := api.DefaultOptions()
	s := Server{
		sched: &Scheduler{
			pendingReqCh:  make(chan *LlmRequest, 1),
			finishedReqCh: make(chan *LlmRequest, 1),
			expiredCh:     make(chan *runnerRef, 1),
			unloadedCh:    make(chan any, 1),
			loaded: map[string]*runnerRef{
				schedulerModelKey(loadedModel): {
					llama:       &mock,
					Options:     &opts,
					model:       loadedModel,
					isImagegen:  true,
					numParallel: 1,
				},
			},
			newServerFn:     newMockServer(&mock),
			getGpuFn:        getGpuFn,
			getSystemInfoFn: getSystemInfoFn,
		},
	}

	go s.sched.Run(t.Context())

	streamFalse := false
	w := createRequest(t, s.GenerateHandler, api.GenerateRequest{
		Model:  "test-image",
		Prompt: "test prompt",
		Stream: &streamFalse,
	})

	if w.Code != http.StatusOK {
		t.Fatalf("expected status 200, got %d: %s", w.Code, w.Body.String())
	}
Repository: ollama/ollama
File: cmd/tui/selector.go (Go)

Code before:
	fm := finalModel.(selectorModel)
	if fm.cancelled {
		return "", ErrCancelled
	}

	return fm.selected, nil
}

// multiSelectorModel is the bubbletea model for multi selection.
type multiSelectorModel struct {
	title        string
	items        []SelectItem
	itemIndex    map[string]int
	filter       string
	cursor       int
	scrollOffset int
	checked      map[int]bool
	checkOrder   []int
	cancelled    bool
	confirmed    bool
	width        int

	// singleMode starts true: shows a single-select picker where Enter
	// adds the chosen model to the existing list. Tab switches to full
	// multi-select editing mode.
	singleMode bool
	singleAdd  string // model picked in single mode
}

func newMultiSelectorModel(title string, items []SelectItem, preChecked []string) multiSelectorModel {
	m := multiSelectorModel{
		title:      title,
		items:      items,
		itemIndex:  make(map[string]int, len(items)),
		checked:    make(map[int]bool),
		singleMode: true,
	}

	for i, item := range items {
		m.itemIndex[item.Name] = i
	}

	// Reverse order so preChecked[0] (the current default) ends up last
	// in checkOrder, matching the "last checked = default" convention.
	for i := len(preChecked) - 1; i >= 0; i-- {
		if idx, ok := m.itemIndex[preChecked[i]]; ok {
			m.checked[idx] = true
			m.checkOrder = append(m.checkOrder, idx)
		}
	}


Review comment (bug):
Should we have `multi    bool` or similar? that way this defaults to false instead of true (less code) and multiple is the edge case right? 

Code after:
	fm := finalModel.(selectorModel)
	if fm.cancelled {
		return "", ErrCancelled
	}

	return fm.selected, nil
}

// multiSelectorModel is the bubbletea model for multi selection.
type multiSelectorModel struct {
	title        string
	items        []SelectItem
	itemIndex    map[string]int
	filter       string
	cursor       int
	scrollOffset int
	checked      map[int]bool
	checkOrder   []int
	cancelled    bool
	confirmed    bool
	width        int

	// multi enables full multi-select editing mode. The zero value (false)
	// shows a single-select picker where Enter adds the chosen model to
	// the existing list. Tab toggles between modes.
	multi     bool
	singleAdd string // model picked in single mode
}

func newMultiSelectorModel(title string, items []SelectItem, preChecked []string) multiSelectorModel {
	m := multiSelectorModel{
		title:     title,
		items:     items,
		itemIndex: make(map[string]int, len(items)),
		checked:   make(map[int]bool),
	}

	for i, item := range items {
		m.itemIndex[item.Name] = i
	}

	// Reverse order so preChecked[0] (the current default) ends up last
	// in checkOrder, matching the "last checked = default" convention.
	for i := len(preChecked) - 1; i >= 0; i-- {
		if idx, ok := m.itemIndex[preChecked[i]]; ok {
			m.checked[idx] = true
			m.checkOrder = append(m.checkOrder, idx)
		}
	}

	// Position cursor on the current default model
Repository: onnx/onnx
File: tools/protoc-gen-mypy.py (Python)

Code before:
#!/usr/bin/env python

# Copyright (c) ONNX Project Contributors

# Copyright (c) ONNX Project Contributors
#
# SPDX-License-Identifier: Apache-2.0

# NOTE: This is deprecated in favour of protogen's own .pyi generation method.
#       See: https://github.com/onnx/onnx/pull/6096

# Taken from https://github.com/dropbox/mypy-protobuf/blob/d984389124eae6dbbb517f766b9266bb32171510/python/protoc-gen-mypy
# (Apache 2.0 License)
# with own fixes to
# - appease flake8
# - exit without error when protobuf isn't installed
# - fix recognition of whether an identifier is defined locally
#   (unfortunately, we use a python package name ONNX_NAMESPACE_FOO_BAR_FOR_CI
#    on CI, which by the original protoc-gen-mypy script was recognized to be
#    camel case and therefore handled as an entry in the local package)


"""Protoc Plugin to generate mypy stubs. Loosely based on @zbarsky's go implementation"""
from __future__ import annotations

import sys
from collections import defaultdict
from contextlib import contextmanager
from typing import Any, Callable, Generator, cast

try:
    import google.protobuf.descriptor_pb2 as d_typed
    from google.protobuf.compiler import plugin_pb2 as plugin
except ImportError as e:

Review comment (suggestion):
```suggestion
# NOTE: This is deprecated in favor of protogen's own .pyi generation method.
```

Code after:
#!/usr/bin/env python

# Copyright (c) ONNX Project Contributors

# Copyright (c) ONNX Project Contributors
#
# SPDX-License-Identifier: Apache-2.0

# NOTE: This is deprecated in favor of protogen's own .pyi generation method.
#       See: https://github.com/onnx/onnx/pull/6096

# Taken from https://github.com/dropbox/mypy-protobuf/blob/d984389124eae6dbbb517f766b9266bb32171510/python/protoc-gen-mypy
# (Apache 2.0 License)
# with own fixes to
# - appease flake8
# - exit without error when protobuf isn't installed
# - fix recognition of whether an identifier is defined locally
#   (unfortunately, we use a python package name ONNX_NAMESPACE_FOO_BAR_FOR_CI
#    on CI, which by the original protoc-gen-mypy script was recognized to be
#    camel case and therefore handled as an entry in the local package)


"""Protoc Plugin to generate mypy stubs. Loosely based on @zbarsky's go implementation"""
from __future__ import annotations

import sys
from collections import defaultdict
from contextlib import contextmanager
from typing import Any, Callable, Generator, cast

try:
    import google.protobuf.descriptor_pb2 as d_typed
    from google.protobuf.compiler import plugin_pb2 as plugin
except ImportError as e:
Repository: onnx/onnx
File: onnx/test/version_converter_test.py (Python)

Code before:
                model_def,
                model_filename,
                save_as_external_data=True,
                all_tensors_to_one_file=True,
                location=data_filename,
                size_threshold=0,
                convert_attribute=False,
            )

            # Load the model and verify external data
            converted_model = onnx.version_converter.convert_version(
                onnx.load(model_filename, load_external_data=False), 21
            )
            self.assertEqual(len(converted_model.graph.initializer), 2)

            # Verify the large tensor has external data
            for i in range(2):
                if converted_model.graph.initializer[i].name == "initializer_tensor":
                    large_tensor = converted_model.graph.initializer[i]
                    self.assertEqual(len(large_tensor.external_data), 3)
                    self.assertEqual(large_tensor.external_data[0].key, "location")
                    self.assertEqual(large_tensor.external_data[0].value, data_filename)
                    self.assertEqual(large_tensor.external_data[1].key, "offset")
                    self.assertEqual(large_tensor.external_data[1].value, "0")
                    self.assertEqual(large_tensor.external_data[2].key, "length")
                    self.assertEqual(large_tensor.external_data[2].value, "24")


if __name__ == "__main__":
    unittest.main()


Review comment (suggestion):
The key-value pairs are not guaranteed in order. I would build a dictionary from `large_tensor.external_data` then assert the python dict directly.

Code after:
                model_def,
                model_filename,
                save_as_external_data=True,
                all_tensors_to_one_file=True,
                location=data_filename,
                size_threshold=0,
                convert_attribute=False,
            )

            # Load the model and verify external data
            converted_model = onnx.version_converter.convert_version(
                onnx.load(model_filename, load_external_data=False), 21
            )
            self.assertEqual(len(converted_model.graph.initializer), 2)

            # Verify the large tensor has external data
            tensors = {init.name: init for init in converted_model.graph.initializer}
            self.assertIn("initializer_tensor", tensors)
            large_tensor = tensors["initializer_tensor"]
            self.assertEqual(large_tensor.data_location, TensorProto.EXTERNAL)
            self.assertEqual(len(large_tensor.external_data), 3)

            # Convert external_data to dictionary for order-independent checking
            external_data_dict = {ed.key: ed.value for ed in large_tensor.external_data}
            self.assertEqual(external_data_dict["location"], data_filename)
            self.assertEqual(external_data_dict["offset"], "0")
            self.assertEqual(external_data_dict["length"], "24")


if __name__ == "__main__":
    unittest.main()