Fix minigrid policy type and add new envs

qgallouedec commented 1 year ago

Description

Motivation and Context

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

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Documentation (update in the documentation)

Checklist:

[ ] I've read the CONTRIBUTION guide (required)
[ ] I have updated the changelog accordingly (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.
[ ] I have reformatted the code using make format (required)
[ ] I have checked the codestyle using make check-codestyle and make lint (required)
[ ] I have ensured make pytest and make type both pass. (required)

Note: we are using a maximum length of 127 characters per line

qgallouedec commented 1 year ago

@araffin why does minigrid need gym-packages arg? If there is no specific reason, I would use import_envs.py, like for other libraries.

araffin commented 1 year ago

@araffin why does minigrid need gym-packages arg? If there is no specific reason, I would use import_envs.py, like for other libraries.

mainly for legacy reasons and to demonstrate how to use --gym-packages.

qgallouedec commented 1 year ago

Would you agree to remove it?

araffin commented 1 year ago

Would you agree to remove it?

Remove the argument completely, no. It is actually useful from time to time (even though the feature is now mostly covered by python config now), and we should probably rename it to packages.

Change the way minigrid is used? yes, but then we should probably add a test for the gym-packages to avoid regressions.

qgallouedec commented 1 year ago

I mean, changing the way that minigrid is used, sorry I agree with your remark about testing ˋgym-packages`

qgallouedec commented 1 year ago

https://wandb.ai/qgallouedec/sb3-minigrid

qgallouedec commented 1 year ago

I just wait to have one run per env (and maybe adjust n_timesteps) before merging

qgallouedec commented 1 year ago

I think that we're good now

qgallouedec commented 1 year ago

Note that I did not run the benchmark script. It fails during the evaluation Pendulum/PPO, but I did not investigate further.

araffin commented 1 year ago

fyi, I removed the need for installing test env package and custom mypy arg in https://github.com/DLR-RM/rl-baselines3-zoo/pull/357/commits/666abb320ef311c99726b8448cf03b08c7c2dbc1

qgallouedec commented 1 year ago

It's much better like this

DLR-RM / rl-baselines3-zoo