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

[Environment, Docs] SMACv2 and docs on action masking #1466

Merged
merged 42 commits into from
Sep 15, 2023

Conversation

matteobettini
Copy link
Contributor

@matteobettini matteobettini commented Aug 18, 2023

Depends on #1421
To be closed: #56
To be closed: #810

SMAC integration was started in #810, this PR aims to integrate SMACv2

Smacv2 is the second version of the popular SMAC enviornment challenge, popular as one of the most used multi-agent benchmarks.

This PR is focused on integrating SMACv2 in torchRL

For more info you can see https://github.com/oxwhirl/smacv2

This PR also introduces the documentation on how to update the action mask for environments with changing available actions.

@ordinskiy

@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 Aug 18, 2023
Signed-off-by: Matteo Bettini <[email protected]>
@matteobettini matteobettini reopened this Aug 18, 2023
Signed-off-by: Matteo Bettini <[email protected]>
@matteobettini matteobettini mentioned this pull request Aug 21, 2023
6 tasks
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]>
# SC2PATH is set in run_test.sh
printf "* Installing StarCraft 2 and SMACv2 maps into '${root_dir}/smacv2/StarCraftII'\n"
cd "${root_dir}"
# TODO: discuss how we can cache it to avoid downloading ~4 GB on each run.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To discuss

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah 4gb is big
IDK how to cache things with our GHA CI though.
@osalpekar How can we cache big files with our GHA worflows?

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 matteobettini marked this pull request as ready for review August 21, 2023 15:51
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/envs/transforms/transforms.py
#	torchrl/modules/distributions/discrete.py
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
torchrl/envs/libs/smacv2.py Outdated Show resolved Hide resolved
torchrl/envs/libs/smacv2.py Show resolved Hide resolved
@matteobettini matteobettini changed the title [Environment] SMACv2 [Environment, Docs] SMACv2 and docs on action masking Sep 5, 2023
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
# Conflicts:
#	test/test_libs.py
@matteobettini matteobettini added the Environments Adds or modifies an environment wrapper label Sep 14, 2023
# Conflicts:
#	.github/unittest/linux_libs/scripts_smacv2/environment.yml
#	.github/unittest/linux_libs/scripts_smacv2/install.sh
#	.github/unittest/linux_libs/scripts_smacv2/post_process.sh
#	.github/unittest/linux_libs/scripts_smacv2/run-clang-format.py
#	.github/unittest/linux_libs/scripts_smacv2/run_test.sh
#	.github/unittest/linux_libs/scripts_smacv2/setup_env.sh
#	docs/source/reference/envs.rst
Signed-off-by: Matteo Bettini <[email protected]>
Signed-off-by: Matteo Bettini <[email protected]>
@matteobettini matteobettini added the documentation Improvements or additions to documentation label Sep 14, 2023
@matteobettini matteobettini marked this pull request as draft September 14, 2023 15:58
@matteobettini matteobettini marked this pull request as ready for review September 14, 2023 18:47
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.

Almost there, great work!


jobs:
unittests:
if: ${{ github.event.label.name == 'Environments' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be in

on:
  pull_request:
  push:
    branches:
      - nightly
      - main
      - release/*
  workflow_dispatch:

?

Copy link
Contributor Author

@matteobettini matteobettini Sep 14, 2023

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing i wonder is if this will be run on main

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will
To me, the on block will go first and check that it's on main. Then the job unittests will be run but the if statement will always be false.
I think this condition goes under on: pull_request

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 don't think conditions can be put there

torchrl/envs/libs/smacv2.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]>
Signed-off-by: Matteo Bettini <[email protected]>
@@ -17,7 +17,7 @@ concurrency:

jobs:
unittests:
if: contains(github.event.pull_request.labels.*.name, 'Environments')
if: ${{ (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'Environments')) || (github.event_name == 'push') }}
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 run every time you push you can avoid checking the label name too no?

Copy link
Contributor Author

@matteobettini matteobettini Sep 15, 2023

Choose a reason for hiding this comment

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

i am still working on it, i thought the push event is onlly triggered on the listed branches, therefore the label is checked for prs

@matteobettini
Copy link
Contributor Author

Ok I think I have reached a current working solution.

My hypothesis is that now the tests will be run either on PR events with the proper label or on pushes to these branches
- nightly
- main
- release/*

I have tested the labelling PR filtering, but ofc the fact that this will run in main is not tested

@matteobettini
Copy link
Contributor Author

another alternative i see is creating 2 workflow files for smac

  • one that runs on pushes to core branches without conditions
  • one that runs on PRs with label confition

@vmoens vmoens merged commit 5f3741b into pytorch:main Sep 15, 2023
48 of 59 checks passed
@matteobettini matteobettini deleted the smacv2 branch September 15, 2023 08:52
albertbou92 pushed a commit to PyTorchRL/rl that referenced this pull request Sep 18, 2023
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. documentation Improvements or additions to documentation enhancement New feature or request Environments Adds or modifies an environment wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants