setuptools-based plugin for StatsWriters #4788
Conversation
| @@ -0,0 +1,11 @@ | |||
| from setuptools import setup | |||
chriselion
Dec 21, 2020
Author
Collaborator
No plans to publish this package, but we could use it to set up tests, e.g. for bad imports.
No plans to publish this package, but we could use it to set up tests, e.g. for bad imports.
| ], | ||
| python_requires=">=3.6.1", | ||
| entry_points={ | ||
| "console_scripts": [ | ||
| "mlagents-learn=mlagents.trainers.learn:main", | ||
| "mlagents-run-experiment=mlagents.trainers.run_experiment:main", | ||
| ] | ||
| ], | ||
| "mlagents.stats_writer": [ |
chriselion
Dec 21, 2020
Author
Collaborator
We would need to add a new key for each type of plugin interface we want, e.g. mlagents.demonstration_provider: [...]
We would need to add a new key for each type of plugin interface we want, e.g. mlagents.demonstration_provider: [...]
| version="0.0.1", | ||
| entry_points={ | ||
| "mlagents.stats_writer": [ | ||
| "example=mlagents_plugin_examples.example_stats_writer:get_example_stats_writer" |
chriselion
Dec 21, 2020
•
Author
Collaborator
The form of this is {entry point name}={plugin module}:{plugin_function}
The form of this is {entry point name}={plugin module}:{plugin_function}
chriselion
changed the title
| "example=mlagents_plugin_examples.example_stats_writer:get_example_stats_writer" | ||
| ] | ||
| ``` | ||
| * `mlagents.stats_writer` is the name of the plugin interface. This must be one of the provided interfaces (see below). |
vincentpierre
Feb 3, 2021
Collaborator
Make the see below a link to line 41
Make the see below a link to line 41
chriselion
Feb 5, 2021
Author
Collaborator
Done
Done
| argparser.add_argument( | ||
| "--results-dir", default="results", help="Results base directory" | ||
| ) |
vincentpierre
Feb 3, 2021
Collaborator
Is this relevant to this PR? Why are we adding this flag ?
Is this relevant to this PR? Why are we adding this flag ?
chriselion
Feb 5, 2021
Author
Collaborator
This was previously hardcoded in learn.py
https://github.com/Unity-Technologies/ml-agents/pull/4788/files#diff-4e0d7b8aef419254e6fc3c63f5baa53f222afda7c908705e857a22f7d4792c47L68
and is now used in CheckpointSettings.maybe_init_path()
https://github.com/Unity-Technologies/ml-agents/pull/4788/files#diff-546e90789e914f8707fd97391f78c4bba39ae69a965858bbb69c2d324db1ec51R719
I can keep it hardcoded if you prefer, but I do think moving all the path logic to part of RunOptions is a win (and basically a pre-req to make TensorBoardWriter act like a plugin)
This was previously hardcoded in learn.py
https://github.com/Unity-Technologies/ml-agents/pull/4788/files#diff-4e0d7b8aef419254e6fc3c63f5baa53f222afda7c908705e857a22f7d4792c47L68
and is now used in CheckpointSettings.maybe_init_path()
https://github.com/Unity-Technologies/ml-agents/pull/4788/files#diff-546e90789e914f8707fd97391f78c4bba39ae69a965858bbb69c2d324db1ec51R719
I can keep it hardcoded if you prefer, but I do think moving all the path logic to part of RunOptions is a win (and basically a pre-req to make TensorBoardWriter act like a plugin)
ervteng
Feb 5, 2021
•
Collaborator
If a user changes this from the default results will it mess with how they're saved/synced in cloud? cc: @hvpeteet
If a user changes this from the default results will it mess with how they're saved/synced in cloud? cc: @hvpeteet
hvpeteet
Feb 5, 2021
•
Contributor
If a user can set it in the run_options then yes this could mess with cloud since we don't already take it into account. I don't think it is a reason not to make the change though. We can add logic to overwrite this field since we already modify run_options.
Just a couple of questions to confirm my understanding:
- Would this be set under
checkpoint_settings --> write_path?
- Could we (cloud) blindly set this for every experiment and it would be ignored by older mlagents versions?
If a user can set it in the run_options then yes this could mess with cloud since we don't already take it into account. I don't think it is a reason not to make the change though. We can add logic to overwrite this field since we already modify run_options.
Just a couple of questions to confirm my understanding:
- Would this be set under
checkpoint_settings --> write_path? - Could we (cloud) blindly set this for every experiment and it would be ignored by older mlagents versions?
chriselion
Feb 5, 2021
Author
Collaborator
- It would be
checkpoint_settings -> results_dir (at the top level)
- This would trigger an error here
- It would be
checkpoint_settings -> results_dir(at the top level) - This would trigger an error here
hvpeteet
Feb 5, 2021
Contributor
Thanks, we can check for it and only overwrite it if already set then. When you check this in can you file a ticket against me to make sure we get this added at some point?
Thanks, we can check for it and only overwrite it if already set then. When you check this in can you file a ticket against me to make sure we get this added at some point?
chriselion
Feb 5, 2021
Author
Collaborator
Will do
Will do
Proposed change(s)
Adds a setuptools-based plugin system, and an interface for registering custom StatsWriters. Also refactored some of the output directory settings so that TensorBoardWriter just needs the RunOptions.
For more background, see this video and example code.
Example output:
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
https://github.com/anthonywritescode/explains/tree/master/sample_code/ep128
https://www.youtube.com/watch?v=fY3Y_xPKWNA
Types of change(s)
Checklist