Skip to content

[SFTTrainer] Check correctly for condition#668

Merged
younesbelkada merged 1 commit intomainfrom
sft-check-peft
Sep 6, 2023
Merged

[SFTTrainer] Check correctly for condition#668
younesbelkada merged 1 commit intomainfrom
sft-check-peft

Conversation

@younesbelkada
Copy link
Copy Markdown
Contributor

@younesbelkada younesbelkada commented Aug 21, 2023

Fixes #667
Fixes #735

if PEFT is not installed SFTTrainer will throw an error when checking isinstance(model, (PreTrainedModel, PeftModel)) as PeftModel is defined nowhere in the script if PEFT is not available

cc @lvwerra

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

HuggingFaceDocBuilderDev commented Aug 21, 2023

The documentation is not available anymore as the PR was closed or merged.

@younesbelkada younesbelkada requested a review from lvwerra August 21, 2023 07:48
@lvwerra
Copy link
Copy Markdown
Member

lvwerra commented Aug 21, 2023

Maybe we can mock in the tests that peft is not installed and test that it still runs?

@younesbelkada
Copy link
Copy Markdown
Contributor Author

younesbelkada commented Sep 6, 2023

Will be doing it in a follow up PR (to unlock #735) !

@younesbelkada younesbelkada merged commit 1111295 into main Sep 6, 2023
@younesbelkada younesbelkada deleted the sft-check-peft branch September 6, 2023 08:24
kushal-tri pushed a commit to kushalarora/trl that referenced this pull request Sep 19, 2023
lapp0 pushed a commit to lapp0/trl that referenced this pull request May 10, 2024
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: NameError: name 'PeftModel' is not defined due to unguarded reference Error in trl 0.5.0

3 participants