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

Remove nbdev dependency #52

Merged
merged 5 commits into from
Dec 20, 2022
Merged

Remove nbdev dependency #52

merged 5 commits into from
Dec 20, 2022

Conversation

younesbelkada
Copy link
Contributor

@younesbelkada younesbelkada commented Dec 16, 2022 •
edited
Loading

What does this PR do?

This PR is a follow-up PR of #50 - it is an attempt to remove the nbdev dependency from trl
This PR also adds a patch to support the latest version of transformers
Draft for now as I have to figure out how to deal with the documentation

For now testing that everything works with python examples/04-ppo-sentiment.py

cc @lvwerra

@younesbelkada
refactor
dfb864d 
- move notebooks to `examples/notebooks``
- removed `_nbdev`file
- refactored `gpt2.py` to make it work with more recent `transformers`
- update `requirements` to add recent `transformers`
@younesbelkada
remove nbdev from requirements
d588fce
@younesbelkada
remove images
fc8e805
@younesbelkada
fix image links
96fbf0b
@younesbelkada younesbelkada marked this pull request as ready for review December 19, 2022 21:25
@younesbelkada
Copy link
Contributor Author

Moved all images inside the org https://huggingface.co/trl-internal-testing and fixed all image links on README + notebooks with the correct ones
Also as discussed, I removed the 3 first notebooks ;) Let me know what is missing here!

lvwerra
lvwerra reviewed Dec 20, 2022
View reviewed changes
Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

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

Thanks @younesbelkada, looks great!

A few minor things to fix:

  • remove the numbering from notebooks and scripts - i think this is no longer necessary
  • remove the CI (should be in .github/workflows)
    I wonder if it's possible to remove generated from note at the top.

@lvwerra
Copy link
Member

lvwerra commented Dec 20, 2022

Seems not possible https://stackoverflow.com/questions/66587174/how-to-remove-generated-from-tag

@younesbelkada
Copy link
Contributor Author

Thanks for the review!
I should have removed the CI, done the renaming of the files ;-)

@younesbelkada younesbelkada merged commit 6fa5248 into huggingface:master Dec 20, 2022
@August-murr August-murr mentioned this pull request Jan 6, 2025
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
2 participants
@younesbelkada @lvwerra