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.

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

multi-gpu: fix tensor device placements for various models #35763

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dvrogozh
Copy link
Contributor

@@ -818,7 +818,7 @@ def forward(
if inputs_embeds is None:
inputs_embeds = self.wte(input_ids)
position_embeds = self.wpe(position_ids)
hidden_states = inputs_embeds + position_embeds
hidden_states = inputs_embeds + position_embeds.to(inputs_embeds.device)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI reviewers, this fix was taken from here:

hidden_states = inputs_embeds + positions.to(inputs_embeds.device)

See #30836 (comment) for associated discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the reference

Copy link
Member

@SunMarc SunMarc left a comment

Choose a reason for hiding this comment

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

See comment here: #35762 (comment)

@qubvel qubvel removed their request for review January 20, 2025 16:49
@dvrogozh
Copy link
Contributor Author

@SunMarc, that's a big change since models reuse the code from each other and that's enforced on ci level with make fix-copies. To simplify the review I have structured commits as follows:

  1. First and second commits are making changed to the "primary" sources (those from which other models copy code)
  2. Third commit is automatically generated with make fix-copies from the first 2

So, I think it might be easier to review first 2 commits separately.

@dvrogozh
Copy link
Contributor Author

@SunMarc : can you, please, help to review again? The issue being fixed here is being noticed by other folks as well, see #35918 and #35920.

Copy link
Member

@SunMarc SunMarc 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 your work and taking the time to make it easy to review for me ! Just a few nits but overall fine with it !

src/transformers/models/bart/modeling_bart.py Outdated Show resolved Hide resolved
src/transformers/models/mbart/modeling_mbart.py Outdated Show resolved Hide resolved
Fixing the following errors in few models:
```
>       hidden_states = inputs_embeds + pos_embeds
E       RuntimeError: Expected all tensors to be on the same device, but found at least two devices, xpu:2 and xpu:3!
```

Fixes: huggingface#35762
Signed-off-by: Dmitry Rogozhkin <[email protected]>
Signed-off-by: Dmitry Rogozhkin <[email protected]>
@SunMarc
Copy link
Member

SunMarc commented Feb 5, 2025

ping @ArthurZucker

@HuggingFaceDocBuilderDev

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants