Repository: sail-sg/envpool (language: python)

Issue:
[BUG] Breakout-v5 Performance Regression
## Describe the bug

PPO can no longer reproduce 400 game scores in the `Breakout-v5` given 10M steps of training (same hyperparameters) as it can in `BreakoutNoFrameskip-v4`. 

![image](https://user-images.githubusercontent.com/5555347/149157679-104ef5b0-a62d-49b2-b2f2-500f294754f5.png)


## To Reproduce

Run the https://wandb.ai/costa-huang/cleanRL/runs/26k4q5jo/code?workspace=user-costa-huang to reproduce envpool's results and https://wandb.ai/costa-huang/cleanRL/runs/1ngqmz96/code?workspace=user-costa-huang to reproduce `BreakoutNoFrameskip-v4` results.

## Expected behavior

PPO should obtain 400 game scores in the `Breakout-v5` given 10M steps of training



## System info

Describe the characteristic of your environment:
 * Describe how the library was installed (pip, source, ...)
 * Python version
 * Versions of any other relevant libraries

```python
import envpool, numpy, sys
print(envpool.__version__, numpy.__version__, sys.version, sys.platform)

>>> import envpool, numpy, sys
__, numpy.__version__, sys.version, sys.platform)
>>> print(envpool.__version__, numpy.__version__, sys.version, sys.platform)
0.4.3 1.21.5 3.9.5 (default, Jul 19 2021, 13:27:26) 
[GCC 10.3.0] linux
```

## Reason and Possible fixes

I ran the gym's `ALE/Breakout-v5` as well and got a regression as well as shown below, but looking into it was because `ALE/Breakout-v5`  by default uses the full action space (14 discrete actions), whereas the `Breakout-v5` has the minimal 4 discrete actions. So I have no idea why the regression happens with envpool...

![image](https://user-images.githubusercontent.com/5555347/149158929-282ef0d1-2334-401d-9f89-6576934ca1c3.png)


## Checklist

- [ x ] I have checked that there is no similar issue in the repo (**required**)
- [ x ] I have read the [documentation](https://envpool.readthedocs.io/) (**required**)
- [ x ] I have provided a minimal working example to reproduce the bug (**required**)



Discussion / hints:
Got some feedback from @JesseFarebro regarding envpool's `Breakout-v5` vs `ALE/Breakout-v5`:

> - They `envpool` aren't doing terminal signal on loss of life which in Breakout is the single most important setting. (we don't recommend it be used)
> - They are doing NOOPs without sticky actions and you're doing both (no one should be using NOOPs anymore).
> - They don't do reward clipping by default? Check the magnitude of the rewards if you aren't clipping yourself (this is fine)

As shown in the script, I was already doing

 ```
     envs = envpool.make(
        args.gym_id,
        env_type="gym",
        num_envs=args.num_envs,
        episodic_life=True,
        reward_clip=True,
    )
```

Maybe the reason is "They are doing NOOPs without sticky actions and you're doing both (no one should be using NOOPs anymore)."?
https://envpool.readthedocs.io/en/latest/api/atari.html

> (no one should be using NOOPs anymore)."?

set noop_max=0
Hmm but the `BreakoutNoFrameskip-v4` result clearly used the `noop_max`.

```
def make_env(gym_id, seed, idx, capture_video, run_name):
    def thunk():
        env = gym.make(gym_id)
        env = gym.wrappers.RecordEpisodeStatistics(env)
        if capture_video:
            if idx == 0:
                env = gym.wrappers.RecordVideo(env, f"videos/{run_name}")
        env = NoopResetEnv(env, noop_max=30)
        env = MaxAndSkipEnv(env, skip=4)
        env = EpisodicLifeEnv(env)
        if "FIRE" in env.unwrapped.get_action_meanings():
            env = FireResetEnv(env)
        env = ClipRewardEnv(env)
        env = gym.wrappers.ResizeObservation(env, (84, 84))
        env = gym.wrappers.GrayScaleObservation(env)
        env = gym.wrappers.FrameStack(env, 4)
        env.seed(seed)
        env.action_space.seed(seed)
        env.observation_space.seed(seed)
        return env

    return thunk
```
Per conversation with @Trinkle23897, we think the problem is reward counting. In my implementation, I counted the rewards from all five lives, but since I was using `reward_clip=True` in `envpool.make`, I was counting the clipped rewards from 5 episodes. Maybe the best solution is to include a `game_score` key in the `info` variable. 

```python
class RecordEpisodeStatistics(gym.Wrapper):
    def __init__(self, env, deque_size=100):
        super(RecordEpisodeStatistics, self).__init__(env)
        self.num_envs = getattr(env, "num_envs", 1)
        self.episode_returns = None
        self.episode_lengths = None
        # get if the env has lives
        self.has_lives = False
        env.reset()
        info = env.step(np.zeros(self.num_envs, dtype=int))[-1]
        if info["lives"].sum() > 0:
            self.has_lives = True
            print("env has lives")


    def reset(self, **kwargs):
        observations = super(RecordEpisodeStatistics, self).reset(**kwargs)
        self.episode_returns = np.zeros(self.num_envs, dtype=np.float32)
        self.episode_lengths = np.zeros(self.num_envs, dtype=np.int32)
        self.lives = np.zeros(self.num_envs, dtype=np.int32)
        self.returned_episode_returns = np.zeros(self.num_envs, dtype=np.float32)
        self.returned_episode_lengths = np.zeros(self.num_envs, dtype=np.int32)
        return observations

    def step(self, action):
        observations, rewards, dones, infos = super(RecordEpisodeStatistics, self).step(
            action
        )
        self.episode_returns += rewards
        self.episode_lengths += 1
        self.returned_episode_returns[:] = self.episode_returns
        self.returned_episode_lengths[:] = self.episode_lengths
        all_lives_exhausted = infos["lives"] == 0
        if self.has_lives:
            self.episode_returns *= (1 - all_lives_exhausted)
            self.episode_lengths *= (1 - all_lives_exhausted)
        else:
            self.episode_returns *= (1 - dones)
            self.episode_lengths *= (1 - dones)
        infos["r"] = self.returned_episode_returns
        infos["l"] = self.returned_episode_lengths
        return (
            observations,
            rewards,
            dones,
            infos,
        )
```

Fix patch:
diff --git a/envpool/atari/atari_env.h b/envpool/atari/atari_env.h
index 25a993c3..fec030d5 100644
--- a/envpool/atari/atari_env.h
+++ b/envpool/atari/atari_env.h
@@ -65,7 +65,8 @@ class AtariEnvFns {
                          conf["img_height"_], conf["img_width"_]},
                         {0, 255})),
                     "discount"_.bind(Spec<float>({-1}, {0.0f, 1.0f})),
