Skip to content

[refactor] Move configuration files to single YAML file #3791

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

Merged
merged 35 commits into from
Apr 29, 2020

Conversation

ervteng
Copy link
Contributor

@ervteng ervteng commented Apr 16, 2020

Proposed change(s)

This PR refactors the YAML configuration files into a per-environment format. Each YAML now contains both the behavior configurations/hyperparameters, as well as curriculum and parameter variation configurations. The example YAMLs were reformatted to reflect this.

To be done in this PR:

  • Documentation changes

Not done in this PR:

  • Move engine configurations and other CLI options (num_envs, env, seed) into YAML files as well
  • Save out final YAML file.

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

https://docs.google.com/document/d/1uyk5JVNevfWy2DmqVHy19lcDzyQCNppPxkPOgJcQ7z4/edit#heading=h.sozm3sblhtke

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

@ervteng
Copy link
Contributor Author

ervteng commented Apr 17, 2020

@anupambhatnagar - this will definitely break daily CI - what steps should I take to adapt the ml-agents-cloud utility to match?

@ervteng ervteng requested a review from awjuliani April 17, 2020 22:39
@ervteng ervteng marked this pull request as ready for review April 17, 2020 22:40
Copy link
Contributor

@awjuliani awjuliani left a comment

Choose a reason for hiding this comment

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

How did you generate all of these files. I worry now about keeping track of changing defaults. I know that moving the defaults into code is out of the scope, but I just want to make sure we aren't signing up for the extra work that might be associated with managing multiple dozen config files now.

I also remember us talking about separating the YAML behavior section into a trainer and model sub-sections. Is that out of scope as well, or has the plan changed? I think it would definitely improve interpretability.

@ervteng
Copy link
Contributor Author

ervteng commented Apr 20, 2020

@awjuliani I still want to split the YAML to trainer and model. With luck that will be done after the other CLI and output file changes are done.

The files were generated using a script that loaded the old config's default and overrode them using the config, then outputted the new YAML. I'm also worried about the files diverging before we are able to create the default values in code. This PR is technically still compatible with the old config files (as long as you add behaviors:) to it.

Would it be a good idea to have an intermediary step that uses the old trainer_config and only migrates generalization/curriculum? We can then migrate the others when the defaults are properly codified.

Copy link

@anupam-142857 anupam-142857 left a comment

Choose a reason for hiding this comment

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

looks good. I recommend running it on one scene with ml-agents-cloud to test the new workflow.

@ervteng ervteng merged commit 7439038 into master Apr 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-single-config branch April 29, 2020 23:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2021
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.

4 participants