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] Allow any name for observation and global_state keys #72

Merged
merged 4 commits into from
Apr 5, 2024

Conversation

matteobettini
Copy link
Collaborator

This PR allows any name for the group observation key and the global state key.

Before benchmarl constrained the observation to be named "observation" and the global state to be named "state"

cc @ezhang7423

@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 Apr 5, 2024
@matteobettini
Copy link
Collaborator Author

matteobettini commented Apr 5, 2024

The next PR will allow any shape observation and state.

However there is a problem. DDPG and SAC critics take as input the concatenation of the state and the actions.

Up until now we had only 1-dimensional states and observation so concatenating the actions was easy.

Now if the state and obs can be pictures, concatenating them to actions is less trivial.

They should ideally be prrcessed by different nets before being concatenated.

This poses a non-trivial problem to the way BenchMARL is designed. In particular:

  • BenchMARL models, algorithms and envs are designed to be decoupled. I want to avoid the rllib way where the codebase realizes that you are using images and instantiates a CNN for you. If the user runs Meltingpot with an MLP i want an error to be thrown telling the user to explicitly add a CNN to their model.

Proposed solution

The proposed solution is to allow models to get more than one key as input.

For example if an MLP receives 2 keys, it will concatenate them before processing (just like in torchrl https://github.com/pytorch/rl/blob/2461eb20d21b79a410e01aed71c26b77712a30d8/torchrl/modules/models/models.py#L295)

If an CNN receives multiple inputs, it will also try to concatenate them, but if they have different shapes, it will just cat the images, process them and then cat the other tensors to the output.

This means that if you have a CNN+MLP model and you pass it an image and a tensor, the CNN will process the image and cat the output to the other inputs and the MLP will process this

I implemented this in #73

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 47.57%. Comparing base (276345b) to head (eb69934).

Files Patch % Lines
benchmarl/algorithms/qmix.py 0.00% 6 Missing ⚠️
benchmarl/algorithms/common.py 0.00% 2 Missing ⚠️
benchmarl/algorithms/maddpg.py 0.00% 2 Missing ⚠️
benchmarl/algorithms/masac.py 0.00% 2 Missing ⚠️
benchmarl/algorithms/iddpg.py 0.00% 1 Missing ⚠️
benchmarl/algorithms/isac.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #72       +/-   ##
===========================================
- Coverage   89.96%   47.57%   -42.40%     
===========================================
  Files          69       69               
  Lines        2731     2739        +8     
===========================================
- Hits         2457     1303     -1154     
- Misses        274     1436     +1162     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matteobettini matteobettini merged commit e6afcf5 into main Apr 5, 2024
13 checks passed
@matteobettini matteobettini deleted the more_general_ob branch April 5, 2024 11:16
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