-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Enable setting a default embedding model in the stack #3803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Enable setting a default embedding model in the stack #3803
Conversation
b8168ff
to
f8cb3c4
Compare
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
fc87c1e
to
86c1e3b
Compare
raise ValueError( | ||
f"Multiple embedding models marked as default_configured=True: {model_ids}. " | ||
"Only one embedding model can be marked as default." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be checked when Stack was initialized instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added additional validation when the stack is initialized (as well as a test to confirm) for this I think we can keep it here as well in case we allow for models to be dynamically registered and a second default were to slip in. Let me know if you'd like me to remove it though. 👍
# Embedding model was provided but dimension wasn't, look it up | ||
embedding_dimension = await self._get_embedding_dimension_for_model(embedding_model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just call this in _get_default_embedding_model_and_dimension
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. 👍
if param_name in body: | ||
value = body.get(param_name) | ||
if param_name in exclude_params: | ||
converted_body[param_name] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe unrelated to this PR, but reading due to the change below: do we only allow one such parameter? if so, assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a break above that silently skips the others. i can add some validation to the above if we want.
"embedding_dimension": 768, | ||
"default_configured": True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: we should formalize these parameters in a EmbeddingModel
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i can do that as a follow up
Signed-off-by: Francisco Javier Arceo <farceo@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG!
What does this PR do?
Enables automatic embedding model detection for vector stores and by using a
default_configured
boolean that can be defined in therun.yaml
.Test Plan
Spin up the stack:
Then test with OpenAI's client:
Previously you needed:
The
extra_body
is now unnecessary.