Add compatibility wrapper for DeepMind Melting Pot #39

elliottower · 2023-03-04T01:35:19Z

Pytest and pre-commit hooks passing.

Also fixed misc.pyrght errors (e.g., step() argument action of type int incompatible with int64 -> action: ActionType (from pettingzoo.utils.env) and made comments a bit more consistent (using the same language to describe the wrappers ,etc).

…ight errors and comment consistency

elliottower · 2023-03-04T01:35:45Z

@jjshoots @pseudo-rnd-thoughts

pseudo-rnd-thoughts · 2023-03-05T12:10:46Z

Could you fix pre-commit and tests

jjshoots

Overall LGTM, aside from some minor comments. @pseudo-rnd-thoughts may want to have a second look as he tends to be more detail oriented and may have preferences over certain aspects.

jjshoots · 2023-03-05T12:11:42Z

tests/test_meltingpot.py

+
+@pytest.mark.parametrize("substrate_name", SUBSTRATES)
+def test_seeding(substrate_name):
+    """Tests the seeding of the openspiel conversion wrapper."""


Typo in this docstring.

jjshoots · 2023-03-05T12:12:06Z

tests/test_meltingpot.py

@@ -0,0 +1,57 @@
+"""Tests the functionality of the MeltingPotCompatibility wrapper on meltingpot substrates."""


The tests look good to me, but perhaps @pseudo-rnd-thoughts may want to have a second look.

jjshoots · 2023-03-05T12:13:01Z

shimmy/utils/meltingpot.py

@@ -0,0 +1,61 @@
+"""Shared utils for meltingpot."""


This looks rather similar to the dm_env utils, do you think the two could be merged, especially the spec_to_space functionality? Or perhaps rename the timestep_to_observations into multiagent_timestep_to_observations to make it more verbose.

Good call I’ll look into that

jjshoots · 2023-03-05T12:16:03Z

shimmy/meltingpot_compatibility.py

+        self.player_roles = substrate.get_config(self.substrate_name).default_player_roles
+        self.max_cycles = max_cycles
+        self.env_config = {"substrate": self.substrate_name, "roles": self.player_roles}
+        EzPickle.__init__(self, self.render_mode, self.env_config, self.max_cycles)


Could the ezpickle be moved up?

Move to be the first line of the function

The EzPickle init line takes a few arguments which need to be extracted from the environment (env config, which contains player_roles) so it can't be done on the first line.

pseudo-rnd-thoughts · 2023-03-05T15:48:40Z

shimmy/meltingpot_compatibility.py

+        self.player_roles = substrate.get_config(self.substrate_name).default_player_roles
+        self.max_cycles = max_cycles
+        self.env_config = {"substrate": self.substrate_name, "roles": self.player_roles}
+        EzPickle.__init__(self, self.render_mode, self.env_config, self.max_cycles)


Move to be the first line of the function

pseudo-rnd-thoughts · 2023-03-05T15:49:47Z

shimmy/meltingpot_compatibility.py

+            self._env.observation_spec()[0]['WORLD.RGB'])  # type: ignore
+
+        # Set agents
+        self._num_players = len(self._env.observation_spec())


Does this include the "WORLD.RGB"? Should it not be len() - 1

This was code from their previous pettingzoo code and the number of players stayed consistent with the observation and such, but I’ll check again to see if it might be an issue.

What it an issue ?

WORLD.RGB is just one entry in each observation, self._env.observation_spec() just has a dict for each agent so is of length(num_players)

pseudo-rnd-thoughts · 2023-03-05T15:50:09Z

shimmy/meltingpot_compatibility.py

+
+        state_space = utils.spec_to_space(
+            self._env.observation_spec()[0]['WORLD.RGB'])
+        return state_space


Why are we not using self.state_space?

Do you mean to make it in the init constructor rather than a separate method? I had it that way originally but saw the action space and observation space in most other examples were separate methods so thought it might be more readable that way. Although those are functions with agent id as input, whereas this is independent of agent. Wasn’t sure if it was necessary at all because a lot of examples don’t have state space included.

It is just confusing as the observation-spec doesn't change and you already define self.state_space which is equivalent to this function. So just use the variable that already exists or remove the variable

pseudo-rnd-thoughts · 2023-03-05T15:50:58Z

shimmy/meltingpot_compatibility.py

+    ) -> Tuple[
+        ObsDict, Dict[str, float], Dict[str, bool], Dict[str, bool], Dict[str, dict]
+    ]:
+        """step.


Update docstring

pseudo-rnd-thoughts · 2023-03-05T15:52:04Z

shimmy/meltingpot_compatibility.py

+        """
+        rgb_arr = self.state()[0]['WORLD.RGB']
+        if self.render_mode == 'human':
+            plt.cla()


@jjshoots Do we care that we are using matplotlib for rendering

True, I think we should avoid import matplotlib if possible. I'm not sure how it's done in meltingpot, but if it's not possible to not use matplotlib, I think that's still fine. We can simply make it a dependency for meltingpot only.

@elliottower is it possible to use PIL instead?

I’m not familiar with PIL but from some googling looks doable. https://stackoverflow.com/questions/57316491/how-to-convert-matplotlib-figure-to-pil-image-object-without-saving-image
Something like this? Or do you mean to avoid matplotlib entirely? Is it because plt is slow? And I’ve on seen pettingzoo games use pygame not PIL, is there a consensus on one or the other?

We generally try to avoid imports of large libraries, especially if we're not using their main functionality. Since we only want to display images, MPL is a bit overkill. That said, PIL is also a bit of a large library, and so is PyGame. I think the consensus for one over the other has thus far just been 'most of the other libraries tend not to use MPL, so let's not use that'. There may be other reasons that I don't know of.

Makes sense, will look into it today.

pseudo-rnd-thoughts · 2023-03-05T16:00:40Z

shimmy/utils/meltingpot.py

+  })
+
+
+def spec_to_space(spec: tree.Structure[dm_env.specs.Array]) -> spaces.Space:


As jet mentioned, remove and replace with the existing dm spec to space

So the existing spec to space is actually less general than this one, and is a bit less clear (imo) so I’ve actually replaced the existing one with this and adapted the functionality to work with both cases. Will commit the changes later today.

tests/test_meltingpot.py

shimmy/meltingpot_compatibility.py

tests/test_meltingpot.py

pseudo-rnd-thoughts · 2023-03-07T15:13:26Z

@jjshoots As meltingpot requires DMLab2D is it possible for test this in the CI?
I forgot if we are able to test with lab or lab2d or neither

…R comments

…for meltingpot env seeding

elliottower · 2023-03-08T01:19:37Z

Addressed these comments (including the minor openspiel changes), cleaned up docstrings and swapped the rendering from plt to pygame. I also added a test for pygame rendering as it's different from how the underlying env does rendering by default, although it may not be necessary.

Problem is now that the seeding seems to be broken and I'm not sure how to go about fixing it. @jjshoots @pseudo-rnd-thoughts

jjshoots · 2023-03-10T15:29:28Z

@elliottower, seeding for meltingpot? It may be the case that meltingpot just has very bad seeding techniques that doesn't actually make it deterministic. This would not be the first time. Our solution to this was to set nondeterministic=True in the gymnasium API check, or disable seeding tests for those that are failing seeding in PZ tests.

elliottower · 2023-03-10T15:33:00Z

@elliottower, seeding for meltingpot? It may be the case that meltingpot just has very bad seeding techniques that doesn't actually make it deterministic. This would not be the first time. Our solution to this was to set nondeterministic=True in the gymnasium API check, or disable seeding tests for those that are failing seeding in PZ tests.

Oh okay that’s good to know. I asked how hard it would be to supply a seed and they said it wouldn’t be easy and recommended something similar to disable seeding.

…o from parallel envs

…for meltingpot import

…on util)

pseudo-rnd-thoughts · 2023-03-14T15:01:39Z

I found this in the CI

----------------------------- Captured stderr call -----------------------------

