mirror of
https://github.com/mudler/LocalAI.git
synced 2026-10-28 09:08:54 -04:00
fix(models): resolver-gated queueing and editor polling for manual imports
Review round ataec658a99: - [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 <p.kosseff@gmail.com> (cherry picked from commit742971c2c4)
This commit is contained in:
1 parent
a50eb4e68a
commit
addcec0c28
4 files changed
+179
-10
No files matched your search
@@ -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())
|
||||
})
|
||||
})
|
||||
@@ -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,
|
||||
})
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -288,6 +288,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)
|
||||
@@ -312,7 +335,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.
|
||||
@@ -343,7 +367,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)
|
||||
|
||||
Reference in new issue
Block a user