Skip to content

Fix lesson incrementing #4279

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

Merged
merged 6 commits into from
Jul 29, 2020
Merged

Fix lesson incrementing #4279

merged 6 commits into from
Jul 29, 2020

Conversation

awjuliani
Copy link
Contributor

Proposed change(s)

Fix issue with curriculum attempting to increment lesson even when there are no more lessons.

[Todo] I haven't added an additional test for this. I might need some help from someone more familiar with the curriculum codebase.

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

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.

Great catch !
I think we should even assert that the last lesson always has a None completion_criteria in this method

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.

Looks good and seconded Vince's comment

@chriselion
Copy link
Contributor

Can you add a unit test that would have caught this before (or if you add the validation to completion_criteria, test that it fails)?

You might need to merge master to get the yamato tests passing again. Let me know if they're still failing after that and I'll take a look.

@awjuliani
Copy link
Contributor Author

@vincentpierre Makes sense. The question is then how to handle this. My preference would be to just throw a warning and ignore the completion criteria of the last lesson, rather than throwing an error.

@vincentpierre
Copy link
Contributor

My preference would be to just throw a warning and ignore the completion criteria of the last lesson, rather than throwing an error.

I agree.

@awjuliani
Copy link
Contributor Author

Added a warning and a test to catch the user error originally reported.

RunOptions.from_dict(
yaml.safe_load(test_bad_curriculum_all_competion_criteria_config_yaml)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a must do for this PR, but to make sure your change worked, can you make a bunch of calls to param_manager.update_lessons to make sure it does not raise an error. (the error you fixed line 134 of environment_parameter_manager.py)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an explicit test for this.

@awjuliani awjuliani merged commit fef445d into master Jul 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-fix-lesson-check branch July 29, 2020 23:10
awjuliani added a commit that referenced this pull request Jul 30, 2020
awjuliani added a commit that referenced this pull request Jul 30, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
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.

4 participants