GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: fix storage issue in torchtensor class by ai-naymul · Pull Request #1833 · docarray/docarray

This repository was archived by the owner on Sep 9, 2026. It is now read-only.

Conversation

@ai-naymul

@ai-naymul 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
JoanFM
JoanFM suggested changes Dec 2, 2023
View reviewed changes

@JoanFM JoanFM left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@codecov

codecov Bot commented Dec 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JoanFM

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ai-naymul
ai-naymul force-pushed the fix-torchtensor-storage-issue branch from d956a97 to c92bca0 Compare December 4, 2023 17:50
@JoanFM

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JoanFM

JoanFM commented Dec 7, 2023

Copy link
Copy Markdown
Member

Hey @ai-naymul ,

Now only DCO is missing
ai-naymul reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JoanFM

JoanFM commented Dec 9, 2023

Copy link
Copy Markdown
Member

Hey @ai-naymul ,

Now only DCO is missing

Can you di the signoff then?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ai-naymul added 4 commits December 9, 2023 22:37
@ai-naymul
fix: fix storage issue in torchtensor class
cc2e62b 
Signed-off-by: Naymul Islam <[email protected]>
@ai-naymul
tests for deepcopy method
39e56ec 
Signed-off-by: Naymul Islam <[email protected]>
@ai-naymul
format code into black
e5ac2f3 
Signed-off-by: Naymul Islam <[email protected]>
@ai-naymul
reformat torchtensor class
48b0f32 
Signed-off-by: Naymul Islam <[email protected]>
@ai-naymul
ai-naymul force-pushed the fix-torchtensor-storage-issue branch from 58c0cfe to 48b0f32 Compare December 9, 2023 16:37
@ai-naymul

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JoanFM 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
3 of 6 tasks
JoanFM
JoanFM approved these changes Dec 9, 2023
View reviewed changes
@JoanFM
JoanFM merged commit 3cfa0b8 into docarray:main Dec 9, 2023
@JoanFM

JoanFM commented Dec 9, 2023

Copy link
Copy Markdown
Member

Thanks a lot for the contribution!
ai-naymul reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ai-naymul

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
samsja
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 samsja Dec 12, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ai-naymul ai-naymul Dec 13, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
samsja reacted with rocket emoji
All reactions
  • 🚀 1 reaction

@ai-naymul ai-naymul Dec 20, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@samsja samsja Dec 20, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ai-naymul reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@samsja samsja mentioned this pull request Dec 21, 2023
refactor test of the torchtensor #1837
Merged
@JoanFM JoanFM mentioned this pull request Dec 22, 2023
Release Note #1842
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@samsja samsja samsja left review comments
@JoanFM JoanFM JoanFM approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@ai-naymul @JoanFM @samsja
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.