-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[ROCm] Add dependencies for ROCm #24900
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
Conversation
bc03779
to
d5d4698
Compare
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.
Code Review
This pull request adds several dependencies to requirements/rocm-test.txt
to support ROCm. The additions of tblib
and timm
align with the existing test dependencies. However, the introduction of lm-eval==0.4.9.1
creates an inconsistency with requirements/test.txt
, which uses a different, git-based version of lm-eval
. This could lead to divergent test behavior between ROCm and other platforms, which is a potential maintenance concern.
28193bd
to
7bbcb2c
Compare
requirements/rocm-test.txt
Outdated
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 believe this can be made a part of the global requirements, to be installed for production similar to https://github.com/vllm-project/vllm/blob/main/docker/Dockerfile#L524
From briefly looking through the uses, there seem to be a couple in the actual code, not just tests
6a544bf
to
81e390e
Compare
Signed-off-by: Yida Wu <yida.wu@amd.com>
81e390e
to
78b6c73
Compare
Signed-off-by: Yida Wu <yida.wu@amd.com>
Signed-off-by: Yida Wu <yida.wu@amd.com> Signed-off-by: charlifu <charlifu@amd.com>
Add dependencies for ROCm