-
Notifications
You must be signed in to change notification settings - Fork 160
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: add support for multimodal model and add embed_image #44
feat: add support for multimodal model and add embed_image #44
Conversation
@staticmethod | ||
def _is_multimodal_model(model_name: str) -> bool: | ||
""" | ||
Check if the embeddings model is multimodal or not. | ||
|
||
Args: | ||
model_name: The embeddings model name. | ||
|
||
Returns: | ||
A boolean, True if the model is multimodal. | ||
""" | ||
return "multimodalembedding" in model_name |
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.
Not sure it makes sense for this to be a static method. This could be an internal @property
that just checks self.model_name
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.
nits: maybe it's time to add a mapping of model names to different properties in one place. we have too many is_this
or is_that
conditions all over the place.
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.
@holtskinner the VertexAI model is happening before the class has been initialized, inside the pydantic validator, so I was assuming I won't have access to the self.model_name
at that point.
@lkuligin to be addressed in another PR? What approach were you thinking about?
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.
@svidiella another PR for general cleanup - sure. maybe smth similar to this approach:
class GoogleModelFamily(str, Enum): |
for this PR - maybe just to keep it consistent, take a look how it's done for chat models / llms, please.
there is a property that uses is_smth
method imported from utils
(to address exactly the point you've mentioned).
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.
@lkuligin I took the approach from GoogleModelFamily
, didn't implement the is_smth
method at the end
Take a look and tell me if you prefer the other apprach.
21278ef
to
85a1b40
Compare
300b9c4
to
2aa6361
Compare
Add support for multimodalembedding model as model type on VertexAIEmbeddings.