Skip to content

🧨 DFT#4042

Merged
qgallouedec merged 4 commits into
mainfrom
dft
Sep 9, 2025
Merged

🧨 DFT#4042
qgallouedec merged 4 commits into
mainfrom
dft

Conversation

@qgallouedec

@qgallouedec qgallouedec commented Sep 9, 2025

Copy link
Copy Markdown
Member

What does this PR do?

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.

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Sep 9, 2025

Copy link
Copy Markdown

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.

lewtun
lewtun approved these changes Sep 9, 2025
View reviewed changes

@lewtun lewtun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very clean PR! LGTM with a question on whether we should unit test the new loss type arg, similar to what we do with DPO here:

trl/tests/test_dpo_trainer.py

Line 221 in 1b799a2

def test_train_loss_types(self, loss_type):

kashif
kashif approved these changes Sep 9, 2025
View reviewed changes

@kashif kashif left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'll add the flag for liger kernel when there is a new release.

cmpatino
cmpatino approved these changes Sep 9, 2025
View reviewed changes

@cmpatino cmpatino left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@qgallouedec

qgallouedec commented Sep 9, 2025

Copy link
Copy Markdown
Member Author

Very clean PR! LGTM with a question on whether we should unit test the new loss type arg, similar to what we do with DPO here:

trl/tests/test_dpo_trainer.py

Line 221 in 1b799a2

def test_train_loss_types(self, loss_type):

where is my head?

added

@qgallouedec qgallouedec merged commit 659d2c1 into main Sep 9, 2025
9 of 11 checks passed
@qgallouedec qgallouedec deleted the dft branch September 9, 2025 14:23
@qgallouedec qgallouedec mentioned this pull request Sep 9, 2025
Closed
5 tasks
@qgallouedec qgallouedec mentioned this pull request Sep 22, 2025
Closed
@qgallouedec qgallouedec mentioned this pull request Oct 30, 2025
Closed
55 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kashif kashif kashif approved these changes

@edbeeching edbeeching edbeeching approved these changes

@cmpatino cmpatino cmpatino approved these changes

@lewtun lewtun lewtun approved these changes

@albertvillanova albertvillanova Awaiting requested review from albertvillanova

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@qgallouedec @HuggingFaceDocBuilderDev @kashif @edbeeching @cmpatino @lewtun