-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Update requirements-dev.txt to include package for benchmarking scripts. #3181
Conversation
To add aiohttp, which is required for benchmarking scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wangchen615, thanks for the PR! I have a quick question: Why is the version fixed? Can we just simply use aiohttp
without a specific version?
requirements-dev.txt
Outdated
@@ -21,3 +21,6 @@ einops # required for MPT | |||
openai | |||
requests | |||
ray | |||
|
|||
# Benchmarking | |||
aiohttp==3.9.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the version fixed? Can we just simply use aiohttp
without a specific version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using aiohttp
without a specific version should be okay since we don't use anything special other than the ClientSession
and ClientTimeout
which shouldn't have breaking changes very often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Removed version requirements.
…ts. (vllm-project#3181) Co-authored-by: Zhuohan Li <[email protected]>
…ts. (vllm-project#3181) Co-authored-by: Zhuohan Li <[email protected]>
To add aiohttp==3.9.3, which is required for benchmarking scripts.