Skip to content

Conversation

@dongxianzhe
Copy link
Collaborator

Add an MMEmbeddingVLM interface for the encode service that loads only the vision model.

const ModelInputParams& input_params) = 0;

virtual torch::Tensor logits(const torch::Tensor& hidden_states,
const torch::Tensor& seleted_idxes) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is seleted_idxes a spelling mistake issue? selected_idxes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed seleted_idxes to selected_idxes.

virtual std::vector<torch::Tensor> encode(
const ModelInputParams& input_params) = 0;

virtual torch::Tensor logits(const torch::Tensor& hidden_states,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to define these interface in MMEmbeddingVLM, move these to MMEmbeddingVLMImpl

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Moved the empty implementation from MMEmbeddingVLM to MMEmbeddingVLMImpl.

}

MMEmbeddingVLMFactory ModelRegistry::get_mm_embedding_vlm_factory(
const std::string& name) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe need yo rename :(

register_vlm_mm_embedding_factory ----> vlm_mm_embedding

and

get_mm_embedding_vlm_factory ----> mm_embedding_vlm

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Rename register_vlm_mm_embedding_factory to register_mm_embedding_vlm_factory.

@yq33victor
Copy link
Collaborator

LGTM

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