Skip to content

Conversation

nicolas-dufour
Copy link
Contributor

@nicolas-dufour nicolas-dufour commented Aug 1, 2022

Description

This PR aims to change tensordict.to_tensordict behaviour. The idea is to make a clone each time the method is called.

Motivation and Context

The motivation behind it is clone the tensordict when performing .to_tensordict.

  • I have raised an issue to propose this change (required for new features and bug fixes)

Types of changes

What types of changes does your code introduce? Remove all that do not apply:

  • New feature (non-breaking change which adds core functionality)

Checklist

Go over all the following points, and put an x in all the boxes that apply.
If you are unsure about any of these, don't hesitate to ask. We are here to help!

@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 Aug 1, 2022
@nicolas-dufour nicolas-dufour requested a review from vmoens August 1, 2022 16:21
@vmoens
Copy link
Collaborator

vmoens commented Aug 1, 2022

Thanks @nicolas-dufour for this.
Here's how I would see this instead:

For TensorDictBase, return a tensordict like this

TensorDict({key: value.clone() if not isinstance(value, TensordictBase) else value.to_tensordict() for key, value in self.items()}, **metadata)

And metadata are all the values precomputed elsewhere EXCEPT memmap (it won't be memmap because we clone) shared mem (idem). We can leave the metatensor empty (since the metadata will change).

For savedtensordict we would need to do the same but load the tensordict first.

Hope that makes sense!

Copy link
Collaborator

@vmoens vmoens left a comment

Choose a reason for hiding this comment

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

LGTM! Let's address these two comments and make the tests pass, then we're good to go!

@vmoens
Copy link
Collaborator

vmoens commented Aug 2, 2022

LGTM! Let's address these two comments and make the tests pass, then we're good to go!

@nicolas-dufour also we should rename this PR
why not: "make to_tensordict() create a copy of the content"?

We should also update the docstrings

@nicolas-dufour nicolas-dufour changed the title [Refactor] Change to_tensordict to clone when called [Refactor] make to_tensordict() create a copy of the conten Aug 2, 2022
@nicolas-dufour nicolas-dufour changed the title [Refactor] make to_tensordict() create a copy of the conten [Refactor] make to_tensordict() create a copy of the content Aug 2, 2022
@vmoens vmoens added enhancement New feature or request performance Performance issue or suggestion for improvement labels Aug 3, 2022
@vmoens vmoens merged commit 5f3c437 into pytorch:main Aug 4, 2022
@nicolas-dufour nicolas-dufour deleted the to_tensordict_clone branch September 16, 2022 16:37
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. enhancement New feature or request performance Performance issue or suggestion for improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants