Feature: add vllm openai api endpoint#49
Merged
pantonante merged 1 commit intorelari-ai:mainfrom Mar 18, 2024
Merged
Conversation
Contributor
There was a problem hiding this comment.
👍 Looks good to me!
- Reviewed the entire pull request up to 284c0e0
- Looked at
30lines of code in1files - Took 1 minute and 17 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
2additional comments because they didn't meet confidence threshold of50%.
1. continuous_eval/llm_factory.py:106:
- Assessed confidence :
80% - Grade:
0% - Comment:
The OpenAI client is instantiated with a hardcoded 'unused' API key. This might cause issues if the OpenAI client requires a valid API key to function, even if the base_url is different. Please confirm if this is the intended behavior. - Reasoning:
The PR adds support for a new type of model, 'vllm_', and it seems to be implemented correctly. However, there's a potential issue with the way the OpenAI client is instantiated for this new model type. The api_key is hardcoded as 'unused', which might cause issues if the OpenAI client actually requires a valid API key to function, even if the base_url is different. I need to check the OpenAI client's documentation or source code to confirm this.
2. continuous_eval/llm_factory.py:120:
- Assessed confidence :
80% - Grade:
0% - Comment:
The condition to check if the API key is not 'unused' might cause issues if other model types also use the 'unused' API key. Please confirm if this is the intended behavior. - Reasoning:
The PR adds a condition to check if the API key is not 'unused' before making a request in JSON mode. This seems to be a workaround for the hardcoded 'unused' API key for the 'vllm_' model type. However, this might cause issues if other model types also use the 'unused' API key. I need to check if other parts of the codebase also use the 'unused' API key.
Workflow ID: wflow_4NvnotezU58pN1QT
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
Contributor
|
Thank you for contributing, this looks great to me! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of all: Cool project 💯
This PR adds
vllmbased endpoints.Just use:
And your vllm deployed model should work :-)
Summary:
This PR adds support for
vllmmodels in theLLMFactoryclass and modifies the_llm_responsemethod to handle cases where theapi_keyis 'unused'.Key points:
vllmmodels inLLMFactoryclass incontinuous_eval/llm_factory.py.vllmmodel is initialized with a base URL from theVLLM_BASE_URLenvironment variable._llm_responsemethod to handle cases whereapi_keyis 'unused'.Generated with ❤️ by ellipsis.dev