[envpool] fix multiplayer players.env_id inference - #347
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: 2dad8c18e9
ℹ️ 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 "info:players.env_id" in state: | ||
| self._last_players_env_id = np.array( | ||
| state["info:players.env_id"], copy=True |
There was a problem hiding this comment.
In async usage, send() can target any env_id subset, not just the batch returned by the most recent recv(). This code overwrites a single _last_players_env_id array on every recv(), so if a caller aggregates multiple recv() batches before sending actions, the older envs lose their cached player mapping and _cached_players_env_id() falls back to uniform inference or raises. For variable-player envs that means Env::ParseAction() in envpool/core/env.h slices the flattened player arrays against the wrong environments.
Useful? React with 👍 / 👎.
| if env_id.shape[0] == 0 or player_count % env_id.shape[0] != 0: | ||
| raise RuntimeError( | ||
| "Cannot infer players.env_id for multiplayer action; " | ||
| "pass a dict action with explicit players.env_id." |
There was a problem hiding this comment.
This only treats non-divisible totals as ambiguous. When the cache is unavailable, a batch like env_id=[0,1] with 4 player actions is still ambiguous for variable-player envs (1+3, 2+2, and 3+1 are all possible; the dummy multiplayer env can produce 1+3 because each env’s player count advances independently). The code then fabricates a uniform players.env_id split, and Env::ParseAction() groups some players’ actions under the wrong env instead of surfacing the ambiguity.
Useful? React with 👍 / 👎.
1 participant
Summary
players.env_idwith the batchenv_id, so flattened per-player actions were grouped against the wrong environments when an env contributed more than one player action.players.env_idfrom the incoming Python action payload, reuse the latest observed player-to-env mapping when player counts vary by env, and add Python-side regression coverage for the multiplayer wrapper path.This fixes the Python wrapper bug behind issue #296 without changing the C++ action parser contract.
Technical Details
EnvPoolMixin._from()to deriveplayers.env_idfrom player-shaped action arrays, fall back to the cachedinfo:players.env_idmapping from the lastrecv(), and raise when the mapping is ambiguous instead of fabricating a wrong one.envpool/python/envpool.py: addsplayers.env_idinference and cachesinfo:players.env_idonrecv()for variable-player batches.envpool/dummy/dummy_py_envpool_test.py: adds regression coverage around the real dummy DM wrapper for uniform multiplayer, cached variable-player, explicit mapping, and ambiguous-input cases.envpool/dummy/BUILD: wires the dummy Python test to the Python API wrapper target.Test Plan
Automated
python3 -m py_compile envpool/python/envpool.py envpool/dummy/dummy_py_envpool_test.py: passed.Suggested Manual
USE_BAZEL_VERSION=8.6.0 bazelisk test //envpool/dummy:dummy_py_envpool_test --config=test --spawn_strategy=local --test_output=errors: exercise the real dummy wrapper path on Linux.dev-0: confirm the flattened multiplayer action now expands to the expectedplayers.env_idsequence instead of the old one-element-per-env mapping.