Skip to content

Conversation

qgallouedec
Copy link
Member

@qgallouedec qgallouedec commented May 1, 2025

Some suggestions for PR #3388:

  • Move generation_batch_size under the section Parameters that control generation
  • use int | None instead of int | str because the the CLI parser doesn't support multi-type (that's why the CI is failing)
  • rephrase some comments to be more precise (are we one the same page on these terms?):
Term Size
Local batch per_device_train_batch_size
Local accumulated batch per_device_train_batch_size * gradient_accumulation_steps
Global batch per_device_train_batch_size * world_size
Effective batch per_device_train_batch_size * world_size * gradient_accumulation_steps
Local generation batch per_device_train_batch_size * steps_per_generation
Generation batch per_device_train_batch_size * world_size * steps_per_generation
  • since num_generations will make less sense to use with this PR, I removed it from the figure, and hopefully gained clarity
  • support the case self.args._steps_per_generation < self.args.gradient_accumulation_steps (not sure why you'd want that, but it just requires to change != into <.

@qgallouedec qgallouedec requested a review from edbeeching May 1, 2025 06:31
@qgallouedec qgallouedec changed the title my suggestions Suggestions for Decouple gradient accumulation from the number of minibatches generated May 1, 2025
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Copy link
Collaborator

@edbeeching edbeeching left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestions, I will merge them

@edbeeching edbeeching merged commit fefe183 into grpo-decouple-grad-acc May 2, 2025
10 checks passed
@edbeeching edbeeching deleted the grpo-decouple-grad-acc-suggestion branch May 2, 2025 06:57
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