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

Super suit migration example #1091

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GiovanniGrotto
Copy link
Contributor

Description

This is a try of migrating clip_reward_v0 and color_reduction_v0 from SuperSuit basic_wrappers.py to PettingZoo.
It is a work in progress so there is a lot of things that needs to be fixed but I would like to know if at least I'm in the good track.
In particular:

  • Test folder should not exist and the test inside are just to check if everything is running and will then be improved and moved in the main test folder of PZ
  • Docstring have been generated by chatGPT and will be double checked improved and fixed

I'm having some trouble with type hinting, in fact some of them could be wrong, since sometimes it is not very clear what a function should accepts, is there a way or a best practice to make this easier or at least cleaner?
I'm open to any type of comment and suggestions about how to make this migration cleaner.

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

This function takes an environment, a module, and a parameter, and creates a new environment with an observation
space and observations modified based on the provided module and parameter.

Parameters:
Copy link
Member

Choose a reason for hiding this comment

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

If you could have the docstrings be more similar to the syntax in conversions.py and other wrappers in PettingZoo that would be good. Also I don't know the wrapper super well but I think my_module and my_param may not be the correct names. In general using GPT to write this stuff is pretty risky because it could very well just be completely made up and I don't have the time to look through all of the specifics to ensure it's correct. If you can do so yourself and double check then that's great but I'm hesitant to include too much details because it could be incorrect. Look elsewhere throughout the repo to see how the formatting is done for the docstrings.

And for the example format, see chess.py as we have an example using the >>> format which gets tested under the doctests with pytest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks for the advice, I'll fix the docstring format and write them from scratch.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, definitely not use the output of CGPT directly for docstrings.

class BaseWrapper(PettingzooWrap):
def __init__(self, env: AECEnv | ParallelEnv):
"""
Creates a wrapper around `env`. Extend this class to create changes to the space.
Copy link
Member

Choose a reason for hiding this comment

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

Also this looks incorrect saying class BaseWrapper(PettingZooWrap), why would it use OrderEnforcingWrapper as the base class? And I'd honestly have to think more about what the base class should be, it's tough cause a lot of the functions like step() and reset() have different return types and aren't uniform between parallel and AEC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't really looked into the wrappers for the moment, this is just as it was on SuperSuit I can try to find a better base class for BaseWrapper

@elliottower
Copy link
Member

Also run the pre-commit hooks as that will give you some feedback about docstrings adhering to the right format.

@jjshoots
Copy link
Member

Looks fairly sensible so far, seems like these are just files taken from SS with docstrings added. For type hinting, I would recommend something like the way Gymnasium does it, but ultimately this is Elliot's call. I'll wait for a more fleshed out version of the PR before doing a more in depth review. Ultimately, as long as the tests are comprehensive and they pass I don't see anything wrong.

@elliottower
Copy link
Member

Looks fairly sensible so far, seems like these are just files taken from SS with docstrings added. For type hinting, I would recommend something like the way Gymnasium does it, but ultimately this is Elliot's call. I'll wait for a more fleshed out version of the PR before doing a more in depth review. Ultimately, as long as the tests are comprehensive and they pass I don't see anything wrong.

Agreed, and we’ve added some better type hinting modeled after that for other internal files in pettingzoo, so that should be easy to replicate looking at env.py and stuff

@elliottower
Copy link
Member

Jets pr got merged which was an example of doubt SuperSuit style wrappers, you should try to follow it as an example

@GiovanniGrotto
Copy link
Contributor Author

GiovanniGrotto commented Sep 28, 2023

Jets pr got merged which was an example of doubt SuperSuit style wrappers, you should try to follow it as an example

Perfect , thanks a lot, I'll try to find some time and start with the migration

@jjshoots
Copy link
Member

@GiovanniGrotto For your reference, it's #1105 :)

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.

3 participants