Skip to content
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

[FIX] prioritize cli options over yaml #5495

Merged
merged 8 commits into from
Aug 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion com.unity.ml-agents/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ and this project adheres to
- Fixed a bug in multi-agent cooperative training where agents might not receive all of the states of
terminated teammates. (#5441)
- Fixed wrong attribute name in argparser for torch device option (#5433)(#5467)

- Fixed conflicting CLI and yaml options regarding resume & initialize_from (#5495)
## [2.1.0-exp.1] - 2021-06-09
### Minor Changes
#### com.unity.ml-agents / com.unity.ml-agents.extensions (C#)
Expand Down
29 changes: 28 additions & 1 deletion ml-agents/mlagents/trainers/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,7 @@ def __missing__(self, key: Any) -> "TrainerSettings":
f"Please add an entry in the configuration file for {key}, or set default_settings."
)
else:
logger.warn(
logger.warning(
f"Behavior name {key} does not match any behaviors specified "
f"in the trainer configuration file. A default configuration will be used."
)
Expand Down Expand Up @@ -769,6 +769,32 @@ def maybe_init_path(self) -> Optional[str]:
def run_logs_dir(self) -> str:
return os.path.join(self.write_path, "run_logs")

def prioritize_resume_init(self) -> None:
"""Prioritize explicit command line resume/init over conflicting yaml options.
if both resume/init are set at one place use resume"""
_non_default_args = DetectDefault.non_default_args
if "resume" in _non_default_args:
if self.initialize_from is not None:
logger.warning(
f"Both 'resume' and 'initialize_from={self.initialize_from}' are set!"
f" Current run will be resumed ignoring initialization."
)
self.initialize_from = parser.get_default("initialize_from")
elif "initialize_from" in _non_default_args:
if self.resume:
logger.warning(
f"Both 'resume' and 'initialize_from={self.initialize_from}' are set!"
f" {self.run_id} is initialized_from {self.initialize_from} and resume will be ignored."
)
self.resume = parser.get_default("resume")
elif self.resume and self.initialize_from is not None:
# no cli args but both are set in yaml file
logger.warning(
f"Both 'resume' and 'initialize_from={self.initialize_from}' are set in yaml file!"
f" Current run will be resumed ignoring initialization."
)
self.initialize_from = parser.get_default("initialize_from")


@attr.s(auto_attribs=True)
class EnvironmentSettings:
Expand Down Expand Up @@ -888,6 +914,7 @@ def from_argparse(args: argparse.Namespace) -> "RunOptions":
configured_dict[key] = val

final_runoptions = RunOptions.from_dict(configured_dict)
final_runoptions.checkpoint_settings.prioritize_resume_init()
# Need check to bypass type checking but keep structure on dict working
if isinstance(final_runoptions.behaviors, TrainerSettings.DefaultTrainerDict):
# configure whether or not we should require all behavior names to be found in the config YAML
Expand Down
7 changes: 6 additions & 1 deletion ml-agents/mlagents/trainers/tests/test_learn.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ def test_commandline_args(mock_file):
full_args = [
"mytrainerpath",
"--env=./myenvfile",
"--resume",
"--inference",
"--run-id=myawesomerun",
"--seed=7890",
Expand All @@ -156,6 +155,12 @@ def test_commandline_args(mock_file):
assert opt.engine_settings.no_graphics is True
assert opt.debug is True
assert opt.checkpoint_settings.inference is True
assert opt.checkpoint_settings.resume is False

# ignore init if resume is set
full_args.append("--resume")
opt = parse_command_line(full_args)
assert opt.checkpoint_settings.initialize_from is None # ignore init if resume set
assert opt.checkpoint_settings.resume is True

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test to check if they were both set in the YAML but not the CLI? I think we can do this in the test_settings.py.


Expand Down
4 changes: 4 additions & 0 deletions ml-agents/mlagents/trainers/tests/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,10 @@ def test_exportable_settings(use_defaults):
# Check that the two exports are the same
assert dict_export == second_export

# check if cehckpoint_settings priorotizes resume over initialize from
run_options2.checkpoint_settings.prioritize_resume_init()
assert run_options2.checkpoint_settings.initialize_from is None


def test_environment_settings():
# default args
Expand Down