Studio: let the hub run non-GGUF image and video models - #8855
Studio: let the hub run non-GGUF image and video models#8855NilayYadav wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74152a831a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| mediaPageForTask( | ||
| taskForMediaPick(model.pipelineTag, model.task) ?? undefined, | ||
| ) !== undefined; |
There was a problem hiding this comment.
For filesystem-backed media rows such as models_dir, LM Studio, Ollama, or custom-folder image/audio models, this unconditionally enables Run from the task alone. However, runSelectedModel only navigates when routableToMediaPage(...) is true and a Hub-style routeId exists; otherwise it falls through to selectModel and opens Chat. Clicking Run on these rows therefore tries to load a media checkpoint as a chat model and can evict the resident chat model before failing. The media override should use the same routability/route-id conditions, or the handler should stop rather than fall through for an unroutable media task.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c6abd39. Confirmed reachable: _local_model_task tags a local non-GGUF diffusers checkpoint text-to-image and is applied to every local row, so a models_dir row reaches the inspector with a media task while routableToMediaPage is false for it. Run enablement now goes through a shared routedMediaPageForRow that requires routability, and runSelectedModel stops with a toast instead of falling through to the chat loader, which covers the routeId null case too. Added regression tests in hub-media-page-for-task.test.ts.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4108b04107
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const runsOnMediaPage = | ||
| routedMediaPageForRow( | ||
| taskForMediaPick(model.pipelineTag, model.task) ?? undefined, | ||
| model.kind, | ||
| model.localSource, | ||
| ) !== undefined; |
There was a problem hiding this comment.
For downloaded curated ASR GGUFs such as unslothai/Qwen3-ASR-0.6B-GGUF, inventory can report text-generation while deliberately setting runtimeCapabilities.canChat to false. This raw task lookup therefore makes runsOnMediaPage false, and the chat-capability fallback disables Run, even though runSelectedModel immediately reclassifies the exact catalog artifact as ASR and routes it to Audio. Use the same curated-audio task resolution (and routability policy) here as in the handler so these supported cached models can actually be run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc4b5b1. You are right and my earlier reading was wrong: I had assumed a GGUF always clears the ordinary chat gate, but cache_inventory sets can_chat False when stt_only or is_curated_stt_repo_id matches, so a curated ASR GGUF reporting text-generation had Run disabled even though the handler reclassifies it from AUDIO_CATALOG and routes it to Audio. The inspector now resolves the task through artifactForRepoId plus curatedAudioInventoryTask, exactly as runSelectedModel does, so enablement and the handler agree.
| // (left on today's route, and the backend preflight now refuses it by name) and a | ||
| // cached repo the inventory pinned to its snapshot directory, whose symlinked entries | ||
| // the pages' containment check rejects anyway. | ||
| const routeId = runId && !looksLikeLocalPath(runId) ? runId : selectedModel.hubRepoId; |
There was a problem hiding this comment.
When a cached TTS checkpoint resides in a configured HF cache other than the active one, runId is its snapshot path, but this replaces it with the display Hub ID before navigating. The Audio loader explicitly relies on meta.loadId for this case because loading by repo ID fails offline or downloads another copy into the active cache; the route handoff carries no load target, so Hub Run regresses exactly that path. Preserve the snapshot load target in the Audio handoff while retaining the repo ID for display and catalog classification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc4b5b1. Confirmed against loadTtsModel, which sends model_path: loadId || repoId and documents that a row in a non-active HF cache is loadable only by its snapshot path. The audio handoff now forwards loadId when the load target differs from the routed id, /audio validateSearch accepts it, and the page threads it through the dedupe key and into the meta it hands handleModelSelect, so it reaches /load the same way the chat picker supplies meta.loadId. The routed model stays the Hub id for display and catalog classification.
| if ( | ||
| !routableToMediaPage(selectedModel.kind, selectedModel.localSource) || | ||
| !routeId |
There was a problem hiding this comment.
The shared predicate was written for Images/Video, but applying it to Audio rejects every filesystem-backed TTS checkpoint even though the Audio route accepts path-valued model parameters and its loader passes local/exported IDs directly to /load; only filesystem STT is excluded by the Audio picker policy. Fresh evidence beyond the earlier review thread is the Audio-specific isLocalCheckpoint routability branch and direct meta.loadId load path. For a local Orpheus/OuteTTS checkpoint with a text-to-speech task, Run now only shows this error instead of opening Audio, so the filesystem restriction needs to remain diffusion-specific.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fc4b5b1. Confirmed: audioPipelineTagFor documents that a local checkpoint never gets the ASR tag because the STT sidecar takes only a curated key or a Hub id, while TTS loads through the main slot which does accept a local path, and filesystemRowsSupportedForTask excludes only automatic-speech-recognition. routedMediaPageForRow now returns audio before the routability gate, so the restriction is diffusion-specific. One correction on the before-state: at 74152a8 a local TTS row was not opening Audio either, it fell through to chat, so this is the parity gap with the chat picker rather than a regression from the previous commit. While fixing it I also found the handler still refused these rows further up: audioPickIsRoutable was called without isLocalCheckpoint, so the family-name heuristic rejected a filesystem checkpoint on its directory name. It now passes that flag for exactly the rows the chat picker marks local, with a test asserting the un-flagged call returns false.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4b5b17b5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const runsOnMediaPage = | ||
| routedMediaPageForRow( | ||
| curatedAudioInventoryTask({ | ||
| inventoryTask: taskForMediaPick(model.pipelineTag, model.task), | ||
| isExactCatalogArtifact: inspectorAudioArtifact !== null, | ||
| catalogScope: inspectorAudioArtifact?.group.scope, | ||
| catalogTask: inspectorAudioArtifact?.group.task, | ||
| }) ?? undefined, | ||
| model.kind, | ||
| model.localSource, | ||
| ) !== undefined; |
There was a problem hiding this comment.
For uncurated Hub models tagged as speech but unsupported by Studio—such as suno/bark or non-Whisper ASR repositories—this marks runsOnMediaPage true solely from the task. The Run button is therefore enabled, but runSelectedModel applies audioPickIsRoutable and immediately returns with an error toast. Use the same audio-family and format gate here so the inspector does not advertise an action that can never run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1bb0638. The inspector now resolves the audio task once and applies audioPickIsRoutable before treating the row as running on its own page, so it uses the same family and format gate as the handler. A curated artifact still short-circuits to routable, so the curated ASR GGUF case stays enabled, and a local Orpheus or OuteTTS row is still admitted on its path or base model. To be precise about the remaining scope: this removes the media override as a source of the mismatch, but an uncurated speech row can still be enabled through the ordinary chat gate, which is pre-existing and unchanged here. That path now ends in the handler's clean refusal instead of loading a speech checkpoint as a chat model, so it is strictly better than before this PR rather than fully closed.
| isLocalCheckpoint: !routableToMediaPage( | ||
| selectedModel.kind, | ||
| selectedModel.localSource, | ||
| ), |
There was a problem hiding this comment.
When a Discover result is matched to a filesystem row, useSelectedModelView supplies the Hub result's pipelineTag but does not carry the local row's backend-inferred task; nevertheless, this condition labels the selection a trusted local checkpoint. Thus a local copy of an unsupported Hub TTS family such as Bark bypasses communityAudioRowIsRunnable, routes to Audio with its path as loadId, and attempts a main-slot load that can evict the resident chat model before failing. Only take this bypass when the audio task actually came from local checkpoint metadata; otherwise retain the normal family gate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 42901d9 by removing the bypass entirely. One correction: both local branches of useSelectedModelView do carry task: selectedLocalRow.task ?? null, so the row is not missing its inventory task. The substance holds by precedence instead: mergedPipelineTag prefers the Hub tag, and local inventory tags no row as speech at all, so a filesystem row's speech task always arrives from Hub metadata via its base model. That makes isLocalCheckpoint, which exists for a checkpoint whose codec the backend read off the weights, never justified here, and it waved through exactly the families the policy calls out as loadable but undecodable. The family gate already admits genuine local Orpheus and OuteTTS rows on their path or base model, so removing the flag loses nothing. Test asserts the Bark case is refused and would pass with the flag.
| void navigate({ | ||
| to: `/${mediaPage}`, |
There was a problem hiding this comment.
For any Discover result carrying an image/video generation pipeline tag, mediaPage is set and this branch routes it without checking the destination page's repository policy. The Images/Video pickers deliberately restrict non-GGUF rows to backend-trusted or curated artifacts, while their loaders reject arbitrary third-party repositories such as an unallowlisted owner/text-to-image-model; Hub now enables Run and navigates only for the destination preflight to refuse the model. Apply the same trusted/curated artifact policy before treating these rows as runnable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one. The destination declines cleanly: src/features/images never calls the main-slot loadModel or unloadModel, so there is no eviction and no failed load against the chat runtime. The user lands on the page that owns the model and its own plan-route preflight refuses it, which is the page's policy to enforce and where it is authoritative. Duplicating that admission policy in the Hub would mean the two can disagree later, and this PR's job is to route the pick to the page that runs it, not to re-decide what that page accepts.
| pipelineTag: routeSearch.task ?? null, | ||
| // A row cached outside the active HF cache loads only by its snapshot path. | ||
| loadId: routeSearch.loadId ?? null, |
There was a problem hiding this comment.
When a cached ASR checkpoint lives outside the active HF cache, the Hub route supplies its snapshot as loadId, but handleModelSelect consumes this field only in the TTS branch; the STT branch calls ensureSttLoaded with the display repo/key and discards the snapshot. All three STT sidecars search only the active HF cache, so clicking Run on such an inventory row fails offline or downloads another copy into the active cache instead of using the selected cached checkpoint. Either thread the target through the STT loader/backend or avoid routing secondary-cache ASR rows as locally runnable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and reachable, but deferring rather than fixing here. ensureSttLoaded takes no load-target parameter, and the sidecar's resolve_model_id accepts only a curated id or an owner/model Hub id, raising SttModelIdError otherwise, so threading the snapshot path through the frontend would 422 rather than load. The alternative you suggest, not routing secondary-cache ASR rows, would break the common online case that works today. This is also not a regression: before this PR such a row fell through to chat and evicted the resident model, so routing it is strictly better. The real fix is giving the STT sidecar a load-target, which is a backend change and out of scope for this frontend PR. Tracking it as follow-up.
| export function mediaPageForTask( | ||
| pipelineTag?: string | null, | ||
| ): "images" | "video" | "audio" | undefined { | ||
| const tag = pipelineTag?.toLowerCase().trim(); | ||
| if (tag && AUDIO_PAGE_TASKS.has(tag)) return "audio"; | ||
| return studioPageForTask(pipelineTag); |
There was a problem hiding this comment.
Although this function now identifies speech tasks as runnable on audio, classifyUnslothSupport still derives supportedIn through the audio-free studioPageForTask, whose return type also excludes Audio. Consequently supported TTS and ASR rows get an enabled Run action while the same inspector renders the red “May not be supported” chip; filesystem-backed cache rows can additionally show the misleading “older Hub cache” warning. Extend the support classification to recognize the Audio destination without changing the chat-picker exclusion behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, it is cosmetic. supportedIn feeds only presentation: showUnsupported, unsupportedReason and showOldCacheHint, across models-table, model-card, models-catalog-rows and the inspector. Run enablement is canRunModel, which reads unslothSupport.status and never supportedIn, so nothing about what the button does or where it routes depends on this. A chip that reads wrong on a routable speech row is worth tidying, but it is a label fix rather than a correctness bug and does not belong in this PR.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb0638a3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const pickedTask = curatedAudioInventoryTask({ | ||
| inventoryTask: taskForMediaPick( | ||
| selectedModel.pipelineTag, | ||
| selectedModel.task, |
There was a problem hiding this comment.
For a TTS checkpoint copied into models_dir, LM Studio, or a custom folder, production inventory does not supply the speech task assumed here: studio/backend/routes/models.py::_local_model_task classifies an ordinary audio GGUF architecture as text-generation and returns no task for non-diffusers safetensors. Because such rows also have no hubRepoId, the curated catalog cannot correct that result, so mediaPage remains undefined and Run falls through to selectModel, loading the TTS model into Chat and potentially evicting the resident model. Fresh evidence beyond the prior filesystem-audio thread is the backend classifier itself; the new test only injects a text-to-speech local task that production does not emit. Infer the audio task from supported family evidence or classify these rows in inventory before allowing the Chat fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, though your facts are right. _arch_to_task has no audio branch so an audio GGUF returns text-generation, a non-diffusers safetensors dir returns None, and a filesystem row has no hubRepoId for the curated catalog to correct. The reason it is not a bug in this PR is the before-state: at 74152a8^ the handler already computed mediaPage via studioPageForTask(taskForMediaPick(...)), which returns undefined for both null and text-generation, and fell through to selectModel exactly as it does now. So this fall-through is unchanged by this PR, not introduced by it, and where the PR does touch this shape it improves it, since a classified filesystem TTS row now reaches Audio instead of chat. The remedy you name, classifying these rows in inventory, is a backend change, and inferring the task from family evidence in the frontend would contradict taskForMediaPick's own rule that the backend verdict outranks the tag. Worth doing in the backend, not here. One concession: you are right that my test input is narrower than it looks. baseModelHubId is only set for model_format adapter, so the reachable case is a LoRA adapter whose base model is a Hub TTS repo, not a plain copied checkpoint.
| const runsOnMediaPage = | ||
| inspectorMediaPage !== undefined && | ||
| (inspectorMediaPage !== "audio" || | ||
| audioPickIsRoutable({ |
There was a problem hiding this comment.
On macOS, this enables Run for curated non-GGUF TTS artifacts that have no GGUF sibling, including CSM, Spark-TTS, and OuteTTS. The Audio picker deliberately excludes these via macTtsHubRowIsRunnable, and AudioPage.handleModelSelect immediately rejects the routed handoff through macTtsPickAction because MLX has no TTS decoder. Thus the Hub advertises an enabled Run action that can only navigate to Audio and show an error; include the same macOS/GGUF-sibling gate in Run enablement and routing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking this one, on the same grounds as the Images/Video admission item. macTtsPickAction lives in the Audio page's own policy and is never called by the Hub handler; the page toasts and returns before any load, so there is no eviction and no crash, just an accurate refusal on the page that owns the decision. The line I am holding is that the Hub should fix gates its OWN handler applies, which is why the audioPickIsRoutable mismatch was a real self-contradiction worth fixing, but should not duplicate a destination page's runtime policy, because two copies can then drift apart. You are right that the signals are reachable from the inspector, so this is a deliberate choice rather than a limitation. Also worth noting the direction of travel: before this PR that row loaded a TTS checkpoint into chat, so it now ends at a clear message about MLX having no TTS decoder.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1bb0638 to
25819fd
Compare
NilayYadav
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25819fde6b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
The hub's Run button gates on canChat, but only for non-GGUF rows GgufDownloadCard never receives canRun. So a safetensors image model like unsloth/Z-Image-Turbo-unsloth-bnb-4bit renders a disabled "No run", while the same model in GGUF form runs fine.
The destination already exists: runSelectedModel routes image and video picks to /images and /video. Only the button was unreachable. Enable Run when the model's task maps to one of those pages, reusing the taskForMediaPick + studioPageForTask pair the hub page and chat picker already use for this decision. Chat gates are unchanged for everything
else.