-
Notifications
You must be signed in to change notification settings - Fork 389
[WIP][CI] Add multi node ci #2345
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: wangli <[email protected]>
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 introduces two example scripts for multi-node serving: multi_node_dp.sh
for data parallelism and multi_node_ray.sh
for Ray-based deployment. My review focuses on improving the robustness, correctness, and reusability of these scripts. For multi_node_dp.sh
, I've identified several critical and high-severity issues including an undefined variable, hardcoded paths, and missing error checks, and I've provided suggestions to address them. For multi_node_ray.sh
, I've pointed out that error messages should be redirected to stderr, which is a best practice for shell scripts. Applying these suggestions will make the examples more reliable and user-friendly.
--gpu-memory-utilization 0.9 \ | ||
--additional-config '{"ascend_scheduler_config":{"enabled":true},"torchair_graph_config":{"enabled":true}}' | ||
else | ||
echo "====> Running worker node" |
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 MASTER_ADDR
variable is used on line 52 but it is not defined. This will cause an error when running as a worker. Please add a check to ensure this variable is set.
echo "====> Running worker node" | |
if [ -z "${MASTER_ADDR-}" ]; then | |
echo "Error: MASTER_ADDR environment variable must be set for worker node." >&2 | |
exit 1 | |
fi | |
echo "====> Running worker node" |
set -euo pipefail | ||
|
||
run_node() { | ||
NODE_TYPE=$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 model path is hardcoded on lines 23 and 45, which limits the script's reusability. It's better to pass the model path as a command-line argument and validate it. You can then use the variable (e.g., $MODEL_PATH
) in the vllm serve
commands on lines 23 and 45.
NODE_TYPE=$1 | |
NODE_TYPE=$1 | |
MODEL_PATH=$2 | |
if [ -z "$MODEL_PATH" ]; then | |
echo "Error: model path must be provided as the second argument" >&2 | |
exit 1 | |
fi |
NODE_TYPE=$1 | ||
echo "====> Running $NODE_TYPE" | ||
|
||
local_ip=$(hostname -I | awk '{print $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 hostname -I
command can fail and return an empty string, which would cause issues in subsequent commands. It's safer to check if an IP was actually found.
local_ip=$(hostname -I | awk '{print $1}') | |
local_ip=$(hostname -I | awk '{print $1}') | |
if [ -z "$local_ip" ]; then | |
echo "Error: Could not determine local IP address." >&2 | |
exit 1 | |
fi |
done | ||
|
||
if [ -z "$ray_address" ]; then | ||
echo "Error: Missing argument --ray_address" |
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.
Error messages should be printed to standard error (stderr) instead of standard output (stdout). This is a standard practice in shell scripting to allow for proper output redirection and error handling. This applies to other error and timeout messages in this script as well (e.g., lines 68, 93, 111, 116).
echo "Error: Missing argument --ray_address" | |
echo "Error: Missing argument --ray_address" >&2 |
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
Signed-off-by: wangli <[email protected]>
This reverts commit 6cbe2d6. Signed-off-by: wangli <[email protected]>
What this PR does / why we need it?
This pr introduces multi-node ci for vllm-ascend on different backend(mp, ray)
Does this PR introduce any user-facing change?
How was this patch tested?