-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
[Frontend] Chat-based Embeddings API #9759
Conversation
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
This pull request has merge conflicts that must be resolved before it can be |
3cc07d5
to
9cd1ac3
Compare
Sorry for the commit spam, I'm done refining the docs now. |
Entrypoints tests pass now. |
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.
Overall LGTM since entrypoint tests all passed. See if @ywang96 is OK with the frontend refactor as well.
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 left a few comments but overall this looks good to me.
Since VLM2Vec has the same model architecture as Phi-3.5-Vision, we have to explicitly pass ``--task embedding`` | ||
to run this model in embedding mode instead of text generation mode. | ||
|
||
Since this schema is not defined by OpenAI client, we post a request to the server using the lower-level ``requests`` library: |
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.
Just leaving this as a thought here: should we perhaps have a fork of the openai client that support our extensions explicitly?
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 sounds good, but not sure whether we have bandwidth to maintain it 😅
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 suggest opening an issue for this.
@@ -7,7 +7,7 @@ class PoolingParams( | |||
msgspec.Struct, | |||
omit_defaults=True, # type: ignore[call-arg] | |||
array_like=True): # type: ignore[call-arg] | |||
"""Pooling parameters for pooling. | |||
"""Pooling parameters for embeddings API. |
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 might be missing something, but the additional_data
attribute doesn't seem to be used anywhere. Which is good, because it can by anything and is passed without validation from the request to the Pooler.forward()
method as part of the PoolingMetadata
object. If there is no use case for this, can we remove it in this PR?
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.
@robertgshaw2-neuralmagic originally added this (#4800 (comment)). I am not sure whether this is still relevant since we can now set the pooling strategy via CLI (#9697).
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.
@robertgshaw2-neuralmagic can you comment on this?
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.
Meanwhile let's merge this PR first.
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.
Left a few comments - PTAL!
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.
LGTM!
This PR extends the existing Embeddings API to accept chat conversations similar to Chat Completions API. This enables multi-modal conversations to be passed to the embedding model.
To reduce code duplication, I've also factored out the common code for handling completion and chat-based inputs into the base
OpenAIServing
class.FIX #8967
FIX #9303 (comment)