Skip to content

Minor optimizations in SFT. #3884

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
Merged
pramodith merged 9 commits into from
Aug 13, 2025
Merged

Minor optimizations in SFT. #3884

pramodith merged 9 commits into from
Aug 13, 2025

Conversation

@pramodith
Copy link
Collaborator

@pramodith pramodith commented Aug 12, 2025 •
edited
Loading

What does this PR do?

A few minor code optimizations to improve run time and memory consumed.

  1. Check if the dataset is conversational just once rather than in every invocation of dataset.map.
  2. Add a no_grad() block when computing accuracy for logging.
  3. Update log dictionary instead of creating a new dict and repopulating.

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 read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a GitHub issue? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?

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.

qgallouedec
qgallouedec reviewed Aug 12, 2025
View reviewed changes
@pramodith pramodith enabled auto-merge (squash) August 12, 2025 17:35
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 12, 2025

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@pramodith
Copy link
Collaborator Author

pramodith commented Aug 13, 2025

@qgallouedec I can make the changes to set batched=True in another PR tomorrow, when I get the time if you'd like to merge this one today.

@qgallouedec
Copy link
Member

qgallouedec commented Aug 13, 2025 •
edited
Loading

Yep, best is to keep things separate 👌

qgallouedec
qgallouedec approved these changes Aug 13, 2025
View reviewed changes
Copy link
Member

@qgallouedec qgallouedec left a comment

Choose a reason for hiding this comment

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

Lgtm!

@pramodith pramodith merged commit 499d9fb into huggingface:main Aug 13, 2025
9 of 10 checks passed
LuisVasquezBSC pushed a commit to langtech-bsc/trl that referenced this pull request Aug 28, 2025
@pramodith @LuisVasquezBSC
Minor optimizations in SFT. (huggingface#3884)
2184c50
LuisVasquezBSC pushed a commit to langtech-bsc/trl that referenced this pull request Aug 28, 2025
@pramodith @LuisVasquezBSC
Minor optimizations in SFT. (huggingface#3884)
5955b9f
SamY724 pushed a commit to SamY724/trl that referenced this pull request Sep 6, 2025
@pramodith
Minor optimizations in SFT. (huggingface#3884)
56da8d9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qgallouedec qgallouedec qgallouedec 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.

3 participants

@pramodith @HuggingFaceDocBuilderDev @qgallouedec