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

[Feature] Heterogeneous Environments compatibility #1411

Merged
merged 106 commits into from
Aug 4, 2023

Conversation

matteobettini
Copy link
Contributor

@matteobettini matteobettini commented Jul 25, 2023

This introduces tests for het envs and key modifications to

  • rollout
  • step_mdp

vmoens and others added 30 commits July 6, 2023 14:56
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
matteobettini and others added 11 commits July 28, 2023 09:34
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
# Conflicts:
#	test/test_specs.py
#	torchrl/data/tensor_specs.py
#	torchrl/data/utils.py
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Copy link
Contributor

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

Thanks for this, really useful

torchrl/envs/common.py Outdated Show resolved Hide resolved
torchrl/data/utils.py Outdated Show resolved Hide resolved
torchrl/data/utils.py Outdated Show resolved Hide resolved
torchrl/data/utils.py Outdated Show resolved Hide resolved
torchrl/data/utils.py Outdated Show resolved Hide resolved
torchrl/data/utils.py Outdated Show resolved Hide resolved
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
@matteobettini
Copy link
Contributor Author

So i have updated and added all the requested changes.
my takes on the new function are

  • it should be public because any user which has some nested lazy stacks somewhere should use it after calling env.rollout(return_contiguous=False)
  • my personal preference is that it should be in tensordict utils but the choice is yours

@vmoens vmoens added the enhancement New feature or request label Aug 2, 2023
@vmoens
Copy link
Contributor

vmoens commented Aug 2, 2023

So i have updated and added all the requested changes. my takes on the new function are

  • it should be public because any user which has some nested lazy stacks somewhere should use it after calling env.rollout(return_contiguous=False)
  • my personal preference is that it should be in tensordict utils but the choice is yours

let's put it in tensordict then!

Comment on lines 249 to 250
shape = source.get_item_shape(key)
if -1 not in shape:
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the overhead here?
Since this will likely be very frequent I'm a bit worried by increasing the control flow for every key

Copy link
Contributor

Choose a reason for hiding this comment

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

For a regular td, this op means that we'll be accessing the tensor twice through get (once to get its shape, a second time to get it). Accessing it once should suffice.
Not I understand that we can't use get if the tensor is heterogeneous... A try/except may not be the right solution either since it'll be slow for heterogeneous envs. However regular envs won't be impacted

Copy link
Contributor

Choose a reason for hiding this comment

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

image
I ran the benchmarks and this implementation is 10 - 35% slower than the current one. Any chance we can avoid taking that hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what I can do here

  • in the past i was using try blocks, but you made me move away from them
  • an isinstance wont work since the key might be het even if the container is a regular td

so I can move back to try except if you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lemme switch to try blocks

ideally tho this problem could be solved once and for all in tensordict, with an efficient way to understand if a key is het

Copy link
Contributor

Choose a reason for hiding this comment

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

yep that's what I'm thinking too
If we have tensorstack we can even leave the block with try / except and it won't break (I guess)

Copy link
Contributor

Choose a reason for hiding this comment

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

Just make sure you comment the try / excpet saying that it's temporary and for efficiency purpose

@matteobettini
Copy link
Contributor Author

updated

Copy link
Contributor

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

Het groene licht is gekomen

@vmoens vmoens merged commit b210665 into pytorch:main Aug 4, 2023
44 of 55 checks passed
@matteobettini matteobettini deleted the het_env branch August 4, 2023 14:20
vmoens added a commit to hyerra/rl that referenced this pull request Oct 10, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants