KEMBAR78
Add comments on RoPE initialization by WoosukKwon · Pull Request #1176 · vllm-project/vllm · GitHub
Skip to content

Conversation

@WoosukKwon
Copy link
Collaborator

@WoosukKwon WoosukKwon commented Sep 25, 2023

Added more comments that can be useful for understanding the differences from HF.

@casper-hansen
Copy link
Contributor

casper-hansen commented Sep 26, 2023

Does this explain the difference between HF and vLLM? I.e. if you enable the same CPU initialization with greedy sampling, we get same outputs?

@WoosukKwon
Copy link
Collaborator Author

@casper-hansen Not always. Because floating-point arithmetics is not associative, different kernel implementations might lead to different outputs. The difference is more significant when reduction operation is involved. Therefore, our custom CUDA kernels for attention and RMS normalization do not produce exactly the same outputs as the original HF implementation, and thus outputs of vLLM models can be different from the HF models. We've checked that the outputs usually match when using FP32 and greedy sampling, but there are some cases where the outputs do not match. However, please note that this does not hurt task accuracy, as vLLM's implementation is mathematically equivalent to HF's.

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 fix!

@WoosukKwon WoosukKwon merged commit 03ffd0a into main Sep 26, 2023
@WoosukKwon WoosukKwon deleted the fix-rope branch September 26, 2023 17:48
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
yiliu30 pushed a commit to yiliu30/vllm-fork that referenced this pull request May 8, 2025
Fix accuracy issue for llama 3.2 vision models that is caused by
is_causal setting to False
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.

3 participants