Skip to content

[NPU]: support use-rollout-routing-replay#292

Open
Windfeng8 wants to merge 3 commits into
vllm-project:ascendfrom
Windfeng8:fix/routing-replay-v2
Open

[NPU]: support use-rollout-routing-replay#292
Windfeng8 wants to merge 3 commits into
vllm-project:ascendfrom
Windfeng8:fix/routing-replay-v2

Conversation

@Windfeng8

@Windfeng8 Windfeng8 commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

[RFC] Steps and Test Result for Running Qwen3-30B on NPU(A3) (use-rollout-routing-replay)
#270

@read-the-docs-community

read-the-docs-community Bot commented Jun 23, 2026

Copy link
Copy Markdown

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

Copy link
Copy Markdown

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 a new training script run-qwen3-30B-A3B-npu-use-routing-replay.sh for running Qwen3-30B-A3B training on NPUs. Feedback on the script highlights two main issues: first, there are duplicate arguments (--use-dynamic-batch-size and --max-tokens-per-gpu) with conflicting values (8192 vs 20480) that need to be cleaned up; second, a hardcoded user directory path is used for the prompt dataset, which should be replaced with a configurable environment variable to improve portability.

Comment on lines +80 to +81
--use-dynamic-batch-size \
--max-tokens-per-gpu 8192 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The arguments --use-dynamic-batch-size and --max-tokens-per-gpu are duplicated in this script (see lines 90-91). Additionally, --max-tokens-per-gpu is specified with conflicting values (8192 vs 20480). Please remove the duplicate entries and keep only the correct configuration (likely 20480 to match the max model length of 20k).

\
--hf-checkpoint /home/data/Qwen3-30B-A3B/ \
\
--prompt-data /home/w00893744/dataset/dapo-math-17k/dapo-math-17k.jsonl \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The path /home/w00893744/dataset/dapo-math-17k/dapo-math-17k.jsonl contains a hardcoded user directory (w00893744), which prevents the script from being portable across different environments or users. Consider using a configurable environment variable with a fallback default.

Suggested change
--prompt-data /home/w00893744/dataset/dapo-math-17k/dapo-math-17k.jsonl \
--prompt-data "${PROMPT_DATA:-/home/data/dapo-math-17k/dapo-math-17k.jsonl}" \

@CalvinXKY

Copy link
Copy Markdown
Collaborator

I'd suggest putting the sh script in #270, and also using the ray submit submission format.

image

@Windfeng8

Copy link
Copy Markdown
Contributor Author

I'd suggest putting the sh script in #270, and also using the ray submit submission format.我建议将 sh 脚本放在 #270中,并且使用 ray submit 提交格式。

image

done

@Windfeng8
Windfeng8 force-pushed the fix/routing-replay-v2 branch from 11740e1 to f992c58 Compare July 22, 2026 12:07
Signed-off-by: Windfeng8 <523758380@qq.com>
…replay.sh

Signed-off-by: Windfeng8 <523758380@qq.com>
Signed-off-by: Windfeng8 <523758380@qq.com>
@Windfeng8
Windfeng8 force-pushed the fix/routing-replay-v2 branch from f992c58 to 5763331 Compare July 22, 2026 12:13

@floatlibai floatlibai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few comments on the script you submitted.

# will prevent ray from buffering stdout/stderr
export PYTHONUNBUFFERED=1
export SLIME_SCRIPT_TRAIN_BACKEND=megatron
export PYTHONPATH="/workspace/issue205/Megatron-LM:/workspace/issue205/Megatron-Bridge/src:${PYTHONPATH}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest export PYTHONPATH="/root/Megatron-Bridge/src:/root/Megatron-LM/:$PYTHONPATH"

)

ROLLOUT_ARGS=(
--prompt-data /home/w00893744/dataset/dapo-math-17k.jsonl

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove your Employee ID and use a general path

)

VLLM_ARGS=(
--rollout-backend vllm

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

useless line

--use-rollout-routing-replay
--vllm-weight-sync-mode native
--vllm-gpu-memory-utilization 0.7
--vllm-enable-sleep-mode

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this line is useless too

--attention-backend flash
--use-flash-attn
--no-gradient-accumulation-fusion
--train-memory-margin-bytes 2147483648

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

unessary I think, you can ref scripts/run-qwen3-30B-A3B-npu.sh on newest ascend branch

}"

ray job submit --address="http://127.0.0.1:8265" \
--runtime-env-json="${RUNTIME_ENV_JSON}" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These runtime-envs are all unessary, I suggest removing them

ray job submit --address="http://127.0.0.1:8265" \
--runtime-env-json="${RUNTIME_ENV_JSON}" \
-- python3 "${REPO_ROOT}/train.py" \
--train-backend megatron \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this line too, only one training backend now.

@floatlibai

floatlibai commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

I wonder if this script/pr is necessary, since it only adds one parameter (--use-rollout-routing-replay) compared to the original run-qwen3-30B-A3B-npu script and no other modification. Would an RFC alone be enough here? @CalvinXKY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants