Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two new shell scripts for nightly CI tests on NPU hardware, one for GRPO and one for PPO training. The scripts are well-structured for CI purposes. My review includes a couple of suggestions to improve clarity and correctness of the scripts by removing an unused variable, a confusing comment, and a redundant training parameter. These changes will make the new CI scripts more maintainable.
| ENGINE=${1:-vllm} | ||
|
|
||
| # Some models are optimized by vllm ascend. While in some case, e.g. rlhf training, | ||
| # the optimized model may not be suitable. In this case, set this value to 0 to disable the optimized model. |
There was a problem hiding this comment.
The ENGINE variable is initialized but never used in the script. The accompanying comment is also unclear as it refers to 'this value' without specifying what it is. This appears to be leftover code and should be removed to improve script clarity and maintainability.
| trainer.total_training_steps=15 \ | ||
| trainer.total_epochs=15 $@ No newline at end of file |
There was a problem hiding this comment.
You have specified both trainer.total_training_steps and trainer.total_epochs. The total_training_steps parameter usually takes precedence, making total_epochs redundant and potentially confusing. To avoid ambiguity and ensure the training duration is explicitly controlled by the number of steps, please remove trainer.total_epochs.
| trainer.total_training_steps=15 \ | |
| trainer.total_epochs=15 $@ | |
| trainer.total_training_steps=15 $@ |
| ENGINE=${1:-vllm} | ||
|
|
||
| # Some models are optimized by vllm ascend. While in some case, e.g. rlhf training, | ||
| # the optimized model may not be suitable. In this case, set this value to 0 to disable the optimized model. |
There was a problem hiding this comment.
The ENGINE variable is initialized but never used in the script. The accompanying comment is also unclear as it refers to 'this value' without specifying what it is. This appears to be leftover code and should be removed to improve script clarity and maintainability.
daikang6
requested review from
FightingZhen,
PeterSH6,
ji-huazhong,
tardis-key and
vermouth1992
as code owners
1 participant
What does this PR do?
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,veomni,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.