-
Notifications
You must be signed in to change notification settings - Fork 31.7k
Fix warning message for PEFT models in text-generation pipeline #36783 #36887
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.
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
Fix warning message for PEFT models in text-generation pipeline #36783 #36887
Conversation
|
Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. When it is ready for review, please click the |
|
Hi, I think adding PEFT support here is a good idea, but I think it would make more sense to add the model type based on |
|
Hi @Rocketknight1 |
|
Checking supported_models_names = [
"PeftModelForCausalLM",
"PeftModelForSeq2SeqLM",
"PeftModelForSequenceClassification",
"PeftModelForQuestionAnswering",
"PeftModelForTokenClassification",
"PeftModel",
"PeftModelForFeatureExtraction",
]means that all PEFT model types will be accepted for all pipelines. Instead, you probably want to match model types to their appropriate pipelines. This means that you will need to set |
|
HI @Rocketknight1 Cannot access gated repo for url https://huggingface.co/mistralai/Mistral-Small-3.1-24B-Instruct-2503/resolve/main/preprocessor_config.jsonBtw, |
|
Hi @Rocketknight1 Appreciate all the effort you put into reviewing others' code. It’s always a lot of work, and it doesn’t go unnoticed! |
Rocketknight1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo but this makes sense to me now, and sorry for the delay! cc @BenjaminBossan for PEFT, are you okay with it?
BenjaminBossan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good, thanks!
@falconlee236 I have added some comments on the test, especially since we should properly check the warning and not rely on print statements. Also, let's use different checkpoints. Moreover, I would suggest to move the tests to test_peft_integration.py, I don't think we need a new file for just this test.
@Rocketknight1 Please also check if my feedback makes sense, as I'm not so familiar with the testing standards in transformers.
|
HI @BenjaminBossan @Rocketknight1 I moved test to To check so The test is passed when output do not contain |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving the test. I did, however, have an issue: The test would pass for me, even without your fix. I made some suggestions that should allow the test to fail without the fix.
|
Hi @BenjaminBossan @Rocketknight1 ! |
BenjaminBossan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on the test, it now fails without the fix. LGTM.
|
Dear @Rocketknight1 , |
11e1682 to
c6e7f07
Compare
c6e7f07 to
c653a35
Compare
|
Yep, this looks good now! Will merge when tests finish, and thank you for the PR! |
|
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. |
Thank you so a lot. |
…ngface#36783 (huggingface#36887) * add peft model in constant * add test * fix formating * make fixup execute * change code * check by self.task * add test * fixup test code * fix minor typo * fix pipeline test * apply maintainers reqests
…ngface#36783 (huggingface#36887) * add peft model in constant * add test * fix formating * make fixup execute * change code * check by self.task * add test * fixup test code * fix minor typo * fix pipeline test * apply maintainers reqests
What does this PR do?
This PR fixes the warning message when using a PeftModel with a text-generation pipeline. Currently, when a user passes a PeftModel to the text-generation pipeline, they receive an error message stating that "The model 'PeftModel' is not supported for text-generation" even though the pipeline works correctly. This PR modifies the model type checking logic in the pipeline to properly handle PEFT-wrapped models, making the user experience smoother by eliminating unnecessary warning messages.
Main Cause
in
src/transformers/pipelines/text_generation.py, there Checks if the model is suitable for generation tasks.but PEFT model didn't include neiter
TF_MODEL_FOR_CAUSAL_LM_MAPPING_NAMESnorMODEL_FOR_CAUSAL_LM_MAPPING_NAMESin
check_model_typefunction, the peft model didn't include valid model. so logger.error was triggered. but text generation worked properly.bug logs
Impelementation
So I include peft model classes in supported_models_names.
Thanks for telling me about all the types of peft models. @BenjaminBossan
resolve message
Test Description
This test verifies that the warning message "The model 'PeftModel' is not supported for text-generation" does not appear when using a PeftModel with the text-generation pipeline. The test:
This test ensures that PeftModel is properly integrated with the transformers pipeline without generating unnecessary warning messages.
Fixes #36783
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
cc: @sayakpaul @BenjaminBossan @Rocketknight1