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

Code before:
}

func checkServerHeartbeat(_ *cobra.Command, _ []string) error {
	client, err := api.ClientFromEnvironment()
	if err != nil {
		return err
	}
	if err := client.Heartbeat(context.Background()); err != nil {
		if !strings.Contains(err.Error(), "connection refused") {
			return err
		}
		if runtime.GOOS == "darwin" {
			if err := startMacApp(client); err != nil {
				return fmt.Errorf("could not connect to ollama app, is it running?")
			}
		} else {
			return fmt.Errorf("could not connect to ollama server, run 'ollama serve' to start it")
		}
	}
	return nil
}

func NewCLI() *cobra.Command {
	log.SetFlags(log.LstdFlags | log.Lshortfile)

	if runtime.GOOS == "windows" {
		// Enable colorful ANSI escape code in Windows terminal (disabled by default)
		console.ConsoleFromFile(os.Stdout)
	}

	rootCmd := &cobra.Command{
		Use:           "ollama",
		Short:         "Large language model runner",
		SilenceUsage:  true,
		SilenceErrors: true,
		CompletionOptions: cobra.CompletionOptions{
			DisableDefaultCmd: true,
		},
		Version: version.Version,
	}

	cobra.EnableCommandSorting = false

	createCmd := &cobra.Command{
		Use:     "create MODEL",
		Short:   "Create a model from a Modelfile",
		Args:    cobra.ExactArgs(1),
		PreRunE: checkServerHeartbeat,
		RunE:    CreateHandler,
	}


Review comment (suggestion):
This looks like an aesthetic change unrelated to the CUDA support, correct? There's gonna be a few things we need to fix in the CLI for an official windows release, so I think this would make more sense as a separate PR.

Code after:
func checkServerHeartbeat(_ *cobra.Command, _ []string) error {
	client, err := api.ClientFromEnvironment()
	if err != nil {
		return err
	}
	if err := client.Heartbeat(context.Background()); err != nil {
		if !strings.Contains(err.Error(), "connection refused") {
			return err
		}
		if runtime.GOOS == "darwin" {
			if err := startMacApp(client); err != nil {
				return fmt.Errorf("could not connect to ollama app, is it running?")
			}
		} else {
			return fmt.Errorf("could not connect to ollama server, run 'ollama serve' to start it")
		}
	}
	return nil
}

func NewCLI() *cobra.Command {
	log.SetFlags(log.LstdFlags | log.Lshortfile)

	rootCmd := &cobra.Command{
		Use:           "ollama",
		Short:         "Large language model runner",
		SilenceUsage:  true,
		SilenceErrors: true,
		CompletionOptions: cobra.CompletionOptions{
			DisableDefaultCmd: true,
		},
		Version: version.Version,
	}

	cobra.EnableCommandSorting = false

	createCmd := &cobra.Command{
		Use:     "create MODEL",
		Short:   "Create a model from a Modelfile",
		Args:    cobra.ExactArgs(1),
		PreRunE: checkServerHeartbeat,
		RunE:    CreateHandler,
	}

	createCmd.Flags().StringP("file", "f", "Modelfile", "Name of the Modelfile (default \"Modelfile\")")

	showCmd := &cobra.Command{
		Use:     "show MODEL",
		Short:   "Show information for a model",
		Args:    cobra.ExactArgs(1),
		PreRunE: checkServerHeartbeat,
Repository: ollama/ollama
File: server/images.go (Go)

Code before:
			}

			layers.Replace(layer)
		default:
			params[c.Name] = append(params[c.Name], c.Args)
		}
	}

	if len(params) > 0 {
		fn(api.ProgressResponse{Status: "creating parameters layer"})

		formattedParams, err := formatParams(params)
		if err != nil {
			return err
		}

		for k, v := range fromParams {
			if _, ok := formattedParams[k]; !ok {
				formattedParams[k] = v
			}
		}

		if config.ModelType == "65B" {
			if gqa, ok := formattedParams["gqa"].(int); ok && gqa == 8 {
				config.ModelType = "70B"
			}
		}

		var b bytes.Buffer
		if err := json.NewEncoder(&b).Encode(formattedParams); err != nil {
			return err
		}

		fn(api.ProgressResponse{Status: "creating config layer"})
		layer, err := NewLayer(&b, "application/vnd.ollama.image.params")
		if err != nil {
			return err
		}

		layers.Replace(layer)
	}

	digests := make([]string, len(layers.items))
	for i, layer := range layers.items {
		digests[i] = layer.Digest
	}

	config.RootFS.DiffIDs = digests

	var b bytes.Buffer
	if err := json.NewEncoder(&b).Encode(config); err != nil {

Review comment (bug):
What makes an EOF ok here? This is quite hidden and feels difficult to track down in the case it causes a bug.

Code after:

						if err := json.NewDecoder(fromParamsFile).Decode(&fromParams); err != nil {
							return err
						}
					}

					layer, err := NewLayerFromLayer(layer.Digest, layer.MediaType, modelpath.GetShortTagname())
					if err != nil {
						return err
					}

					layers.Add(layer)
				}

				deleteMap[manifest.Config.Digest] = struct{}{}
				continue
			}
			defer bin.Close()

			var offset int64
			for {
				fn(api.ProgressResponse{Status: "creating model layer"})

				bin.Seek(offset, io.SeekStart)
				ggml, err := llm.DecodeGGML(bin)
				if errors.Is(err, io.EOF) {
					break
				} else if err != nil {
					return err
				}

				config.SetModelFormat(ggml.Name())
				config.SetModelFamily(ggml.ModelFamily())
				config.SetModelType(ggml.ModelType())
				config.SetFileType(ggml.FileType())

				mediatype := mediatype
				if ggml.ModelFamily() == "clip" {
					mediatype = "application/vnd.ollama.image.projector"
				}

				sr := io.NewSectionReader(bin, offset, ggml.Size)
				layer, err := NewLayer(sr, mediatype)
				if err != nil {
					return err
				}

				layers.Add(layer)

				offset += ggml.Size
			}
