-
Notifications
You must be signed in to change notification settings - Fork 296
[MUSA] Add shell script to generate requirements-musa.txt and update doc #1175
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: Xiaodong Ye <xiaodong.ye@mthreads.com>
Summary of ChangesHello @yeahdongcn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances LightLLM's compatibility by adding explicit support for Moore Threads (MUSA) GPUs. It provides a streamlined process for users to set up the necessary Python environment for MUSA hardware, ensuring that incompatible CUDA-specific dependencies are excluded and MUSA-specific libraries are included. The accompanying documentation updates guide users through this new installation path, making LightLLM more accessible to a broader range of hardware platforms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@shihaobai Could you please take a look? Thanks. |
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 support for MUSA GPUs by introducing a script to generate a MUSA-specific requirements file and updating the installation documentation. The approach is sound. My review focuses on the new shell script, where I've identified a bug in package name parsing and suggested improvements for robustness. I've also recommended a small change in the documentation to improve the user experience when running the new script.
| fi | ||
|
|
||
| # Extract package name (before ==, >=, <=, ~=, etc.) | ||
| pkg_name=$(echo "$line" | sed -E 's/^([a-zA-Z0-9_-]+).*/\1/') |
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 regular expression used to extract the package name is not robust. It doesn't account for dots (.) in package names (e.g., ruamel.yaml), which will lead to incorrect package name extraction. This may cause the script to fail to filter packages as intended if a package with a dot in its name needs to be filtered. A more reliable method is to remove the version specifier and everything that follows it.
| pkg_name=$(echo "$line" | sed -E 's/^([a-zA-Z0-9_-]+).*/\1/') | |
| pkg_name=$(echo "$line" | sed -E 's/[<>=!~].*//') |
| $ pip install -r requirements.txt --extra-index-url https://download.pytorch.org/whl/cu124 | ||
| $ | ||
| $ # 安装lightllm的依赖 (摩尔线程 GPU) | ||
| $ ./generate_requirements_musa.sh |
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 ./generate_requirements_musa.sh requires the script to have execute permissions. A user cloning the repository might not have this permission set by default, forcing them to run chmod +x first. To provide a smoother experience, you can invoke the script with bash directly, which doesn't require the execute bit.
| $ ./generate_requirements_musa.sh | |
| $ bash generate_requirements_musa.sh |
| $ pip install -r requirements.txt --extra-index-url https://download.pytorch.org/whl/cu124 | ||
| $ | ||
| $ # Install Lightllm dependencies (Moore Threads GPU) | ||
| $ ./generate_requirements_musa.sh |
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 ./generate_requirements_musa.sh requires the script to have execute permissions. A user cloning the repository might not have this permission set by default, forcing them to run chmod +x first. To provide a smoother experience, you can invoke the script with bash directly, which doesn't require the execute bit.
| $ ./generate_requirements_musa.sh | |
| $ bash generate_requirements_musa.sh |
| while IFS= read -r line || [ -n "$line" ]; do | ||
| # Skip empty lines and comments (but keep them in output) | ||
| if [[ -z "$line" || "$line" =~ ^[[:space:]]*# ]]; then | ||
| echo "$line" >> "$OUTPUT_FILE" | ||
| continue | ||
| fi | ||
|
|
||
| # Extract package name (before ==, >=, <=, ~=, etc.) | ||
| pkg_name=$(echo "$line" | sed -E 's/^([a-zA-Z0-9_-]+).*/\1/') | ||
|
|
||
| # Check if this is a CUDA package to skip | ||
| skip=false | ||
| for pattern in "${CUDA_PACKAGES[@]}"; do | ||
| if [[ "$pkg_name" =~ $pattern ]]; then | ||
| echo " Removing CUDA package: $line" | ||
| skip=true | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| if $skip; then | ||
| continue | ||
| fi | ||
|
|
||
| # Check if this is a torch-related package (remove version pin) | ||
| for pattern in "${TORCH_PACKAGES[@]}"; do | ||
| if [[ "$line" =~ $pattern ]]; then | ||
| # Remove version pin, keep just the package name | ||
| pkg_only=$(echo "$line" | sed -E 's/==.*//') | ||
| echo " Unpinning version for: $pkg_only (pre-installed in MUSA container)" | ||
| echo "$pkg_only" >> "$OUTPUT_FILE" | ||
| skip=true | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| if $skip; then | ||
| continue | ||
| fi | ||
|
|
||
| # Keep the package as-is | ||
| echo "$line" >> "$OUTPUT_FILE" | ||
|
|
||
| done < "$INPUT_FILE" |
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.
Throughout this loop, echo is used to print variables to files and standard output (e.g., lines 48, 59, 74, 75, 86). This can be unsafe if the variable content starts with a hyphen or contains backslashes, as echo might interpret them as options. It's a best practice in shell scripting to use printf for printing variable data to avoid unexpected behavior.
For example:
- line 48 could be
printf "%s\n" "$line" >> "$OUTPUT_FILE" - line 59 could be
printf " Removing CUDA package: %s\n" "$line" - line 74 could be
printf " Unpinning version for: %s (pre-installed in MUSA container)\n" "$pkg_only"
Testing Done
Tested in a clean docker container without vllm installed.