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

Following petting_zoo registry API #5557

Merged
merged 14 commits into from
Oct 18, 2021

Conversation

maryamhonari
Copy link
Contributor

@maryamhonari maryamhonari commented Sep 29, 2021

Proposed change(s)

Follow the pettingZoo environment import:

from pettingzoo_unity.envs import StrikersVsGoalie

env = StrikersVsGoalie.env()
  • Utilize increasing port number if the current port is occupied

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

https://jira.unity3d.com/browse/MLA-2235
Try this for democolab link for this branch
 

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

@maryamhonari maryamhonari marked this pull request as ready for review October 6, 2021 14:28
@maryamhonari maryamhonari changed the title [WIP] petting_zoo registry Following petting_zoo registry API Oct 6, 2021
Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

Left some comments. Looks good overall.

.github/workflows/pre-commit.yml Show resolved Hide resolved
.github/workflows/pytest.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
@@ -20,6 +20,7 @@ and this project adheres to
terminated teammates. (#5441)
- Fixed wrong attribute name in argparser for torch device option (#5433)(#5467)
- Fixed conflicting CLI and yaml options regarding resume & initialize_from (#5495)
- Added minimal analytics collection to LL-API (#5511)
Copy link
Contributor

Choose a reason for hiding this comment

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

strange that this PR would contain this change, are you sure you cherry picked the right commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this commit contains the default analytics side channels for custom trainers.

pettingzoo-unity/Colab_PettingZoo.ipynb Outdated Show resolved Hide resolved
pettingzoo-unity/pettingzoo_unity/envs/__init__.py Outdated Show resolved Hide resolved
@maryamhonari maryamhonari force-pushed the exp-petting-registry branch 2 times, most recently from 7b4cd0f to cb869c8 Compare October 8, 2021 18:22
Copy link
Collaborator

@miguelalonsojr miguelalonsojr left a comment

Choose a reason for hiding this comment

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

Looks good. +1 to all the current comments and fixes, and I just added one. Cheers!

pettingzoo-unity/Colab_PettingZoo.ipynb Outdated Show resolved Hide resolved
update the clone branch in colab
Copy link
Collaborator

@miguelalonsojr miguelalonsojr left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@vincentpierre vincentpierre left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please wait on another approval to merge


# Register each environment in default_registry as a PettingZooEnv
for key in default_registry:
key = key.replace("3", "Three")
Copy link
Contributor

Choose a reason for hiding this comment

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

This line does not look scalable. Is there a better way to do it. Like having a constant dictionary of key to replace value?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally. However, a complete solution to this is non-trival. We could use a package like inflect (https://pypi.org/project/inflect/), however, this would require splitting the string into it's composite integer characters and letter components, convert the integer characters to their word equivalent, and then recombining, since inflect's number_to_words() strips all other non-numeric characters. This would be the most scalable solution, however, not sure if the added complexity is worth it at this point, since this really only applies to 3DBall.

@maryamhonari maryamhonari merged commit 0a2f57f into develop-petting-api Oct 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the exp-petting-registry branch October 18, 2021 16:54
miguelalonsojr pushed a commit that referenced this pull request Jan 27, 2022
* init petting_zoo registry

* cherrypick Custom trainer editor analytics (#5511)

* cherrypick "Update dotnet-format to address breaking changes introduced by upstream changes (#5528)"

* Update colab to match pettingZoo import api

* ToRevert: pull exp-petting-registry branch

* Add init file to tests

* Install pettingzoo-unity requirements for pytest

* update pytest command

* Add docstrings and comments

* update coverage to pettingzoo folder

* unset log level

* update env string
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants