Skip to content

Conversation

@wind-many
Copy link

@wind-many wind-many commented Nov 28, 2025

What this PR does / why we need it?

Does this PR introduce any user-facing change?

How was this patch tested?

@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Nov 28, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This PR adds a new tutorial for running Qwen3 dense models on multiple NPUs. The tutorial is comprehensive and covers various optimization points, environment setup, and deployment scenarios. I've found one high-severity issue in the environment setup script that could lead to errors if commands are copied directly. My suggestion makes the script more robust by adding conditional logic for different operating systems.

Comment on lines +165 to +169
# Performance optimization of memory management
# if os is Ubuntu,install it using `apt install libjemalloc2`
export LD_PRELOAD=/usr/lib/aarch64-linux-gnu/libjemalloc.so.2:$LD_PRELOAD
#if os is openEuler,install it using `yum install jemalloc`
export LD_PRELOAD=/usr/lib64/libjemalloc.so.2:$LD_PRELOAD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current script for setting LD_PRELOAD is error-prone. If a user copies the entire block, the second export command will overwrite the first one. This means a user on Ubuntu would incorrectly have LD_PRELOAD set for openEuler, which would likely cause the application to fail. To prevent this, it's better to use a conditional statement to detect the OS and set the variable accordingly. This makes the script safe to copy and paste.

Suggested change
# Performance optimization of memory management
# if os is Ubuntu,install it using `apt install libjemalloc2`
export LD_PRELOAD=/usr/lib/aarch64-linux-gnu/libjemalloc.so.2:$LD_PRELOAD
#if os is openEuler,install it using `yum install jemalloc`
export LD_PRELOAD=/usr/lib64/libjemalloc.so.2:$LD_PRELOAD
# Performance optimization of memory management
if [ -f /etc/os-release ]; then
. /etc/os-release
if [ "$ID" = "ubuntu" ]; then
# For Ubuntu, install with `apt install libjemalloc2`
export LD_PRELOAD=/usr/lib/aarch64-linux-gnu/libjemalloc.so.2:$LD_PRELOAD
elif [ "$ID" = "openEuler" ]; then
# For openEuler, install with `yum install jemalloc`
export LD_PRELOAD=/usr/lib64/libjemalloc.so.2:$LD_PRELOAD
fi
fi

@wind-many wind-many closed this by deleting the head repository Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant