[envpool] modernize benchmark stack and switch procgen upstream - #337
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80d5636e7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - if (options.use_sequential_levels && step_data.level_complete) { | ||
| - step_data.done = false; | ||
| - } | ||
| + // if (options.use_sequential_levels && step_data.level_complete) { | ||
| + // step_data.done = false; |
There was a problem hiding this comment.
The new local patch comments out upstream's if (options.use_sequential_levels && step_data.level_complete) step_data.done = false;. In EnvPool, any env that reports done is force-reset on the next action (envpool/core/async_envpool.h:126), so with use_sequential_levels=True a completed level now causes a full reset instead of advancing to the next sequential level. That turns an exposed Procgen option into a guaranteed misbehavior for every task that enables it.
Useful? React with 👍 / 👎.
| env.close() | ||
| print(f"FPS = {frame_skip * total_step * num_envs / (time.time() - t):.2f}") |
There was a problem hiding this comment.
The timer is still running when env.close() is called, so the printed FPS now includes environment teardown instead of only stepping time. This especially skews the small smoke runs from the test plan (--total-step 20) and any short benchmark, and it penalizes AsyncVectorEnv most because worker shutdown is non-trivial. Measuring before close() keeps the result comparable to the previous benchmark numbers.
Useful? React with 👍 / 👎.
| env = make_vector_env(num_envs, async_, lambda: gym.make(task_id)) | ||
| elif env == "box2d": | ||
| task_id = "LunarLander-v2" | ||
| task_id = "LunarLander-v3" |
There was a problem hiding this comment.
For --env box2d, this script now runs LunarLander-v3, but benchmark/test_envpool.py:81 and envpool/box2d/registration.py:51 still use LunarLander-v2. That means the modernized benchmark stack no longer compares Gym and EnvPool on the same Box2D environment, so any throughput numbers collected for Box2D are not apples-to-apples.
Useful? React with 👍 / 👎.
1 participant
Why
The benchmark path was still pinned to an older Gym/Sample Factory era setup, and Procgen was still sourced from a fork that carried a small set of EnvPool-specific changes. This PR modernizes the benchmark path without touching the larger Bazel/toolchain upgrade track, and moves Procgen back to official upstream with a bounded local patch.
Summary
make bazel-buildworking on Linuxbenchmark/test_envpool.pychoose a safe defaultbatch_sizefor small smoke runs while still rejecting invalid explicit configsTrinkle23897/procgento officialopenai/procgen0.10.7with a small local patch for Qt5 includes and EnvPool behaviorTest plan
dev-0:make flake8dev-0:make py-formatdev-0:make docstyledev-0:make clang-tidydev-0verify workspace:make bazel-testdev-0verify workspace:make bazel-builddev-0fresh venv:python benchmark/test_gym.py --env atari --num-envs 1 --total-step 20dev-0fresh venv:python benchmark/test_gym.py --env mujoco --num-envs 1 --total-step 20dev-0fresh venv:python benchmark/test_gym.py --env box2d --num-envs 1 --total-step 20dev-0fresh venv:python benchmark/test_envpool.py --env atari --num-envs 1 --total-step 20dev-0fresh venv:python benchmark/test_envpool.py --env mujoco --num-envs 1 --total-step 20dev-0fresh venv:python benchmark/test_envpool.py --env box2d --num-envs 1 --total-step 20