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.

Sign up for GitHub

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

Jump to bottom

[WIP][Feature] Add support of SMAC environment #810

Closed
wants to merge 14 commits into from
Closed

[WIP][Feature] Add support of SMAC environment #810

wants to merge 14 commits into from

Conversation

ordinskiy
Copy link
Contributor

THIS IS DRAFT COMMIT TO TEST CI AND DISCUSS.

Description

Added support of SMAC (StarCraft Multi-Agent Challenge) environment.
Includes CI and documentation.
New tensor specs came from #56.

Motivation and Context

SMAC is a popular library for research in field of multi-agent reinforcement learning (MARL). Its support was proposed by @vmoens in #509.

  • I have raised an issue to propose this change (required for new features and bug fixes)

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)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)
  • Example (update in the folder of examples)

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).
  • I have updated the documentation accordingly.

@ordinskiy
[Feature] Initial commit
c6f38d9
@ordinskiy
Merge remote-tracking branch 'upstream/main' into add_smac_env
b41f356 
# Conflicts:
#	test/test_libs.py
@ordinskiy
amend
1ac5401
@ordinskiy
Merge remote-tracking branch 'upstream/main' into add_smac_env
502ca50 
# Conflicts:
#	test/test_libs.py
#	torchrl/data/__init__.py
#	torchrl/data/tensor_specs.py
@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 9, 2023
@ordinskiy ordinskiy changed the title [Feature] Add support of SMAC environment [WIP][Feature] Add support of SMAC environment Jan 9, 2023
@vmoens vmoens added the Environments Adds or modifies an environment wrapper label Jan 10, 2023
@vmoens
Copy link
Contributor

vmoens commented Jan 10, 2023

cc @matteobettini @PaLeroy
This may be of interest

@matteobettini
Copy link
Contributor

This is cool!

I am interested in the choice of not adding the agent dimension in the batch_size. Is there a reason for this?

@vmoens
Copy link
Contributor

vmoens commented Jan 10, 2023

This is cool!

I am interested in the choice of not adding the agent dimension in the batch_size. Is there a reason for this?

That was actually the reason I was pinging you on this. @ordinskiy It would be nice if the batch_size matched the number of agents no?

@ordinskiy
Copy link
Contributor Author

This is cool!
I am interested in the choice of not adding the agent dimension in the batch_size. Is there a reason for this?

That was actually the reason I was pinging you on this. @ordinskiy It would be nice if the batch_size matched the number of agents no?

Thank you for the feedback @matteobettini @vmoens!
I will set default batch_size of the environment to the number of agents on a given map.

matteobettini
matteobettini reviewed Jan 11, 2023
View reviewed changes
torchrl/envs/libs/smac.py Outdated

def _make_observation_spec(self, env: smac.env.StarCraft2Env) -> TensorSpec:
info = env.get_env_info()
size = torch.Size((info["n_agents"], info["obs_shape"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

This Nd specs are doing what we were discussing in #766 (comment)

Aka we are leaking part of the batch_size (agents) into the specs.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you add the agents to the batch_size, normal specs will likely suffice. But #766 might be helpful for this as well

@vmoens vmoens mentioned this pull request Jan 11, 2023
Open
36 tasks
@PaLeroy
Copy link
Contributor

PaLeroy commented Jan 11, 2023

Nice work!
A nice step towards MARL integration. 🎉

@ordinskiy
Copy link
Contributor Author

I'm going to refactor this using MultiOneHotDiscreteTensorSpec from #829 which, as I understand, is the result of #766 discussion.
Meanwhile, SMACv2 (https://github.com/oxwhirl/smacv2) released recently. I wonder if it makes sense to support both v1 and v2 or go with v2 from the start. From integration perspective v2 looks the same, except state and observation specs and CI (different package and maps), and it is not backwards compatible with old SMAC maps (at least for now). @vmoens what do you think?

@matteobettini matteobettini mentioned this pull request Aug 18, 2023
Merged
matteobettini
matteobettini reviewed Aug 21, 2023
View reviewed changes
.circleci/unittest/linux_libs/scripts_smac/setup_env.sh
printf "* Installing StarCraft 2 and SMAC maps into '${root_dir}/smac/StarCraftII'\n"
mkdir $root_dir/smac
cd $root_dir/smac/
# TODO: discuss how we can cache it to avoid downloading ~4 GB on each run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to figure out how to cache

@vmoens vmoens closed this in #1466 Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matteobettini matteobettini matteobettini left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Environments Adds or modifies an environment wrapper
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ordinskiy @vmoens @matteobettini @PaLeroy @facebook-github-bot