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

Add tests for models #922

Merged
merged 13 commits into from
Sep 1, 2023
Merged

Add tests for models #922

merged 13 commits into from
Sep 1, 2023

Conversation

WoosukKwon
Copy link
Collaborator

@WoosukKwon WoosukKwon commented Aug 31, 2023

This PR adds basic tests for the vLLM models (that can run on a single GPU). The tests compare the outputs of HF and vLLM when using greedy sampling. NOTE: vLLM currently does not pass some of the tests.

@zhuohan123 I think you can extend HfRunner and VllmRunner for the tests for #857.

TODOs (in the future PRs):

  • Make vLLM pass all the tests.
  • Add tests for tensor_parallel_size > 1.
  • Cache HF outputs to speed up the test execution.

@WoosukKwon WoosukKwon changed the title Add tests for model outputs Add tests for models Aug 31, 2023
Copy link
Member

@zhuohan123 zhuohan123 left a comment

Choose a reason for hiding this comment

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

Thanks! Left some comments.

max_new_tokens=max_tokens)


@pytest.fixture
Copy link
Member

Choose a reason for hiding this comment

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

What is the main reason to add @pytest.fixture decorator compared to directly use these runners in the test function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To my understanding, there's no good way to import these runners into tests. Please let me know if there is.

tests/models/test_models.py Outdated Show resolved Hide resolved
@WoosukKwon
Copy link
Collaborator Author

@zhuohan123 Thanks for your comments. I've updated the PR. PTAL.

Copy link
Member

@zhuohan123 zhuohan123 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 PR!

@WoosukKwon WoosukKwon merged commit 32b6816 into main Sep 1, 2023
2 checks passed
@WoosukKwon WoosukKwon deleted the model-test branch September 1, 2023 02:19
@WoosukKwon WoosukKwon restored the model-test branch September 1, 2023 02:20
@WoosukKwon WoosukKwon deleted the model-test branch September 1, 2023 02:20
liuyanyi pushed a commit to liuyanyi/vllm that referenced this pull request Sep 12, 2023
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
sjchoi1 pushed a commit to casys-kaist-internal/vllm that referenced this pull request May 7, 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