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

Set vllm version limit to avoid upstream API incompatibility #292

Closed
wants to merge 1 commit into from

Conversation

vicoooo26
Copy link

According to vllm upstream v0.6.2 release note, beam search has been soft deprecated and attribute use_beam_search in SamplingParams has been removed in v0.6.3. To keep vllm backend available without refactoring, setting vllm max version limit will help.

@vicoooo26 vicoooo26 changed the title Fix: vllm version Set vllm version limit to avoid upstream API incompatibility Oct 21, 2024
@IlyasMoutawwakil
Copy link
Member

Hi, I don't think limiting the version and all the new models/fixes that come with it is justifiable for something like a deprecated argument. I prefer compatibility with the latest version because that's more "fair" when it comes to benchmarks.

@vicoooo26
Copy link
Author

Hi, I do agree with you about 'fairness'. To make it compatible with the latest vllm, maybe we could leverage the log probabilities and replace use_beam_search just like vllm beam search implementation, or we could use some higher API instead of vllm core. What do you think?

@IlyasMoutawwakil
Copy link
Member

Yes I think just removing the beam_search argument from the generate methods for makes total sense, and using the new beam_search method with log probs if ever someone passes a beam search argument, since we don't care whether the output is correct, all we care about is that the model ran the correct amount of forward passes.
Unfortunately I don't have much bandwidth to tackle this, but would appreciate a PR.

@vicoooo26 vicoooo26 changed the title Set vllm version limit to avoid upstream API incompatibility [DRAFT] Set vllm version limit to avoid upstream API incompatibility Oct 23, 2024
@vicoooo26 vicoooo26 closed this Oct 23, 2024
@vicoooo26 vicoooo26 deleted the fix/vllm-version branch October 23, 2024 02:31
@vicoooo26 vicoooo26 restored the fix/vllm-version branch October 23, 2024 02:32
@vicoooo26
Copy link
Author

vicoooo26 commented Oct 23, 2024

Hi, I remove use_beam_search param and let logprobs = 2 * num_beams if a user passes num_beams that greater than 1. If it looks good to you, I will create a new PR or reopen this PR.

@vicoooo26 vicoooo26 changed the title [DRAFT] Set vllm version limit to avoid upstream API incompatibility Set vllm version limit to avoid upstream API incompatibility Oct 24, 2024
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.

2 participants