Repository: ollama/ollama
File: examples/python-simplechat/client.py (Python)

Code before:
        "http://0.0.0.0:11434/api/chat",
        json={"model": model, "messages": messages, "stream": True},
    )
    r.raise_for_status()
    output = ""

    for line in r.iter_lines():
        body = json.loads(line)
        if body.get("done") is False:
            message = body.get("message", "")
            content = message.get("content", "")
            output += content
            # the response streams one token at a time, print that as we receive it
            print(content, end="", flush=True)

        if "error" in body:
            raise Exception(body["error"])

        if body.get("done", False):
            message["content"] = output
            return message


def main():
    messages = []
    )  # the context stores a conversation history, you can use this to make the model more context aware
    while True:
        user_input = input("Enter a prompt: ")
        print()
        messages.append({"role": "user", "content": user_input})
        message = chat(messages)
        messages.append(message)
        print("\n\n")


if __name__ == "__main__":
    main()


Review comment (suggestion):
```suggestion
```

I think I missed this in my suggestion block

Code after:
        "http://0.0.0.0:11434/api/chat",
        json={"model": model, "messages": messages, "stream": True},
    )
    r.raise_for_status()
    output = ""

    for line in r.iter_lines():
        body = json.loads(line)
        if "error" in body:
            raise Exception(body["error"])
        if body.get("done") is False:
            message = body.get("message", "")
            content = message.get("content", "")
            output += content
            # the response streams one token at a time, print that as we receive it
            print(content, end="", flush=True)


        if body.get("done", False):
            message["content"] = output
            return message


def main():
    messages = []
    
    while True:
        user_input = input("Enter a prompt: ")
        print()
        messages.append({"role": "user", "content": user_input})
        message = chat(messages)
        messages.append(message)
        print("\n\n")


if __name__ == "__main__":
    main()
Repository: ollama/ollama
File: examples/typescript-simplechat/client.ts (TypeScript)

Code before:
  const response = await fetch("http://localhost:11434/api/chat", {
    method: "POST",
    body: JSON.stringify(body)
  })

  const reader = response.body?.getReader()
  if (!reader) {
    throw new Error("Failed to read response body")
  }
  const content: string[] = []
  while (true) {
    const { done, value } = await reader.read()
    if (done) {
      break;
    }
    const rawjson = new TextDecoder().decode(value);
    const json = JSON.parse(rawjson)

    if (json.done === false) {
      process.stdout.write(json.message.content);
      content.push(json.message.content)
      // messages.push({role: "system", content: text})
    }

  }
  return { role: "assistant", content: content.join("") };
}

async function askQuestion(): Promise<void> {
  return new Promise<void>((resolve) => {
    rl.question("\n\nAsk a question: (press enter alone to quit)\n\n", async (user_input) => {
      if (user_input.trim() === "") {
        rl.close();
        console.log("Thankyou. Goodbye.\n")
        console.log("=======\nHere is the message history that was used in this conversation.\n=======\n")
        messages.forEach(message => {
          console.log(message)
        })
        resolve();
      } else {
        console.log();
        messages.push({ role: "user", content: user_input });
        messages.push(await chat(messages));
        await askQuestion(); // Ask the next question
      }
    });
  });
}