Downloading CMU mocap data:   0%|          | 0.00/466M [00:00<?, ?it/s]
Downloading CMU mocap data:   1%|          | 4.00M/466M [00:00<00:21, 22.0Mit/s]
Downloading CMU mocap data:   3%|▎         | 16.0M/466M [00:00<00:08, 55.7Mit/s]
Downloading CMU mocap data:   5%|▌         | 24.0M/466M [00:00<00:08, 57.4Mit/s]
Downloading CMU mocap data:   9%|▊         | 40.0M/466M [00:00<00:05, 78.1Mit/s]
Downloading CMU mocap data:  12%|█▏        | 56.1M/466M [00:00<00:04, 95.9Mit/s]
Downloading CMU mocap data:  15%|█▌        | 72.1M/466M [00:00<00:03, 109Mit/s] 
Downloading CMU mocap data:  19%|█▉        | 88.1M/466M [00:01<00:03, 111Mit/s]
Downloading CMU mocap data:  23%|██▎       | 105M/466M [00:01<00:02, 128Mit/s] 
Downloading CMU mocap data:  26%|██▌       | 122M/466M [00:01<00:02, 141Mit/s]
Downloading CMU mocap data:  31%|███       | 143M/466M [00:01<00:02, 163Mit/s]
Downloading CMU mocap data:  34%|███▍      | 160M/466M [00:01<00:02, 156Mit/s]
Downloading CMU mocap data:  38%|███▊      | 175M/466M [00:01<00:02, 144Mit/s]
Downloading CMU mocap data:  41%|████▏     | [192](https://github.com/Farama-Foundation/Shimmy/actions/runs/4355883425/jobs/7613638215#step:4:193)M/466M [00:01<00:02, 129Mit/s]
Downloading CMU mocap data:  46%|████▋     | 216M/466M [00:01<00:01, 153Mit/s]
Downloading CMU mocap data:  50%|████▉     | 231M/466M [00:01<00:01, 155Mit/s]
Downloading CMU mocap data:  53%|█████▎    | 247M/466M [00:02<00:01, 124Mit/s]
Downloading CMU mocap data:  58%|█████▊    | 269M/466M [00:02<00:01, 150Mit/s]
Downloading CMU mocap data:  61%|██████▏   | 285M/466M [00:02<00:01, 127Mit/s]
Downloading CMU mocap data:  64%|██████▍   | 299M/466M [00:02<00:01, 116Mit/s]
Downloading CMU mocap data:  68%|██████▊   | 318M/466M [00:02<00:01, 133Mit/s]
Downloading CMU mocap data:  71%|███████▏  | 332M/466M [00:02<00:01, 107Mit/s]
Downloading CMU mocap data:  75%|███████▌  | 349M/466M [00:03<00:00, 122Mit/s]
Downloading CMU mocap data:  81%|████████  | 376M/466M [00:03<00:00, 158Mit/s]
Downloading CMU mocap data:  84%|████████▍ | 393M/466M [00:03<00:00, 134Mit/s]
Downloading CMU mocap data:  88%|████████▊ | 408M/466M [00:03<00:00, 96.7Mit/s]
Downloading CMU mocap data:  93%|█████████▎| 433M/466M [00:03<00:00, 126Mit/s] 
Downloading CMU mocap data:  96%|█████████▋| 449M/466M [00:03<00:00, 120Mit/s]
Downloading CMU mocap data: 100%|██████████| 466M/466M [00:03<00:00, 124Mit/s]

However, I don't seem to be able to replicate the issue locally.
I know that we don't register the cmu_2020_tracking.cmu_humanoid_tracking as this has mocap tracking

…stency across farama repos

shimmy/utils/dm_env.py

shimmy/registration.py

shimmy/utils/meltingpot.py

tests/test_meltingpot.py

…ring

…res for unrealted envs)

jjshoots

Overall LGTM, some minor questions which I think once solved get my greenlight for merging.

jjshoots · 2023-03-21T20:50:53Z

bin/install_dm_lab.sh

+#!/bin/sh
+set -eu
+
+if [[ "$(uname -s)" == 'Linux' ]]; then


Minor nitpick, could we name /bin to /scripts?

