[ci] fix: Add more test cases in e2e_ppo_trainer_megatron_sglang_ascend.yml#6877
Conversation
There was a problem hiding this comment.
Code Review
This pull request removes the configuration enabling flash attention for the reference model on NPU in the PPO trainer Megatron end-to-end test. The reviewer notes that this removal could lead to severe performance degradation or Out-Of-Memory (OOM) errors, and suggests keeping the configuration by correcting the double plus (++) prefix to a single plus (+) prefix to avoid Hydra parsing issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "${common_params[@]}" \ | ||
| +actor_rollout_ref.actor.megatron.override_transformer_config.context_parallel_size=${ACTOR_CP} \ | ||
| +actor_rollout_ref.actor.megatron.override_transformer_config.use_flash_attn=True \ | ||
| ++actor_rollout_ref.ref.megatron.override_transformer_config.use_flash_attn=True \ |
There was a problem hiding this comment.
Removing use_flash_attn=True for the reference model (ref) on NPU can lead to severe performance degradation or Out-Of-Memory (OOM) errors during PPO training, as the reference model will run without flash attention. If this was removed due to a Hydra parsing error caused by the double plus (++), please consider keeping it with a single plus (+) to match the actor's configuration style.
| ++actor_rollout_ref.ref.megatron.override_transformer_config.use_flash_attn=True \ | |
| +actor_rollout_ref.ref.megatron.override_transformer_config.use_flash_attn=True \ |
References
- Use
+instead of++as the prefix for overriding configuration values in Hydra.
What does this PR do?
Fixed some problems of e2e_ppo_trainer_megatron_sglang.yml on Ascend and added more CI cases for maintenance.
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,fully_async,one_step_off,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.