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

Adds resuming and saving model to sb3 example #135

Merged
merged 10 commits into from
Jul 23, 2023

Conversation

Ivan-267
Copy link
Collaborator

@Ivan-267 Ivan-267 commented Jul 21, 2023

Adds the ability to save and load model for resuming training, run inference and save periodic checkpoints with CL arguments.

Limitations of this implementation:

  • While sb3 will save logs if starting multiple runs with the same experiment name by adding e.g. expname_1, expname_2 etc. to the folder name, my current checkpoint saving implementation requires a unique experiment dir or name argument to be set and prompts the user to set a different folder if the checkpoint folder exists, this is to prevent overwriting checkpoints from a previous run with the same experiment dir/name.

For example, if using the default arguments (no exp dir or name specified), the logs will be saved to:
.\logs\sb3\Experiment_1
and checkpoints will be saved to:
.\logs\sb3\Experiment_checkpoints

  • Resuming training does not keep track of previous timesteps in this implementation. So for example training with --timesteps=10_000 and saving a model, then resuming training with the same setting and saving the model again will train the model for 20_000 total timesteps, but 10_000 steps will be displayed in the console log after the final training run.

This is just a draft implementation, I welcome your feedback on whether it is sufficient as specified before starting changes to the documentation.

Copy link
Collaborator

@visuallization visuallization left a comment

Choose a reason for hiding this comment

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

Nice, looking good! I haven't had time to test it though.

examples/stable_baselines3_example.py Outdated Show resolved Hide resolved

parser.add_argument(
"--timesteps",
default=1_000_000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nice!

examples/stable_baselines3_example.py Show resolved Hide resolved
"remove the folder containing the checkpoints. ")

if args.inference and args.resume_model_path is None:
raise parser.error("Using --inference requires --resume_model_path to be set.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice even with proper error handling. ❤️

Co-authored-by: Florentin Luca Rieger <[email protected]>
@Ivan-267 Ivan-267 marked this pull request as ready for review July 23, 2023 12:43
@Ivan-267
Copy link
Collaborator Author

Nice, looking good! I haven't had time to test it though.

Thanks for the review. I ran a quick test on it in which it worked, but I didn't test every use case in-depth.

@@ -21,34 +24,114 @@
"--experiment_dir",
default="logs/sb3",
type=str,
help="The name of the experiment directory, in which the tensorboard logs are getting stored",
help="The name of the experiment directory, in which the tensorboard logs and checkpoints (if enabled) are getting stored."
Copy link
Owner

@edbeeching edbeeching Jul 23, 2023

Choose a reason for hiding this comment

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

small nit, can the default experiment_name be lower case, "experiment"?

Copy link
Owner

@edbeeching edbeeching left a comment

Choose a reason for hiding this comment

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

This is great! Maybe we could expose the show_window parameter, in other scripts I have this option as --viz. I think training can be faster if you don't render. (but obviously you can't see the agent's progress)

Otherwise LGTM feel free to merge.

@Ivan-267
Copy link
Collaborator Author

This is great! Maybe we could expose the show_window parameter, in other scripts I have this option as --viz. I think training can be faster if you don't render. (but obviously you can't see the agent's progress)

Otherwise LGTM feel free to merge.

Thank you for the review, I agree. Camera sensors may need --viz when used (I haven't tested them at all yet so I'm not sure about this, also whether they work better with a GPU, which I can't test yet), if they do we can consider adding a help note later to where the argument appears.

Will merge after all tests pass.

@Ivan-267 Ivan-267 merged commit 2c348c8 into main Jul 23, 2023
@Ivan-267 Ivan-267 deleted the sb3_example_add_save_and_resume branch July 23, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants