Repository navigation
Conversation
There was a problem hiding this comment.
Thanks for the quick initial draft! I've marked out some initial places where changes are needed. PettingZoo support will not be part of v1.12 feature due to amount of work still required & changes required for upstream PettingZoo package. So take your time, no rush. 😉
Update:
pettingzoo>=1.27 has a Gymnasium-like registry
| id="intersection-multi-agent-v3", | ||
| entry_point="highway_env.envs.intersection_pz_env:IntersectionParallelEnv", | ||
| ) | ||
| if "intersection-multi-agent-v4" not in registry: |
There was a problem hiding this comment.
Like the non-PettingZoo registration function above. It's cleaner to just check for whether "intersection-multi-agent-v3" is in registry and:
- return if yes
- register all pettingzoo envs
Note that this is dependent on the comment above, we shouldn't register PettingZoo environments into Gymnasium's registry.
There was a problem hiding this comment.
I completely removed the _register_pettingzoo_envs() function since we're not registering PZ envs in Gymnasium at all.
| RecordConstructorArgs.__init__(self) | ||
|
|
||
| def step(self, action): | ||
| def step(self, action): # unchanged |
There was a problem hiding this comment.
This comment is pointless and potentially confusing
There was a problem hiding this comment.
removed the # unchanged comment.
| except ImportError: | ||
| return # pettingzoo not installed; skip silently | ||
| if "intersection-multi-agent-v3" not in registry: | ||
| register( |
There was a problem hiding this comment.
We shouldn't use register here as these are PettingZoo environments not Gymnasium ones. I'll talk with PettingZoo maintainers and see if they'd be happy to support a PettingZoo registry & potentially a pz.make.
There was a problem hiding this comment.
removed all gymnasium.register() calls for PZ environments entirely. Users will instantiate via direct import.
| from highway_env.envs.common.pettingzoo_env import HighwayParallelEnv # noqa: F401 | ||
| from highway_env.envs.intersection_pz_env import ( # noqa: F401 | ||
| ConnectedLaneIntersectionParallelEnv, | ||
| IntersectionParallelEnv, | ||
| ) | ||
|
|
||
| __all__ += [ | ||
| "HighwayParallelEnv", | ||
| "IntersectionParallelEnv", | ||
| "ConnectedLaneIntersectionParallelEnv", | ||
| ] |
There was a problem hiding this comment.
This is not an especially good pattern as:
- IDEs, language servers, etc. don't like mutating
__all__dynamically - The
noqacomments are not necessarily respected by all linters & type checkers
Please let me consider it for a bit.
There was a problem hiding this comment.
Removed the dynamic all mutation block entirely. PZ classes will just be accessed directly via their own module
| Parameters | ||
| ---------- | ||
| env: | ||
| A fully-initialised :class:`AbstractEnv` with ``MultiAgentObservation`` | ||
| and ``MultiAgentAction`` configured. |
There was a problem hiding this comment.
This appears to be an incomplete piece of numpy-style docstring (with Returns cutoff), we don't use numpy-style within the repo, please use pep287 style docstring instead (the :param env: ... kind). I see that the docstring above this part seems to be pep287 style as well.
There was a problem hiding this comment.
Converted all docstrings in this file to PEP 287 (:param:, :returns: style) and trimmed the redundant prose
| Returns | ||
| ------- | ||
| observations: | ||
| ``{agent_name: obs_array}`` for all agents in :attr:`possible_agents`. | ||
| infos: | ||
| ``{agent_name: info_dict}`` (shared info dict for now). |
There was a problem hiding this comment.
Similar to a previous comment above, Parameters is missing and please don't use numpy-style
There was a problem hiding this comment.
Converted all of these to PEP 287 format and trimmed the redundant text.
| Returns | ||
| ------- | ||
| observations, rewards, terminations, truncations, infos: | ||
| All keyed by agent name, covering only the agents that were | ||
| *active at the start of this step* (i.e. the pre-step | ||
| :attr:`agents` list). |
| if isinstance(env_spec.entry_point, str) and "highway_env" in env_spec.entry_point | ||
| if isinstance(env_spec.entry_point, str) | ||
| and "highway_env" in env_spec.entry_point | ||
| and "intersection_pz_env" not in env_spec.entry_point |
There was a problem hiding this comment.
PettingZoo environments shouldn't be included in Gymnasium registry as previously mentioned.
There was a problem hiding this comment.
Since PZ envs are no longer in the registry at all, this filter is no longer needed. Reverted it back to upstream
| agents_rewards: tuple = info.get( | ||
| "agents_rewards", (_reward,) * len(self.possible_agents) | ||
| ) | ||
| agents_terminated: tuple = info.get( | ||
| "agents_terminated", (_terminated,) * len(self.possible_agents) | ||
| ) |
There was a problem hiding this comment.
The use of default in this way is not preferable as they are always evaluated eagerly at runtime (also I believe erroring is fine as if the info doesn't contain these keys something is wrong with the environment? Correct me if I'm wrong as I haven't looked *too* closely)
There was a problem hiding this comment.
Switched .get() to direct dictionary access (e.g., info["agents_rewards"]). If the base environment ever forgets to provide those keys, this will fail fast with a KeyError rather than silently swallowing the bug.
| if hasattr(space, "n"): | ||
| return min(1, space.n - 1) | ||
| # Continuous: return zeros | ||
| import numpy as np |
There was a problem hiding this comment.
moved to the top of the file.
|
@kydrahul would you be kind enough to make the changes requested in review and fix merge conflicts? |
ba5f9f7 to
26b3452
Compare
|
@kydrahul would you be so kind as to also rebase off main? |
Description
Adds PettingZoo
ParallelEnvsupport for HighwayEnv's multi-agent intersection environments, implementing true per-agent termination semantics (crashed/arrived agents are removed fromself.agentswhile the rest continue — following PettingZoo's convention, since Gymnasium itself has no "removing agent" mechanism).This is the initial PR as discussed — the goal here is to get PettingZoo integration working end-to-end. A follow-up PR will expand the regression test suite, add documentation, and cover additional QoL items.
Dependencies added:
pettingzoo>=1.24.0, added as an optional[multi-agent]extra so single-agent users see zero change to their install.Fixes #677
Type of change
Summary of Changes
pyproject.toml: Add[multi-agent]optional extra (pettingzoo>=1.24.0)highway_env/envs/common/abstract.py: DeprecateMultiAgentWrapperwith aDeprecationWarningpointing to the new API (old code kept, not removed)highway_env/envs/common/pettingzoo_env.py: New —HighwayParallelEnv, a PettingZooParallelEnvwrapper around anyAbstractEnvconfigured for multi-agent controlhighway_env/envs/intersection_pz_env.py: New —IntersectionParallelEnvandConnectedLaneIntersectionParallelEnv, concrete user-facing subclasseshighway_env/__init__.py: Registerintersection-multi-agent-v3/v4(only whenpettingzoois installed; skipped silently otherwise)highway_env/envs/__init__.py: Export the new PZ classes at package leveltests/envs/test_gym.py: Exclude PZParallelEnvIDs from the existinggym.make()API test, since these are PettingZooParallelEnvinstances, notgymnasium.Envsubclasses, andgym.make()correctly rejects themtests/envs/test_pettingzoo.py: New — test suite covering PettingZoo API compliance (parallel_api_test), per-agent termination semantics, seed reproducibility, and the deprecation warningTesting
parallel_api_testpasses for bothIntersectionParallelEnvandConnectedLaneIntersectionParallelEnvself.agents)MultiAgentWrapperdeprecation warning verifiedpettingzoois not installedBreaking Changes
None. Existing
intersection-multi-agent-v0/v1/v2IDs continue to work exactly as before.MultiAgentWrapperis deprecated (raises a warning) but not removed.Checklist
pre-commitchecks withpre-commit run --all-files