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 trl version to 0.2.12 and add Python venv instructions #143

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

duncantech
Copy link

@duncantech duncantech commented Jan 14, 2025

What does this PR do?

  • Fixes the trl version to 0.2.12 to keep existing example working
  • Add in instructions for setting up a Python virtual environment on a TPU VM

Fixes # (issue)

Before submitting

  • [] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

…ions for setting up Python virtual environment.
Copy link
Collaborator

@tengomucho tengomucho left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!
Small nits and then LGTM!

"id": "ba1788d9",
"metadata": {},
"source": [
"Once we have access to the TPU VM, we can setup a virtual Python environment to install our packages in:"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be an optional step. Could you rename it into something like: "While not strictly necessary, you can do an extra step to isolate your python environment by installing python-venv:" (or something like this).

@@ -65,7 +92,7 @@
"# Install Optimum tpu\n",
"pip install -e . -f https://storage.googleapis.com/libtpu-releases/index.html\n",
"# Install TRL and PEFT for training (see later how they are used)\n",
"pip install trl peft\n",
"pip install trl==0.2.12 peft\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the problem with trl 0.2.13? Can you open an issue describing the issue you observed so that we can fix it in the future?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed the same issue as @duncantech. Trl 0.2.13 as some breaking changes and so the interface is different. We need to adapt the examples to make them work with the latest version. I think this could be done in another PR tho

Copy link
Collaborator

Choose a reason for hiding this comment

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

@baptistecolle can you open an issue to do that later?

Copy link
Collaborator

@baptistecolle baptistecolle left a comment

Choose a reason for hiding this comment

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

LGTM thanks for pinning the trl dependency. Btw the llama fine-tuning example as the same problem and pinning dependency would be best there too https://github.com/huggingface/optimum-tpu/blob/main/examples/language-modeling/llama_tuning.ipynb

if this is not done in this PR i can open a small one to fix this

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