-
Notifications
You must be signed in to change notification settings - Fork 112
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
validate use_remove_padding
when applying sequence parallelism
#153
base: main
Are you sure you want to change the base?
Conversation
Could you format the code by running |
Done! |
@@ -86,4 +86,13 @@ def check_mutually_exclusive(mbs, mbs_per_gpu, name: str): | |||
assert config.critic.ppo_mini_batch_size % config.critic.ppo_micro_batch_size == 0 | |||
assert config.critic.ppo_micro_batch_size * sp_size >= n_gpus | |||
|
|||
# Check if use_remove_padding is enabled when using sequence parallelism | |||
if config.actor_rollout_ref.actor.ulysses_sequence_parallel_size > 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.
It seems that the correct key is config. actor_rollout_ref.model.use_remove_padding, critic.model.use_remove_padding and reward_model.model.use_remove_padding
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.
Fixed now!
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.
Unfortunately, the key is still not correct :(
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.
@chujiezheng The key should be actor_rollout_ref.model.use_remove_padding
. The actor and ref are not necessary as they have the same model type.
This is because the ulysses_sp is activated only when
use_remove_padding
is enabled:verl/verl/workers/actor/dp_actor.py
Lines 71 to 128 in ab525bc
Without this check, users may encounter OOM issues when the set sp_size > 1 but
use_remove_padding
is mistakenly disabled.