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

Fix: use torch.from_numpy() to create tensors for np.ndarrays #33201

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

shinyano
Copy link
Contributor

What does this PR do?

Fixes #33185 by using torch.from_numpy() to create tensors for np.ndarrays, instead of torch.tensor() which may cause hanging.

I did some rough search in the code and made changes where I thought necessary. Code using tensor() with dtype specified remains unchanged to prevent potential type errors.

Also, torch.from_numpy() would share memory with numpy array, while torch.tensor() copies data. If this change would cause potential problems, please let me know.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

@qubvel Thanks for your review!

Copy link
Member

@qubvel qubvel left a comment

Choose a reason for hiding this comment

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

@shinyano Thanks for fixing this! Looks good overall!

The only potential danger is that from_numpy shares the same storage as the original NumPy array, whereas torch.tensor copies the data. If someone modifies the original array in place, the tensor will also be modified, and vice versa.
However, I'm not sure that will be a real problem.

@amyeroberts for final review

Copy link
Collaborator

@amyeroberts amyeroberts 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!

@@ -579,9 +579,15 @@ def normalize(self, image, mean, std, rescale=False):
import torch

if not isinstance(mean, torch.Tensor):
mean = torch.tensor(mean)
Copy link
Collaborator

Choose a reason for hiding this comment

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

as a general note: this class isn't used anymore so changing this won't have any effect. Let's keep though to be consistent with the other mixins

@amyeroberts amyeroberts merged commit cff06aa into huggingface:main Sep 2, 2024
20 checks passed
@shinyano shinyano deleted the tensor_fix branch September 3, 2024 01:04
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
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.

Replace torch.tensor() with torch.from_numpy() when processing numpy arrays
3 participants