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.

Sign up for GitHub

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

Jump to bottom

Develop remove academy done #2519

Merged
merged 18 commits into from
Sep 17, 2019
Merged

Develop remove academy done #2519

merged 18 commits into from
Sep 17, 2019

Conversation

vincentpierre
Copy link
Contributor

@vincentpierre vincentpierre commented Sep 9, 2019 •
edited
Loading

Design

Design Document

List of changes :

  • Removed the Done flag of the Academy
  • Removed the MaxStep of the Academy
  • Removed/Modified the unit tests that tested for this functionality
  • Removed the Academy.Done() and Academy.IsDone() methods
  • Removed the global_done flag of the BaseUnityEnvironment abstract class
  • Added a ForceReset method on the Agent so the Academy can force all agents to reset when Python requests it
  • It is no longer necessary to call reset on the environment right after creating it (step will call reset if it is called for the first time)

@CLAassistant
Copy link

CLAassistant commented Sep 9, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@vincentpierre vincentpierre self-assigned this Sep 9, 2019
@vincentpierre
Replace global_done with reserved.
1356c63 
Addressing Chris Elion's comment regarding the deprecation of the global_done field. We will use a reserved field to make sure the global done does not get replaced in the future causing errors.
@harperj
Copy link
Contributor

harperj commented Sep 11, 2019

Code looks good to me aside from fixing the minor linting issue from CircleCI. You should add a reference to this change in the changelog. It would also be nice to see a test that the "reset on first step" works correctly.

@chriselion
Copy link
Contributor

@harperj Is there a specific changelog doc, or do you mean https://github.com/Unity-Technologies/ml-agents/blob/master/docs/Migrating.md? Definitely think we need to update that, but can be in another PR.

@vincentpierre
Copy link
Contributor Author

vincentpierre commented Sep 13, 2019 •
edited
Loading

DO NOT MERGE
Once the PR is approved, I will make the commits for documentation changes.
Documentation changes made

@vincentpierre
Copy link
Contributor Author

@chriselion I am rerequesting review since I edited the documentation.

chriselion
chriselion approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@chriselion chriselion left a comment

Choose a reason for hiding this comment

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

Doc changes look good. Were you going to update the Migration Guide too?

@vincentpierre
Copy link
Contributor Author

@chriselion I just did, thanks for catching this.

awjuliani
awjuliani reviewed Sep 16, 2019
View reviewed changes
docs/Migrating.md Outdated Show resolved Hide resolved
awjuliani
awjuliani reviewed Sep 17, 2019
View reviewed changes
Copy link
Contributor

@awjuliani awjuliani 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. Just a couple suggestions for documentation, and some apparent issues with merge conflicts apparent in the diffs.

@vincentpierre
Addressing comments :
7534855 
 - Removing dead code
 - Resolving forgotten merged conflicts
 - Editing documentations like suggested in the review
chriselion
chriselion reviewed Sep 17, 2019
View reviewed changes
docs/Migrating.md Outdated Show resolved Hide resolved
@vincentpierre vincentpierre merged commit 2a06b63 into develop Sep 17, 2019
@vincentpierre vincentpierre deleted the develop-remove-academy-done branch September 17, 2019 23:51
@chriselion chriselion mentioned this pull request Sep 25, 2019
Merged
@GProulx GProulx mentioned this pull request Oct 30, 2019
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chriselion chriselion chriselion approved these changes

@awjuliani awjuliani awjuliani left review comments

@harperj harperj Awaiting requested review from harperj

Assignees

@vincentpierre vincentpierre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vincentpierre @CLAassistant @harperj @chriselion @awjuliani