mirror of https://github.com/ollama/ollama.git
bugfix: restore the current runOptions if loading fails in the CLI (#12402)
There are two bugs when using `/load <model>` for a model that doesn't exist, namely: 1. it will not restore the current model settings if the current model is a thinking model; and 2. it will crash is the current model is a non-thinking model This bug fix saves the current runOptions and then restores them if the model load doesn't happen. It also fixes the crash happening for non-thinking models.
This commit is contained in:
parent
34efbbd3f0
commit
b04e46da3e
45
cmd/cmd.go
45
cmd/cmd.go
|
|
@ -1118,6 +1118,51 @@ type runOptions struct {
|
||||||
ShowConnect bool
|
ShowConnect bool
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (r runOptions) Copy() runOptions {
|
||||||
|
var messages []api.Message
|
||||||
|
if r.Messages != nil {
|
||||||
|
messages = make([]api.Message, len(r.Messages))
|
||||||
|
copy(messages, r.Messages)
|
||||||
|
}
|
||||||
|
|
||||||
|
var images []api.ImageData
|
||||||
|
if r.Images != nil {
|
||||||
|
images = make([]api.ImageData, len(r.Images))
|
||||||
|
copy(images, r.Images)
|
||||||
|
}
|
||||||
|
|
||||||
|
var opts map[string]any
|
||||||
|
if r.Options != nil {
|
||||||
|
opts = make(map[string]any, len(r.Options))
|
||||||
|
for k, v := range r.Options {
|
||||||
|
opts[k] = v
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
var think *api.ThinkValue
|
||||||
|
if r.Think != nil {
|
||||||
|
cThink := *r.Think
|
||||||
|
think = &cThink
|
||||||
|
}
|
||||||
|
|
||||||
|
return runOptions{
|
||||||
|
Model: r.Model,
|
||||||
|
ParentModel: r.ParentModel,
|
||||||
|
Prompt: r.Prompt,
|
||||||
|
Messages: messages,
|
||||||
|
WordWrap: r.WordWrap,
|
||||||
|
Format: r.Format,
|
||||||
|
System: r.System,
|
||||||
|
Images: images,
|
||||||
|
Options: opts,
|
||||||
|
MultiModal: r.MultiModal,
|
||||||
|
KeepAlive: r.KeepAlive,
|
||||||
|
Think: think,
|
||||||
|
HideThinking: r.HideThinking,
|
||||||
|
ShowConnect: r.ShowConnect,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
type displayResponseState struct {
|
type displayResponseState struct {
|
||||||
lineLength int
|
lineLength int
|
||||||
wordBuffer string
|
wordBuffer string
|
||||||
|
|
|
||||||
284
cmd/cmd_test.go
284
cmd/cmd_test.go
|
|
@ -8,6 +8,7 @@ import (
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"os"
|
"os"
|
||||||
|
"reflect"
|
||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
@ -953,3 +954,286 @@ func TestNewCreateRequest(t *testing.T) {
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestRunOptions_Copy(t *testing.T) {
|
||||||
|
// Setup test data
|
||||||
|
originalKeepAlive := &api.Duration{Duration: 5 * time.Minute}
|
||||||
|
originalThink := &api.ThinkValue{Value: "test reasoning"}
|
||||||
|
|
||||||
|
original := runOptions{
|
||||||
|
Model: "test-model",
|
||||||
|
ParentModel: "parent-model",
|
||||||
|
Prompt: "test prompt",
|
||||||
|
Messages: []api.Message{
|
||||||
|
{Role: "user", Content: "hello"},
|
||||||
|
{Role: "assistant", Content: "hi there"},
|
||||||
|
},
|
||||||
|
WordWrap: true,
|
||||||
|
Format: "json",
|
||||||
|
System: "system prompt",
|
||||||
|
Images: []api.ImageData{
|
||||||
|
[]byte("image1"),
|
||||||
|
[]byte("image2"),
|
||||||
|
},
|
||||||
|
Options: map[string]any{
|
||||||
|
"temperature": 0.7,
|
||||||
|
"max_tokens": 1000,
|
||||||
|
"top_p": 0.9,
|
||||||
|
},
|
||||||
|
MultiModal: true,
|
||||||
|
KeepAlive: originalKeepAlive,
|
||||||
|
Think: originalThink,
|
||||||
|
HideThinking: false,
|
||||||
|
ShowConnect: true,
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test the copy
|
||||||
|
copied := original.Copy()
|
||||||
|
|
||||||
|
// Test 1: Verify the copy is not the same instance
|
||||||
|
if &copied == &original {
|
||||||
|
t.Error("Copy should return a different instance")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 2: Verify all fields are copied correctly
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
got interface{}
|
||||||
|
want interface{}
|
||||||
|
}{
|
||||||
|
{"Model", copied.Model, original.Model},
|
||||||
|
{"ParentModel", copied.ParentModel, original.ParentModel},
|
||||||
|
{"Prompt", copied.Prompt, original.Prompt},
|
||||||
|
{"WordWrap", copied.WordWrap, original.WordWrap},
|
||||||
|
{"Format", copied.Format, original.Format},
|
||||||
|
{"System", copied.System, original.System},
|
||||||
|
{"MultiModal", copied.MultiModal, original.MultiModal},
|
||||||
|
{"HideThinking", copied.HideThinking, original.HideThinking},
|
||||||
|
{"ShowConnect", copied.ShowConnect, original.ShowConnect},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
if !reflect.DeepEqual(tt.got, tt.want) {
|
||||||
|
t.Errorf("%s mismatch: got %v, want %v", tt.name, tt.got, tt.want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 3: Verify Messages slice is deeply copied
|
||||||
|
if len(copied.Messages) != len(original.Messages) {
|
||||||
|
t.Errorf("Messages length mismatch: got %d, want %d", len(copied.Messages), len(original.Messages))
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Messages) > 0 && &copied.Messages[0] == &original.Messages[0] {
|
||||||
|
t.Error("Messages should be different instances")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Modify original to verify independence
|
||||||
|
if len(original.Messages) > 0 {
|
||||||
|
originalContent := original.Messages[0].Content
|
||||||
|
original.Messages[0].Content = "modified"
|
||||||
|
if len(copied.Messages) > 0 && copied.Messages[0].Content == "modified" {
|
||||||
|
t.Error("Messages should be independent after copy")
|
||||||
|
}
|
||||||
|
// Restore for other tests
|
||||||
|
original.Messages[0].Content = originalContent
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 4: Verify Images slice is deeply copied
|
||||||
|
if len(copied.Images) != len(original.Images) {
|
||||||
|
t.Errorf("Images length mismatch: got %d, want %d", len(copied.Images), len(original.Images))
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Images) > 0 && &copied.Images[0] == &original.Images[0] {
|
||||||
|
t.Error("Images should be different instances")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Modify original to verify independence
|
||||||
|
if len(original.Images) > 0 {
|
||||||
|
originalImage := original.Images[0]
|
||||||
|
original.Images[0] = []byte("modified")
|
||||||
|
if len(copied.Images) > 0 && string(copied.Images[0]) == "modified" {
|
||||||
|
t.Error("Images should be independent after copy")
|
||||||
|
}
|
||||||
|
// Restore for other tests
|
||||||
|
original.Images[0] = originalImage
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 5: Verify Options map is deeply copied
|
||||||
|
if len(copied.Options) != len(original.Options) {
|
||||||
|
t.Errorf("Options length mismatch: got %d, want %d", len(copied.Options), len(original.Options))
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Options) > 0 && &copied.Options == &original.Options {
|
||||||
|
t.Error("Options map should be different instances")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Modify original to verify independence
|
||||||
|
if len(original.Options) > 0 {
|
||||||
|
originalTemp := original.Options["temperature"]
|
||||||
|
original.Options["temperature"] = 0.9
|
||||||
|
if copied.Options["temperature"] == 0.9 {
|
||||||
|
t.Error("Options should be independent after copy")
|
||||||
|
}
|
||||||
|
// Restore for other tests
|
||||||
|
original.Options["temperature"] = originalTemp
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 6: Verify KeepAlive pointer is copied (shallow copy)
|
||||||
|
if copied.KeepAlive != original.KeepAlive {
|
||||||
|
t.Error("KeepAlive pointer should be the same (shallow copy)")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 7: Verify Think pointer creates a new instance
|
||||||
|
if original.Think != nil && copied.Think == original.Think {
|
||||||
|
t.Error("Think should be a different instance")
|
||||||
|
}
|
||||||
|
|
||||||
|
if original.Think != nil && copied.Think != nil {
|
||||||
|
if !reflect.DeepEqual(copied.Think.Value, original.Think.Value) {
|
||||||
|
t.Errorf("Think.Value mismatch: got %v, want %v", copied.Think.Value, original.Think.Value)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// Test 8: Test with zero values
|
||||||
|
zeroOriginal := runOptions{}
|
||||||
|
zeroCopy := zeroOriginal.Copy()
|
||||||
|
|
||||||
|
if !reflect.DeepEqual(zeroCopy, zeroOriginal) {
|
||||||
|
fmt.Printf("orig: %#v\ncopy: %#v\n", zeroOriginal, zeroCopy)
|
||||||
|
t.Error("Copy of zero value should equal original zero value")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRunOptions_Copy_EmptySlicesAndMaps(t *testing.T) {
|
||||||
|
// Test with empty slices and maps
|
||||||
|
original := runOptions{
|
||||||
|
Messages: []api.Message{},
|
||||||
|
Images: []api.ImageData{},
|
||||||
|
Options: map[string]any{},
|
||||||
|
}
|
||||||
|
|
||||||
|
copied := original.Copy()
|
||||||
|
|
||||||
|
if copied.Messages == nil {
|
||||||
|
t.Error("Empty Messages slice should remain empty, not nil")
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Images == nil {
|
||||||
|
t.Error("Empty Images slice should remain empty, not nil")
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Options == nil {
|
||||||
|
t.Error("Empty Options map should remain empty, not nil")
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Messages) != 0 {
|
||||||
|
t.Error("Empty Messages slice should remain empty")
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Images) != 0 {
|
||||||
|
t.Error("Empty Images slice should remain empty")
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Options) != 0 {
|
||||||
|
t.Error("Empty Options map should remain empty")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRunOptions_Copy_NilPointers(t *testing.T) {
|
||||||
|
// Test with nil pointers
|
||||||
|
original := runOptions{
|
||||||
|
KeepAlive: nil,
|
||||||
|
Think: nil,
|
||||||
|
}
|
||||||
|
|
||||||
|
copied := original.Copy()
|
||||||
|
|
||||||
|
if copied.KeepAlive != nil {
|
||||||
|
t.Error("Nil KeepAlive should remain nil")
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Think != nil {
|
||||||
|
t.Error("Nil Think should remain nil")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRunOptions_Copy_ThinkValueVariants(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
think *api.ThinkValue
|
||||||
|
}{
|
||||||
|
{"nil Think", nil},
|
||||||
|
{"bool true", &api.ThinkValue{Value: true}},
|
||||||
|
{"bool false", &api.ThinkValue{Value: false}},
|
||||||
|
{"string value", &api.ThinkValue{Value: "reasoning text"}},
|
||||||
|
{"int value", &api.ThinkValue{Value: 42}},
|
||||||
|
{"nil value", &api.ThinkValue{Value: nil}},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
original := runOptions{Think: tt.think}
|
||||||
|
copied := original.Copy()
|
||||||
|
|
||||||
|
if tt.think == nil {
|
||||||
|
if copied.Think != nil {
|
||||||
|
t.Error("Nil Think should remain nil")
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Think == nil {
|
||||||
|
t.Error("Non-nil Think should not become nil")
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Think == original.Think {
|
||||||
|
t.Error("Think should be a different instance")
|
||||||
|
}
|
||||||
|
|
||||||
|
if !reflect.DeepEqual(copied.Think.Value, original.Think.Value) {
|
||||||
|
t.Errorf("Think.Value mismatch: got %v, want %v", copied.Think.Value, original.Think.Value)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRunOptions_Copy_Independence(t *testing.T) {
|
||||||
|
// Test that modifications to original don't affect copy
|
||||||
|
originalThink := &api.ThinkValue{Value: "original"}
|
||||||
|
original := runOptions{
|
||||||
|
Model: "original-model",
|
||||||
|
Messages: []api.Message{{Role: "user", Content: "original"}},
|
||||||
|
Options: map[string]any{"key": "value"},
|
||||||
|
Think: originalThink,
|
||||||
|
}
|
||||||
|
|
||||||
|
copied := original.Copy()
|
||||||
|
|
||||||
|
// Modify original
|
||||||
|
original.Model = "modified-model"
|
||||||
|
if len(original.Messages) > 0 {
|
||||||
|
original.Messages[0].Content = "modified"
|
||||||
|
}
|
||||||
|
original.Options["key"] = "modified"
|
||||||
|
if original.Think != nil {
|
||||||
|
original.Think.Value = "modified"
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify copy is unchanged
|
||||||
|
if copied.Model == "modified-model" {
|
||||||
|
t.Error("Copy Model should not be affected by original modification")
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(copied.Messages) > 0 && copied.Messages[0].Content == "modified" {
|
||||||
|
t.Error("Copy Messages should not be affected by original modification")
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Options["key"] == "modified" {
|
||||||
|
t.Error("Copy Options should not be affected by original modification")
|
||||||
|
}
|
||||||
|
|
||||||
|
if copied.Think != nil && copied.Think.Value == "modified" {
|
||||||
|
t.Error("Copy Think should not be affected by original modification")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -195,16 +195,24 @@ func generateInteractive(cmd *cobra.Command, opts runOptions) error {
|
||||||
fmt.Println("Usage:\n /load <modelname>")
|
fmt.Println("Usage:\n /load <modelname>")
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
origOpts := opts.Copy()
|
||||||
|
|
||||||
opts.Model = args[1]
|
opts.Model = args[1]
|
||||||
opts.Messages = []api.Message{}
|
opts.Messages = []api.Message{}
|
||||||
fmt.Printf("Loading model '%s'\n", opts.Model)
|
fmt.Printf("Loading model '%s'\n", opts.Model)
|
||||||
opts.Think, err = inferThinkingOption(nil, &opts, thinkExplicitlySet)
|
opts.Think, err = inferThinkingOption(nil, &opts, thinkExplicitlySet)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
if strings.Contains(err.Error(), "not found") {
|
||||||
|
fmt.Printf("Couldn't find model '%s'\n", opts.Model)
|
||||||
|
opts = origOpts.Copy()
|
||||||
|
continue
|
||||||
|
}
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
if err := loadOrUnloadModel(cmd, &opts); err != nil {
|
if err := loadOrUnloadModel(cmd, &opts); err != nil {
|
||||||
if strings.Contains(err.Error(), "not found") {
|
if strings.Contains(err.Error(), "not found") {
|
||||||
fmt.Printf("error: %v\n", err)
|
fmt.Printf("Couldn't find model '%s'\n", opts.Model)
|
||||||
|
opts = origOpts.Copy()
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
if strings.Contains(err.Error(), "does not support thinking") {
|
if strings.Contains(err.Error(), "does not support thinking") {
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue