FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: enable torchembedding deepcopy by AnneYang720 · Pull Request #1143 · docarray/docarray · GitHub

fix: enable torchembedding deepcopy - #1143

Merged
AnneYang720 merged 5 commits into
feat-rewrite-v2from
fix-torchembedding-deepcopy
Feb 20, 2023
Merged

fix: enable torchembedding deepcopy#1143
AnneYang720 merged 5 commits into
feat-rewrite-v2from
fix-torchembedding-deepcopy

Conversation

Copy link
Copy Markdown
Contributor

Goals:

This PR is related to #1131

Signed-off-by: AnneY <evangeline-lun@foxmail.com>

Copy link
Copy Markdown
Contributor Author

After the modification

import torch
from docarray import BaseDocument
from docarray.typing.tensor.embedding.torch import TorchEmbedding

class Meow(BaseDocument):
    embedding : TorchEmbedding

a = Meow(embedding=torch.randn(42))
print(id(a.embedding), id(a.copy(deep=True).embedding))

output has no error

140248435632272 140248435636032

AnneYang720 self-assigned this Feb 16, 2023
AnneYang720 marked this pull request as draft February 16, 2023 14:58
AnneYang720 marked this pull request as ready for review February 17, 2023 08:11

samsja commented Feb 17, 2023

Copy link
Copy Markdown
Member

Can u add to test to check that it is actual deep copied and not shallow copy ?

Signed-off-by: AnneY <evangeline-lun@foxmail.com>

Copy link
Copy Markdown
Member

Can you explain this fix briefly? It is not obvious to me why this is fixing it

AnneYang720 commented Feb 20, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

Can you explain this fix briefly? It is not obvious to me why this is fixing it

The deepcopy error is thrown from here https://github.com/pytorch/pytorch/blob/master/torch/_tensor.py#L170, and lines above are

new_tensor = self.new_empty([])
if type(new_tensor) is not type(self):

where type(new_tensor) is <class 'torch.Tensor'> and
type(self) is <class 'docarray.typing.tensor.embedding.torch.TorchEmbedding'>.

So according to the error msg, I should implement new_empty() for the subclass, which means the new_tensor should also be a TorchEmbedding.

samsja commented Feb 20, 2023

Copy link
Copy Markdown
Member

Can you explain this fix briefly? It is not obvious to me why this is fixing it

The deepcopy error is thrown from here https://github.com/pytorch/pytorch/blob/master/torch/_tensor.py#L170, and lines above are

new_tensor = self.new_empty([])
if type(new_tensor) is not type(self):

where type(new_tensor) is <class 'torch.Tensor'> and type(self) is <class 'docarray.typing.tensor.embedding.torch.TorchEmbedding'>.

So according to the error msg, I should implement new_empty() for the subclass, which means the new_tensor should also be a TorchEmbedding.

@AnneYang720 can u add this to the docstring of new_empty in your PR so that ppl who will read the code will understand the same ?

Signed-off-by: AnneY <evangeline-lun@foxmail.com>

Copy link
Copy Markdown

📝 Docs are deployed on https://ft-fix-torchembedding-deepcopy--jina-docs.netlify.app 🎉

AnneYang720 merged commit dcf19a1 into feat-rewrite-v2 Feb 20, 2023
AnneYang720 deleted the fix-torchembedding-deepcopy branch February 20, 2023 12:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL