Skip to content

fix bug in batched_forward_pass#144

Merged
lvwerra merged 3 commits intohuggingface:mainfrom
ArvinZhuang:fix-batch-forward
Feb 14, 2023
Merged

fix bug in batched_forward_pass#144
lvwerra merged 3 commits intohuggingface:mainfrom
ArvinZhuang:fix-batch-forward

Conversation

@ArvinZhuang
Copy link
Copy Markdown
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

Copy link
Copy Markdown
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.

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!

[{"input_ids": q, "attention_mask": torch.ones_like(q)} for q in queries]
).to(self.accelerator.device)

attention_mask = [torch.ones_like(r) for r in responses]
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

HuggingFaceDocBuilderDev commented Feb 11, 2023

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

ArvinZhuang commented Feb 11, 2023

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

Copy link
Copy Markdown
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
Copy Markdown
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.

@Rebecca-Qian
Copy link
Copy Markdown

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

@ArvinZhuang
Copy link
Copy Markdown
Contributor Author

ArvinZhuang commented Feb 14, 2023

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

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

Copy link
Copy Markdown
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
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
* fix bug in batched_forward_pass

* style and quality

* revert examples
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.

5 participants