-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
setuptools-based plugin for StatsWriters #4788
Conversation
@@ -0,0 +1,11 @@ | |||
from setuptools import setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No plans to publish this package, but we could use it to set up tests, e.g. for bad imports.
ml-agents/setup.py
Outdated
], | ||
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": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The form of this is {entry point name}={plugin module}:{plugin_function}
docs/Training-Plugins.md
Outdated
"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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the see below a link to line 41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
argparser.add_argument( | ||
"--results-dir", default="results", help="Results base directory" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this relevant to this PR? Why are we adding this flag ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user changes this from the default results
will it mess with how they're saved/synced in cloud? cc: @hvpeteet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It would be
checkpoint_settings -> results_dir
(at the top level) - This would trigger an error here
def check_and_structure(key: str, value: Any, class_type: type) -> Any:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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