Honour EMBEDDER_PROVIDER, and allow model/dimensions from the environment - #65
Open
a-optic wants to merge 1 commit into
Open
Honour EMBEDDER_PROVIDER, and allow model/dimensions from the environment#65a-optic wants to merge 1 commit into
a-optic wants to merge 1 commit into
Conversation
…ironment .env.example documents EMBEDDER_PROVIDER and tells the reader to "set EMBEDDER_PROVIDER and provide the matching API key below", but EmbedderConfig only ever read `provider` from config/settings.yaml. Setting the documented variable had no effect. The same is true of `model` and `dimensions`, which is the harder problem in a container deployment: settings.yaml is baked into the image at build time, so running a different embedding model means editing a tracked file and rebuilding. That is also the file most likely to conflict on every upgrade, for anyone not using the default model. All three now follow the env-over-config precedence that max_chars and chunk_overlap already use, so a deployment can select its model and vector width without touching a tracked file. A set-but-empty variable is treated as unset rather than as a configured empty string: .env files routinely carry blank placeholders, and letting one through would silently select no provider at all. dimensions is validated as a positive integer, matching the existing max_chars and chunk_overlap checks. Verified against a running deployment: no env set -> settings.yaml wins (arctic-embed2, 1024) EMBEDDER_MODEL/-DIMENSIONS set -> env wins (mxbai-embed-large, 512) EMBEDDER_MODEL="" or " " -> falls back to settings.yaml EMBEDDER_DIMENSIONS=0 -> ValueError Note this does not change where the SQL dimension comes from: schema.sql and migration 015 still hardcode 768, so a deployment using a different width still has to manage the column itself. Happy to follow up on that separately if you'd like it driven from the same setting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A documented variable that does nothing
.env.exampleshipsEMBEDDER_PROVIDER=ollamaand tells the reader to "set EMBEDDER_PROVIDER and provide the matching API key below".EmbedderConfigonly ever readsproviderfromconfig/settings.yaml, so setting the documented variable has no effect.The harder half: model and dimensions
modelanddimensionsare config-file-only too, and in a container deploymentconfig/settings.yamlis baked into the image at build time. Running a different embedding model therefore means editing a tracked file and rebuilding — and that file is then the one most likely to conflict on every upgrade, for anyone not using the default model.The change
All three now follow the same env-over-config precedence
max_charsandchunk_overlapalready use:Two details worth calling out:
.envfiles routinely carry blank placeholders, and letting one through would silently select no provider at all.dimensionsis validated as a positive integer, matching the existingmax_chars/chunk_overlapchecks..env.exampledocuments the two new variables, including the constraint that they must agree with the width of theembeddingcolumn.Verification
Against a running deployment:
Scope
This does not change where the SQL dimension comes from —
schema.sqland migration015_embedding_dim_change.sqlstill hardcode 768, so a deployment on a different width still manages the column itself. Happy to follow up on driving that from the same setting if you'd want it.