Skip to content

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
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 just make sure that we don't have keys that collide

@vmoens vmoens added the bug Something isn't working label Jan 4, 2023
@vmoens
Copy link
Collaborator

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