[refactor] Structure configuration files into classes #3936
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 27 commits
April 29, 2020 21:12
andrewcoh
reviewed
May 15, 2020
| @@ -0,0 +1,110 @@ | |||
| import attr | |||
There was a problem hiding this comment.
How long do you think we'll need to keep this script around?
There was a problem hiding this comment.
Hopefully only for 1-2 releases. Or perhaps it should be released with Trainer 1.0 and removed after that.
andrewcoh
reviewed
May 15, 2020
docs/Training-Configuration-File.md
Outdated
| | `hyperparameters -> buffer_size` | (default = `10240` for PPO and `50000` for SAC) Number of experiences to collect before updating the policy model. Corresponds to how many experiences should be collected before we do any learning or updating of the model. **This should be multiple times larger than `batch_size`**. Typically a larger `buffer_size` corresponds to more stable training updates. In SAC, the max size of the experience buffer - on the order of thousands of times longer than your episodes, so that SAC can learn from old as well as new experiences. <br><br>Typical range: PPO: `2048` - `409600`; SAC: `50000` - `1000000` | | ||
| | `hyperparameters -> learning_rate` | (default = `3e-4`) Initial learning rate for gradient descent. Corresponds to the strength of each gradient descent update step. This should typically be decreased if training is unstable, and the reward does not consistently increase. <br><br>Typical range: `1e-5` - `1e-3` | | ||
| | `hyperparameters -> learning_rate_schedule` | (default = `linear` for PPO and `constant` for SAC) Determines how learning rate changes over time. For PPO, we recommend decaying learning rate until max_steps so learning converges more stably. However, for some cases (e.g. training for an unknown amount of time) this feature can be disabled. For SAC, we recommend holding learning rate constant so that the agent can continue to learn until its Q function converges naturally. <br><br>`linear` decays the learning_rate linearly, reaching 0 at max_steps, while `constant` keeps the learning rate constant for the entire training run. | | ||
| | `max_steps` | (default = `500000`) Total number of experience points that must be collected from the simulation before ending the training process. <br><br>Typical range: `5e5` - `1e7` | |
andrewcoh
reviewed
May 15, 2020
andrewcoh
reviewed
May 15, 2020
| @@ -26,9 +20,11 @@ def __init__( | |||
| :param encoding_size: The size of the hidden encoding layer for the ICM | |||
harperj
approved these changes
May 20, 2020
Comment on lines
83
to
85
| self.trainer_settings["sequence_length"] | ||
| > self.trainer_settings["batch_size"] | ||
| and self.trainer_settings["use_recurrent"] |
There was a problem hiding this comment.
I think this can be:
Suggested change
| self.trainer_settings["sequence_length"] | |
| > self.trainer_settings["batch_size"] | |
| and self.trainer_settings["use_recurrent"] | |
| self.trainer_settings.sequence_length | |
| > self.trainer_settings.batch_size | |
| and self.trainer_settings.use_recurrent |
There was a problem hiding this comment.
Ah, this method isn't actually used anymore, the check is done in settings.py in _check_batch_size_seq_length. I've removed them from PPO and SAC.
| vis_encode_type = policy_network_settings.vis_encode_type | ||
|
|
||
| self.tau = hyperparameters.tau | ||
| self.burn_in_ratio = 0.0 |
There was a problem hiding this comment.
Looks like this used to be a trainer param but is not just being set to 0.0? Any reason why?
There was a problem hiding this comment.
This wasn't configurable before (also set to 0 as a constant at the top of the file). Currently the burn-in feature doesn't actually work properly with values greater than 0. Will fix or remove in a PR that just touches that.
Comment on lines
85
to
87
| self.trainer_settings["sequence_length"] | ||
| > self.trainer_settings["batch_size"] | ||
| and self.trainer_settings["use_recurrent"] |
There was a problem hiding this comment.
Suggested change
| self.trainer_settings["sequence_length"] | |
| > self.trainer_settings["batch_size"] | |
| and self.trainer_settings["use_recurrent"] | |
| self.trainer_settings.sequence_length | |
| > self.trainer_settings.batch_size | |
| and self.trainer_settings.use_recurrent |
There was a problem hiding this comment.
Ah, this method isn't actually used anymore, the check is done in settings.py in _check_batch_size_seq_length. I've removed them from PPO and SAC.
| ) | ||
| init_path: Optional[str] = None | ||
| output_path: str = "default" | ||
| # TODO: Remove parser default and remove from CLI |
10 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed change(s)
This PR implements the changes described in this design doc. In particular, it replaces the configuration dictionary (called
trainer_paramsin many files) with objects based on theattrslibrary. These are more flexible than NamedTuples and support smart defaults, validators, typing, and can be structured/unstructured from Dicts based on thecattrslibrary.This PR also assigns reasonable defaults to every hyperparameter, and
mlagents-learnnow allows for training without any configuration file specified (defaults to PPO with extrinsic reward signal). In the future, it will allow for defaults that are being set relative to other defaults (as done in the SelfPlaySettings).The core of the changes are in settings.py.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
Attrs:
https://www.attrs.org/en/stable/
Cattrs:
https://cattrs.readthedocs.io/en/latest/
This PR does not yet refactor how the settings are passed down from Trainer->Policy-> Optimizer. The other two objects still receive the entire
TrainerSettingsobject.TODO: Improved testing for error handling in
settings.py,Update docs.NOTE: The appearance of
output_pathasdefaultin the configs is a bit odd, asoutput_pathis not settable through the configuration (it's determined by therun-id). I plan to removeoutput_pathin a follow-up PR.Types of change(s)
Checklist
Other comments