[debug] Require all behavior names to have a matching YAML entry #5210
Conversation
This file contains 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 2 commits
April 1, 2021 17:41
ervteng
changed the title
Comment on lines
862
to
863
| # If we're loading for a file, make sure we have strict on | ||
| configured_dict["strict"] = True |
There was a problem hiding this comment.
Would it be simpler to remove these two lines and make strict True by default rather than False line 817 ?
| @@ -800,6 +814,7 @@ class RunOptions(ExportableSettings): | |||
| # These are options that are relevant to the run itself, and not the engine or environment. | |||
| # They will be left here. | |||
| debug: bool = parser.get_default("debug") | |||
| strict: bool = False | |||
There was a problem hiding this comment.
Can you add a little comment here on what strict does?
vincentpierre
approved these changes
Apr 13, 2021
added 9 commits
April 13, 2021 14:22
vincentpierre
approved these changes
Apr 19, 2021
ervteng
pushed a commit
that referenced
this pull request
Apr 20, 2021
* Add strict check to settings.py * Remove warning from trainer factory, add test * Add changelog * Fix test * Update changelog * Remove strict CLI options * Remove strict option, rename, make strict default * Remove newline * Update comments * Set default dict to actually default to a default dict * Fix tests * Fix tests again * Default trainer dict to requiring all fields * Fix settings typing * Use logger * Add default_settings to error (cherry picked from commit 86a4070)
10 tasks
ervteng
pushed a commit
that referenced
this pull request
Apr 22, 2021
…) (#5296) * Add strict check to settings.py * Remove warning from trainer factory, add test * Add changelog * Fix test * Update changelog * Remove strict CLI options * Remove strict option, rename, make strict default * Remove newline * Update comments * Set default dict to actually default to a default dict * Fix tests * Fix tests again * Default trainer dict to requiring all fields * Fix settings typing * Use logger * Add default_settings to error (cherry picked from commit 86a4070)
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)
For most cases, require that the behavior name be specified in the RunOptions, or that
default_settingsis set. The exception is the case that no YAML is specified at all; then, use default parameters for all settings.Note that the Changelog conflict was merged assuming this would be cherry-picked into R17.
Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)
JIRA MLA-1865
Types of change(s)
Checklist
Other comments