fix: Read embedder URL from env #9936
Conversation
I had hardcoded it before 🤦
Dschoordsch
force-pushed
the
fix/readEmbedderUrlFromEnv
branch
from
697cc83
to
707aea0
Compare
| const availableModels = | ||
| process.env.AI_EMBEDDING_MODELS && JSON.parse(process.env.AI_EMBEDDING_MODELS) | ||
| return availableModels.find( | ||
| ({model}: {model?: string}) => model?.split(':')[1] === 'llmrails/ember-v1' |
There was a problem hiding this comment.
@Dschoordsch why is llmrails/ember-v1 hardcoded? That's just a possibility but in the future we might have other thing there. I might be lacking of JS knowledge to read this expression 😺
There was a problem hiding this comment.
You're right, it's hardcoded. It's also hardcoded here:
I tried to strike the balance between getting stuff out now vs. making it future proof. Even if we add more models, we would need to test which one to use here and adjust accordingly. Having it configurable via environment for example add some complexity which might never be needed.
There was a problem hiding this comment.
If I'm not mistaking, on that file TextEmbeddingsInference.ts you have that model but there is another one there too.
There was a problem hiding this comment.
If I'm not mistaking, on that file TextEmbeddingsInference.ts you have that model but there is another one there too.
I had hardcoded it before 🤦
Demo
[If possible, please include a screenshot or gif/video, it'll make it easier for reviewers to understand the scope of the changes and how the change is supposed to work. If you're introducing something new or changing the existing patterns, please share a Loom and explain what decisions you've made and under what circumstances]
Testing scenarios
[Please list all the testing scenarios a reviewer has to check before approving the PR]
Scenario A
Scenario B
Final checklist