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

Remove Sampler copy stream #2209

Merged
merged 3 commits into from
Dec 20, 2023
Merged

Conversation

Yard1
Copy link
Collaborator

@Yard1 Yard1 commented Dec 20, 2023

We don't really need an extra stream in sampler to still benefit from non-blocking execution. This will add 5-10 microseconds of overhead, which I think is an acceptable tradeoff for stronger consistency guarantees.

Supersedes #2190 and #2177

cc @esmeetu @ZiyueHuang

@Yard1
Copy link
Collaborator Author

Yard1 commented Dec 20, 2023

@esmeetu can you see if this fixes your issue?

@esmeetu
Copy link
Collaborator

esmeetu commented Dec 20, 2023

@esmeetu can you see if this fixes your issue?

Yes. This is ok for me.

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.

@WoosukKwon WoosukKwon merged commit bd29cf3 into vllm-project:main Dec 20, 2023
2 checks passed
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 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