[Liger] add native liger-kernel orpo loss #2482
Conversation
|
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. |
|
2 questions/remarks:
|
|
I think we should bump liger version to v0.5 (it doesn't include the loss before), see https://github.com/linkedin/Liger-Kernel/releases/tag/v0.5.0 |
Co-authored-by: Quentin Gallouédec <[email protected]>
kashif
changed the title
| loss = loss_fct(logits, labels) | ||
| return loss | ||
| if self.args.use_liger_loss: | ||
| # skip the lm head and get the last hidden state |
There was a problem hiding this comment.
Nice!
I guess we don't have much of an option beyond using a config parameter for now.
Given that we run forward pass on a submodule, it would be very nice to have some validation so that there are no unexpected failures etc with different distributed training settings. But in this case, I feel there might be compatibility issues with FSDP given the limitation from the docs: https://pytorch.org/docs/stable/fsdp.html
"FSDP does not support running the forward pass of a submodule that is contained in an FSDP instance. This is because the submodule’s parameters will be sharded, but the submodule itself is not an FSDP instance, so its forward pass will not all-gather the full parameters appropriately."
(might be fixed by just making the base model attribute an FSDP instance as well, coz why not)
Beyond that this looks fine! I have a couple nits (don't matter that much):
- Does
model.get_decoder()work all the time btw for AutoModelForCausalLM instances? Was wondering if that's a cleaner solution for getting the base model attribute. But I think some base model classes have some further wrapping over the actual decoder (to format outputs, etc) https://github.com/huggingface/transformers/blob/a7f5479b45a8040392af80bf1107a2bdd796931c/src/transformers/models/opt/modeling_opt.py#L1044 - Maybe the config is
base_model_attribute_namesince its the attribute name of the base model in the CausalLM object?
There was a problem hiding this comment.
thanks @SumanthRH yes you are right get_decoder() will work
Yes next is to verify the distributed training cases
| def test_orpo_trainer_with_liger(self): | ||
| """Test ORPO trainer with Liger loss enabled.""" | ||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| training_args = ORPOConfig( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
so the plot was with the same parameters with and without the liger_loss flag... rather than in general... so trying to figure out why there is a difference between the two settings...
|
waiting on linkedin/Liger-Kernel#486 |
|
waiting on #2502 |
|
@kashif can you share the curves once it's ready? |
What does this PR do?
Adds support for Liger ORPO loss kernel to the ORPO Trainer natively.