-                    "info:lives"_.bind(Spec<int>({-1}, {0, 5})));
+                    "info:lives"_.bind(Spec<int>({-1}, {0, 5})),
+                    "info:reward"_.bind(Spec<float>({-1})));
   }
   template <typename Config>
   static decltype(auto) ActionSpec(const Config& conf) {
@@ -173,11 +174,12 @@ class AtariEnv : public Env<AtariEnvSpec> {
     state["discount"_] = 1.0f;
     state["reward"_] = 0.0f;
     state["info:lives"_] = lives_ = env_->lives();
+    state["info:reward"_] = 0.0f;
     WriteObs(state);
   }
 
   void Step(const Action& action) override {
-    float reward = 0;
+    float reward = 0.0f;
     done_ = false;
     int act = action["action"_];
     int skip_id = frame_skip_;
@@ -199,13 +201,6 @@ class AtariEnv : public Env<AtariEnvSpec> {
     // push the maxpool outcome to the stack_buf
     PushStack(false, skip_id == 0);
     ++elapsed_step_;
-    if (reward_clip_) {
-      if (reward > 0) {
-        reward = 1;
-      } else if (reward < 0) {
-        reward = -1;
-      }
-    }
     done_ |= (elapsed_step_ >= max_episode_steps_);
     if (episodic_life_ && env_->lives() < lives_) {
       done_ = true;
@@ -216,6 +211,14 @@ class AtariEnv : public Env<AtariEnvSpec> {
     } else {
       state["discount"_] = 1.0f - done_;
     }
+    state["info:reward"_] = reward;
+    if (reward_clip_) {
+      if (reward > 0) {
+        reward = 1;
+      } else if (reward < 0) {
+        reward = -1;
+      }
+    }
     state["reward"_] = reward;
     state["info:lives"_] = lives_ = env_->lives();
     WriteObs(state);
diff --git a/third_party/pip_requirements/requirements.txt b/third_party/pip_requirements/requirements.txt
index 5955f5a5..635c2c09 100644
--- a/third_party/pip_requirements/requirements.txt
+++ b/third_party/pip_requirements/requirements.txt
@@ -9,7 +9,7 @@ jinja2
 numpy
 opencv-python-headless
 scipy
-setuptools
+setuptools<=59.5.0
 six
 tensorboard
 tianshou


Test patch:
diff --git a/envpool/atari/api_test.py b/envpool/atari/api_test.py
index 4b2a9f3f..46cf5bff 100644
--- a/envpool/atari/api_test.py
+++ b/envpool/atari/api_test.py
@@ -258,7 +258,7 @@ def test_lowlevel_step(self) -> None:
     np.testing.assert_allclose(done.shape, (num_envs,))
     self.assertEqual(done.dtype, np.bool_)
     self.assertIsInstance(info, dict)
-    self.assertEqual(len(info), 4)
+    self.assertEqual(len(info), 5)
     self.assertEqual(info["env_id"].dtype, np.int32)
     self.assertEqual(info["lives"].dtype, np.int32)
     self.assertEqual(info["players"]["env_id"].dtype, np.int32)
@@ -294,7 +294,7 @@ def test_highlevel_step(self) -> None:
     np.testing.assert_allclose(done.shape, (num_envs,))
     self.assertEqual(done.dtype, np.bool_)
     self.assertIsInstance(info, dict)
-    self.assertEqual(len(info), 4)
+    self.assertEqual(len(info), 5)
     self.assertEqual(info["env_id"].dtype, np.int32)
     self.assertEqual(info["lives"].dtype, np.int32)
     self.assertEqual(info["players"]["env_id"].dtype, np.int32)