Sounds like a good idea, do you agree @pseudo-rnd-thoughts ?

setup.py

tests/test_openspiel.py

tests/test_melting_pot.py

jjshoots · 2023-03-21T20:58:50Z

tests/test_dm_control.py

+#         "dm_control/compatibility-env-v0", env=wrapped_env, disable_env_checker=True
+#     )
+#     check_env(env.unwrapped, skip_render_check=True)
+#     env.close()


Apologies if I missed out the discussion on this, but is there a reason this entire file is commented out?

Oh this is definitely a mistake from my part, I thought for some reason when I synced with master that this changed but now looking at master that file hasn’t changed for a month. Will clean it up.

shimmy/utils/meltingpot.py

jjshoots · 2023-03-21T21:05:50Z

shimmy/openspiel_compatibility.py

+
+        Print the current game state.
+        """
+        print(self.game_state)


This is kinda weird since some openspiel environments actually have a render function, but I guess c'est la vie.

I wasn’t actually aware of that, the current code just ignores rendering and says it’s not implemented but I could check if the env has a render function and if not just print the state.

pseudo-rnd-thoughts

IMHO, this PR has become several different PRs: adding meltingpot support, documentation updates and possibly more given the number of files edited.
Therefore, I would propose closing this PR and opening new PRs for each of the problems addressed. This should make reviewing and understanding the PRs later if necessary

jjshoots · 2023-03-22T16:02:13Z

@pseudo-rnd-thoughts Actually I think that's a bit counter productive. I mean for us it makes sense, but for Elliot it will be a whole lot of work

elliottower · 2023-03-22T16:34:49Z

@pseudo-rnd-thoughts Actually I think that's a bit counter productive. I mean for us it makes sense, but for Elliot it will be a whole lot of work

It’s all good, it’s best practice so I’m happy to practice it now and spend the extra day splitting things up and organizing. Looks messy to do a single PR commit with all these changes (although committing directly to master I feel like the sequential changes would be fine, it’s just that PRs get condensed into one commit afaik)

pseudo-rnd-thoughts · 2023-03-22T16:40:41Z

@elliottower I would recommend not using main for prs as it can cause issues if you need to revert commits etc

elliottower · 2023-03-22T16:50:27Z

@elliottower I would recommend not using main for prs as it can cause issues if you need to revert commits etc

I meant like if you had made some of these changes directly to main because you have access, without doing a PR, as separate commits, then it would probably be fine. Definitely agree about PRs though there’s a reason separate branches are used. Anyways I’m going to refactor and clean things up today and reopen some PRs here and on pettingzoo.

Added compatibility wrapper for DeepMind Melting Pot, fixed misc. pyr…

babbb09

…ight errors and comment consistency

jjshoots requested changes Mar 5, 2023

View reviewed changes

pseudo-rnd-thoughts requested changes Mar 5, 2023

View reviewed changes

elliottower mentioned this pull request Mar 7, 2023

Update pettingzoo stable-baselines3 training script & compatibility wrapper google-deepmind/meltingpot#117

Closed

elliottower added 3 commits March 7, 2023 16:24

Switched rendering from plt to pygame, added docstrings & addressed P…

e74b0c7

…R comments

Minor linting fix

6e085fe

Update testing to use gymnasium data_equivalence, added initial code …

5e4232f

…for meltingpot env seeding

elliottower and others added 9 commits March 13, 2023 17:12

Finalized meltingpot wrapper (no seeding support), Removed return_inf…

606eead

…o from parallel envs

Merge branch 'main' into main

4470477

Fixed bugs for CI tests, removed accidental changes to dm_control utils

8d68582

Merge branch 'main' of github.com:elliottower/Shimmy

82634c3

Added CI testing for meltingpot using DeepMind Dockerfile, try-catch …

34046c5

…for meltingpot import

Separated dm_control and meltingpot utils (fix CI issue with conversi…

f7553f8

…on util)

Pre-commit hooks

20a6081

Update dockerfile with correct path to shimmy (fixes CI error)

91d3e45

Update dockerfile with correct path to shimmy (fixes CI error)

aef1e44

elliottower added 4 commits March 14, 2023 12:12

Add install of xvfb to meltingpot dockerfile (used in docker_entrypoint)

1227256

Fix linting error (TimeStep is not a known member of module dm_env)

a5a9878

Fix typo in dockerfile and renamed them back to remove .sh, for consi…

6cbcf4c

…stency across farama repos

Revert changes with pre-downloaded AutoROM (caused dependency issues)

d836a37

pseudo-rnd-thoughts requested changes Mar 15, 2023

View reviewed changes

shimmy/utils/dm_env.py Outdated Show resolved Hide resolved

shimmy/registration.py Outdated Show resolved Hide resolved

shimmy/utils/meltingpot.py Show resolved Hide resolved

tests/test_meltingpot.py Outdated Show resolved Hide resolved

elliottower added 2 commits March 15, 2023 13:51

Fix unneded type ignore, add importorskip, fix spacing in d_env docst…

ff38f5c

…ring

Moved autorom dep from testing to atari (can sometimes cause CI failu…

c0d57be

…res for unrealted envs)

elliottower and others added 20 commits March 18, 2023 19:03

Add bazel install to meltingpot dockerfile

47ab2f5

Update meltingpot.Dockerfile

145e67b

Added openspiel rendering of game state, removed unused import

01bdf5a

Merge branch 'main' of github.com:elliottower/Shimmy

4594297

Fix typo in meltingpot dockerfile

03884ae

Fix typo in meltingpot dockerfile

ec40546

Update docs with images/descriptions, update dependencies

a64a5fc

Add multi agent dm control dockerfile

2839057

Add shell scripts to install meltingpot and dm_lab

ff9be07

Fix typo in meltingpot dockerfile

fc3a922

Fix shell scripts RUN not having ./ in dockerfile

fd7b658

Update Dockerfile to archive image (35 mins to build)

b3a9892

Update optional-install-test.yml

a41a453

Update optional-install-test.yml

a00ae91

Try to fix 'No such container: shimmy-melting-pot-docker' docker error

9651b6b

Update pre-commit version which failed in CI test

8a9bfa2

Pre-commit

66852ce

Added static images for env documentation

c321941

Removed meltingpot CI, fixed gym V22 refs to V21, updated docs

f0b8bd0

Commented out DM control CI tests (waiting for gym v27)

0bacd66

jjshoots approved these changes Mar 21, 2023

View reviewed changes

pseudo-rnd-thoughts requested changes Mar 22, 2023

View reviewed changes

Merge branch 'Farama-Foundation:main' into main

42f9d0d

elliottower mentioned this pull request Mar 22, 2023

Update documentation, add PR template, CONTRIBUTING.md #43

Merged

elliottower closed this Mar 22, 2023

elliottower mentioned this pull request Mar 22, 2023

Add melting pot compatibility, fix slight bugs/pre-commit #44

Merged

		@@ -0,0 +1,57 @@
		"""Tests the functionality of the MeltingPotCompatibility wrapper on meltingpot substrates."""

		})


		def spec_to_space(spec: tree.Structure[dm_env.specs.Array]) -> spaces.Space:

Add compatibility wrapper for DeepMind Melting Pot #39

Add compatibility wrapper for DeepMind Melting Pot #39

Conversation

elliottower commented Mar 4, 2023

elliottower commented Mar 4, 2023

pseudo-rnd-thoughts commented Mar 5, 2023

jjshoots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pseudo-rnd-thoughts commented Mar 7, 2023 • edited Loading

elliottower commented Mar 8, 2023

jjshoots commented Mar 10, 2023

elliottower commented Mar 10, 2023

pseudo-rnd-thoughts commented Mar 14, 2023 • edited Loading

jjshoots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

jjshoots commented Mar 22, 2023

elliottower commented Mar 22, 2023

pseudo-rnd-thoughts commented Mar 22, 2023

elliottower commented Mar 22, 2023

pseudo-rnd-thoughts commented Mar 7, 2023 •

edited

Loading

pseudo-rnd-thoughts commented Mar 14, 2023 •

edited

Loading