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

Updated PettingZoo+RLlib tutorial #19069

Merged
merged 13 commits into from
Oct 29, 2021
Merged

Updated PettingZoo+RLlib tutorial #19069

merged 13 commits into from
Oct 29, 2021

Conversation

Rohan138
Copy link
Contributor

@Rohan138 Rohan138 commented Oct 4, 2021

Updated the tutorial and added link to the blog post by the PettingZoo team.

Why are these changes needed?

Checks

  • [ Y] I've run scripts/format.sh to lint the changes in this PR.
  • [ Y] I've included any doc changes needed for https://docs.ray.io/en/master/.
  • [ Y] I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Rohan138 and others added 2 commits October 4, 2021 00:31
Updated the tutorial and added link to the blog post by the PettingZoo team.
@jkterry1
Copy link
Contributor

jkterry1 commented Oct 5, 2021

@richardliaw @sven1977 could you please merge this when you get a chance?

@jkterry1
Copy link
Contributor

@sven1977 @richardliaw could this please be merged? it's been 6 days

@richardliaw
Copy link
Contributor

Hi @jkterry1 @Rohan138, it looks like there is a merge conflict and tests are also not passing; could you help address those? Happy to merge after.

Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Hey @Rohan138 , thanks for the PR. Could you make these few changes I suggested?


# Train once
trainer.train()
env_name = "pistonball_v4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move most of the comments back here? They were quite useful for understanding, why we define certain config keys.

Comment on lines 72 to 74
test_env = ParallelPettingZooEnv(env_creator({}))
obs_space = test_env.observation_space
act_space = test_env.action_space
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this anymore. Just use:

multiagent:
   policies: {"policy0"}
   policy_mapping_fn: ...

},
"gamma": 0.99,
}
return (None, obs_space, act_space, config)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above. I don't think it's needed anymore. Just move the "model" key (and its values) into the main config below. Same for gamma.

@Rohan138
Copy link
Contributor Author

Rohan138 commented Oct 18, 2021

@sven1977 Per discussion with @jkterry1, we would prefer to move the pettingzoo example so that it links to the example in PettingZoo/tutorials. This way, we can maintain it to keep it compatible with both PettingZoo and RLlib. I've removed the outdated pettingzoo_env.py example and updated the docs accordingly for your review.

There's also a Medium article on PettingZoo+RLlib that might be a useful link.

Copy link
Contributor

@sven1977 sven1977 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 now, sorry for the delay. Was out last week and last few days.
We may still have to remove the pettingzoo.py script from our BUILD file as it's currently part of our tests. Will take care of it.

@sven1977 sven1977 merged commit b9c9cc5 into ray-project:master Oct 29, 2021
@Rohan138 Rohan138 deleted the patch-1 branch December 1, 2021 18:48
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