Skip to content

Commit 29cfcfe

Browse files
committed
fix(embeddings): one rule for a configured Ollama server, and stop forcing its width
Fourth round of review findings: - Make the setup wizard's EMBEDDING_OUTPUT_DIMS optional and undefaulted for Ollama; requiring it suppressed the server-side width detection entirely, and its 768 default was wrong for a 384-wide model - Replace three different notions of 'an Ollama server is configured' with one predicate shared by the selector, the width lookup, and the embedding client: self-hosted is served by the loopback default as the chat provider already is, and only hosted must be pointed at a server - Document that an ollama/ model id is taken at face value rather than falling back, so a model absent from the server fails creation
1 parent 906f94f commit 29cfcfe

5 files changed

Lines changed: 54 additions & 31 deletions

File tree

apps/docs/content/docs/platform/self-hosting/environment-variables.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ See [Observability](/platform/self-hosting/observability).
190190

191191
| Variable | Description |
192192
|----------|-------------|
193-
| `KB_EMBEDDING_MODEL` | Embedding model for new knowledge bases. Defaults to `text-embedding-3-small`; use `ollama/<model>` for a model on your own Ollama. An unsupported value falls back to the default |
193+
| `KB_EMBEDDING_MODEL` | Embedding model for new knowledge bases. Defaults to `text-embedding-3-small`; use `ollama/<model>` for a model on your own Ollama. An unrecognised hosted model id falls back to the default, but an `ollama/` id is taken at face value — if that model is not on the server, knowledge-base creation fails rather than falling back |
194194
| `EMBEDDING_OUTPUT_DIMS` | Vector width new knowledge bases are stored at: `384`, `768`, `1024`, `1536` (default), or `3072`. It must be a width the chosen model can emit; anything else falls back to `1536` with a warning |
195195
| `OPENROUTER_API_KEY` | Fallback route for the OpenAI embedding models — used when it is set and `OPENAI_API_KEY` is not the chosen path |
196196
| `COHERE_API_KEY` | Enables the Knowledge block reranker |

apps/sim/lib/embeddings/client.test.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,11 +1431,24 @@ describe('ollama embeddings', () => {
14311431
).rejects.toThrow('has 1024 unexpected dimensions; expected 768')
14321432
})
14331433

