[Feature] Append, init and insert transforms in ReplayBuffer #695

altre · 2022-11-21T16:50:25Z

Description

Implemented passing transforms in init, appending and inserting transforms as methods in ReplayBuffer
closes #612 and closes #692.

Skips any code in transforms that accesses the parent without it being defined.

I think this approach is preferable to: #690

Inserting at an out of bounds index throws an error as implemented in Compose. Note that this is not the same behavior as a native python list, which just inserts at the beginning or end of list and doesn't throw.

I'm not quite sure what is meant by transform_*_spec returning an error, since these methods are defined as public on transform subclasses and are not called in this implementation.

We could prevent the use of them by subclassing Compose, but I don't think it's necessary?

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)

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

altre · 2022-11-22T12:21:14Z

The remaining failure: FAILED test/test_libs.py::TestCollectorLib::test_collector_run[device1-GymEnv-env_args1-env_kwargs1] - EOFError (https://app.circleci.com/pipelines/github/pytorch/rl/4652/workflows/88023457-c917-479c-bcff-9ebd04e46af6/jobs/114173?invite=true#step-108-3532) does not seem to be related to my changes, I think?

vmoens · 2022-11-22T13:09:57Z

The remaining failure: FAILED test/test_libs.py::TestCollectorLib::test_collector_run[device1-GymEnv-env_args1-env_kwargs1] - EOFError (https://app.circleci.com/pipelines/github/pytorch/rl/4652/workflows/88023457-c917-479c-bcff-9ebd04e46af6/jobs/114173?invite=true#step-108-3532) does not seem to be related to my changes, I think?

Nope those seem to be flacky tests

vmoens

Let's keep the errors as they were, #697 will implement more informative error messages.

vmoens · 2022-11-22T13:11:56Z

test/test_rb.py

+@pytest.mark.parametrize("transform", transforms)
+def test_smoke_replay_buffer_transform(transform):
+    rb = rb_prototype.ReplayBuffer(
+        collate_fn=lambda x: torch.stack(x, 0),


we should be able to remove the collate_fn as of #688

Ok, merged upstream.

test/test_rb.py

torchrl/data/replay_buffers/rb_prototype.py

vmoens · 2022-11-22T13:18:47Z

torchrl/envs/transforms/transforms.py

        if self._unsqueeze_dim_orig < 0:
            self._unsqueeze_dim = self._unsqueeze_dim_orig
-        else:
+        elif self.parent:


The three errors of #692 are legit and should be raised.
They come from the fact that one should not assume that data comes with a certain batch size when creating a transform without parent, ie the number of first dimensions cannot be determined beforehand.
Hence

transform = UnsqueezeTransform(0)

should raise an issue because we don't know if the batch size of the tensors that will come in and hard to debug errors will occur. Most of the time, users will know the last dimensions of their tensors and can perfectly code

transform = UnsqueezeTransform(-n)

to get the desired behaviour.

vmoens · 2022-11-22T13:19:17Z

torchrl/envs/transforms/transforms.py

-                if self.last_dim >= 0:
-                    self.last_dim = self.last_dim - len(observation_spec.shape)
-                break
+        if isinstance(self.parent, EnvBase):


See #697 and my comment below regarding this

Ok, reverted. This will break the test until #692 is merged.

codecov · 2022-11-22T22:00:41Z

Codecov Report

Merging #695 (9280d2a) into main (ef0a78f) will increase coverage by 0.10%.
The diff coverage is 98.85%.

@@            Coverage Diff             @@
##             main     #695      +/-   ##
==========================================
+ Coverage   88.60%   88.70%   +0.10%     
==========================================
  Files         121      121              
  Lines       20690    20759      +69     
==========================================
+ Hits        18333    18415      +82     
+ Misses       2357     2344      -13

Flag	Coverage Δ
habitat-gpu	`24.44% <6.25%> (ø)`
linux-cpu	`84.81% <98.85%> (+0.10%)`	⬆️
linux-gpu	`85.70% <98.85%> (+0.09%)`	⬆️
linux-jumanji	`29.38% <6.25%> (ø)`
linux-outdeps-gpu	`72.41% <97.67%> (+0.30%)`	⬆️
linux-stable-cpu	`84.67% <98.85%> (+0.10%)`	⬆️
linux-stable-gpu	`85.39% <98.85%> (+0.11%)`	⬆️
linux_examples-gpu	`43.15% <83.33%> (+<0.01%)`	⬆️
macos-cpu	`84.48% <98.85%> (+0.10%)`	⬆️
olddeps-gpu	`74.50% <98.83%> (+0.14%)`	⬆️

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

Impacted Files	Coverage Δ
torchrl/envs/transforms/transforms.py	`87.12% <93.75%> (+0.88%)`	⬆️
examples/dreamer/dreamer_utils.py	`78.28% <100.00%> (ø)`
test/test_rb.py	`96.89% <100.00%> (+0.46%)`	⬆️
test/test_transforms.py	`96.21% <100.00%> (+0.01%)`	⬆️
torchrl/data/replay_buffers/rb_prototype.py	`88.59% <100.00%> (+1.18%)`	⬆️
torchrl/trainers/helpers/envs.py	`74.87% <100.00%> (ø)`
torchrl/envs/vec_env.py	`69.06% <0.00%> (+0.50%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vmoens

LGTM!

* amend * [BugFix] ConvNet forward method with tensors of more than 4 dimensions (#686) * cnn forward fix * more general code * cnn testing * precommit run check * convnet tests * [Feature] add `standard_normal` for RewardScaling (#682) * Add standard_normal * give attribute access * Update standard_normal * Update tests * Fix tests * Address in-place scaling of reward * Improvise tests * [Feature] Jumanji envs (#674) * amend * [Feature] Default collate_fn (#688) * init * amend * amend * [BugFix] Fix Examples (#687) * amend * [Refactoring] Replace direct gym version checks with decorated functions (#691) * [Refactoring] Replace gym version checking with decorated functions (#) Initial commit. Only tests. * Refactoring in gym.py * More refactoring in gym.py * Completed refactoring * amend * amend * Version 0.0.3 (#696) * [Docs] Host TensorDict docs inside TorchRL docs (#693) * Pull tensordict docs into TorchRL docs * Add banner for tensordict docs * [BugFix] Fix docs build (#698) * [BugFix] Proper error messages for orphan transform creation (#697) * amend * [Feature] Append, init and insert transforms in ReplayBuffer (#695) * lint Co-authored-by: albertbou92 <albertbou92@users.noreply.github.com> Co-authored-by: Aditya Gandhamal <61016383+adityagandhamal@users.noreply.github.com> Co-authored-by: yingchenlin <yc.jon.lin@gmail.com> Co-authored-by: Sergey Ordinskiy <113687736+ordinskiy@users.noreply.github.com> Co-authored-by: Tom Begley <tomcbegley@gmail.com> Co-authored-by: Alan Schelten <alan@schelten.net>

* init * tests1 * run examples in tests * [Feature] MPPI Planner (#694) * amend * [BugFix] ConvNet forward method with tensors of more than 4 dimensions (#686) * cnn forward fix * more general code * cnn testing * precommit run check * convnet tests * [Feature] add `standard_normal` for RewardScaling (#682) * Add standard_normal * give attribute access * Update standard_normal * Update tests * Fix tests * Address in-place scaling of reward * Improvise tests * [Feature] Jumanji envs (#674) * amend * [Feature] Default collate_fn (#688) * init * amend * amend * [BugFix] Fix Examples (#687) * amend * [Refactoring] Replace direct gym version checks with decorated functions (#691) * [Refactoring] Replace gym version checking with decorated functions (#) Initial commit. Only tests. * Refactoring in gym.py * More refactoring in gym.py * Completed refactoring * amend * amend * Version 0.0.3 (#696) * [Docs] Host TensorDict docs inside TorchRL docs (#693) * Pull tensordict docs into TorchRL docs * Add banner for tensordict docs * [BugFix] Fix docs build (#698) * [BugFix] Proper error messages for orphan transform creation (#697) * amend * [Feature] Append, init and insert transforms in ReplayBuffer (#695) * lint Co-authored-by: albertbou92 <albertbou92@users.noreply.github.com> Co-authored-by: Aditya Gandhamal <61016383+adityagandhamal@users.noreply.github.com> Co-authored-by: yingchenlin <yc.jon.lin@gmail.com> Co-authored-by: Sergey Ordinskiy <113687736+ordinskiy@users.noreply.github.com> Co-authored-by: Tom Begley <tomcbegley@gmail.com> Co-authored-by: Alan Schelten <alan@schelten.net> * lint Co-authored-by: albertbou92 <albertbou92@users.noreply.github.com> Co-authored-by: Aditya Gandhamal <61016383+adityagandhamal@users.noreply.github.com> Co-authored-by: yingchenlin <yc.jon.lin@gmail.com> Co-authored-by: Sergey Ordinskiy <113687736+ordinskiy@users.noreply.github.com> Co-authored-by: Tom Begley <tomcbegley@gmail.com> Co-authored-by: Alan Schelten <alan@schelten.net>

Append, init and insert transforms in ReplayBuffer

b20b382

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 21, 2022

altre and others added 3 commits November 21, 2022 18:02

Merge branch 'main' into transforms_to_replaybuffer

a3b9c1f

Skip tests needing torchvision if not installed.

e4cf0c5

Fix torchvsion test check.

bd119cd

vmoens changed the title ~~Append, init and insert transforms in ReplayBuffer~~ [Feature] Append, init and insert transforms in ReplayBuffer Nov 22, 2022

vmoens reviewed Nov 22, 2022

View reviewed changes

altre added 3 commits November 22, 2022 16:55

Merge remote-tracking branch 'upstream' into transforms_to_replaybuffer

b6fea4d

Assert all transforms are called.

b6396ed

Keep missing parent errors.

1ae774c

vmoens added the enhancement New feature or request label Nov 22, 2022

vmoens added 4 commits November 22, 2022 20:45

Merge branch 'main' into transforms_to_replaybuffer

f6caff9

amend

59aeba7

amend

518cd07

lint

9280d2a

vmoens approved these changes Nov 23, 2022

View reviewed changes

vmoens merged commit 9dc9312 into pytorch:main Nov 23, 2022

vmoens mentioned this pull request Nov 23, 2022

Prototype transformed replay buffers using extra class. #690

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Feature] Append, init and insert transforms in ReplayBuffer #695

[Feature] Append, init and insert transforms in ReplayBuffer #695

Uh oh!

altre commented Nov 21, 2022

Uh oh!

altre commented Nov 22, 2022

Uh oh!

vmoens commented Nov 22, 2022

Uh oh!

vmoens left a comment

Uh oh!

vmoens Nov 22, 2022

Uh oh!

altre Nov 22, 2022

Uh oh!

Uh oh!

Uh oh!

vmoens Nov 22, 2022

Uh oh!

vmoens Nov 22, 2022

Uh oh!

altre Nov 22, 2022

Uh oh!

codecov bot commented Nov 22, 2022 •

edited

Loading

Uh oh!

vmoens left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Feature] Append, init and insert transforms in ReplayBuffer #695

[Feature] Append, init and insert transforms in ReplayBuffer #695

Uh oh!

Conversation

altre commented Nov 21, 2022

Description

Types of changes

Checklist

Uh oh!

altre commented Nov 22, 2022

Uh oh!

vmoens commented Nov 22, 2022

Uh oh!

vmoens left a comment

Choose a reason for hiding this comment

Uh oh!

vmoens Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

altre Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vmoens Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

vmoens Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

altre Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vmoens left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 22, 2022 •

edited

Loading