Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix bug in batched_forward_pass #144

Merged
merged 3 commits into from
Feb 14, 2023
Merged

fix bug in batched_forward_pass #144

merged 3 commits into from
Feb 14, 2023

Conversation

ArvinZhuang
Copy link
Contributor

fix the bug that will cause the devices not match issue in the batched_forward_pass method.

The reason:
self. data_collator returned tensors will be on CPU, thus the later self.model(**input_kwargs) will give error as model is on GPU.

The proposed solution:
do .to(self.accelerator.device) after self.data_collator

My transformers version: 4.26.1

younesbelkada
younesbelkada reviewed Feb 11, 2023
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow thanks a lot for fixing the bug!
I am ok with this fix in the principle that it is a safety checker to make sure all the returned tensors will be on the correct device (regardless where the dataloader will send the device)
Let's run the tests and see!
Wdyt @lvwerra ?

@ArvinZhuang can you run the styling and quality checks? (make style && make quality) Thanks!

trl/trainer/ppo_trainer.py

attention_mask = [torch.ones_like(r) for r in responses]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum why this has been removed? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because seems attention_mask variable is never used

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, can you run the styling checks so that the testing suite will be executed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just did

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Feb 11, 2023 •
edited
Loading

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

younesbelkada
younesbelkada reviewed Feb 11, 2023
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing! Could you revert your changes in all the scripts inside examples/ after that we should be good to merge!

@ArvinZhuang
Copy link
Contributor Author

ArvinZhuang commented Feb 11, 2023 •
edited
Loading

Thanks for fixing! Could you revert your changes in all the scripts inside examples/ after that we should be good to merge!

Hi
these changes are automatically generated by style && quality, but I reverted them anyway

younesbelkada
younesbelkada approved these changes Feb 11, 2023
View reviewed changes
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating! 🚀

@ArvinZhuang
Copy link
Contributor Author

Also I notice this "Forward batch size > 1 is not well supported yet for encoder-decoder models.
Could you please give me more hits why this is the case? Looks like forward batch size =1 will greatly slow down the inference, I probably can give it a shot to fix this.

@DaehanKim DaehanKim mentioned this pull request Feb 12, 2023
Closed
@Rebecca-Qian
Copy link

Had just prepared a PR to fix this 😄 thanks @ArvinZhuang

@ArvinZhuang
Copy link
Contributor Author

ArvinZhuang commented Feb 14, 2023 •
edited
Loading

Had just prepared a PR to fix this 😄 thanks @ArvinZhuang

@Rebecca-Qian You are welcome :) Im glad to contribute as well

lvwerra
lvwerra approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@lvwerra lvwerra merged commit 00aa31e into huggingface:main Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lvwerra lvwerra lvwerra approved these changes

@younesbelkada younesbelkada younesbelkada approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ArvinZhuang @HuggingFaceDocBuilderDev @Rebecca-Qian @lvwerra @younesbelkada