async function main() {
  await askQuestion();

Review comment (suggestion):
```suggestion
  return { role: "assistant", content: content };
```

(along with the suggestion to change this to a string.

Code after:
  const response = await fetch("http://localhost:11434/api/chat", {
    method: "POST",
    body: JSON.stringify(body)
  })

  const reader = response.body?.getReader()
  if (!reader) {
    throw new Error("Failed to read response body")
  }
  let content = ""
  while (true) {
    const { done, value } = await reader.read()
    if (done) {
      break;
    }
    const rawjson = new TextDecoder().decode(value);
    const json = JSON.parse(rawjson)

    if (json.done === false) {
      process.stdout.write(json.message.content);
      content += json.message.content
    }

  }
  return { role: "assistant", content: content };
}

async function askQuestion(): Promise<void> {
  return new Promise<void>((resolve) => {
    rl.question("\n\nAsk a question: (press enter alone to quit)\n\n", async (user_input) => {
      if (user_input.trim() === "") {
        rl.close();
        console.log("Thankyou. Goodbye.\n")
        console.log("=======\nHere is the message history that was used in this conversation.\n=======\n")
        messages.forEach(message => {
          console.log(message)
        })
        resolve();
      } else {
        console.log();
        messages.push({ role: "user", content: user_input });
        messages.push(await chat(messages));
        await askQuestion(); // Ask the next question
      }
    });
  });
}

async function main() {
  await askQuestion();
Repository: ollama/ollama
File: examples/typescript-simplechat/client.ts (TypeScript)

Code before:
  const content: string[] = []
  while (true) {
    const { done, value } = await reader.read()
    if (done) {
      break;
    }
    const rawjson = new TextDecoder().decode(value);
    const json = JSON.parse(rawjson)

    if (json.done === false) {
      process.stdout.write(json.message.content);
      content.push(json.message.content)
      // messages.push({role: "system", content: text})
    }

  }
  return { role: "assistant", content: content.join("") };
}

async function askQuestion(): Promise<void> {
  return new Promise<void>((resolve) => {
    rl.question("\n\nAsk a question: (press enter alone to quit)\n\n", async (user_input) => {
      if (user_input.trim() === "") {
        rl.close();
        console.log("Thankyou. Goodbye.\n")
        console.log("=======\nHere is the message history that was used in this conversation.\n=======\n")
        messages.forEach(message => {
          console.log(message)
        })
        resolve();
      } else {
        console.log();
        messages.push({ role: "user", content: user_input });
        messages.push(await chat(messages));
        await askQuestion(); // Ask the next question
      }
    });
  });
}

async function main() {
  await askQuestion();

}

main();

Review comment (suggestion):
You can just log the messages array too and it will output nicely. This is also fine though.

Here's what just logging messages looks like:
```
[
  { role: 'system', content: 'You are a helpful AI agent.' },
  { role: 'user', content: 'hi' },
  {
    role: 'assistant',
    content: "Hello! It's nice to meet you. I'm here to help with any questions or tasks you may have, how can I assist you today? Please let me know if there is anything specific you need help with."
  },
  { role: 'user', content: 'what is up?' }
]
```

Code after:
  let content = ""
  while (true) {
    const { done, value } = await reader.read()
    if (done) {
      break;
    }
    const rawjson = new TextDecoder().decode(value);
    const json = JSON.parse(rawjson)

    if (json.done === false) {
      process.stdout.write(json.message.content);
      content += json.message.content
    }

  }
  return { role: "assistant", content: content };
}

async function askQuestion(): Promise<void> {
  return new Promise<void>((resolve) => {
    rl.question("\n\nAsk a question: (press enter alone to quit)\n\n", async (user_input) => {
      if (user_input.trim() === "") {
        rl.close();
        console.log("Thankyou. Goodbye.\n")
        console.log("=======\nHere is the message history that was used in this conversation.\n=======\n")
        messages.forEach(message => {
          console.log(message)
        })
        resolve();
      } else {
        console.log();
        messages.push({ role: "user", content: user_input });
        messages.push(await chat(messages));
        await askQuestion(); // Ask the next question
      }
    });
  });
}

async function main() {
  await askQuestion();

}

main();
Repository: ollama/ollama
File: llm/llama.go (Go)

