Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed some type annotations of trl.trainer.PPoTrainer #392

Merged
merged 4 commits into from
Jun 6, 2023
Merged

Fixed some type annotations of trl.trainer.PPoTrainer #392

merged 4 commits into from
Jun 6, 2023

Conversation

JulesGM
Copy link
Contributor

@JulesGM JulesGM commented May 29, 2023

  • Ref model should be Optional
  • The usual annotation for the Huggingface tokenizers is PreTrainedTokenizerBase. Not using that messes up people's annotation checks.
  • Fixed the comments wrt the other two points

@JulesGM
Fixed some type annotations of trl.trainer.PPoTrainer
efcb2bd 
- Ref model should be Optional
- The usual annotation for the Huggingface tokenizers is PreTrainedTokenizerBase. Not using that messes up people's annotation checks.
- Fixed the comments wrt the other two points
@lvwerra
Copy link
Member

lvwerra commented May 30, 2023

Thanks for the PR, could you run make quality && make style to fix the code quality checks?

@JulesGM
Copy link
Contributor Author

JulesGM commented May 30, 2023

Should be good now. Are there tests I should be running?

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 30, 2023 •
edited
Loading

The documentation is not available anymore as the PR was closed or merged.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@JulesGM
Copy link
Contributor Author

JulesGM commented May 30, 2023

forgot to sync

younesbelkada
younesbelkada approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@younesbelkada younesbelkada requested a review from lvwerra May 31, 2023 09:48
@lvwerra lvwerra merged commit 20111ad into huggingface:main Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@younesbelkada younesbelkada younesbelkada approved these changes

@lvwerra lvwerra lvwerra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JulesGM @lvwerra @HuggingFaceDocBuilderDev @younesbelkada