1434-
it('refuses to fall back to the loopback default when OLLAMA_URL names no server', async () => {
1435-
await expect(
1436-
embed(['hello'], { model: 'ollama/nomic-embed-text', dimensions: 768, projectInputs: null })
1437-
).rejects.toThrow('OLLAMA_URL must be configured for Ollama embeddings')
1438-
expect(fetchMock).not.toHaveBeenCalled()
1434+
/**
1435+
* A self-hosted deployment runs alongside its own Ollama, so the loopback
1436+
* default is a working configuration that needs no env var — the same rule
1437+
* the chat provider and the block's model selector apply. Requiring the
1438+
* variable here would make embedding stricter than the list that offers the
1439+
* models.
1440+
*/
1441+
it('serves a self-hosted deployment from the loopback default', async () => {
1442+
fetchMock.mockResolvedValue(jsonResponse(ollamaBody([[1, 2, 3]], 768)))
1443+
1444+
const result = await embed(['hello'], {
1445+
model: 'ollama/nomic-embed-text',
1446+
dimensions: 768,
1447+
projectInputs: null,
1448+
})
1449+
1450+
expect(fetchMock.mock.calls[0][0]).toBe('http://localhost:11434/api/embed')
1451+
expect(result.dimensions).toBe(768)
14391452
})
14401453

14411454
it('ignores a caller-supplied key rather than sending one Ollama cannot use', async () => {

apps/sim/lib/embeddings/client.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {
1515
readResponseJsonWithLimit,
1616
readResponseTextWithLimit,
1717
} from '@/lib/core/utils/stream-limits'
18-
import { getOllamaUrl, isOllamaUrlConfigured } from '@/lib/core/utils/urls'
18+
import { getOllamaUrl } from '@/lib/core/utils/urls'
1919
import {
2020
DEFAULT_EMBEDDING_MODEL,
2121
type EmbeddingModelInfo,
@@ -25,6 +25,7 @@ import {
2525
resolveDimensions,
2626
} from '@/lib/embeddings/catalog'
2727
import { resolveProviderKey } from '@/lib/embeddings/keys'
28+
import { isOllamaServerConfigured } from '@/lib/embeddings/ollama-model-catalog.server'
2829
import { DEFAULT_OPENROUTER_EMBEDDING_MODEL } from '@/lib/embeddings/openrouter-models'
2930
import { getAdapterFactory } from '@/lib/embeddings/providers'
3031
import {
@@ -364,13 +365,15 @@ async function resolveProvider(model: string, options: EmbedOptions): Promise<Re
364365
/**
365366
* Ollama runs on the deployment's own server and takes no credential, so it
366367
* resolves before every key-bearing path and ignores a caller-supplied key
367-
* rather than pretending one applies. `OLLAMA_URL` must name a server: the
368-
* loopback default `getOllamaUrl` falls back to is a development convenience,
369-
* and silently indexing a knowledge base against a localhost that answers
370-
* nothing is worse than saying so.
368+
* rather than pretending one applies.
369+
*
370+
* A self-hosted deployment may leave `OLLAMA_URL` unset and be served by the
371+
* loopback default, exactly as the chat provider is; only hosted Sim, which
372+
* runs no Ollama, has to be pointed at one. Requiring the variable everywhere
373+
* would have made this stricter than the selector that offers the models.
371374
*/
372375
if (info.provider === 'ollama') {
373-
if (!isOllamaUrlConfigured()) {
376+
if (!isOllamaServerConfigured()) {
374377
throw new Error('OLLAMA_URL must be configured for Ollama embeddings')
375378
}
376379
const baseUrl = getOllamaUrl().replace(/\/+$/, '')

apps/sim/lib/embeddings/ollama-model-catalog.server.ts

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,22 @@ export interface OllamaEmbeddingModel {
2727
dimensions?: number
2828
}
2929

30+
/**
31+
* Whether this deployment has an Ollama server to talk to.
32+
*
33+
* Self-hosted deployments run alongside their own Ollama, so the loopback
34+
* default `getOllamaUrl` falls back to is a working configuration that needs no
35+
* env var — the same rule the chat provider's model route applies. Hosted Sim
36+
* runs none, so there the address must be stated or there is nothing to reach.
37+
*
38+
* Exported because the selector, this module's width lookup, and the embedding
39+
* client all have to answer it identically: a selector that lists models the
40+
* client then refuses to embed with is worse than either behaviour alone.
41+
*/
42+
export function isOllamaServerConfigured(): boolean {
43+
return !isHosted || isOllamaUrlConfigured()
44+
}
45+
3046
export class OllamaEmbeddingModelNotFoundError extends Error {
3147
constructor(model: string) {
3248
super(
@@ -109,12 +125,8 @@ export async function fetchOllamaEmbeddingModelCatalog(
109125
async function loadOllamaEmbeddingModelCatalog(
110126
signal?: AbortSignal
111127
): Promise<{ models: OllamaEmbeddingModel[]; unreachable?: string }> {
112-
/**
113-
* Hosted Sim runs no Ollama, and the loopback default cannot answer there, so
114-
* an unconfigured hosted deployment is not dialled at all. An explicit
115-
* `OLLAMA_URL` states an intent to reach a real server and is still honoured.
116-
*/
117-
if (isHosted && !isOllamaUrlConfigured()) return { models: [] }
128+
/** Nothing to dial: an unconfigured hosted deployment has no server to reach. */
129+
if (!isOllamaServerConfigured()) return { models: [] }
118130

119131
let names: string[]
120132
try {
@@ -177,13 +189,8 @@ export async function getOllamaEmbeddingModelMetadata(
177189
): Promise<Required<OllamaEmbeddingModel>> {
178190
const name = isOllamaEmbeddingModel(model) ? ollamaEmbeddingModelName(model) : model
179191
if (!name) throw new OllamaEmbeddingModelNotFoundError(model)
180-
/**
181-
* The same requirement `resolveProvider` enforces before embedding. Without it
182-
* a developer machine with an unset `OLLAMA_URL` and a local server answering
183-
* on the loopback default would resolve a width here and create a knowledge
184-
* base that every later embedding call refuses to serve.
185-
*/
186-
if (!isOllamaUrlConfigured()) {
192+
/** The same rule the selector and the embedding client apply. */
193+
if (!isOllamaServerConfigured()) {
187194
throw new OllamaUnreachableError('OLLAMA_URL is not configured')
188195
}
189196

packages/sim-setup/src/capability-config.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -952,16 +952,16 @@ export const KNOWLEDGE_EMBEDDINGS_SETUP = defineCapabilitySetup(KNOWLEDGE_EMBEDD
952952
},
953953
{
954954
/**
955-
* Asked rather than derived: Sim cannot know what a local model emits
956-
* until it is running, and a width that does not match is the one
957-
* misconfiguration this path cannot detect for the operator.
955+
* Optional and undefaulted on purpose. Sim reads the width from the
956+
* server when this is unset, which is more reliable than any answer
957+
* the operator can give here — and a default of 768 would be wrong
958+
* for a 384-wide model such as `all-minilm` while also suppressing
959+
* the lookup that would have got it right.
958960
*/
959961
type: 'field',
960962
key: 'EMBEDDING_OUTPUT_DIMS',
961963
input: 'text',
962-
required: true,
963-
defaultValue: '768',
964-
hint: 'must equal what the model emits: 384, 768, 1024, 1536, or 3072',
964+
hint: 'optional — leave blank and Sim reads the width from your Ollama server',
965965
validate: true,
966966
},
967967
],

0 commit comments

Comments
 (0)