From 13dc34fc4be39716316079545e7a3768cb4db16b Mon Sep 17 00:00:00 2001 From: Dimitris Karakasilis Date: Thu, 30 Jul 2026 22:54:41 +0300 Subject: [PATCH] fix(gallery): persist inference defaults where the loader reads them The recommended sampling parameters for a model family were applied at install and then never took effect. Two things went wrong on the way to disk. They were written as top level keys. ModelConfig embeds PredictionOptions under the "parameters" yaml key, so temperature, top_p, top_k, min_p, repeat_penalty and presence_penalty are only read from there. At the top level they parse without error and are then ignored for the life of the model. They were also merged in after the YAML had already been marshalled. The only re-marshal sat behind the artifact binding, which an entry carrying files: never reaches, so for those entries the defaults were computed and then dropped before anything was written. Neither failure was visible in normal use. ApplyInferenceDefaults runs again at load time and fills the same values from the same table, so the model ends up tuned correctly while the file on disk pins nothing. It surfaces when someone edits one of those values expecting it to win, or when a family is absent from inference_defaults.json and there is nothing to refill from. Both install paths are covered: an entry carrying files:, and one that binds a primary artifact instead. The empty base spec asserted that the authored parameters block landed verbatim. It now checks the authored keys individually, because the family defaults are merged into that same block. Assisted-by: Claude:claude-opus-5 Signed-off-by: Dimitris Karakasilis --- core/gallery/empty_base_install_test.go | 11 +- .../inference_defaults_install_test.go | 189 ++++++++++++++++++ core/gallery/models.go | 62 +++--- 3 files changed, 237 insertions(+), 25 deletions(-) create mode 100644 core/gallery/inference_defaults_install_test.go diff --git a/core/gallery/empty_base_install_test.go b/core/gallery/empty_base_install_test.go index 6b104e233f02..b2a15c1ea5a0 100644 --- a/core/gallery/empty_base_install_test.go +++ b/core/gallery/empty_base_install_test.go @@ -236,8 +236,15 @@ var _ = Describe("InstallModelFromGallery with an empty base config", func() { Expect(install(e.Name, gallery.GalleryModel{})).To(Succeed()) cfg := installedConfig(e.Name) Expect(cfg["name"]).To(Equal(e.Name)) - // The catalog's own overrides, verbatim, laid over the empty base. - Expect(cfg["parameters"]).To(Equal(e.Overrides["parameters"])) + // The catalog's own overrides, laid over the empty base. parameters is + // checked key by key rather than as a whole map: the install also merges + // the model family's inference defaults into it, and what matters here is + // that the authored keys survive that. + authored, ok := e.Overrides["parameters"].(map[string]any) + Expect(ok).To(BeTrue()) + for key, want := range authored { + Expect(cfg["parameters"]).To(HaveKeyWithValue(key, want)) + } Expect(cfg["known_usecases"]).To(Equal(e.Overrides["known_usecases"])) }) }) diff --git a/core/gallery/inference_defaults_install_test.go b/core/gallery/inference_defaults_install_test.go new file mode 100644 index 000000000000..11f46bc42d68 --- /dev/null +++ b/core/gallery/inference_defaults_install_test.go @@ -0,0 +1,189 @@ +package gallery_test + +import ( + "context" + "fmt" + "maps" + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "gopkg.in/yaml.v3" + + "github.com/mudler/LocalAI/core/config" + "github.com/mudler/LocalAI/core/gallery" + "github.com/mudler/LocalAI/pkg/modelartifacts" + "github.com/mudler/LocalAI/pkg/system" +) + +// The recommended sampling parameters for a model family are applied at install +// and persisted into the model YAML. Persisting them is only worth anything if +// they are written where the loader reads them back: PredictionOptions is nested +// under "parameters" in ModelConfig, so a top level "temperature" key parses +// without error and is then ignored for the life of the model. +// +// The expected values are read from the family table rather than written out +// here, so that retuning a family stays a one file change. +// +// Nothing here reaches the network. +var _ = Describe("Inference defaults persisted at install", func() { + var tempdir string + var galleries []config.Gallery + var systemState *system.SystemState + // The gallery listing is cached on the name and URL pair, so every spec + // needs a gallery of its own or it reads the previous spec's catalog. + galleryRevision := 0 + + // The name has to contain a pattern from inference_defaults.json, otherwise + // no defaults are applied and every assertion below passes vacuously. + const modelName = "qwen3.5-install-defaults" + + newGallery := func(entries ...gallery.GalleryModel) { + out, err := yaml.Marshal(entries) + Expect(err).ToNot(HaveOccurred()) + name := fmt.Sprintf("inference-defaults-%d", galleryRevision) + galleryRevision++ + galleryPath := filepath.Join(tempdir, name+".yaml") + Expect(os.WriteFile(galleryPath, out, 0600)).To(Succeed()) + galleries = []config.Gallery{{Name: name, URL: "file://" + galleryPath}} + } + + install := func(name string) error { + return gallery.InstallModelFromGallery( + context.TODO(), galleries, []config.Gallery{}, systemState, nil, + name, gallery.GalleryModel{}, func(string, string, string, float64) {}, false, false, false) + } + + installedConfig := func(name string) map[string]any { + dat, err := os.ReadFile(filepath.Join(tempdir, name+".yaml")) + Expect(err).ToNot(HaveOccurred()) + content := map[string]any{} + Expect(yaml.Unmarshal(dat, &content)).To(Succeed()) + return content + } + + // Seeding the weights keeps the install off the network: the downloader + // treats an already-present destination with no declared sha256 as fetched. + // extra goes into parameters:, so a spec can pin a value the defaults would + // otherwise supply. + seedGallery := func(extra map[string]any) { + Expect(os.WriteFile(filepath.Join(tempdir, "weights.gguf"), []byte("weights"), 0600)).To(Succeed()) + + params := map[string]any{"model": "weights.gguf"} + maps.Copy(params, extra) + + e := gallery.GalleryModel{Overrides: map[string]any{ + "backend": "llama-cpp", + "parameters": params, + }} + e.Name = modelName + e.AdditionalFiles = []gallery.File{{Filename: "weights.gguf", URI: "https://example.com/weights.gguf"}} + newGallery(e) + } + + // Guards the fixture itself. If the name stops matching a family the specs + // below would still pass while asserting nothing at all. + expectedFamily := func() map[string]float64 { + family := config.MatchModelFamily(modelName) + Expect(family).ToNot(BeEmpty(), "fixture name no longer matches a family in inference_defaults.json") + return family + } + + BeforeEach(func() { + var err error + tempdir, err = os.MkdirTemp("", "inference-defaults-install") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { Expect(os.RemoveAll(tempdir)).To(Succeed()) }) + + systemState, err = system.GetSystemState(system.WithModelPath(tempdir)) + Expect(err).ToNot(HaveOccurred()) + }) + + It("writes them under parameters, where the loader reads them back", func() { + family := expectedFamily() + seedGallery(nil) + + Expect(install(modelName)).To(Succeed()) + + params, ok := installedConfig(modelName)["parameters"].(map[string]any) + Expect(ok).To(BeTrue(), "parameters should be a map") + + for key, want := range family { + Expect(params).To(HaveKey(key)) + Expect(params[key]).To(BeNumerically("==", want), "parameters.%s", key) + } + }) + + It("does not leave them at the top level, where they are ignored", func() { + family := expectedFamily() + seedGallery(nil) + + Expect(install(modelName)).To(Succeed()) + + cfg := installedConfig(modelName) + for key := range family { + Expect(cfg).ToNot(HaveKey(key), "%s at the top level is never read", key) + } + }) + + It("leaves a value the entry already sets alone", func() { + family := expectedFamily() + Expect(family).To(HaveKey("temperature")) + Expect(family["temperature"]).ToNot(BeNumerically("==", 0.05), "pick a value the family does not use") + + seedGallery(map[string]any{"temperature": 0.05}) + + Expect(install(modelName)).To(Succeed()) + + params, ok := installedConfig(modelName)["parameters"].(map[string]any) + Expect(ok).To(BeTrue(), "parameters should be a map") + Expect(params["temperature"]).To(BeNumerically("==", 0.05)) + }) + + // An entry that binds a primary artifact carries no files: of its own, so it + // takes the other branch of the install and none of the specs above reach it. + // It is also the one branch that already re-marshalled, which is why the + // defaults did land on disk there, at the top level where nothing reads them. + It("writes them under parameters on the artifact binding path too", func() { + family := expectedFamily() + + definition := &gallery.ModelConfig{ConfigFile: ` +backend: transformers +artifacts: + - name: model + target: model + source: + type: huggingface + repo: owner/repo +parameters: + model: owner/repo +`} + // Standing in for the materializer keeps the install off the network. + materializer := &fakeArtifactMaterializer{result: modelartifacts.Result{ + Spec: modelartifacts.Spec{ + Name: "model", Target: "model", + Source: modelartifacts.Source{Type: "huggingface", Repo: "owner/repo", Revision: "main"}, + Resolved: &modelartifacts.Resolved{ + Endpoint: "https://huggingface.co", + Revision: "0123456789abcdef0123456789abcdef01234567", + CacheKey: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + }, + }, + RelativePath: ".artifacts/huggingface/0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef/snapshot", + }} + + _, err := gallery.InstallModel(context.TODO(), systemState, modelName, definition, nil, nil, false, + gallery.WithArtifactMaterializer(materializer)) + Expect(err).ToNot(HaveOccurred()) + + cfg := installedConfig(modelName) + params, ok := cfg["parameters"].(map[string]any) + Expect(ok).To(BeTrue(), "parameters should be a map") + for key, want := range family { + Expect(params).To(HaveKey(key)) + Expect(params[key]).To(BeNumerically("==", want), "parameters.%s", key) + Expect(cfg).ToNot(HaveKey(key), "%s at the top level is never read", key) + } + }) +}) diff --git a/core/gallery/models.go b/core/gallery/models.go index f3184648b15e..190ed215943c 100644 --- a/core/gallery/models.go +++ b/core/gallery/models.go @@ -622,35 +622,51 @@ func InstallModel(ctx context.Context, systemState *system.SystemState, nameOver lconfig.ApplyInferenceDefaults(&modelConfig, name, modelConfig.Model) // Merge inference defaults into configMap so they are persisted without losing unknown fields. - if modelConfig.Temperature != nil { - if _, exists := configMap["temperature"]; !exists { - configMap["temperature"] = *modelConfig.Temperature + // They belong under "parameters": ModelConfig embeds PredictionOptions with + // that yaml key, so a top level "temperature" parses without error and is + // then ignored for the life of the model. + params, mergeable := configMap["parameters"].(map[string]any) + if configMap["parameters"] == nil { + params, mergeable = map[string]any{}, true + } + if mergeable { + // An entry that sets one of these keeps its own value. ApplyInferenceDefaults + // already skipped those fields; this keeps the write side symmetric. + setDefault := func(key string, value any) { + if _, exists := params[key]; !exists { + params[key] = value + } } - } - if modelConfig.TopP != nil { - if _, exists := configMap["top_p"]; !exists { - configMap["top_p"] = *modelConfig.TopP + if modelConfig.Temperature != nil { + setDefault("temperature", *modelConfig.Temperature) } - } - if modelConfig.TopK != nil { - if _, exists := configMap["top_k"]; !exists { - configMap["top_k"] = *modelConfig.TopK + if modelConfig.TopP != nil { + setDefault("top_p", *modelConfig.TopP) } - } - if modelConfig.MinP != nil { - if _, exists := configMap["min_p"]; !exists { - configMap["min_p"] = *modelConfig.MinP + if modelConfig.TopK != nil { + setDefault("top_k", *modelConfig.TopK) } - } - if modelConfig.RepeatPenalty != 0 { - if _, exists := configMap["repeat_penalty"]; !exists { - configMap["repeat_penalty"] = modelConfig.RepeatPenalty + if modelConfig.MinP != nil { + setDefault("min_p", *modelConfig.MinP) } - } - if modelConfig.PresencePenalty != 0 { - if _, exists := configMap["presence_penalty"]; !exists { - configMap["presence_penalty"] = modelConfig.PresencePenalty + if modelConfig.RepeatPenalty != 0 { + setDefault("repeat_penalty", modelConfig.RepeatPenalty) } + if modelConfig.PresencePenalty != 0 { + setDefault("presence_penalty", modelConfig.PresencePenalty) + } + if len(params) > 0 { + configMap["parameters"] = params + } + } + + // The marshal above predates this merge, and the only other re-marshal is + // behind the artifact binding below, which an entry carrying files: never + // reaches. Without this the defaults are computed and then dropped on the + // way to disk. + updatedConfigYAML, err = yaml.Marshal(configMap) + if err != nil { + return nil, fmt.Errorf("failed to marshal config with inference defaults: %v", err) } if valid, err := modelConfig.Validate(); !valid {