-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[DOCs] Update ROCm installation docs section #24691
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
Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
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 updates the ROCm installation documentation to align with newer dependency versions and practices, which is a valuable improvement. I've identified a couple of areas for enhancement to ensure clarity and adherence to best practices. Specifically, there's a minor version inconsistency in an example that could confuse users, and a deprecated installation command is being introduced. Addressing these points will make the documentation more robust and user-friendly.
if [ ! -f setup.py ]; then cd python; fi | ||
python3 setup.py install |
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.
The command python3 setup.py install
is a legacy command and is deprecated. It's recommended to use pip install .
instead, which is the modern standard for installing packages from source. This will correctly handle dependencies and use the PEP 517 build process. The previous version of the documentation correctly used pip3 install .
, so this change is a regression.
if [ ! -f setup.py ]; then cd python; fi | |
python3 setup.py install | |
if [ ! -f setup.py ]; then cd python; fi | |
pip install . |
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.
This mimics the way it is installed in the ROCm docker image for consistency.
Install ROCm's flash attention (v2.7.2) following the instructions from [ROCm/flash-attention](https://github.com/ROCm/flash-attention#amd-rocm-support) | ||
Alternatively, wheels intended for vLLM use can be accessed under the releases. | ||
|
||
For example, for ROCm 6.3, suppose your gfx arch is `gfx90a`. To get your gfx architecture, run `rocminfo |grep gfx`. |
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.
The documentation has been updated to use ROCm 6.4 in the PyTorch installation step, but this example for flash attention still refers to ROCm 6.3. To maintain consistency throughout the document and avoid confusion for users, please update this to refer to ROCm 6.4.
For example, for ROCm 6.3, suppose your gfx arch is `gfx90a`. To get your gfx architecture, run `rocminfo |grep gfx`. | |
For example, for ROCm 6.4, suppose your gfx arch is `gfx90a`. To get your gfx architecture, run `rocminfo |grep gfx`. |
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.
ROCm support goes 2 versions back, so 6.3 and above are all currently supported. The installation instructions assume the lowest
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 overall, but Gemini's comments seem valid
Added explanations |
Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
Bringing the ROCm installation part of the docs up to date
Updated the dependency repositories and recommended commits to match the Dockerfile.rocm_base
Removed some parts that are no longer relevant, namely the deprecated rocm:vllm-dev:navi_base