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

Hanabi Integration #63

Closed
wants to merge 13 commits into from
Closed

Hanabi Integration #63

wants to merge 13 commits into from

Conversation

dissendahl
Copy link

  • Hanabi, integrated from official repo as git submodule.

  • See hanabi/README.md for how to pull and setup git submodule.

  • See documentation within class for full information.

  • Debugged and E2E tested with unittest, including api test as part of the unit tests.

  • CHANGE REQUEST: Add typing annotation in utils/env.py, see last commit.

@dissendahl dissendahl added the enhancement New feature or request label Apr 22, 2020
@dissendahl dissendahl self-assigned this Apr 22, 2020
@jkterry1
Copy link
Member

@weepingwillowben are you happy with this?

@jkterry1
Copy link
Member

jkterry1 commented Apr 23, 2020

@dissendahl can you fix the merge conflict with init?

@jkterry1
Copy link
Member

@dissendahl

Can you please elaborate on:

Debugged and E2E tested with unittest, including api test as part of the unit tests.

CHANGE REQUEST: Add typing annotation in utils/env.py, see last commit.

@dissendahl
Copy link
Author

dissendahl commented Apr 23, 2020

@justinkterry:

  • Merge conflict is resolved.

-To clarify: Naturally I did functionally testing with unit tests. Furthermore I have included two unit tests for E2E and integration testing: 1st test runs a full hanabi game, then resets the env, then runs another game on the same env instance. 2nd test calls utils/test_api.py to validate if env is compliant with the api design. -> see commit ddc7252

-To clarify - CHANGE REQUEST: I added type hints to the top level env definition, so that it is easier to understand which return values are expected when implementing a inheriting environment. IMO this makes life easier for people who want to build their own environment based on the pettingzoo api. -> see commit 890ed9f

@benblack769
Copy link
Contributor

benblack769 commented Apr 24, 2020

Hanabi should match the pettingzoo expectations of what an environment should look like.
In particular, you should be create an environment with no parameters. This means the environment should have good defaults for every parameter. So you need to choose some good defaults. If there is some ambiguity as to what is the best parameter, try to choose the most standard one possible, or at least a sensible one.

After this is finished, I'll add hanabi to the CI tests and take a look at it again.

@benblack769
Copy link
Contributor

On a seperate note, it is not clear to me that git submodules is the best option we have. I had some trouble getting it set up myself, and it seems likely that this will cause us a lot of headache in the future. As an alternative we can fork the codebase to https://github.com/PettingZoo-Team/ and host it on pypi ourselves, which may be a better long term option. We are already planning on this for MAgent and ALE.

@jkterry1
Copy link
Member

  1. I think that Ben's right. We forked HLE, and Mario will release it on PyPI for us: https://github.com/PettingZoo-Team/hanabi-learning-environment
  2. @dissendahl there's still a merge conflict
  3. @weepingwillowben are we sure the type recommendations are good ideas? Very oten it's a single int or something instead.

@jkterry1
Copy link
Member

  1. Alright, it's now available on PyPI. Can you please update to use this?
  2. Environments shouldn't have their own readmes
  3. I'm confused by what you were saying earlier about testing @dissendahl

@dissendahl
Copy link
Author

@weepingwillowben and @justinkterry : Thanks for reviewing!

  1. I will update the env to work with a default game configuration. @weepingwillowben: So I understoud now that envs should work without config. But is it okay to pass them parameters or is this not wanted all?
  2. I will remove the git submodule thing and the belonging readme.
  3. Take a look at the merge conflict.

@benblack769
Copy link
Contributor

The goal is something like this:

class env(AECEnv):

    metadata = {'render.modes': ['human']}

    def __init__(self, seed=None, spawn_rate=20, num_archers=2, num_knights=2, killable_knights=True, killable_archers=True, pad_observation=True, max_frames=900):

@benblack769
Copy link
Contributor

So the environment will have parameters, but they will all have a default value.

@jkterry1
Copy link
Member

@clemens4321 Please move the type hints from the AECEnv class specifically to hanabi

@dissendahl dissendahl closed this Apr 28, 2020
@dissendahl dissendahl deleted the hanabi branch April 28, 2020 07:57
@dissendahl
Copy link
Author

Removed the branch, therefore have to reopen pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants