Skip to content

Reverting bug introduced in #4071 #4101

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
Merged
merged 1 commit into from
Jun 9, 2020
Merged

Reverting bug introduced in #4071 #4101

merged 1 commit into from
Jun 9, 2020

Conversation

vincentpierre
Copy link
Contributor

Proposed change(s)

Describe the changes made in this PR.

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

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

@vincentpierre vincentpierre self-assigned this Jun 9, 2020
@ervteng
Copy link
Contributor

ervteng commented Jun 9, 2020

Why did the original PR break just FoodCollector?

@vincentpierre
Copy link
Contributor Author

Why did the original PR break just FoodCollector?

Because of all our example environments, only FoodCollector has a dependency on EnvironmentReset

@ervteng
Copy link
Contributor

ervteng commented Jun 9, 2020

Why did the original PR break just FoodCollector?

Because of all our example environments, only FoodCollector has a dependency on EnvironmentReset

Should we add a unit test for EnvironmentReset?

@vincentpierre
Copy link
Contributor Author

Should we add a unit test for EnvironmentReset?

We do not have a way to unit test when the Python communication is happening. This bug does not happen at inference

ervteng
ervteng approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@ervteng ervteng left a comment

Choose a reason for hiding this comment

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

🐛 🚫

@ervteng
Copy link
Contributor

ervteng commented Jun 9, 2020

Should we add a unit test for EnvironmentReset?

We do not have a way to unit test when the Python communication is happening. This bug does not happen at inference

Sounds good. I guess this sounds like a Yamato test but it's probably out-of-scope for this PR.

@vincentpierre vincentpierre merged commit 9ec2fe2 into master Jun 9, 2020
@delete-merged-branch delete-merged-branch bot deleted the hotfix-first-reset branch June 9, 2020 21:42
ervteng pushed a commit that referenced this pull request Jun 9, 2020
@vincentpierre
Reverting bug introduced in #4071 (#4101)
7b17049
@ervteng ervteng mentioned this pull request Jun 9, 2020
Merged
10 tasks
ervteng pushed a commit that referenced this pull request Jun 9, 2020
@emmaajordan @vincentpierre
Release 3 Cherry-pick bug-fixes and doc changes from master (#4102)
dc43c8d 
* [bug-fix] Fix regression in --initialize-from feature (#4086)

* Fixed text in GettingStarted page specifying the logdir for tensorboard. Before it was in a directory summaries which no longer existed. Results are now saved to the results dir. (#4085)

* [refactor] Remove nonfunctional `output_path` option from TrainerSettings (#4087)

* Reverting bug introduced in #4071 (#4101)

Co-authored-by: Scott <[email protected]>
Co-authored-by: Vincent-Pierre BERGES <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Hunter-Unity Hunter-Unity Awaiting requested review from Hunter-Unity

1 more reviewer

@ervteng ervteng ervteng approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@vincentpierre vincentpierre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vincentpierre @ervteng