Skip to content
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

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

svidiella
Copy link
Contributor

@svidiella svidiella commented Feb 28, 2024

Add support for multimodalembedding model as model type on VertexAIEmbeddings.

  • Add embed_image method, only valid for multimodalembedding model.
  • Make embed_documents and embed_query only available for text models.
  • TODO: adapt embded_documents and embded_query to also work with multimodalembedding models.

Comment on lines 378 to 389
@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
Copy link
Collaborator

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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).

Copy link
Contributor Author

@svidiella svidiella Feb 29, 2024

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.

@svidiella svidiella force-pushed the feat/multimodal-embeddings branch from 21278ef to 85a1b40 Compare February 29, 2024 11:24
@svidiella svidiella force-pushed the feat/multimodal-embeddings branch from 300b9c4 to 2aa6361 Compare February 29, 2024 14:35
@lkuligin lkuligin merged commit b72e14c into langchain-ai:main Feb 29, 2024
12 checks passed
@svidiella svidiella deleted the feat/multimodal-embeddings branch February 29, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants