Skip to content

Conversation

@albertbou92
Copy link
Contributor

@albertbou92 albertbou92 commented Nov 23, 2022

Description

Initialising the class LazyTensorStorage with a nested TensorDict raises the following error

RuntimeError: the number of sizes provided (1) must be greater or equal to the number of dimensions in the TensorDict (2)

It it due to a call to tensor.expand . I just added the the dimensions of tensor to the output shape, and now it does not crash anymore.

The error can be reproduced with the following code:


import torch
from tensordict import TensorDict
from torchrl.data.replay_buffers.storages import LazyTensorStorage

dummy_td = TensorDict({
    "key1": torch.ones(3, 4),
    "key2": torch.ones(3, 4),
    "next": TensorDict({
        "next_key1": torch.ones(3, 4),
        "next_key2": torch.ones(3, 4),
    }, [3, 4])
}, [3, 4])

mystorage = LazyTensorStorage(1000)
mystorage._init(dummy_td)

Types of changes

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds core functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)
  • Example (update in the folder of examples)

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!

  • I have read the CONTRIBUTION guide (required)
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.

@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 Nov 23, 2022
@vmoens
Copy link
Collaborator

vmoens commented Nov 23, 2022

Good catch!
can you look at LazyMemmapStorage, where this error has been solved? You can pretty much copy-paste the _init method from there

@albertbou92 albertbou92 changed the title [BugFix] Initialising the class LazyTensorStorage with a nested TensorDict raises error [BugFix] Initialising the classes LazyTensorStorage and LazyMemmapStorage with a nested TensorDict raises errors Nov 23, 2022
@vmoens
Copy link
Collaborator

vmoens commented Nov 23, 2022

I might be missing something but wasn't the implementation of lazymemmaptensor already solving that issue?
What bug did you get with it when creating a nested TD?
Since it's what we do in the training scripts I would have expected the tests of the training scripts to fail.

1 similar comment
@vmoens
Copy link
Collaborator

vmoens commented Nov 23, 2022

I might be missing something but wasn't the implementation of lazymemmaptensor already solving that issue?
What bug did you get with it when creating a nested TD?
Since it's what we do in the training scripts I would have expected the tests of the training scripts to fail.

@albertbou92
Copy link
Contributor Author

albertbou92 commented Nov 23, 2022

I might be missing something but wasn't the implementation of lazymemmaptensor already solving that issue? What bug did you get with it when creating a nested TD? Since it's what we do in the training scripts I would have expected the tests of the training scripts to fail.

Seems like I did not have the latest version of tensordict, that is why I was getting also an error for LazyTensorStorage. I reverted the latest changes.

And then fixed LazyTensorStorage _init based on the solution in LazyMemmapStorage _init

@albertbou92 albertbou92 force-pushed the bugfix_lazytensorstorage branch from f48bb28 to 761acb5 Compare November 23, 2022 19:37
@albertbou92 albertbou92 changed the title [BugFix] Initialising the classes LazyTensorStorage and LazyMemmapStorage with a nested TensorDict raises errors [BugFix] Initialising the classes LazyTensorStorage with a nested TensorDict raises error Nov 23, 2022
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
Do you mind copying the code snippet that caused the error in a test in test_rb.py for the tensor and memmap version?

@albertbou92
Copy link
Contributor Author

LGTM Do you mind copying the code snippet that caused the error in a test in test_rb.py for the tensor and memmap version?

done!

@vmoens vmoens merged commit c5454de into pytorch:main Nov 24, 2022
@albertbou92 albertbou92 deleted the bugfix_lazytensorstorage branch November 30, 2022 14:35
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