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.

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

fix: easier environment setup; pin trl, transformers #199

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

ctjlewis
Copy link
Contributor

@ctjlewis ctjlewis commented Feb 6, 2025

trl/transformers versions pinned (thanks to @yeshsurya for the note):

Includes lighteval/math-verify version bump from #193.

setup.sh has been tested from cold start on clean Debian CUDA 12.1 with GRPO example, 8x H100. Easier to set up, prints out the example command for the user at the end, shorter README section.

Please test and edit README as appropriate.

@ctjlewis ctjlewis changed the title fix: easier environment setup; pin trl, transformers, math-verify fix: easier environment setup; pin trl, transformers Feb 6, 2025
@ctjlewis
Copy link
Contributor Author

ctjlewis commented Feb 6, 2025

cc @lewtun if you get a minute, thanks for reviews - setup big issue for lot of people

@ctjlewis
Copy link
Contributor Author

ctjlewis commented Feb 6, 2025

Ah, the README got changed, I gotta fix merge conflicts... Would be good to maybe get a review first and let me know how we think we should do README.

Do we want that to be the default experiment, the minimal reason one from #197, that setup.sh prints out for user to copy as well?

@qgallouedec
Copy link
Member

qgallouedec commented Feb 7, 2025

No it doesn't fix the issues. Installing the latest main of trl solves the issue actually. pinning trl 0.14 will, on the contrary, introduce the issue

@ctjlewis
Copy link
Contributor Author

ctjlewis commented Feb 7, 2025

How much do you wanna bet that if I make a docker image with cu121 Debian, it is stable on this branch? I tested a ton from a clean env. We should really do a docker image anyway.

@qgallouedec
Copy link
Member

I'm sure it works. Actually to simplify here is the timeline:

commit 1 (v0.14): works
commit 2 : introduced a bug
commit 3 (head): solved the bug

User that locally have commit 2 should pull the latest commit from main to fix the issue, not pin 0.14. Pinning 0.14 should work, but it's not what we want.

@ctjlewis
Copy link
Contributor Author

ctjlewis commented Feb 8, 2025

Oh, OK. Do you want to junk this PR or do you want me to just update it to pin it to git at a certain commit? Let's at least pin it to a specific commit rather than just reference main HEAD since that always moves. The comments in setup.py wanted that for fast-moving packages.

For the ones down in that section, when trl or transformers or anyone publishes something on their dev branch or whatever that you want, the commit hash just has to be manually updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants