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

Initialize-from custom checkpoints #5525

Merged
merged 18 commits into from
Sep 14, 2021
Merged

Conversation

maryamhonari
Copy link
Contributor

@maryamhonari maryamhonari commented Sep 1, 2021

Proposed change(s)

Initialize from custom checkpoints

Usage:

behaviors:
  BigWallJump:
    Init_path: BigWallJump-654098.pt #specify a checkpoint_name in the behavior directory
    trainer_type: ppo
  MediumWallJump:
    Init_path: results/previous_run/MediumWallJump/MediumWallJump-654098.pt # full path is also supported
    trainer_type: ppo
SmallWallJump: # if none specified, will initialize from most recent checkpoint
    trainer_type: ppo
checkpoint_settings:
  initialize_from: previous_run

TODO: Extra tests, Documentation, changeLog

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

@maryamhonari maryamhonari changed the title [WIP] Initialize-from checkpoints [WIP] Initialize-from custom checkpoints Sep 2, 2021
Comment on lines -103 to -104
if init_path is not None:
trainer_settings.init_path = os.path.join(init_path, brain_name)
Copy link
Contributor Author

@maryamhonari maryamhonari Sep 8, 2021

Choose a reason for hiding this comment

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

Not sure why init_path is passed up to here, was there a specific reason to set this in trainer factory?
I moved this logic to learn.py and if it's harmless will remove the init_path attribute.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried using init_path and see if it does what we expect out of it?
Reading a bit of the code in torch_model_saver.py it looks like setting init_path in the trainer settings initializes the policy from a checkpoint. Is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Previously trainer_settings.init_path="result_dir/run_id/brain_name and we added the checkpoint name at the end in torch_model_saver.py
This PR sets the full path trainer_settings.init_path="result_dir/run_id/brain_name/checkpoint_name.pt in learn.py:102

@maryamhonari maryamhonari marked this pull request as ready for review September 8, 2021 19:20
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.

I made some comments.
I would like some clarification on what init_path in the trainer parameters do. It seems a bit redundant with the checkpoint settings.

ml-agents/mlagents/trainers/learn.py Outdated Show resolved Hide resolved
ml-agents/mlagents/trainers/learn.py Outdated Show resolved Hide resolved
ml-agents/mlagents/trainers/learn.py Outdated Show resolved Hide resolved
ml-agents/mlagents/trainers/learn.py Outdated Show resolved Hide resolved
ml-agents/mlagents/trainers/learn.py Outdated Show resolved Hide resolved
Comment on lines -103 to -104
if init_path is not None:
trainer_settings.init_path = os.path.join(init_path, brain_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried using init_path and see if it does what we expect out of it?
Reading a bit of the code in torch_model_saver.py it looks like setting init_path in the trainer settings initializes the policy from a checkpoint. Is that correct?

maryamhonari and others added 4 commits September 8, 2021 13:03
Co-authored-by: Vincent-Pierre BERGES <vincentpierre@unity3d.com>
Co-authored-by: Vincent-Pierre BERGES <vincentpierre@unity3d.com>
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.

I approve of the code.
Got one question before you merge. If the user fills both init_path and the checkpoint_list. Which one gets priority.
(Please add documentation and changelog before merging)

@maryamhonari maryamhonari changed the title [WIP] Initialize-from custom checkpoints Initialize-from custom checkpoints Sep 10, 2021
com.unity.ml-agents/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Vincent-Pierre BERGES <vincentpierre@unity3d.com>
@maryamhonari maryamhonari merged commit 6b2c127 into main Sep 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the develop-init-custom-checkpoint branch September 14, 2021 23:33
maryamhonari added a commit that referenced this pull request Sep 24, 2021
* init from any checkpoint including older ones
* moving init_path logic ahead to learn.py
* fixing pytest to take the full path
* doc & changelog
sini pushed a commit that referenced this pull request Sep 29, 2021
* init from any checkpoint including older ones
* moving init_path logic ahead to learn.py
* fixing pytest to take the full path
* doc & changelog
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 15, 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