transformers icon indicating copy to clipboard operation
transformers copied to clipboard

added warning to Trainer when label_names is not specified for PeftModel

Open MilkClouds opened this issue 1 year ago • 8 comments

What does this PR do?

in Trainer, it automatically finds label_names with input arguments name of model. But with model which is PeftModel, the input arguments are all hidden and we can't find the input argument's name automatically.

So, for the case then the user didn't specified label_names in TrainerArguments, I made warning message pop up.

p.s. it makes the change when I add call trainer.predict; if label_names it not set, there's no way I can get get label_ids from trainer.predict outcomes.

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 or the forum? Please add a link to it if that's the case.
  • [ ] Did you make sure to update the documentation with your changes? Here are the documentation guidelines, and here are tips on formatting docstrings.
  • [ ] 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.

MilkClouds avatar Jul 19 '24 10:07 MilkClouds

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.

Basically inside Trainer's init function, it automatically finds whether 'labels' argument exist in model class's forward function.

e.g. LlamaForCausalLM: have labels field in forward, so 'labels' is detected e.g. PeftModel-wrapped LlamaForCausalLM: since PeftModel only have *args, **kwargs in forward's argument, it can't detect 'labels' of LlamaForCausalLM.

MilkClouds avatar Aug 01 '24 13:08 MilkClouds

Thanks for clarifying, that makes sense. I had already shared this concern internally in the past (but got no response :cry:):

Users are reporting issues with Trainer not showing evals when using PEFT models (e.g. https://github.com/huggingface/peft/issues/1881). The core of the problem appear to functions like

that inspect the forward signature of the model class. As PEFT wraps the transformers model and uses mostly **kwargs in the signature, this check returns False even if the underlying model would return True. I'm not sure how to resolve this. On the PEFT side, I could just add the arguments to the signature, but this can lead to false positives, as the base model may or may not actually support this. Therefore, it would be better if on the transformers side there would be a check if the model is a PEFT model, or if it has a base_model attribute, and then check the signature of the base model instead. This of course won't work if the model class is passed, it would require the model instance, so this would be a somewhat bigger change.

So I guess this PR helps a little bit but the fundamental problem still remains. Thus from my side, it's okay to merge with my suggestion applied.

BenjaminBossan avatar Aug 01 '24 13:08 BenjaminBossan

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.

github-actions[bot] avatar Aug 26 '24 08:08 github-actions[bot]

Does it need any other consideration, @amyeroberts ?

MilkClouds avatar Aug 26 '24 08:08 MilkClouds

I think we were waiting for the suggestion from @BenjaminBossan to be merged 🤗

ArthurZucker avatar Aug 26 '24 11:08 ArthurZucker

Ah, I see. I missed the suggestion. I accepted the suggestion!

MilkClouds avatar Aug 26 '24 11:08 MilkClouds

Now let's make sure make fixup and the rest of the CIs related are green!

ArthurZucker avatar Aug 27 '24 13:08 ArthurZucker

Sorry for the late reply, I've forgotten and thought this PR was merged. I ran make fixup and fixed some, but some of CI/CD fail until now.

MilkClouds avatar Jan 22 '25 09:01 MilkClouds

Failing test is unrelated don't worry!

ArthurZucker avatar Feb 12 '25 11:02 ArthurZucker

Basically inside Trainer's init function, it automatically finds whether 'labels' argument exist in model class's forward function.

e.g. LlamaForCausalLM: have labels field in forward, so 'labels' is detected e.g. PeftModel-wrapped LlamaForCausalLM: since PeftModel only have *args, **kwargs in forward's argument, it can't detect 'labels' of LlamaForCausalLM.

Yes, I am using LlamaForCausalLM, and after I give my lora_config and use "model = get_peft_model(model, lora_config)", the trainer will still remind me "No label_names provided for model class PeftModelForCausalLM. Since PeftModel hides base models input arguments". Could you tell me how you solve this problem? Thank you very much.

CFSJXDM avatar Mar 31 '25 04:03 CFSJXDM