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

fix: fix storage issue in torchtensor class by ai-naymul · Pull Request #1833 · docarray/docarray · GitHub

/ docarray Public

fix: fix storage issue in torchtensor class - #1833

Merged
JoanFM merged 4 commits into
docarray:mainfrom
ai-naymul:fix-torchtensor-storage-issue
Dec 9, 2023
ConversationCommits (4)ChecksFiles changed
Merged

fix: fix storage issue in torchtensor class#1833
JoanFM merged 4 commits into
docarray:mainfrom
ai-naymul:fix-torchtensor-storage-issue

Conversation

ai-naymul commented Dec 2, 2023

Copy link
Copy Markdown
Contributor

Issue no : #1831

The primary purpose of adding the deepcopy method is to provide a more controlled and tailored approach to deep copying instances of the TorchTensor class. This helps prevent any unexpected behaviors arising from shared references to the same memory.

JoanFM requested changes Dec 2, 2023
View reviewed changes

JoanFM left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We need to add tests showing that the original issue is fixed

codecov Bot commented Dec 3, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (522811f) 85.05% compared to head (48b0f32) 85.02%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1833      +/-   ##
==========================================
- Coverage   85.05%   85.02%   -0.03%     
==========================================
  Files         135      135              
  Lines        9031     9035       +4     
==========================================
+ Hits         7681     7682       +1     
- Misses       1350     1353       +3
Flag Coverage Δ
docarray 85.02% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoanFM commented Dec 3, 2023

Copy link
Copy Markdown
Member

Hey @ai-naymul ,

We need you to apply black formatting to your code and to sign-off your commits.

ai-naymul force-pushed the fix-torchtensor-storage-issue branch from d956a97 to c92bca0 Compare December 4, 2023 17:50

JoanFM commented Dec 4, 2023

Copy link
Copy Markdown
Member

hey @ai-naymul,

Now you should also apply the sign off and black in this PR?

JoanFM commented Dec 7, 2023

Copy link
Copy Markdown
Member

Hey @ai-naymul ,

Now only DCO is missing

JoanFM commented Dec 9, 2023

Copy link
Copy Markdown
Member

Hey @ai-naymul ,

Now only DCO is missing

Can you di the signoff then?

fix: fix storage issue in torchtensor class
cc2e62b 
Signed-off-by: Naymul Islam <naymul504@gmail.com>
tests for deepcopy method
39e56ec 
Signed-off-by: Naymul Islam <naymul504@gmail.com>
format code into black
e5ac2f3 
Signed-off-by: Naymul Islam <naymul504@gmail.com>
reformat torchtensor class
48b0f32 
Signed-off-by: Naymul Islam <naymul504@gmail.com>
ai-naymul force-pushed the fix-torchtensor-storage-issue branch from 58c0cfe to 48b0f32 Compare December 9, 2023 16:37

ai-naymul commented Dec 9, 2023

Copy link
Copy Markdown
Contributor Author

Hey @ai-naymul ,
Now only DCO is missing

Can you di the signoff then?

Fixed that.Thanks a lot for your patience

JoanFM linked an issue Dec 9, 2023 that may be closed by this pull request
TorchTensor deepcopy raises error when dtype is not float32 #1831
Closed
6 tasks
JoanFM merged commit 3cfa0b8 into docarray:main Dec 9, 2023

JoanFM commented Dec 9, 2023

Copy link
Copy Markdown
Member

Thanks a lot for the contribution!

ai-naymul commented Dec 9, 2023

Copy link
Copy Markdown
Contributor Author

Thanks a lot for the contribution!

It's a great pleasure to be a part of Jina.

samsja reviewed Dec 12, 2023
View reviewed changes
Comment thread tests/integrations/typing/test_torch_tensor.py
Comment on lines +43 to +45
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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Okay let me fix that..!

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes you would have to create another PR for this

samsja mentioned this pull request Dec 21, 2023
Merged
JoanFM mentioned this pull request Dec 22, 2023
Closed
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

samsja samsja left review comments
JoanFM JoanFM approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TorchTensor deepcopy raises error when dtype is not float32

3 participants

Back | FazBrowse Home | New Git URL