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

[FIX] prioritize cli options over yaml #5495

Merged
merged 8 commits into from
Aug 18, 2021
Merged

Conversation

maryamhonari
Copy link
Contributor

Proposed change(s)

steps to reproduce the bug:

  1. run exp1 for 1M steps
  2. initialize exp2 from last one with yaml["checkpoint_settings"]["initialize_from"] and continue running for eg. 200k
  3. resume exp2  from cmd and the step count continues from 1M instead of 200k

solution: checkpointSetting class now prioritize the command line arguments (resume or initialize_from for now) over conflicting yaml options. yaml option is nuke with a warning

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

# ignore init if resume set
full_args.append("--resume")
opt = parse_command_line(full_args)
assert opt.checkpoint_settings.initialize_from is None # ignore init if resume set
assert opt.checkpoint_settings.resume is True

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test to check if they were both set in the YAML but not the CLI? I think we can do this in the test_settings.py.

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.

Forgot in my last approval - add this bug fix to the CHANGELOG.md file and reference the PR before merging.

@maryamhonari maryamhonari merged commit 45dc5dc into main Aug 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix-resume-priority branch August 18, 2021 18:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022
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.

2 participants