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

Remove _copy_tensor from usage #633

Merged
merged 6 commits into from
Apr 2, 2024
Merged

Remove _copy_tensor from usage #633

merged 6 commits into from
Apr 2, 2024

Conversation

rohan-varma
Copy link
Member

Context

  • _copy_tensor was not needed and was added due to a misunderstanding on how pytorch internally handles tensor reference counting. The thinking was that we're creating a linear layer and want to save the bias, so we just deep clone the bias before returning it. This however is inaccurate, and its sufficient to just return a reference to the bias. This is because torch will internally bump the refcount to the bias's storage by 2, and when the function goes out of scope, we'll still have a reference count of 1 due to the returned bias variable.
  • This change already exists for weight.

Changelog

  • Get rid of _copy_tensor

Test plan

  • CI

Copy link

pytorch-bot bot commented Apr 1, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/torchtune/633

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit a38d17c with merge base 2940941 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 1, 2024
Copy link
Contributor

@kartikayk kartikayk 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 revisiting this!

@rohan-varma rohan-varma merged commit 752597d into main Apr 2, 2024
20 checks passed
tcapelle pushed a commit to tcapelle/torchtune that referenced this pull request Apr 5, 2024
@joecummings joecummings deleted the rm_copy branch April 11, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants