Support local embeddings in KnowPro; configurable embedding size and batch size - #3096
Conversation
Add embedding.size and embedding.maxBatchSize to the typed config (TYPEAGENT_EMBEDDING_SIZE / _MAX_BATCH_SIZE). KnowPro now sizes indexes from getEmbeddingSize() instead of a hardcoded 1536, so the local MiniLM provider (384-d) works out of the box.
| const MaxBatchSize = 64; | ||
| const DefaultMaxBatchSize = 64; | ||
|
|
||
| export type CopilotEmbeddingOptions = { |
There was a problem hiding this comment.
Why not just call this EmbeddingOptions?
There was a problem hiding this comment.
disregard...thought I was looking at embeddingProvider.ts.
| const LocalDefaultEmbeddingSize = 384; // Xenova/all-MiniLM-L6-v2 | ||
| const HostedDefaultEmbeddingSize = 1536; // ada-002 / text-embedding-3-small | ||
|
|
||
| function readPositiveInt(name: string): number | undefined { |
There was a problem hiding this comment.
It might be useful to warn if this reads a non-positive integer here. Right now this just swallows the misconfiguration and just continues so no one's the wiser.
| /** | ||
| * The embedding vector size for the configured provider. An explicit | ||
| * `embedding.size` (`TYPEAGENT_EMBEDDING_SIZE`) always wins; otherwise the | ||
| * default model's size is used (set `size` when using a non-default model) (384 for the local MiniLM model, 1536 for |
There was a problem hiding this comment.
THis comment and the values above can drift. I recommend reusing the constant names here so if their values change the comment is still valid.
| }); | ||
| case "copilot": | ||
| return createCopilotEmbeddingModel( | ||
| process.env[EmbeddingEnvVars.MODEL]?.trim() || |
There was a problem hiding this comment.
We moved away from .env config a while ago but there are still places where we go directly to env vars. We should avoid adding new ones and instead should work to get rid of existing process.env references. Everything should migrate to the YAML based configuration: https://github.com/microsoft/TypeAgent/tree/main/ts/packages/config.
These new configuration options should go into the config package and that can do the proper process.env, .env, or YAML loading and then this package can just use the values.
| ? {} | ||
| : { | ||
| model: settings.modelName, | ||
| model: options?.modelName ?? settings.modelName, |
There was a problem hiding this comment.
Here you're overriding a pool setting with the supplied options...maybe another good place to make a n informational logging call so there's enough trace information to troubleshoot issues if needed.
| embeddingModel = tryCreateEmbeddingModel(); | ||
| embeddingSize ??= getEmbeddingSize(); | ||
| } | ||
| embeddingSize ??= 1536; |
There was a problem hiding this comment.
We should modify this line to use the configured default from aiclient.embeddingProvider.
Follow-up to microsoft#3096. - `embeddingProvider` reads the `embedding:` section from the typed runtime config instead of `process.env`. - Config warns when a positive-integer setting (e.g. `embedding.size`) is invalid, instead of ignoring it silently. - `getEmbeddingSize` doc refers to the default-size constants, so it cannot drift. - `createEmbeddingModel` logs model/batch-size overrides of the pool settings (`typeagent:openai`). - KnowPro uses `getEmbeddingSize()` for caller-supplied models too, instead of a hardcoded 1536.
Adds support for local embeddings in KnowPro, and makes the embedding size and batch size configurable:
embedding.size/embedding.maxBatchSizeconfig (TYPEAGENT_EMBEDDING_SIZE/_MAX_BATCH_SIZE).getEmbeddingSize()(384 for local, 1536 for hosted) instead of a hardcoded 1536. Caller-supplied models keep the old default.model,size, andmaxBatchSizetoo.