Studio: list Ollama models in the chat picker and load their manifest refs - #9237
Conversation
… refs The read-only hub inventory returns Ollama rows whose load id is an opaque ollama-manifest: reference, and the picker deliberately dropped them because nothing resolved that reference on load. Wire up the hand-off the design already documented: POST /load (and validate) now materialize the reference into a .gguf link via materialize_ollama_model_ref, and the picker lists the rows under Custom Folders as direct loads. Ollama rows stay out of the API auto-switch promise: isOllamaLinkPath now also recognizes manifest refs, so apiLoadable remains false, and AUTO_LOAD_LOCAL_SOURCES is untouched. Fixes unslothai#9226
|
Thanks — I reproduced one issue end to end at Fork repro: https://github.com/wasimysaid/unsloth/actions/runs/32273429355. Could you either preserve the supported manifest settings/layers or reject rich manifests explicitly, then reverify with a behavioral test? The malformed/out-of-root ref checks looked good. |
for more information, see https://pre-commit.ci
|
Pushed the fix directly. Studio now withholds Ollama manifests whose template/params/system/messages/adapters or unknown layers cannot be preserved by the raw llama.cpp handoff, and direct opaque refs get a clear 400 instead of silently changing model behavior. Plain model/projector/license manifests remain loadable. Final head is |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
for more information, see https://pre-commit.ci
|
Addressed both Codex findings and pushed them to the PR. The materialized GGUF link is now used only as the load artifact while the opaque Final head @codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed and pushed. Ollama rows now keep both their GGUF format and an explicit direct-artifact marker, so Chat sends GGUF load metadata, matches loaded state without a variant, and still handles names containing Added regression coverage for the direct/variant policy, merged current @codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
unslothai#9237 landed a large Ollama rewrite on main that overlapped this branch in studio/backend/hub/services/models/ollama.py. Resolved by reapplying this branch's refactors on top of main's version and adopting what unslothai#9237 added: - Dropped this branch's is_ollama_manifest_ref; main added the same helper. - The delete now resolves a reference through main's _validated_ollama_manifest_location instead of re-deriving it. That validates against the discovered AND registered Ollama roots, canonicalizes the path, and deletes the symlink-fallback branch this branch had written for the same job. Two validations that could disagree about which references are in bounds is how a delete reaches somewhere it never should have. - unslothai#9237 gave materialization a per-manifest lock, which makes it the second writer of a model's links and blobs. The delete now takes that same lock for the whole plan-and-remove, so a load landing mid-delete cannot re-create the .studio_links directory the delete just collected, nor open a manifest whose blobs are already unlinked. The wait is bounded and answers 409 rather than hanging: a load holds its lease for the length of the load. The links-root and stem-hash refactors reapply unchanged -- main did not touch either -- so the scan and the delete still read one definition of where link files live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3 participants
What
Models pulled with Ollama never appeared in the chat model picker: the hub inventory returned them (
source="ollama"), and the frontend threw the rows away. Fixes #9226.Why it was missing
The exclusion was deliberate, not an oversight.
/api/hub/localscans read-only and hands back an opaqueollama-manifest:reference; the design documented inhub/services/models/ollama.pywas for the load path to resolve it viamaterialize_ollama_model_ref()— but nothing ever called it, so the picker withheld rows that would have failed on click (the old comment inlocal-model-options.tssays exactly this).What this does
Backend —
_resolve_model_identifier_for_request()now materializesollama-manifest:refs into the.gguflink (covers bothPOST /loadand validate; 400 with redacted detail on a bad ref). New test file pins the hand-off, including the outside-known-dirs rejection.Frontend —
"ollama"joinsPICKER_LOCAL_SOURCES/CHAT_LOCAL_SOURCESand theLocalModelInfosource union; rows list under Custom Folders (where the reporter expected them) as direct loads, labelled "Ollama" in Chat's local list.Contracts kept — Ollama stays out of the API auto-switch promise:
isOllamaLinkPath()now also recognizes manifest refs soapiLoadableremains false at both call sites,AUTO_LOAD_LOCAL_SOURCESis untouched (comment updated to state the policy reason, since "not loadable" no longer holds), and all pinned assertions intests/studio/test_model_picker_contracts.pystill pass unchanged.Known limitation
After a restart, a loaded Ollama model's row won't show the loaded badge: the runtime reports the resolved link path while the row id is the manifest ref. In-session selection state works. Happy to follow up with a ref→path identity mapping if you want it in this PR.
Tests
studio/backend/tests/test_ollama_manifest_load_resolution.py— new, 3 testsstudio/backend/hub/tests/test_model_services.py— 262 passtests/studio/test_model_picker_contracts.py— unchanged, 183 pass (1 pre-existing failure on main, unrelated)studio/frontend/tests/chat-local-model-options.test.ts— first test flipped from "withheld" to "offered", per its own stated flip conditiontscclean; kwarg-spacing formatter applied