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.

Sign up for GitHub

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

Jump to bottom

remove floats == 0 comparison #285

Merged
merged 3 commits into from
Jun 28, 2023
Merged

remove floats == 0 comparison #285

merged 3 commits into from
Jun 28, 2023

Conversation

LiuXiaoxuanPKU
Copy link
Collaborator

fix for #71

Ying1123
Ying1123 suggested changes Jun 28, 2023
View reviewed changes
Copy link

@Ying1123 Ying1123 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Could you change it to 1e-5? We also use 1e-5 in FastChat in the main branch now, which is safer for temperature.
https://github.com/lm-sys/FastChat/blob/69d085eadab8e9fcf831a97e17e132658c3d2272/fastchat/serve/inference.py#L141

zhuohan123
zhuohan123 reviewed Jun 28, 2023
View reviewed changes
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.

In addition to @Ying1123's comment, maybe we can set a constant SAMPLING_EPS = 1e-4, and use SAMPLING_EPS to replace all the zeros?

@zhuohan123
Copy link
Member

Could you also help fix the comparisons in sampling_params.py? For example:

vllm/vllm/sampling_params.py

Line 109 in bdd6b4c

if self.temperature > 0.0:

zhuohan123
zhuohan123 approved these changes Jun 28, 2023
View reviewed changes
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! Thank you for your contribution.

@zhuohan123 zhuohan123 merged commit 425040d into vllm-project:main Jun 28, 2023
@zhuohan123 zhuohan123 mentioned this pull request Jun 29, 2023
Closed
michaelfeil pushed a commit to michaelfeil/vllm that referenced this pull request Jul 1, 2023
@LiuXiaoxuanPKU @michaelfeil
remove floats == 0 comparison (vllm-project#285)
c0734b5
@LiuXiaoxuanPKU LiuXiaoxuanPKU deleted the remove_compare branch July 6, 2023 17:55
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
@LiuXiaoxuanPKU
remove floats == 0 comparison (vllm-project#285)
af5c537
sjchoi1 pushed a commit to casys-kaist-internal/vllm that referenced this pull request May 7, 2024
@LiuXiaoxuanPKU
remove floats == 0 comparison (vllm-project#285)
06d56c4
yukavio pushed a commit to yukavio/vllm that referenced this pull request Jul 3, 2024
@derekk-nm
add more models, new num_logprobs (vllm-project#285)
87571b8 
adding the `microsoft/phi-2`, `google/gemma-1.1-2b-it`, and
`HuggingFaceH4/zephyr-7b-gemma-v0.1` models to
test_basic_server_correctness.py. this required increasing the number of
logprobs included in the evaluation to avoid unexpected failure for a
few prompts with these models. this did not negatively impact the other
models.

ran the test locally multiple times.  each time we passed, like this:
```
/root/pyvenv/nmv3119a/bin/python3 /root/.local/share/JetBrains/IntelliJIdea2023.3/python/helpers/pycharm/_jb_pytest_runner.py --target test_basic_server_correctness.py::test_models_on_server -- --forked 
Testing started at 2:24 PM ...
Launching pytest with arguments --forked test_basic_server_correctness.py::test_models_on_server --no-header --no-summary -q in /network/derekk/testdev1/nm-vllm/tests/basic_correctness

============================= test session starts ==============================
collecting ... collected 7 items
Running 7 items in this shard: tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-mistralai/Mistral-7B-Instruct-v0.2-4096-None-None], tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-neuralmagic/OpenHermes-2.5-Mistral-7B-pruned50-4096-sparse_w16a16-None], tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-NousResearch/Llama-2-7b-chat-hf-4096-None-None], tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-neuralmagic/Llama-2-7b-pruned70-retrained-ultrachat-4096-sparse_w16a16-None], tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-microsoft/phi-2-2048-None-None], tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-google/gemma-1.1-2b-it-2056-None-None], tests/basic_correctness/test_basic_server_correctness.py::test_models_on_server[None-5-32-HuggingFaceH4/zephyr-7b-gemma-v0.1-4096-None-None]

test_basic_server_correctness.py::test_models_on_server[None-5-32-mistralai/Mistral-7B-Instruct-v0.2-4096-None-None] 
test_basic_server_correctness.py::test_models_on_server[None-5-32-neuralmagic/OpenHermes-2.5-Mistral-7B-pruned50-4096-sparse_w16a16-None] 
test_basic_server_correctness.py::test_models_on_server[None-5-32-NousResearch/Llama-2-7b-chat-hf-4096-None-None] 
test_basic_server_correctness.py::test_models_on_server[None-5-32-neuralmagic/Llama-2-7b-pruned70-retrained-ultrachat-4096-sparse_w16a16-None] 
test_basic_server_correctness.py::test_models_on_server[None-5-32-microsoft/phi-2-2048-None-None] 
test_basic_server_correctness.py::test_models_on_server[None-5-32-google/gemma-1.1-2b-it-2056-None-None] 
test_basic_server_correctness.py::test_models_on_server[None-5-32-HuggingFaceH4/zephyr-7b-gemma-v0.1-4096-None-None] 

======================== 7 passed in 1332.51s (0:22:12) ========================
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ying1123 Ying1123 Ying1123 approved these changes

@zhuohan123 zhuohan123 zhuohan123 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LiuXiaoxuanPKU @zhuohan123 @Ying1123