Skip to content

collecting latest step as a stat #5264

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 19 commits into from
Apr 19, 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
1 change: 1 addition & 0 deletions ml-agents/mlagents/plugins/stats_writer.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def get_default_stats_writers(run_options: RunOptions) -> List[StatsWriter]:
TensorboardWriter(
checkpoint_settings.write_path,
clear_past_data=not checkpoint_settings.resume,
hidden_keys=["Is Training", "Step"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of the magic strings here. I think these would be better as a static frozenset() in TensorBoardWriter.
Any thoughts?

Copy link
Contributor

@sini sini Apr 17, 2021

Choose a reason for hiding this comment

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

I encouraged @mahon94 to remove it as a magic string from the TensorboardWriter. My thinking is that the writer has no responsibility for the implementation of the trainer or its stats and as such configuration/behavior should be removed from the details of said implementation. The get_default_stats_writers in stats_writer.py plugin on the otherhand is more closely related to the main entry-point/execution of the trainer. et

If it wasn't mutable, I'd say this should just be the default value of TensorboardWriter param and call it a day. Transforming it from a list to a frozenset resolves the mutability issue -- even if it's still a collection of magic strings in the writer. Do you have opinions on the coupling implications?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion. I am okay leaving it as is.

),
GaugeWriter(),
ConsoleWriter(),
Expand Down
15 changes: 12 additions & 3 deletions ml-agents/mlagents/trainers/stats.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from collections import defaultdict
from enum import Enum
from typing import List, Dict, NamedTuple, Any
from typing import List, Dict, NamedTuple, Any, Optional
import numpy as np
import abc
import os
Expand All @@ -14,7 +14,6 @@
from torch.utils.tensorboard import SummaryWriter
from mlagents.torch_utils.globals import get_rank


logger = get_logger(__name__)


Expand Down Expand Up @@ -212,24 +211,34 @@ def add_property(


class TensorboardWriter(StatsWriter):
def __init__(self, base_dir: str, clear_past_data: bool = False):
def __init__(
self,
base_dir: str,
clear_past_data: bool = False,
hidden_keys: Optional[List[str]] = None,
):
"""
A StatsWriter that writes to a Tensorboard summary.

:param base_dir: The directory within which to place all the summaries. Tensorboard files will be written to a
{base_dir}/{category} directory.
:param clear_past_data: Whether or not to clean up existing Tensorboard files associated with the base_dir and
category.
:param hidden_keys: If provided, Tensorboard Writer won't write statistics identified with these Keys in
Tensorboard summary.
"""
self.summary_writers: Dict[str, SummaryWriter] = {}
self.base_dir: str = base_dir
self._clear_past_data = clear_past_data
self.hidden_keys: List[str] = hidden_keys if hidden_keys is not None else []

def write_stats(
self, category: str, values: Dict[str, StatsSummary], step: int
) -> None:
self._maybe_create_summary_writer(category)
for key, value in values.items():
if key in self.hidden_keys:
continue
self.summary_writers[category].add_scalar(
f"{key}", value.aggregated_value, step
)
Expand Down
25 changes: 25 additions & 0 deletions ml-agents/mlagents/trainers/tests/test_stats.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,31 @@ def test_tensorboard_writer_clear(tmp_path):
assert len(os.listdir(os.path.join(tmp_path, "category1"))) == 1


@mock.patch("mlagents.trainers.stats.SummaryWriter")
def test_tensorboard_writer_hidden_keys(mock_summary):
# Test write_stats
category = "category1"
with tempfile.TemporaryDirectory(prefix="unittest-") as base_dir:
tb_writer = TensorboardWriter(
base_dir, clear_past_data=False, hidden_keys="hiddenKey"
)
statssummary1 = StatsSummary(
full_dist=[1.0], aggregation_method=StatsAggregationMethod.AVERAGE
)
tb_writer.write_stats("category1", {"hiddenKey": statssummary1}, 10)

# Test that the filewriter has been created and the directory has been created.
filewriter_dir = "{basedir}/{category}".format(
basedir=base_dir, category=category
)
assert os.path.exists(filewriter_dir)
mock_summary.assert_called_once_with(filewriter_dir)

# Test that the filewriter was not written to since we used the hidden key.
mock_summary.return_value.add_scalar.assert_not_called()
mock_summary.return_value.flush.assert_not_called()


def test_gauge_stat_writer_sanitize():
assert GaugeWriter.sanitize_string("Policy/Learning Rate") == "Policy.LearningRate"
assert (
Expand Down
1 change: 1 addition & 0 deletions ml-agents/mlagents/trainers/trainer/rl_trainer.py
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ def _increment_step(self, n_steps: int, name_behavior_id: str) -> None:
p = self.get_policy(name_behavior_id)
if p:
p.increment_step(n_steps)
self.stats_reporter.set_stat("Step", float(self.get_step))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ervteng Is there a place that I can collect more granular/frequent step updates?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the right place if we care about synchronizing with the other metrics.

The other place would be in the AgentProcessor, but that is a bit decoupled from the other metrics as it would be added pre-trajectory assembly.

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point (e.g. if the inference and training are separate processes) it might be useful to emit both as different stats (e.g. steps_executed and steps_processed)


def _get_next_interval_step(self, interval: int) -> int:
"""
Expand Down