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

[FIX] Fix beam search test #2930

Merged
merged 4 commits into from
Feb 20, 2024
Merged

[FIX] Fix beam search test #2930

merged 4 commits into from
Feb 20, 2024

Conversation

zhuohan123
Copy link
Collaborator

Revert a change in #2869 that causes beam search test to fail.

tests/conftest.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@ronensc
Copy link
Contributor

ronensc commented Feb 20, 2024

Please note that the proposed change currently only reads the first line (i.e., the first prompt) of the file and ignores the others.

AFAIU, the beam search test fails on the second prompt. I believe the fix should be applied to the beam search test. A temporary solution would be to pass only the first prompt to the beam search test. This way, all other tests relying on _read_prompts() would benefit from testing with multiple prompts.

zhuohan123 and others added 3 commits February 20, 2024 11:51
Co-authored-by: Woosuk Kwon <woosuk.kwon@berkeley.edu>
@zhuohan123
Copy link
Collaborator Author

Please note that the proposed change currently only reads the first line (i.e., the first prompt) of the file and ignores the others.

AFAIU, the beam search test fails on the second prompt. I believe the fix should be applied to the beam search test. A temporary solution would be to pass only the first prompt to the beam search test. This way, all other tests relying on _read_prompts() would benefit from testing with multiple prompts.

Thanks for pointing this out! Changed this PR accordingly.

@zhuohan123 zhuohan123 merged commit 63e2a64 into main Feb 20, 2024
21 checks passed
@WoosukKwon
Copy link
Collaborator

@zhuohan123 Let's merge it?

xjpang pushed a commit to xjpang/vllm that referenced this pull request Feb 22, 2024
@zhuohan123 zhuohan123 deleted the fix-beam-search-test branch February 22, 2024 18:47
xjpang pushed a commit to xjpang/vllm that referenced this pull request Mar 4, 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.

None yet

3 participants