Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pickling tests, adapt all envs to be picklable #928

Merged
merged 24 commits into from
Apr 21, 2023

Conversation

elliottower
Copy link
Contributor

@elliottower elliottower commented Mar 29, 2023

Description

This PR adds tests to check that each environment can be saved and loaded via pickling. Waterworld and mpe simple envs both have pygame objects (pygame.Clock and pygame Surface, respectively) and cannot be pickled without EzPickle. I have removed the changes I made to version numbers and other miscellaneous changes to make this PR as simple as possible.

Previous comments:

and includes them in the API test. I'm setting this as a draft because I'm not sure if we should include it in the API test, as that includes third-party environments which may not care about pickling. But perhaps it is good to enforce that all environments can be pickled, even custom ones? All tests pass internally.

I have moved them to a separate file from API tests as it was messy and causing other tests to fail due to rendering and video issues, also seems simplest to do it this way.

Fixes # (issue), Depends on # (pull request)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.
To upload images to a PR -- simply drag and drop or copy paste.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have run pytest -v and no errors are present.
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@elliottower
Copy link
Contributor Author

Do we care if rendered pygame envs can be pickled? Currently they all work with render_mode=None but with pygame rendering there are some issues like chess has a pygame.time.Clock object which can't be pickled. Could add EzPickle to that file but it's a bit weird to do that as it's all python and I don't imagine people will try to pickle rendered envs. When you load an env which has been pickled and uses pygame it also doesn't have the video system initialized, as the pygame.init() call is in the constructor rather than in reset().
@pseudo-rnd-thoughts @jkterry1

@elliottower elliottower changed the title Add pickling tests in API test, adapt waterworld to be picklable Add pickling tests, adapt all envs to be picklable Mar 30, 2023
@elliottower elliottower marked this pull request as ready for review March 31, 2023 16:37
test/pickle_test.py Outdated Show resolved Hide resolved
test/pickle_test.py Outdated Show resolved Hide resolved
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeat comments for the second class

pettingzoo/mpe/simple/simple.py Outdated Show resolved Hide resolved
pettingzoo/mpe/simple/simple.py Outdated Show resolved Hide resolved
pettingzoo/mpe/simple/simple.py Outdated Show resolved Hide resolved
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the changes, looks good

pettingzoo/mpe/simple_spread/simple_spread.py Show resolved Hide resolved
pettingzoo/mpe/simple_tag/simple_tag.py Show resolved Hide resolved
pettingzoo/mpe/simple_adversary/simple_adversary.py Outdated Show resolved Hide resolved
pettingzoo/mpe/simple_crypto/simple_crypto.py Outdated Show resolved Hide resolved
pettingzoo/mpe/simple_push/simple_push.py Outdated Show resolved Hide resolved
test/all_parameter_combs_test.py Outdated Show resolved Hide resolved
test/pickle_test.py Outdated Show resolved Hide resolved
@elliottower elliottower merged commit 6aca84c into Farama-Foundation:master Apr 21, 2023
@elliottower elliottower deleted the pickle-test branch April 21, 2023 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants