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

[BugFix] Sorting nested keys #787

Merged
merged 7 commits into from
Jan 4, 2023
Merged

[BugFix] Sorting nested keys #787

merged 7 commits into from
Jan 4, 2023

Conversation

matteobettini
Copy link
Contributor

Description

This PR solves issue #775

@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 Jan 3, 2023
@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Merging #787 (51ab9ab) into main (51ab9ab) will not change coverage.
The diff coverage is n/a.

❗ Current head 51ab9ab differs from pull request most recent head e714dd8. Consider uploading reports for the commit e714dd8 to get more accurate results

@@           Coverage Diff           @@
##             main     #787   +/-   ##
=======================================
  Coverage   88.78%   88.78%           
=======================================
  Files         123      123           
  Lines       21253    21253           
=======================================
  Hits        18869    18869           
  Misses       2384     2384           
Flag Coverage Δ
habitat-gpu 24.79% <0.00%> (ø)
linux-brax 29.37% <0.00%> (ø)
linux-cpu 85.28% <0.00%> (ø)
linux-gpu 86.25% <0.00%> (ø)
linux-jumanji 30.15% <0.00%> (ø)
linux-outdeps-gpu 72.36% <0.00%> (ø)
linux-stable-cpu 85.14% <0.00%> (ø)
linux-stable-gpu 85.90% <0.00%> (ø)
linux_examples-gpu 42.68% <0.00%> (ø)
macos-cpu 85.04% <0.00%> (ø)
olddeps-gpu 76.12% <0.00%> (ø)

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

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

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.

LGTM, let's just make sure that we don't have keys that collide

torchrl/envs/vec_env.py Outdated Show resolved Hide resolved
@vmoens vmoens added the bug Something isn't working label Jan 4, 2023
@vmoens
Copy link
Contributor

vmoens commented Jan 4, 2023

@matteobettini since the problem was occuring with Brax I updated the brax wrapper and tests to make sure that things were passing.
In general, it does not really make sense to execute brax in parallel envs (since it's vectorized), but it's a good way to spot bugs.

@vmoens vmoens merged commit e4e485b into pytorch:main Jan 4, 2023
@matteobettini
Copy link
Contributor Author

@matteobettini since the problem was occuring with Brax I updated the brax wrapper and tests to make sure that
things were passing.
In general, it does not really make sense to execute brax in parallel envs (since it's vectorized), but it's a good way to spot bugs.

@vmoens
I actually think it is important to use parallel envs with brax (and vmas).

This is because yes they perform batched simulation but parallelenv can furtherly distribute this to the various cpu cores.

This is also why rllib has the 2 parameters: num_workers and num_envs_per_worker

@matteobettini matteobettini deleted the sorted branch January 4, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 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