preference loss sign is inverted and leads to negative loss

[winglian]

Summary

In testing cases where there is no alpha/NLL loss used, the loss becomes negative, which is probably not the intended behavior. see https://github.com/huggingface/trl/blob/main/trl/trainer/dpo_trainer.py#L1234

• Hardware Type:
• run make test to ensure correctness
• run make checkstyle to ensure code style
• run make test-convergence to ensure convergence

[austin362667]

Would you mind updating the corresponding tests? They failed in my CI as expected.

[austin362667]

I think the corresponding snippet lies here:

Liger-Kernel/test/utils.py

Line 514 in 0bb6c72

loss = policy_nll_loss * self.alpha - losses.mean()

.

[austin362667]

Thank you!

[shivam15s]

Seems like the original logic is correct for all losses except dpo_loss. I think the sign change should be here instead:

Liger-Kernel/src/liger_kernel/chunked_loss/dpo_loss.py

Line 52 in 21baccc

loss = -F.logsigmoid(logits_diff).sum() / (full_target.shape[0] // 2)

[austin362667]

Hi @shivam15s Thanks for noticing this. What are your thoughts on negating each preference loss term to align with the formulas in the docstrings? This would allow us to maintain the base preference structure as nll_loss + preference_loss while making both terms represent losses to be minimized.

[shivam15s]

Sure @austin362667 ! I think that might help with readability.
If you have some time, could you add the fix to this PR?

[austin362667]

Ya sure! I might need to open a new PR (base on this one) cc @winglian