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

[RFC]: Make any vLLM model a pooling model #10674

Closed
5 of 7 tasks
DarkLight1337 opened this issue Nov 26, 2024 · 6 comments · Fixed by #11469
Closed
5 of 7 tasks

[RFC]: Make any vLLM model a pooling model #10674

DarkLight1337 opened this issue Nov 26, 2024 · 6 comments · Fixed by #11469
Labels

Comments

@DarkLight1337
Copy link
Member

DarkLight1337 commented Nov 26, 2024

Motivation.

Currently, we have to open new PRs to add pooling functionality for existing architectures supported in vLLM. Since the code involved is basically the same for each model, there is potential to automate away this boilerplate.

Proposed Change.

Implement a pooling adapter that can be applied to any existing text generation model in vLLM. To preserve features such as LoRA, PP and multimodality, the adapter simply creates a new subclass of the original model.

The pooling adapter to apply depends on the purpose of the model. To facilitate this, the embedding task will be split up, so that the user can specify which adapter to apply to the model via --task:

Meanwhile, current embedding-related classes will be renamed to avoid confusion between embed and other pooling tasks:

Feedback Period.

1-2 weeks

CC List.

@youkaichao @mgoin @robertgshaw2-neuralmagic @maxdebayser

Any Other Things.

Note that we can still directly map to pooling models in the model registry. This is used when the model architecture has different pooling defaults (e.g. pooling_type=CLS for BERT) or additional modules (e.g. score in Qwen2ForRewardModel). For models that already support pooling, the adapter returns the original model without modifications.

Before submitting a new issue...

  • Make sure you already searched for relevant issues, and asked the chatbot living at the bottom right corner of the documentation page, which can answer lots of frequently asked questions.
@maxdebayser
Copy link
Contributor

I think this makes sense, but we should add a verification to prevent models that are already loaded as embedding models to be wrapped again. Would this wrapper assume a default Pooling configuration or should we raise an error if the user doesn't provide this information on the command line?

@DarkLight1337
Copy link
Member Author

DarkLight1337 commented Nov 28, 2024

I think this makes sense, but we should add a verification to prevent models that are already loaded as embedding models to be wrapped again. Would this wrapper assume a default Pooling configuration or should we raise an error if the user doesn't provide this information on the command line?

I'm thinking of removing the existing embedding model implementations and replacing them with the wrapper. The embedding wrapper's default pooler config is shown in the above example. I'm thinking of resolving the pooler config in the same way as current code (i.e.: user > sentence-transformers > model), so the user doesn't have to provide this information in most cases.

@maxdebayser
Copy link
Contributor

What would happen if the user loads a BertModel without --task-embedding? Would there be a validation to prevent this from happening our would we allow the decoder-only model runner to try to run the model for text generation?

@DarkLight1337
Copy link
Member Author

In is_text_generation_model, we check for the existence of compute_logits and sample methods. Since those methods aren't in BertModel, --task generation is not allowed for that model.

@maxdebayser
Copy link
Contributor

Ok, I think this makes sense.

@DarkLight1337 DarkLight1337 changed the title [RFC]: Make any model an embedding model [RFC]: Make any model an pooling model Nov 29, 2024
@DarkLight1337
Copy link
Member Author

Update: I have expanded the scope of this RFC to cover all pooling-based models.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants