Studio: customizable RAG embedding model with HF search, settings tab reorganization#6800
Conversation
…tabs Chat with files, project sources, and knowledge bases previously always embedded with unsloth/bge-small-en-v1.5. This adds a Settings option to pick any Hugging Face embedding model (or local path), with HF search autocomplete, server-side verification that the repo is actually an embedding model, and a save anyway escape hatch for offline or local models. The setting persists in app_settings and applies at runtime to both the sentence-transformers and llama-server GGUF embedder backends without a restart. Also reorganizes the General settings tab: Documents & RAG sits above Uploads, Helper LLM moved above the danger zone, and Model auto-switch (OpenAI API) moved to the bottom of the API tab.
for more information, see https://pre-commit.ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for customizable RAG embedding models, allowing users to configure and persist custom models via the settings. It includes backend utility functions for validation and caching, updated llama-server backend logic to dynamically resolve model paths and handle model changes, new API endpoints, frontend integration with a Hugging Face model search combobox, and comprehensive tests. The review feedback suggests several improvements, including refining GGUF repository detection to avoid false positives, optimizing the frontend combobox by removing redundant state, defensively handling null search results, and sanitizing Hugging Face access tokens across both the frontend and backend to prevent API client errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return model | ||
| return f"{model}-GGUF" |
There was a problem hiding this comment.
Avoid using a simple raw substring check for 'gguf' to prevent false positives (e.g., matching a model name like bigguf-model or ggufy that is not actually a GGUF repo). Instead, split the repository name into segments using delimiters like dashes, underscores, and dots, and verify if the tag appears as a whole segment.
| return model | |
| return f"{model}-GGUF" | |
| import re | |
| if any(seg == "gguf" for seg in re.split(r"[-_.]", model.lower())): | |
| return model |
References
- When statically detecting model types or tags from repository IDs or names, avoid simple raw substring checks to prevent false positives. Instead, split the repository name into segments using delimiters like dashes, underscores, and dots, and verify if the tag appears as a whole segment.
There was a problem hiding this comment.
The current code already matches whole segments: _names_gguf splits on any non alphanumeric run (re.split r"[^a-z0-9]+"), which is a superset of the dash/underscore/dot delimiters suggested, so bigguf and ggufy do not match. Covered by test_effective_gguf_repo_ignores_gguf_substring.
| const [inputValue, setInputValue] = useState(value); | ||
| const selectingRef = useRef(false); | ||
| const anchorRef = useRef<HTMLDivElement>(null); | ||
| const debouncedQuery = useDebouncedValue(inputValue); | ||
|
|
||
| useEffect(() => { | ||
| setInputValue(value); | ||
| }, [value]); |
There was a problem hiding this comment.
The local inputValue state and its corresponding useEffect to sync with the value prop are redundant because the component is fully controlled by the parent (which updates draftEmbeddingModel on every keystroke). This causes unnecessary double-renders on every keystroke. You can remove the inputValue state and useEffect entirely, and pass the value prop directly to useDebouncedValue.
| const [inputValue, setInputValue] = useState(value); | |
| const selectingRef = useRef(false); | |
| const anchorRef = useRef<HTMLDivElement>(null); | |
| const debouncedQuery = useDebouncedValue(inputValue); | |
| useEffect(() => { | |
| setInputValue(value); | |
| }, [value]); | |
| const selectingRef = useRef(false); | |
| const anchorRef = useRef<HTMLDivElement>(null); | |
| const debouncedQuery = useDebouncedValue(value); |
References
- To reset a component's state when a specific prop changes, prefer using React's built-in key prop instead of manually tracking the previous prop value in state and triggering a state update during render.
There was a problem hiding this comment.
Already addressed in an earlier commit: the combobox is fully controlled now, the inputValue state and sync effect were removed and value feeds useDebouncedValue directly.
| const { results, isLoading } = useHubModelSearch(debouncedQuery, { | ||
| task: EMBEDDING_TASKS, | ||
| accessToken, | ||
| excludeGguf: true, | ||
| enabled: !disabled, | ||
| }); |
There was a problem hiding this comment.
Sanitize the accessToken before passing it to useHubModelSearch. If the token is a malformed string (such as 'undefined', 'null', or empty/whitespace), the Hugging Face API client may throw an error instead of gracefully falling back to anonymous access for public repositories.
| const { results, isLoading } = useHubModelSearch(debouncedQuery, { | |
| task: EMBEDDING_TASKS, | |
| accessToken, | |
| excludeGguf: true, | |
| enabled: !disabled, | |
| }); | |
| const sanitizedToken = useMemo(() => { | |
| if (!accessToken) return undefined; | |
| const trimmed = accessToken.trim(); | |
| if (!trimmed || trimmed.toLowerCase() === "undefined" || trimmed.toLowerCase() === "null") { | |
| return undefined; | |
| } | |
| return trimmed; | |
| }, [accessToken]); | |
| const { results, isLoading } = useHubModelSearch(debouncedQuery, { | |
| task: EMBEDDING_TASKS, | |
| accessToken: sanitizedToken, | |
| excludeGguf: true, | |
| enabled: !disabled, | |
| }); |
There was a problem hiding this comment.
The token comes from the chat runtime store, which trims on write, and useHubModelSearch passes it as a Bearer header only when truthy. A literal 'undefined'/'null' string cannot reach this prop from our store, so the extra sanitizing would be dead code. Leaving as is.
| const items = useMemo(() => { | ||
| const ids = results.map((item) => item.id); | ||
| const selected = value.trim(); | ||
| if (selected && !ids.includes(selected)) { | ||
| ids.push(selected); | ||
| } | ||
| return ids; | ||
| }, [results, value]); |
There was a problem hiding this comment.
Defensively handle cases where results might be undefined or null (e.g., during error states or initial loading) to prevent a potential runtime TypeError when calling results.map.
| const items = useMemo(() => { | |
| const ids = results.map((item) => item.id); | |
| const selected = value.trim(); | |
| if (selected && !ids.includes(selected)) { | |
| ids.push(selected); | |
| } | |
| return ids; | |
| }, [results, value]); | |
| const items = useMemo(() => { | |
| const ids = (results || []).map((item) => item.id); | |
| const selected = value.trim(); | |
| if (selected && !ids.includes(selected)) { | |
| ids.push(selected); | |
| } | |
| return ids; | |
| }, [results, value]); |
There was a problem hiding this comment.
useHubModelSearch initializes results to [] and never sets it to null or undefined (error paths keep the previous array), so the guard cannot fire. Leaving as is.
| if model != default_embedding_model() and not payload.force: | ||
| if not is_embedding_model(model, hf_token = payload.hf_token or None): |
There was a problem hiding this comment.
Sanitize the hf_token before passing it to is_embedding_model. If the token is a malformed string (such as 'undefined', 'null', or whitespace), the Hugging Face API client may throw an error instead of gracefully falling back to anonymous access for public repositories.
if model != default_embedding_model() and not payload.force:
hf_token = payload.hf_token
if hf_token:
hf_token = hf_token.strip()
if not hf_token or hf_token.lower() in ("undefined", "null"):
hf_token = None
if not is_embedding_model(model, hf_token = hf_token):There was a problem hiding this comment.
Already addressed in an earlier commit: the route trims the payload token and passes None when empty ((payload.hf_token or '').strip() or None). Covered by test_put_embedding_model_trims_hf_token. The 'undefined'/'null' literal handling would be new behavior not used elsewhere in the codebase, so leaving that part out.
| from huggingface_hub import hf_hub_download, list_repo_files | ||
|
|
||
| repo = config.EMBED_GGUF_REPO | ||
| token = os.environ.get("HF_TOKEN") or None |
There was a problem hiding this comment.
Sanitize the HF_TOKEN environment variable before passing it to list_repo_files and hf_hub_download. If the token is a malformed string (such as 'undefined', 'null', or whitespace), the Hugging Face API client may throw an error instead of gracefully falling back to anonymous access for public repositories.
| token = os.environ.get("HF_TOKEN") or None | |
| token = os.environ.get("HF_TOKEN") | |
| if token: | |
| token = token.strip() | |
| if not token or token.lower() in ("undefined", "null"): | |
| token = None |
There was a problem hiding this comment.
os.environ.get('HF_TOKEN') or None is the existing pattern across the backend for this env var; special casing literal 'undefined'/'null' strings here would diverge from every other call site. Leaving as is for consistency.
![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: 394d721039
ℹ️ 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".
| candidates = [repo] | ||
| model = config.effective_embedding_model() | ||
| if model != repo: | ||
| candidates.append(model) |
There was a problem hiding this comment.
When a user saves a local SentenceTransformer directory on a CPU/macOS install where RAG_EMBED_BACKEND=auto picks llama-server (checked core/rag/embeddings.py), the settings route accepts it because is_embedding_model returns true for local paths with modules.json. These new candidates then treat that filesystem path as a Hugging Face repo for list_repo_files/hf_hub_download, so the next document upload fails instead of using the configured embedder. Please either reject local ST paths for the GGUF backend or handle local GGUF/ST paths explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in earlier commits: _resolve_local_gguf handles local files/dirs without hub probing, and the settings route returns 409 when a local dir has no .gguf on the llama-server backend (test_put_local_dir_without_gguf_on_llama_backend_409). A dir without GGUF now fails resolution with a clear error instead of being treated as a hub repo.
| filename = sorted(match, key = len)[0] | ||
| logger.info("resolving GGUF embedder %s/%s", repo, filename) | ||
| self._model_path = hf_hub_download(repo_id = repo, filename = filename, token = token) | ||
| self._model_repo = config.effective_gguf_repo() |
There was a problem hiding this comment.
If the embedding setting changes while this HF download/listing is in progress, this line rereads the global setting and can tag the just-downloaded old model path as belonging to the new effective repo. _current() will then consider the live server current for the new setting even though _spawn_once started it with the old model_path, so queries keep using the wrong vectors until another restart. Capture the effective repo key once at the start of resolution and store that key after the download instead of rereading it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in an earlier commit: _resolve_model_path captures desired = config.effective_gguf_repo() once at entry and tags _model_repo with that captured key after the download, so a mid-download settings change leaves the server marked stale and _ensure_ready respawns. Covered by test_resolve_tags_path_with_repo_captured_at_entry.
| "you may be offline)." | ||
| ), | ||
| ) | ||
| set_rag_embedding_model(model) |
There was a problem hiding this comment.
If any document has already created chunks_vec with the default model's dimension, saving a different-width embedder here (for example a 768-dim model after the 384-dim default) leaves the sqlite-vec table at the old embedding float[dim] because ensure_vec uses CREATE VIRTUAL TABLE IF NOT EXISTS. Subsequent re-indexing with the new model will still write vectors into the old-dimension table and fail; the setting needs to reject dimension changes with existing vectors or rebuild/drop the vec index as part of the switch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in an earlier commit: ensure_vec now drops and recreates chunks_vec when the requested dimension differs from the existing column (with a warning log), and search_dense returns [] on a width mismatch instead of erroring. Covered by test_ensure_vec_rebuilds_on_dim_change and test_dense_dim_mismatch_returns_empty.
| reindexWarning: | ||
| "Only affects newly indexed documents. Re-upload existing ones after changing the model.", |
There was a problem hiding this comment.
This tells users to re-upload existing documents after changing the model, but start_ingestion dedupes completed documents by scope and SHA before chunking/embedding, returns the old document, and deletes the new upload. In that scenario following this instruction does not regenerate vectors under the new embedder; either include the embedding model in the dedupe key/rebuild path or tell users they must delete the old document first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in an earlier commit: start_ingestion records embedding_model per document and treats a completed duplicate indexed under a different model as stale, re-ingesting instead of deduping, so the re-upload instruction now actually re-indexes. Covered by test_reupload_after_model_change_reingests. As of 1a79a63 the old document is also kept until the replacement completes.
| # Token for gated/private repos during verification (not stored). | ||
| hf_token: Optional[str] = Field(default = None, max_length = 512) |
There was a problem hiding this comment.
This token is only used for the metadata verification call; the actual embedding loaders are not given it (LlamaServerBackend falls back to os.environ.get('HF_TOKEN'), and the SentenceTransformer path loads by model name only). A gated/private embedding repo can therefore verify and save with the UI token, then fail on the first indexing request unless the same token also exists in the server environment; persist or thread the token through the later download/load path, or don't accept it for verification-only saves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. Threading the UI token into the later loaders means persisting a credential server side, which this PR deliberately avoids (the payload token is verification-only and not stored). Gated repos work by setting HF_TOKEN in the server environment, which both loaders already read. Leaving as a documented limitation rather than quietly persisting tokens.
| if model == EMBEDDING_MODEL: | ||
| return EMBED_GGUF_REPO |
There was a problem hiding this comment.
When RAG_EMBEDDING_MODEL is set to a custom model but RAG_EMBED_GGUF_REPO is not set, EMBEDDING_MODEL already equals that custom value, so this branch returns the hardcoded/default GGUF repo instead of deriving <custom>-GGUF. On CPU/macOS where auto selects llama-server, users who configure only RAG_EMBEDDING_MODEL will still index/search with the bge GGUF model rather than their configured embedder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in an earlier commit: effective_gguf_repo only short circuits when RAG_EMBED_GGUF_REPO is explicitly set; otherwise it compares against DEFAULT_EMBEDDING_MODEL, so an env-selected RAG_EMBEDDING_MODEL derives its -GGUF companion. Covered by test_env_custom_model_derives_companion.
…saves Found by simulation testing of the embedding model setting: Local paths saved as the embedding model now work on the llama-server GGUF backend (the default backend on macOS and CPU). A path to a .gguf file is used directly and a directory is scanned for a variant-matching non-mmproj .gguf, with a clear error when none exists. Previously a local path was sent to the HF hub API and failed with a repo lookup error. Saving the default model explicitly no longer stores an override, so is_custom stays false and the UI does not show a reset button for the default value.
|
Ran a dedicated simulation pass over this branch in an isolated uv venv (Python 3.12, minimal deps, throwaway UNSLOTH_STUDIO_HOME so no real install is touched). 84 simulation tests across six areas:
Two real issues found and fixed in the follow-up commit:
After the fixes: 84 simulation tests pass, the 63 branch tests plus surrounding RAG suites pass in both the real Studio venv and the clean sandbox venv, and the live server was restarted and re-verified end to end (default state, explicit default save staying non-custom, custom save, 409 on a non-embedding model, forced local path save, reset). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf616ac7fc
ℹ️ 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".
| for a ~1.5x speedup at negligible accuracy loss.""" | ||
| global _model, _name | ||
| name = model_name or config.EMBEDDING_MODEL | ||
| name = model_name or config.effective_embedding_model() |
There was a problem hiding this comment.
When a user changes the embedding setting while an upload is running, this fallback to config.effective_embedding_model() is re-evaluated on each default token_counter/encode path because the RAG upload routes start ingestion without a model_name. The worker can therefore chunk with the old tokenizer and embed later batches with the new model (or mix embedder batches after _name reloads), corrupting vectors within a single document/KB. Capture the effective model once when the ingestion job starts and pass it through instead of reading the mutable setting on each batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. start_ingestion now resolves the effective model once and passes it into the worker thread, so chunk token counting and every embed batch use the model captured at job start regardless of settings changes mid-run. Fixed in 1a79a63, covered by test_ingestion_pins_model_across_setting_change.
| except Exception: | ||
| stored = None |
There was a problem hiding this comment.
When a custom RAG embedder is saved and a transient app_settings read error occurs (for example while the settings DB is briefly unavailable), this handler turns the failure into None and then caches it, so effective_embedding_model() falls back to the default on the embed/search hot path for up to 2 seconds. That can make a running ingestion batch or a query vector use the default model against documents indexed with the custom model, corrupting or hiding results; keep the last known value or propagate the error instead of caching a default fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. A store read failure now keeps the last known cached value (re-arming the TTL) instead of caching a None that reverts the hot path to the default model. Fixed in 1a79a63, covered by test_stored_value_survives_transient_read_error.
| if model != default_embedding_model() and not payload.force: | ||
| if not is_embedding_model(model, hf_token = payload.hf_token or None): |
There was a problem hiding this comment.
On CPU/macOS installs where RAG_EMBED_BACKEND=auto selects llama-server (or when that backend is configured explicitly), this check accepts any Hugging Face repo whose metadata says it is an embedding model, but the runtime then needs a .gguf file in either <model>-GGUF or the model repo. A normal SentenceTransformer repo with no GGUF companion will save successfully here and only fail on the next document upload/search when _resolve_model_path() cannot find a .gguf; either verify a GGUF candidate for the active backend or reject these saves unless forced with a clearer warning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The save route now also verifies GGUF availability for HF repos when the llama-server backend is active: it lists the derived -GGUF companion and the repo itself and returns 409 when neither has a .gguf (force still bypasses). Fixed in 1a79a63, covered by test_put_hf_repo_without_gguf_on_llama_backend_409 and test_hf_gguf_backend_error_checks_candidates.
Review follow-ups, each verified by new tests: Re-uploading a document after an embedding model change now re-indexes instead of deduping by content hash. Documents record the embedder that produced their vectors (lazy embedding_model column, NULL legacy rows keep deduping) and a mismatch replaces the old document. A vector width change no longer bricks the dense index. ensure_vec drops and recreates chunks_vec when the dim changes (old vectors are in a foreign space and only block inserts) and search_dense returns empty on a width mismatch instead of surfacing a vec0 error, so lexical search keeps working until documents are re-uploaded. Saving a local sentence-transformers folder with no .gguf now returns 409 with a clear message when the install embeds via llama-server, instead of failing at first index. force still saves. A custom RAG_EMBEDDING_MODEL env without RAG_EMBED_GGUF_REPO now derives the -GGUF companion repo instead of silently keeping the bge GGUF on CPU and macOS installs. The resolved GGUF path is tagged with the repo captured at entry, so a setting change during a download cannot mark the old model as current. GGUF repo detection matches gguf as a whole name segment rather than a substring, hf_token is trimmed before verification, and the settings combobox drops a redundant state mirror of its controlled value.
|
Went through every review comment, verified each against the code, and pushed fixes for the ones that held up. Summary: Implemented, each with new tests:
Not implemented:
Full test state after the fixes: 134 backend tests across the settings and RAG suites pass, the frontend typechecks and builds, and an end to end run on a live install (upload, chunk, real GGUF embed, dense and hybrid search, dedupe re-upload, local dir 409, reset) passes. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d9905eea6
ℹ️ 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".
| if dim is not None and dim != len(vector): | ||
| # Embedding model switched widths and nothing re-indexed yet; the stale | ||
| # table cannot answer new-model queries (vec0 errors on the MATCH). | ||
| return [] |
There was a problem hiding this comment.
When switching between embedders with the same width (for example two 384-dim SentenceTransformer models), this dimension-only guard does not fire, so the query below still searches chunks_vec rows produced by the old model with a query vector from the new model. The new embedding_model value is stored only on documents and is not used by dense retrieval, so hybrid/dense results can mix incompatible vector spaces until every old document is re-uploaded; the dense index needs to be keyed, filtered, or rebuilt based on the effective embedding model, not just vector length.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. search_dense now takes the effective embedding model, over-fetches, and drops hits whose document was indexed under a different model (NULL legacy rows stay served, matching the dedupe rule), so same-width switches no longer mix vector spaces. Fixed in 1a79a63, covered by test_dense_filters_same_width_stale_model.
| needles = ( | ||
| rag_config.EMBEDDING_MODEL.split("/")[-1].lower(), | ||
| rag_config.EMBED_GGUF_REPO.split("/")[-1].lower(), | ||
| rag_config.effective_embedding_model().split("/")[-1].lower(), |
There was a problem hiding this comment.
When the saved embedding model is a local path with a generic basename such as /tmp/models/model, this turns model into a substring needle. _is_hidden_model is used by the local and cached model listing endpoints, so after saving that path it filters out unrelated chat models whose id or path merely contains model; local paths should be hidden by exact resolved path (or an exact repo id), not by basename substring.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. _is_hidden_model now distinguishes repo ids from local paths: repo ids keep the basename needle (cache paths embed it), while local paths are hidden by exact resolved path only, so a generic basename like model cannot substring-hide unrelated chat models. Fixed in 1a79a63, covered by test_is_hidden_model_local_path_matches_exactly.
| if empty_completed or stale_model: | ||
| # A prior ingest of identical bytes yielded zero chunks (e.g. a scanned | ||
| # PDF uploaded before a vision model loaded). Re-ingest, don't dedupe. | ||
| # PDF uploaded before a vision model loaded), or was embedded with a | ||
| # different model. Re-ingest, don't dedupe. | ||
| store.delete_document(conn, existing) |
There was a problem hiding this comment.
When a duplicate upload is considered stale because the embedding model changed, this deletes the existing completed document before the replacement worker has parsed, embedded, or stored anything. If the new model was force-saved incorrectly, is gated without a runtime token, or otherwise fails during ingestion, the previously searchable document and its stored upload are gone and the user is left with only a failed replacement; keep the old row/chunks until the new ingestion completes successfully or make the replacement atomic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The stale duplicate is no longer deleted up front: the replacement worker deletes the old document and its upload only after the new ingestion completes successfully, so a failed re-index leaves the searchable original intact. Fixed in 1a79a63, covered by test_failed_reingest_preserves_old_document.
The combobox wrapper applies className to the outer input group, so the size utility must target the inner input element; the previous text-xs never reached it and the field rendered at the browser default.
52740dd to
519a1cc
Compare
|
Moved the fits-on-device model filter out of this PR into #6802 to keep this one scoped to the RAG embedding model setting. This branch now only contains the embedding model work. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52740ddc52
ℹ️ 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".
| # The env/default model needs no verification; saving it is a no-op override. | ||
| if model != default_embedding_model() and not payload.force: | ||
| hf_token = (payload.hf_token or "").strip() or None | ||
| if not is_embedding_model(model, hf_token = hf_token): |
There was a problem hiding this comment.
On llama-server installs, a valid local GGUF file or directory cannot be saved with the normal Save flow because this calls is_embedding_model before the new local-GGUF resolver is considered. That helper only returns true for local paths containing a SentenceTransformer modules.json, so GGUF-only paths always get a 409 and require “Save anyway” despite _resolve_local_gguf supporting them; check local GGUF paths before this HF/ST metadata gate so supported offline embedders are accepted without a misleading verification failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. When the llama-server backend is active and the value resolves as a local GGUF file or directory, the save now skips HF verification entirely (the artifact is what the backend loads), so no Save anyway is needed. Fixed in 1a79a63, covered by test_put_local_gguf_saves_without_verification.
| stale_model = ( | ||
| doc is not None | ||
| and doc.get("embedding_model") is not None | ||
| and doc.get("embedding_model") != effective_model |
There was a problem hiding this comment.
When the same file uploaded under model A is still pending/running and the user changes the embedder before uploading the same bytes again, stale_model becomes true here because the document status is not checked. start_ingestion then deletes the row while the first worker can still continue and insert chunks/FTS/vec rows for a document id that no longer exists, polluting retrieval indexes and leaving that job inconsistent; only completed rows should be treated as replaceable stale vectors, or the running job must be canceled/awaited.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. stale_model now requires the existing document to be completed, so a pending or running duplicate dedupes against the in-flight row instead of having it deleted under a live worker. Fixed in 1a79a63, covered by test_inflight_duplicate_not_deleted_on_model_change.
| matchesCapability(row.capabilities, deferredCapabilityFilter) && | ||
| (!activeChannel?.finetunableOnly || isUnslothFinetunable(row.result)), | ||
| (!activeChannel?.finetunableOnly || isUnslothFinetunable(row.result)) && | ||
| (!fitOnDeviceOnly || hfModelFitsDevice(row.result, gpu)), |
There was a problem hiding this comment.
On the default Discover feed (no query/section), this predicate only filters the Latest list via filteredDiscoverRows; the Trending cards passed to HubFeed are built separately from feedTrendingRows without checking fitOnDeviceOnly, so turning on the new toolbar toggle still shows oversized models above the filtered list. Apply the same fit predicate to the feed rows so the toggle actually hides non-fitting models throughout the Discover feed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This file is no longer part of this PR: the fits-on-device filter moved to #6802 to keep this one scoped to the embedding model setting. The feed/trending gap is fixed there (feedTrendingRows applies the same fit predicate).
| with self._dim_lock: | ||
| self._dim = None |
There was a problem hiding this comment.
When a Settings model change races with a llama-server dim() probe, dim() can already be holding _dim_lock while its internal encode() re-enters _ensure_ready() for the new repo; the respawn path then reaches this lock while the lifecycle restart is in progress, so that request can hang indefinitely (or lock-order deadlock with another restart). Clear the cached dimension without taking the same lock from _resolve_model_path, or avoid calling encode() while dim() holds it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The dim lock is gone: probes are idempotent (a duplicate one-text encode is benign) and the cached width is now cleared and set by plain assignment, so dim() -> encode() -> _ensure_ready() -> _resolve_model_path() can no longer self-deadlock on a mid-probe model change. Fixed in 1a79a63.
The empty-query listing was the global top-downloads page, which holds no unsloth mirrors for the unsloth-first float to reorder, so the dropdown opened on third-party models. Match the model picker: curated unsloth listing when empty, whole-Hub search once a query is typed.
519a1cc to
50477fa
Compare
Keep the last known embedding model on settings store errors, remove the re-entrant dim lock in the llama-server backend, accept local GGUF saves and verify GGUF availability for HF repos on that backend, match local path embedders exactly in model list filters, drop same-width stale vectors from dense search, pin the embedder per ingestion job, and only replace completed documents after the re-index succeeds.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35c5d6fea1
ℹ️ 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".
| def _current(self) -> bool: | ||
| """Alive AND serving the effective repo (a Settings model change makes a | ||
| live server stale).""" | ||
| return self._process_alive() and self._model_repo == config.effective_gguf_repo() |
There was a problem hiding this comment.
On llama-server installs (explicitly configured, or auto on CPU/macOS), ingestion now pins effective_model at job start, but this staleness check still follows the mutable global setting while the backend's encode/token_counter ignore their model_name argument. If the embedding setting changes during a multi-batch upload, _ensure_ready() can restart onto the new repo between tokenize/embed calls while the document row remains tagged with the old pinned model, mixing vector spaces. Fresh evidence is that this backend still compares _model_repo to config.effective_gguf_repo() instead of a caller-supplied model key.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, deferring as a known limitation. Ingestion pins the model at job start, but the llama-server backend is a shared singleton whose encode/token_counter ignore model_name and follow the global effective repo, so a Settings change during an active multi-batch upload can respawn the subprocess between batches. Honoring the pinned model here means threading a caller-supplied vector-space key through _ensure_ready/_current (and deciding how concurrent jobs on different models share one subprocess), which is the same backend-keying change as the store/rag_db comments. It requires the user to change the model mid-upload, which the UI already advises against, so I am grouping it into that one follow-up rather than a partial fix. Noted.
| # table cannot answer new-model queries (vec0 errors on the MATCH). | ||
| return [] | ||
| # Over-fetch when filtering so stale-model hits don't starve the top-k. | ||
| fetch = k * 3 if embedding_model else k |
There was a problem hiding this comment.
When a same-width embedding switch leaves many old-model chunks in a scope, this applies the KNN LIMIT before filtering by documents.embedding_model. If the nearest 3 * k neighbors are all stale, search_dense returns fewer or even zero dense hits even though current-model chunks exist just after that prefix, so newly re-uploaded documents can remain undiscoverable until stale docs are deleted. Fresh evidence is that the fixed path uses a hard-coded k * 3 prefilter rather than filtering in SQL or continuing until it has k valid hits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real gap, deferring with the same follow-up. The k*3 prefilter can miss current-model hits when more than 3k stale-model neighbors precede them. A fully correct fix needs the model filter pushed into the KNN itself (a vector-space partition/metadata column on chunks_vec) since sqlite-vec cannot post-filter a MATCH by a joined documents column without possibly starving the top-k. That is the per-model partitioning work tracked with the two comments below; I did not want to ship a wider-but-still-heuristic prefilter that only moves the boundary. Noted.
| sha = _sha256_file(stored_path) | ||
| conn = rag_db.get_connection() | ||
| try: | ||
| effective_model = model_name or config.effective_embedding_model() |
There was a problem hiding this comment.
When RAG_EMBED_GGUF_REPO is set, config.effective_gguf_repo() always returns that explicit repo, so the llama-server backend keeps producing the same vectors regardless of the Settings embedding-model label. Recording effective_embedding_model() here makes those same GGUF vectors look stale after any later Settings change, and dense search filters the old documents out even though they are still in the active vector space; use the backend's actual vector-space key (or block Settings changes under an explicit GGUF override) for the stored/filter model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real edge case, deferring as a known limitation. It only triggers when RAG_EMBED_GGUF_REPO is explicitly pinned AND the Settings label is then changed: the vectors stay in the pinned GGUF space but the document gets keyed by the new label, so dense search filters them out. The correct fix is to key documents by the actual vector-space identifier (the GGUF repo on the llama backend, the model name on the ST backend) rather than the label, which needs a backend-aware key threaded through ingestion and search. That is the same partitioning change comments on embed_llama_server.py and rag_db.py point at, so I would rather do it as one deliberate change than bolt on a partial version here. Noted for a follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f87a757cd9
ℹ️ 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".
| and not (_llama_backend_active() and _resolves_as_local_gguf(model)) | ||
| ): | ||
| hf_token = (payload.hf_token or "").strip() or None | ||
| if not is_embedding_model(model, hf_token = hf_token): |
There was a problem hiding this comment.
When the active backend is llama-server and the user enters a Hugging Face GGUF repo id directly (for example a custom *-GGUF repo with embedding weights but without sentence-transformers/feature-extraction metadata), this metadata gate returns 409 before the GGUF-file check below can run. The runtime already treats GGUF-named models as loadable via effective_gguf_repo(), so a supported online GGUF repo incorrectly requires “Save anyway”; skip this metadata check or verify GGUF availability first for GGUF repo ids on the llama backend.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5314b6536. On the llama-server backend a GGUF-named repo id now skips the sentence-transformers metadata gate (GGUF repos rarely carry that metadata) and is verified via GGUF availability instead, so a valid online GGUF embedder saves without Save anyway. Non-GGUF repos still go through the embedding-metadata check, so a plain chat repo is still rejected. Verified: a GGUF-named repo with is_embedding_model=False now returns 200, while a non-GGUF repo failing metadata still returns 409.
| existing, | ||
| int(dim), | ||
| ) | ||
| conn.execute("DROP TABLE chunks_vec") |
There was a problem hiding this comment.
If a user switches to a different-width embedding model while an old upload is still running, a newer upload under the current model can create/populate chunks_vec first, and then the old pinned job can finish later and hit this DROP TABLE because its vector width differs. That deletes the dense vectors for the current-model document and leaves dense search returning no hits for the active model until another reindex; gate rebuilds on the job still matching the active vector-space key or store separate vec tables/partitions per model.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, deferring with the same follow-up. If a width-changing model switch happens while an old-width upload is still running, the old job finishing late can hit ensure_vec and drop the current-model table. The safe fix is to gate the rebuild on the job still matching the active vector-space key, or per-model vec partitions, which is the backend-keying change tracked in the comments above. The sequential UI flow (change model, then re-upload) does not hit this; it needs a concurrent in-flight upload across a width change. Noted as a known limitation for the partitioning follow-up rather than a partial guard here.
| return cached[1] | ||
| return None | ||
| value = _coerce_embedding_model(stored) | ||
| _cached = (now, value) |
There was a problem hiding this comment.
When an embedding-model save races with a hot-path read that started just before the commit, that reader can fetch the old app_setting and then write it back into _cached after set_rag_embedding_model() has invalidated the cache. The PUT may then report the old model, or an immediate upload/search can use the old model for the full TTL after the user saved the new one; protect cache updates with a lock/generation so stale pre-save reads cannot repopulate the cache.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5314b6536. get_stored_embedding_model now captures a generation counter (bumped by every write/invalidate) before the DB read and only fills the cache if the generation is unchanged afterward, so a read that overlapped a save can no longer repopulate the cache with the pre-save value for the TTL. Verified with a stub where a save commits and invalidates mid-read: the overlapping read returns the old value once, but the next read returns the new value instead of a poisoned cache.
Accept a GGUF-named HF repo on the llama-server backend by verifying GGUF availability instead of the sentence-transformers metadata gate, and guard the settings cache with a generation counter so a read overlapping a save cannot repopulate it with the pre-save value.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 833e4553f2
ℹ️ 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".
| if p.is_file() and p.suffix.lower() == ".gguf": | ||
| return str(p) |
There was a problem hiding this comment.
When the saved value points directly at a local *mmproj*.gguf projector file, this branch accepts it as a valid local GGUF and the settings route skips HF verification because _resolves_as_local_gguf() succeeds. The directory and Hub paths both exclude mmproj, but a direct file path does not, so the next llama-server spawn will try to load a projector as the embedding model and fail after the setting has already been saved.
Useful? React with 👍 / 👎.
| if _cached is not None: | ||
| _cached = (time.monotonic(), _cached[1]) | ||
| return _cached[1] | ||
| return None |
There was a problem hiding this comment.
Fresh evidence after the earlier cache fix is that this no-cache exception path still returns None, so after a process restart (or before any successful read) a transient app_settings failure makes get_rag_embedding_model() fall back to the default model even if a custom embedder is persisted. If an upload or query hits that window, it can index/search with the default vector space instead of failing or preserving the stored override.
Useful? React with 👍 / 👎.
| if _HF_REPO_ID_RE.match(model): | ||
| needles.append(model.split("/")[-1].lower()) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier basename fix is that an existing relative local path like models/model matches this repo-id regex before any filesystem resolution, so it is treated as an HF repo and adds model as a substring needle. After saving that local embedder, the local/cache model endpoints can again hide unrelated chat models whose id or path merely contains model; check for an existing local path before applying the repo-id heuristic.
Useful? React with 👍 / 👎.
|
|
||
| try: | ||
| raw = (rag_config.EMBED_BACKEND or "auto").strip().lower() | ||
| key = embeddings._resolve_auto() if raw in embeddings._AUTO_ALIASES else raw |
There was a problem hiding this comment.
On RAG_EMBED_BACKEND=auto with a visible GPU, this returns the sentence-transformers choice without exercising the same ST load/probe path that _get_backend() uses; if ST is broken and the real backend later falls back to llama-server, the settings save skips the GGUF availability check and can accept a normal embedding repo with no GGUF weights. The next upload then fails in the fallback backend while resolving <model>-GGUF, so auto detection here needs to account for the supported llama fallback or be conservative when it can occur.
Useful? React with 👍 / 👎.
| desired = config.effective_gguf_repo() | ||
| if self._model_path is not None and self._model_repo == desired: | ||
| return self._model_path | ||
| local = self._resolve_local_gguf(config.effective_embedding_model()) |
There was a problem hiding this comment.
When RAG_EMBED_GGUF_REPO is set, config.effective_gguf_repo() says that env override always wins, but this still resolves the saved Settings value as a local GGUF before downloading desired. If a user saves a local .gguf, the backend serves that file while tagging _model_repo with the explicit repo; because desired stays constant, later Settings changes also hit the cache at lines 151-152 and keep serving the local file instead of the admin-configured repo.
Useful? React with 👍 / 👎.
1 participant
What
Studio Chat with files, project sources, and knowledge bases always embedded documents with
unsloth/bge-small-en-v1.5. This PR adds a Settings option so users can pick their own embedding model, plus a small reorganization of the settings tabs.Embedding model setting
Settings -> General -> Documents & RAG has a new "Embedding model" field:
sentence-similarity,feature-extraction), with unsloth models pinned first. Free text still works for exact ids and local paths.is_embedding_model). Unverifiable models (wrong type, gated, offline) return 409 and the UI offers "Save anyway" for offline installs and local paths.Backend
utils/embedding_model_settings.py: persistsrag_embedding_modelinapp_settings(same pattern as the upload limit setting), with validation and a short TTL cache since the value is consulted on the embedder hot path.GET/PUT/DELETE /api/settings/embedding-model.core/rag/config.pygainseffective_embedding_model()andeffective_gguf_repo(). Precedence: saved setting, thenRAG_EMBEDDING_MODELenv, then the bge default. An explicitRAG_EMBED_GGUF_REPOenv still wins for the GGUF repo.-GGUFcompanion repo, falling back to the model repo itself if the companion does not exist. The subprocess tracks which repo it serves and respawns with a fresh dim probe when the setting changes, so no restart is needed. The sentence-transformers backend already reloaded on name change.Settings tab reorganization
Testing
test_embedding_model_settings.pycovering the settings module, effective config resolution including GGUF repo derivation, and all three routes (verified save, 409 on unverifiable, force bypass, reset).test_rag_embed_llama_server.py; existing RAG suites (test_rag_embeddings,test_rag_embed_llama_server,test_rag_ingestion,test_rag_retrieval,test_rag_store, settings route tests) pass.nomic-ai/nomic-embed-text-v1.5through the UI, confirmed a chat model is rejected with a clear error, confirmed force save and reset, and confirmed the persisted value drives both embedder backends.Notes
There is currently no reindex pipeline, so switching models leaves previously indexed chunks in the old vector space until documents are re-uploaded. The UI states this next to the field. A follow-up could add a per-KB model check with a rebuild action.