Maskable Recurrent PPO

[akbaig]

A simple merge of maskable PPO and recurrent PPO for the latest version of sb3-contrib

Description

Closes #101

This may be one of the most controversial PR here. The reason for this PR are the following:

1. The feature has been requested multiple times by different people since 2022.
2. Many people have attempted to create forks in the last years, however they fail to maintain it.
3. This code utilises existing lines of code from other implementations (maskable and recurrent) - acting as a pure merger.
4. Since maintainability is a concern raised by @araffin, I've attempted to strike a balance between inheriting functionality from existing classes and extending functionality by code duplication.

Instructions

Import using from sb3_contrib import MaskableRecurrentPPO.
Rest of the usage details are the same e.g., lstm_states in predict and action_masks method inside gym env.

Notes

1. Please let me know if we have to add separate section in documentation for it with an example, because I'm unsure of it.
2. Please let me know if we should merge test_invalid_actions and test_lstm to create a test for this variant or not.

P.S: I wrote this for v1.7.0 last year. However, recently someone tagged me on the issue again. Therefore, it felt the right time to invest some time to re-do it.

Context

• I have raised an issue to propose this change (required)

Types of changes

• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• 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)
• The functionality/performance matches that of the source (required for new training algorithms or training-related features).
• I have updated the tests accordingly (required for a bug fix or a new feature).
• I have included an example of using the feature (required for new features).
• I have included baseline results (required for new training algorithms or training-related features).
• I have updated the documentation accordingly.
• I have updated the changelog accordingly (required).
• 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

[araffin]

Hello,

Many people have attempted to create forks in the last years, however they fail to maintain it.
Since maintainability is a concern raised by @araffin,

Just to be clear, because people want the feature but don't have time to maintain it, you are suggesting to give me the additional maintenance load? (In addition to SB3, RL Zoo, algorithms already in SB3-Contrib and SBX)

[akbaig]

Hello,

Many people have attempted to create forks in the last years, however they fail to maintain it.
Since maintainability is a concern raised by @araffin,

Just to be clear, because people want the feature but don't have time to maintain it, you are suggesting to give me the additional maintenance load? (In addition to SB3, RL Zoo, algorithms already in SB3-Contrib and SBX)

Hello,

Thanks for getting back on this.

While it may seem like a maintenance load, it's actually not.

It's because I didn't write any new line of code here. I was just a facilitator to take common code of both implementation and create a merger while keeping code reusability at maximum.

And I personally don't think it'd be much of a hassle to modify changes in this algorithm in case implementation of RPPO or MPPO gets modified.

Regards

[araffin]

While it may seem like a maintenance load, it's actually not.

It's not a maintenance load, but somehow no one, not even you, wants to maintain it in the medium/long term?

It's because I didn't write any new line of code here. I was just a facilitator to take common code of both implementation and create a merger while keeping code reusability at maximum.

I agree it's mostly a copy-pasting of the two implementations, but that's also why I would prefer a separate repo (and a link to that repo in our doc), instead of adding 1400 lines of duplicated code.

[akbaig]

While it may seem like a maintenance load, it's actually not.

It's not a maintenance load, but somehow no one, not even you, wants to maintain it in the medium/long term?

Yeah because it's nearly impossible for me to keep up with the rest of the changes happening in the parent repository, that's why we try to make PR in the first place?

It's because I didn't write any new line of code here. I was just a facilitator to take common code of both implementation and create a merger while keeping code reusability at maximum.

I agree it's mostly a copy-pasting of the two implementations, but that's also why I would prefer a separate repo (and a link to that repo in our doc), instead of adding 1400 lines of duplicated code.

Then I think the most rational option here would be to have an implementation of current RecurrentPPO which could just accept a parameter to enable masking? What do you think about this?