Adgudime/async sac

[AdityaGudimella]

Why are these changes needed?

Fixes Bug in execution plan where setting prioritized_replay in config to False does not actually turn it off.

Related issue number

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/latest/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested (please justify below)

[RuofanKong]

Just have one question. otherwise. looks good to me! Thank you @AdityaGudimella!

[RuofanKong]

LGTM!

[Edilmo]

I just took a quick look, so maybe I missed important details, but my thought is the following:

• Is there a simplest way to have this flag available without duplicating all these code? (I think it's possible).
• Why do you want to turn off the prioritized replay in first place?
• Why is this reducing the sensitivity to episode horizon?

[Edilmo]

What is the difference here with the LocalReplayBuffer beside not including the weights and the indices in the Batch?

[AdityaGudimella]

The three changes are

1. PrioritizedReplayBuffer vs VanillaReplayBuffer
2. SampleBatch containing different elements
3. Stats not containing update_priorities time.

[Edilmo]

Why are we excluding the weights and the indices?

[Edilmo]

What you want is to avoid the update_priorities method to perform its job, right?

[AdityaGudimella]

Yes. update_priorities should not do anything, but also, weights and batch_idxs are not returned by the vanilla replay buffer. These are only returned by the PrioritizedReplayBuffer.

[RuofanKong]

I just took a quick look, so maybe I missed important details, but my thought is the following:

• Is there a simplest way to have this flag available without duplicating all these code? (I think it's possible).
• Why do you want to turn off the prioritized replay in first place?
• Why is this reducing the sensitivity to episode horizon?

@Edilmo First, Tuning "no-done-at-end" shouldn't be the right way to go. Two major reasons:
a) Given fixed number of workers & simulators, Apex-SAC on our Ray-forked Rllib without tuning this hyper-param converges very well without big variance.
b) It's not a generic hyper-param that works for generic case. Even if it works by tuning it, it only works for infinite horizon rl problem but all finite horizon rl problem will never converge because it breaks the MDP sequence.

Second, back to question Why turn-off prioritized replay buffer and why reducing the sensitivity to episode horizon?
It's not episode horizon sensitivity at all, although the symptom could be related. Briefly, the objective of the current ApeX algorithms (regardless of ApeX-SAC) are all optimizing the objective of maximizing the expected reward. When it comes to optimize both Critic and Actor models (neural networks) with serving samples efficiently. It does the weighted importance sampling with priority TD errors with the k-maximum TD error rank as the priority, on where it efficiently pushes GD direction to. Energy-based algorithm is different, and the core objective is on Policy entropy maximization besides expected rewards, and Policy entropy maximization is important on the exploration. In terms of the ApeX framework on Energy based algorithm, if the important sampling still keeps using the priority on TD-Error, it would lose significant amount of high policy entropy samples which varies much, which's directly related to what samples it gathers so it's sensitive to the scheduling way on sampling, and is definitely exaggerated when number of sampling workers and simulators varies especially the non-fixed number of simulators on Bonsai MT platform wrt auto-scaling, because it changes the sample distribution sampling same amount of data. Although eventually it converges, that general where high variance on convergence introduces. However, this also pushes us to think about the appropriate prioritized replay buffer from the theoretic perspective on the energy based algorithm (Soft Q Learning, SAC, etc.).

This high-level details may not give you full understanding, if you don't have deep understanding on the energy based RL algorithm, prioritized replay buffer, etc. So I listed all recommended papers to help deeply understand:

• Prioritized Experience Replay: https://arxiv.org/pdf/1511.05952.pdf
• Distributed Prioritized Experience Replay: https://arxiv.org/pdf/1803.00933.pdf
• Energy based Deep Reinforcement Learning: https://arxiv.org/pdf/1702.08165.pdf
• Soft Actor Critic v1 (Original Format: Critic Q and V and Policy models with fixed manually tunable temperature parameter): https://arxiv.org/pdf/1801.01290.pdf
• Soft Actor Critic v2 (Modified Format: Only Critic Q and Policy models with trainable temperature parameter): https://arxiv.org/pdf/1812.05905.pdf

[Edilmo]

LGTM

Great Finding guys!!!

[Edilmo]

I just took a quick look, so maybe I missed important details, but my thought is the following:

• Is there a simplest way to have this flag available without duplicating all these code? (I think it's possible).
• Why do you want to turn off the prioritized replay in first place?
• Why is this reducing the sensitivity to episode horizon?

@Edilmo First, Tuning "no-done-at-end" shouldn't be the right way to go. Two major reasons:
a) Given fixed number of workers & simulators, Apex-SAC on our Ray-forked Rllib without tuning this hyper-param converges very well without big variance.
b) It's not a generic hyper-param that works for generic case. Even if it works by tuning it, it only works for infinite horizon rl problem but all finite horizon rl problem will never converge because it breaks the MDP sequence.

Second, back to question Why turn-off prioritized replay buffer and why reducing the sensitivity to episode horizon?
It's not episode horizon sensitivity at all, although the symptom could be related. Briefly, the objective of the current ApeX algorithms (regardless of ApeX-SAC) are all optimizing the objective of maximizing the expected reward. When it comes to optimize both Critic and Actor models (neural networks) with serving samples efficiently. It does the weighted importance sampling with priority TD errors with the k-maximum TD error rank as the priority, on where it efficiently pushes GD direction to. Energy-based algorithm is different, and the core objective is on Policy entropy maximization besides expected rewards, and Policy entropy maximization is important on the exploration. In terms of the ApeX framework on Energy based algorithm, if the important sampling still keeps using the priority on TD-Error, it would lose significant amount of high policy entropy samples which varies much, which's directly related to what samples it gathers so it's sensitive to the scheduling way on sampling, and is definitely exaggerated when number of sampling workers and simulators varies especially the non-fixed number of simulators on Bonsai MT platform wrt auto-scaling, because it changes the sample distribution sampling same amount of data. Although eventually it converges, that general where high variance on convergence introduces. However, this also pushes us to think about the appropriate prioritized replay buffer from the theoretic perspective on the energy based algorithm (Soft Q Learning, SAC, etc.).

This high-level details may not give you full understanding, if you don't have deep understanding on the energy based RL algorithm, prioritized replay buffer, etc. So I listed all recommended papers to help deeply understand:

• Prioritized Experience Replay: https://arxiv.org/pdf/1511.05952.pdf
• Distributed Prioritized Experience Replay: https://arxiv.org/pdf/1803.00933.pdf
• Energy based Deep Reinforcement Learning: https://arxiv.org/pdf/1702.08165.pdf
• Soft Actor Critic v1 (Original Format: Critic Q and V and Policy models with fixed manually tunable temperature parameter): https://arxiv.org/pdf/1801.01290.pdf
• Soft Actor Critic v2 (Modified Format: Only Critic Q and Policy models with trainable temperature parameter): https://arxiv.org/pdf/1812.05905.pdf

I think you totally misunderstood my comments. They are just about avoid code duplication.