Skip to content

fix: fix storage issue in torchtensor class #1833

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

Merged
JoanFM merged 4 commits into from
Dec 9, 2023
Merged

fix: fix storage issue in torchtensor class #1833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc2e62b
fix: fix storage issue in torchtensor class
ai-naymul Dec 2, 2023
39e56ec
tests for deepcopy method
ai-naymul Dec 3, 2023
e5ac2f3
format code into black
ai-naymul Dec 4, 2023
48b0f32
reformat torchtensor class
ai-naymul Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docarray/typing/tensor/torch_tensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,16 @@ def __torch_function__(cls, func, types, args=(), kwargs=None):
)
return super().__torch_function__(func, types_, args, kwargs)

def __deepcopy__(self, memo):
"""
Custom implementation of deepcopy for TorchTensor to avoid storage sharing issues.
"""
# Create a new tensor with the same data and properties
new_tensor = self.clone()
# Set the class to the custom TorchTensor class
new_tensor.__class__ = self.__class__
return new_tensor

@classmethod
def _docarray_from_ndarray(cls: Type[T], value: np.ndarray) -> T:
"""Create a `tensor from a numpy array
Expand Down
18 changes: 18 additions & 0 deletions tests/integrations/typing/test_torch_tensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import torch
from docarray.typing.tensor.torch_tensor import TorchTensor
import copy

from docarray import BaseDoc
from docarray.typing import TorchEmbedding, TorchTensor
Expand All @@ -25,3 +27,19 @@ class MyDocument(BaseDoc):
assert isinstance(d.embedding, TorchEmbedding)
assert isinstance(d.embedding, torch.Tensor)
assert (d.embedding == torch.zeros((128,))).all()


def test_torchtensor_deepcopy():
# Setup
original_tensor_float = TorchTensor(torch.rand(10))
original_tensor_int = TorchTensor(torch.randint(0, 100, (10,)))

# Exercise
copied_tensor_float = copy.deepcopy(original_tensor_float)
copied_tensor_int = copy.deepcopy(original_tensor_int)

# Verify
assert torch.equal(original_tensor_float, copied_tensor_float)
assert original_tensor_float is not copied_tensor_float
assert torch.equal(original_tensor_int, copied_tensor_int)
assert original_tensor_int is not copied_tensor_int
Comment on lines +43 to +45

@samsja samsja Dec 12, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this test is not enough. You should rather do

assert original_tensor_float.data_ptr() != copied_tensor_float.data_ptr()

indeed in pytorch each view of a tensor would have a different python ID but what matter is the underlying storage.

Good PR otherwise !!

@ai-naymul ai-naymul Dec 13, 2023

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.

Okay let me fix that..!

@ai-naymul ai-naymul Dec 20, 2023

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.

I think this test is not enough. You should rather do

assert original_tensor_float.data_ptr() != copied_tensor_float.data_ptr()

indeed in pytorch each view of a tensor would have a different python ID but what matter is the underlying storage.

Good PR otherwise !!

Hi @samsja
Accidently I deleted the repo from my local machine that's why I had to clone it again and regarding that I am unable to change in this merged pull request, so for to reafctor the test I need to create another PR.
So should I create a another PR regarding that?

@samsja samsja Dec 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes you would have to create another PR for this