-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Update tokenizers in pr_test_peft_backend
#10132
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
Conversation
| else | ||
| python -m uv pip install -U peft --no-deps | ||
| python -m uv pip install -U transformers accelerate --no-deps | ||
| python -m uv pip install -U transformers accelerate tokenizers --no-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it break for the latest stable too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some failed runs on both main and latest, some of the more recent runs seem to be working though.
https://github.com/huggingface/diffusers/actions/workflows/pr_test_peft_backend.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try once more before the change then? Not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is ok now. Reviewing logs from failed runs it looks like transformers hadn't installed correctly https://github.com/huggingface/diffusers/actions/runs/12185630364/job/33992514505#step:5:19
https://github.com/huggingface/diffusers/actions/runs/12196730370/job/34024971182?pr=10125#step:5:19
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright then I am cool!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will cause issues if we merge this anyway but we can also just close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing! Single comment.
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
@hlky do we want to resolve the conflicts and merge? |
What does this PR do?
tokenizersbumped ontransformershuggingface/transformers#34972 brokepr_test_peft_backendworkflow.Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@sayakpaul @DN6