From 742971c2c4e6d91f25c7713f83188d44d46c1edc Mon Sep 17 00:00:00 2001 From: "Plamen K. Kosseff" Date: Tue, 6 Oct 2026 16:45:48 +0000 Subject: [PATCH] fix(models): resolver-gated queueing and editor polling for manual imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round at aec658a99: - [P1] acquisition is decided by PrimaryArtifactSpec: managed artifacts (hf:// repos, bare owner/repo, explicit artifacts:) enqueue with no file entries so the worker's InstallModel runs the artifact binding and repository materializer — flattening them into file entries routed repositories through the single-file downloader and skipped the materializer. Plain remote files keep the legacy file path; malformed references fail at admission. Covered by endpoint specs and a worker-level materializer spec. - [P2] both ModelEditor create paths wait on the returned job before announcing success and navigating; download failures surface as save errors instead of after a success toast. Assisted-by: Claude:claude-fable-5 Signed-off-by: Plamen K. Kosseff --- core/gallery/queued_import_install_test.go | 68 +++++++++++++++++++ core/http/endpoints/localai/import_model.go | 38 ++++++++--- .../endpoints/localai/import_model_test.go | 54 +++++++++++++++ core/http/react-ui/src/pages/ModelEditor.jsx | 29 +++++++- 4 files changed, 179 insertions(+), 10 deletions(-) create mode 100644 core/gallery/queued_import_install_test.go diff --git a/core/gallery/queued_import_install_test.go b/core/gallery/queued_import_install_test.go new file mode 100644 index 000000000..7031cde8b --- /dev/null +++ b/core/gallery/queued_import_install_test.go @@ -0,0 +1,68 @@ +package gallery_test + +import ( + "context" + "path/filepath" + + . "github.com/mudler/LocalAI/core/gallery" + "github.com/mudler/LocalAI/pkg/modelartifacts" + "github.com/mudler/LocalAI/pkg/system" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// The queued manual-import op carries only the config YAML — no files: — +// so InstallModel must resolve and bind the managed artifact itself, +// exactly as it does for a gallery entry. These specs pin that for the +// references the import endpoint forwards without flattening; a file +// entry here would have routed the repository through the single-file +// downloader and skipped the materializer entirely. +var _ = Describe("queued manual import installs", func() { + run := func(configFile string) (*fakeArtifactMaterializer, string) { + modelsPath := GinkgoT().TempDir() + state, err := system.GetSystemState(system.WithModelPath(modelsPath)) + Expect(err).ToNot(HaveOccurred()) + + // 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 = InstallModel(context.TODO(), state, "queued-model", + &ModelConfig{ConfigFile: configFile}, nil, nil, false, + WithArtifactMaterializer(materializer)) + Expect(err).ToNot(HaveOccurred()) + return materializer, modelsPath + } + + It("binds an hf:// repository through the materializer", func() { + m, dir := run(`name: queued-model +backend: transformers +parameters: + model: hf://owner/repo +`) + Expect(m.seen).ToNot(BeEmpty()) + Expect(m.seen[0].Source.Repo).To(Equal("owner/repo")) + Expect(filepath.Join(dir, "queued-model.yaml")).To(BeAnExistingFile()) + }) + + It("binds a bare owner/repo reference through the materializer", func() { + m, dir := run(`name: queued-model +backend: transformers +parameters: + model: owner/repo +`) + Expect(m.seen).ToNot(BeEmpty()) + Expect(m.seen[0].Source.Repo).To(Equal("owner/repo")) + Expect(filepath.Join(dir, "queued-model.yaml")).To(BeAnExistingFile()) + }) +}) diff --git a/core/http/endpoints/localai/import_model.go b/core/http/endpoints/localai/import_model.go index f9e9d9fc5..083fe5ef9 100644 --- a/core/http/endpoints/localai/import_model.go +++ b/core/http/endpoints/localai/import_model.go @@ -240,13 +240,33 @@ func ImportModelEndpoint(cl *config.ModelConfigLoader, gs *galleryop.GalleryServ return c.JSON(http.StatusInternalServerError, response) } - // A config referencing remote assets is a download, not just a file - // write: route it through the gallery op queue so the UI tracks it - // like a gallery install (job UUID + /models/jobs/{uuid}), instead - // of downloading synchronously inside this request via Preload. The - // op writes the config file and reloads once the downloads finish; - // configs with no remote assets keep the fast synchronous path. - if files := remoteAssetFiles(&modelConfig); len(files) > 0 && gs != nil { + // A config whose assets are not on disk yet is a download, not just + // a file write: route it through the gallery op queue so the UI + // tracks it like a gallery install (job UUID + /models/jobs/{uuid}) + // instead of acquiring synchronously inside this request. The op + // writes the config file and reloads once acquisition finishes; + // configs needing no acquisition keep the fast synchronous path. + // + // WHICH acquisition is the artifact resolver's call, not a string + // test: a managed artifact (explicit artifacts:, an hf:// repo or + // bare owner/repo on a directory-consuming backend) must enqueue + // with NO files, so the worker's InstallModel runs the same binding + // and repository materializer as a gallery install — flattening a + // repository into file entries would send it to the single-file + // downloader and skip the materializer. Plain remote files + // (download_files, URL model/mmproj on single-file backends) ride + // as file entries through the same downloader preload would use. + // (A URL mmproj on an artifact config stays preload's job, as it + // was before queued imports.) + _, _, hasArtifact, err := modelConfig.PrimaryArtifactSpec(appConfig.SystemState.Model.ModelsPath) + if err != nil { + return c.JSON(http.StatusBadRequest, ModelResponse{Success: false, Error: "Invalid model reference: " + err.Error()}) + } + var queueFiles []gallery.File + if !hasArtifact { + queueFiles = remoteAssetFiles(&modelConfig) + } + if gs != nil && (hasArtifact || len(queueFiles) > 0) { jobUUID, err := uuid.NewUUID() if err != nil { return err @@ -261,7 +281,9 @@ func ImportModelEndpoint(cl *config.ModelConfigLoader, gs *galleryop.GalleryServ GalleryElement: &gallery.ModelConfig{ Name: modelConfig.Name, ConfigFile: string(yamlData), - Files: files, + // nil for artifact configs: InstallModel's empty-Files + // branch re-resolves and binds the artifact itself. + Files: queueFiles, }, BackendGalleries: appConfig.BackendGalleries, }) diff --git a/core/http/endpoints/localai/import_model_test.go b/core/http/endpoints/localai/import_model_test.go index 7829af3b0..409162315 100644 --- a/core/http/endpoints/localai/import_model_test.go +++ b/core/http/endpoints/localai/import_model_test.go @@ -190,6 +190,60 @@ var _ = Describe("ImportModelEndpoint queued imports", func() { Expect(op.GalleryElement.Files[0].SHA256).To(Equal("abc123")) }) + It("enqueues a managed-artifact repo with no file entries so the worker binds it", func() { + // The reviewer's case: an hf:// repository on a directory-consuming + // backend must reach InstallModel's artifact binding, which only + // runs when the op carries no files — a file entry would route the + // repository through the single-file downloader instead. + rec := post(`{"name": "hfrepo", "backend": "transformers", "parameters": {"model": "hf://sshleifer/tiny-gpt2"}}`) + Expect(rec.Code).To(Equal(http.StatusOK)) + var resp map[string]any + Expect(json.Unmarshal(rec.Body.Bytes(), &resp)).To(Succeed()) + Expect(resp).To(HaveKey("uuid")) + + var op galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig] + Eventually(galleryService.ModelGalleryChannel).Should(Receive(&op)) + Expect(op.GalleryElement.Files).To(BeEmpty()) + Expect(op.GalleryElement.ConfigFile).To(ContainSubstring("hf://sshleifer/tiny-gpt2")) + }) + + It("enqueues a bare owner/repo reference instead of importing it synchronously", func() { + rec := post(`{"name": "barerepo", "backend": "transformers", "parameters": {"model": "sshleifer/tiny-gpt2"}}`) + Expect(rec.Code).To(Equal(http.StatusOK)) + Expect(filepath.Join(tempDir, "barerepo.yaml")).ToNot(BeAnExistingFile()) + var op galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig] + Eventually(galleryService.ModelGalleryChannel).Should(Receive(&op)) + Expect(op.GalleryElement.Files).To(BeEmpty()) + }) + + It("enqueues an explicit artifacts config with no file entries", func() { + body := `name: artifactual +backend: transformers +artifacts: +- name: model + target: model + source: + type: huggingface + repo: sshleifer/tiny-gpt2 +parameters: + model: sshleifer/tiny-gpt2 +` + req := httptest.NewRequest("POST", "/models/import", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/x-yaml") + rec := httptest.NewRecorder() + app.ServeHTTP(rec, req) + Expect(rec.Code).To(Equal(http.StatusOK)) + var op galleryop.ManagementOp[gallery.GalleryModel, gallery.ModelConfig] + Eventually(galleryService.ModelGalleryChannel).Should(Receive(&op)) + Expect(op.GalleryElement.Files).To(BeEmpty()) + }) + + It("rejects a malformed Hugging Face reference at admission", func() { + rec := post(`{"name": "badref", "backend": "transformers", "parameters": {"model": "owner/repo?bad"}}`) + Expect(rec.Code).To(Equal(http.StatusBadRequest)) + Expect(rec.Body.String()).To(ContainSubstring("Invalid model reference")) + }) + It("keeps the synchronous path for configs without remote assets", func() { rec := post(`{"name": "local", "backend": "llama-cpp", "parameters": {"model": "already-here.gguf"}}`) Expect(rec.Code).To(Equal(http.StatusOK)) diff --git a/core/http/react-ui/src/pages/ModelEditor.jsx b/core/http/react-ui/src/pages/ModelEditor.jsx index d58dbb864..54f599e94 100644 --- a/core/http/react-ui/src/pages/ModelEditor.jsx +++ b/core/http/react-ui/src/pages/ModelEditor.jsx @@ -284,6 +284,29 @@ export default function ModelEditor() { return null }, [isCreateMode, vramEstimate.loading, vramEstimate.vramDisplay]) + // A queued import (config with remote assets) answers with a job id; + // the config file exists only once the job finishes, so creation must + // not navigate to the editor before that — and a download failure has + // to surface here instead of after a premature success toast. A + // response without a job id is a synchronous import: config written. + const waitForImportJob = async (result) => { + const jobId = result?.uuid || result?.ID + if (!jobId) return false + addToast('Import queued — downloading model assets…', 'info') + for (;;) { + await new Promise(r => setTimeout(r, 1000)) + let data + try { data = await modelsApi.getJobStatus(jobId) } catch { continue } + if (data?.error) { + throw new Error(typeof data.error === 'string' ? data.error : 'model asset download failed') + } + // The raw /models/jobs endpoint reports `processed`, not the + // `completed` the /api/models/job shim synthesizes; failed jobs + // also set `processed`, but the error branch above runs first. + if (data?.completed || data?.processed) return true + } + } + // Interactive save — uses PATCH (edit mode) or importConfig (create mode) const handleInteractiveSave = async () => { setSaving(true) @@ -304,7 +327,8 @@ export default function ModelEditor() { const modelName = values['name'] if (!modelName?.trim()) { addToast('Model name is required', 'error'); setSaving(false); return } if (!/^[a-zA-Z0-9_.-]+$/.test(modelName.trim())) { addToast('Invalid model name — use only letters, numbers, hyphens, underscores, and dots', 'error'); setSaving(false); return } - await modelsApi.importConfig(JSON.stringify(config), 'application/json') + const result = await modelsApi.importConfig(JSON.stringify(config), 'application/json') + await waitForImportJob(result) addToast('Model created successfully', 'success') // replace: the transient create URL shouldn't sit in history, so // Back (browser or in-page) skips it and returns to the linking page. @@ -334,7 +358,8 @@ export default function ModelEditor() { try { if (isCreateMode) { // In create mode, import the YAML as a new config - await modelsApi.importConfig(yamlText, 'application/x-yaml') + const result = await modelsApi.importConfig(yamlText, 'application/x-yaml') + await waitForImportJob(result) addToast('Model created successfully', 'success') try { const parsed = YAML.parse(yamlText)