Code before:

	if resp.StatusCode >= 400 {
		bodyBytes, err := io.ReadAll(resp.Body)
		if err != nil {
			return fmt.Errorf("failed reading llm error response: %w", err)
		}
		log.Printf("llm predict error: %s", bodyBytes)
		return fmt.Errorf("%s", bodyBytes)
	}

	scanner := bufio.NewScanner(resp.Body)
	// increase the buffer size to avoid running out of space
	buf := make([]byte, 0, maxBufferSize)
	scanner.Buffer(buf, maxBufferSize)
	for scanner.Scan() {
		select {
		case <-ctx.Done():
			// This handles the request cancellation
			return ctx.Err()
		default:
			line := scanner.Bytes()
			if len(line) == 0 {
				continue
			}

			if evt, ok := bytes.CutPrefix(line, []byte("data: ")); ok {
				var p prediction
				if err := json.Unmarshal(evt, &p); err != nil {
					return fmt.Errorf("error unmarshaling llm prediction response: %v", err)
				}

				if p.Content != "" {
					fn(PredictResult{
						Model:     predict.Model,
						CreatedAt: time.Now().UTC(),
						Content:   p.Content,
					})
				}

				if p.Stop {
					fn(PredictResult{
						Model:         predict.Model,
						CreatedAt:     time.Now().UTC(),
						TotalDuration: time.Since(predict.CheckpointStart),

						Done:               true,
						PromptEvalCount:    p.Timings.PromptN,
						PromptEvalDuration: parseDurationMs(p.Timings.PromptMS),
						EvalCount:          p.Timings.PredictedN,
						EvalDuration:       parseDurationMs(p.Timings.PredictedMS),
					})

Review comment (suggestion):
This diff is hard to read. I changed the logic to return the error when the response stream can't be parsed.

Code after:
	defer resp.Body.Close()

	if resp.StatusCode >= 400 {
		bodyBytes, err := io.ReadAll(resp.Body)
		if err != nil {
			return fmt.Errorf("failed reading llm error response: %w", err)
		}
		log.Printf("llm predict error: %s", bodyBytes)
		return fmt.Errorf("%s", bodyBytes)
	}

	scanner := bufio.NewScanner(resp.Body)
	// increase the buffer size to avoid running out of space
	buf := make([]byte, 0, maxBufferSize)
	scanner.Buffer(buf, maxBufferSize)
	for scanner.Scan() {
		select {
		case <-ctx.Done():
			// This handles the request cancellation
			return ctx.Err()
		default:
			line := scanner.Bytes()
			if len(line) == 0 {
				continue
			}

			evt, ok := bytes.CutPrefix(line, []byte("data: "))
			if !ok {
				return fmt.Errorf("error parsing llm response stream: %s", line)
			}

			var p prediction
			if err := json.Unmarshal(evt, &p); err != nil {
				return fmt.Errorf("error unmarshaling llm prediction response: %v", err)
			}

			if p.Content != "" {
				fn(PredictResult{
					Model:     predict.Model,
					CreatedAt: time.Now().UTC(),
					Content:   p.Content,
				})
			}

			if p.Stop {
				fn(PredictResult{
					Model:         predict.Model,
					CreatedAt:     time.Now().UTC(),
					TotalDuration: time.Since(predict.CheckpointStart),

					Done:               true,
Repository: ollama/ollama
File: server/routes_test.go (Go)

Code before:

	httpSrv := httptest.NewServer(router)
	t.Cleanup(httpSrv.Close)

	run := func(t *testing.T, tc testCase) {
		u := httpSrv.URL + tc.Path
		req, err := http.NewRequestWithContext(context.TODO(), tc.Method, u, nil)
		assert.NilError(t, err)

		if tc.Setup != nil {
			tc.Setup(t, req)
		}

		resp, err := httpSrv.Client().Do(req)
		assert.NilError(t, err)

		if tc.Expected != nil {
			tc.Expected(t, resp)
		}
	}

	for _, tc := range testCases {
		t.Run(tc.Name, func(t *testing.T) {
			run(t, tc)
		})
	}

}


Review comment (suggestion):
```suggestion
	for _, tc := range testCases {
		t.Run(tc.Name, func(t *testing.T) {
			u := httpSrv.URL + tc.Path
		        req, err := http.NewRequestWithContext(context.TODO(), tc.Method, u, nil)
		        assert.NilError(t, err)

		        if tc.Setup != nil {
			    tc.Setup(t, req)
		        }

		        resp, err := httpSrv.Client().Do(req)
		        assert.NilError(t, err)

		       if tc.Expected != nil {
		           tc.Expected(t, resp)
		       }
		})
	}
```

I would personally prefer not using the callback function in this case to remove a layer of indirection, I find it easier to read

Code after:
package server

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

	"github.com/stretchr/testify/assert"
)

func setupServer(t *testing.T) (*Server, error) {
	t.Helper()

	return NewServer()
}

func Test_Routes(t *testing.T) {
	type testCase struct {
		Name     string
		Method   string
		Path     string
		Setup    func(t *testing.T, req *http.Request)
		Expected func(t *testing.T, resp *http.Response)
	}

	testCases := []testCase{
		{
			Name:   "Version Handler",
			Method: http.MethodGet,
			Path:   "/api/version",
			Setup: func(t *testing.T, req *http.Request) {
			},
			Expected: func(t *testing.T, resp *http.Response) {
				contentType := resp.Header.Get("Content-Type")
				assert.Equal(t, contentType, "application/json; charset=utf-8")
				body, err := io.ReadAll(resp.Body)
				assert.Nil(t, err)
				assert.Equal(t, `{"version":"0.0.0"}`, string(body))
			},
		},
	}

	s, err := setupServer(t)
	assert.Nil(t, err)

	router := s.GenerateRoutes()

	httpSrv := httptest.NewServer(router)
	t.Cleanup(httpSrv.Close)

	for _, tc := range testCases {
		u := httpSrv.URL + tc.Path
		req, err := http.NewRequestWithContext(context.TODO(), tc.Method, u, nil)
		assert.Nil(t, err)

		if tc.Setup != nil {
			tc.Setup(t, req)
		}

		resp, err := httpSrv.Client().Do(req)
		assert.Nil(t, err)

		if tc.Expected != nil {
			tc.Expected(t, resp)
		}
	}

}
Repository: ollama/ollama
File: cmd/cmd.go (Go)

Code before:
					default:
						fmt.Print("No prompt template was specified for this model.\n\n")
					}
				default:
					fmt.Printf("Unknown command '/show %s'. Type /? for help\n", args[1])
				}
			} else {
				usageShow()
			}
		case strings.HasPrefix(line, "/help"), strings.HasPrefix(line, "/?"):
			args := strings.Fields(line)
			if len(args) > 1 {
				switch args[1] {
				case "set", "/set":
					usageSet()
				case "show", "/show":
					usageShow()
				}
			} else {
				usage()
			}
		case line == "/exit", line == "/bye":
			return nil
		case strings.HasPrefix(line, "/"):
			args := strings.Fields(line)
			if multiModal && len(imgFileRegex.FindAllString(args[0], -1)) > 0 {
				// this is a file path, not a command, add it to the prompt
				prompt += line
			} else {
				fmt.Printf("Unknown command '%s'. Type /? for help\n", args[0])
				continue
			}
		default:
			prompt += line
		}

		if len(prompt) > 0 && multiline == MultilineNone {
			opts.Prompt = prompt
			if multiModal {
				newPrompt, images, err := extractFileNames(prompt)
				if err != nil {
					return err
				}
				opts.Prompt = newPrompt

				// reset the context if we find another image
				if len(images) > 0 {
					opts.Images = images
					ctx := cmd.Context()
					ctx = context.WithValue(ctx, generateContextKey("context"), []int{})
					cmd.SetContext(ctx)

Review comment (refactor):
I think you can just check if it's a multimodal image, and then call extractFileNames to get each of the filenames on the string. Then you can just look to see if the prefix of the line matches one of the filenames returned by the function. That avoids the global regex and keeps the logic still in the extractFileNames function.


Code after:
				}
			} else {
				usageShow()
			}
		case strings.HasPrefix(line, "/help"), strings.HasPrefix(line, "/?"):
			args := strings.Fields(line)
			if len(args) > 1 {
				switch args[1] {
				case "set", "/set":
					usageSet()
				case "show", "/show":
					usageShow()
				}
			} else {
				usage()
			}
		case line == "/exit", line == "/bye":
			return nil
		case strings.HasPrefix(line, "/"):
			args := strings.Fields(line)
			isFile := false

			if multiModal {
				for _, f := range extractFileNames(line) {
					if strings.HasPrefix(f, args[0]) {
						isFile = true
						break
					}
				}
			}

			if isFile {
				prompt += line
			} else {
				fmt.Printf("Unknown command '%s'. Type /? for help\n", args[0])
				continue
			}
		default:
			prompt += line
		}

		if len(prompt) > 0 && multiline == MultilineNone {
			opts.Prompt = prompt
			if multiModal {
				newPrompt, images, err := extractFileData(prompt)
				if err != nil {
					return err
				}
				opts.Prompt = newPrompt

				// reset the context if we find another image