collecting latest step as a stat #5264

maryamhonari · 2021-04-15T01:04:47Z

Proposed change(s)

Reporting the latest step as a separate stat.
A new property in TensorboardWriter to hide irrelevant stats based on their key

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

Types of change(s)

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

CLAassistant · 2021-04-15T01:04:52Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

mahon94 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

maryamhonari · 2021-04-15T01:09:33Z

ml-agents/mlagents/trainers/trainer/rl_trainer.py

@@ -209,6 +209,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))


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

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.

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)

…stats from user

ml-agents/mlagents/trainers/stats.py

ervteng

Minor comments on typing but otherwise LGTM

Co-authored-by: Ervin T. <[email protected]>

…evelop-report-step-in-trainer

ml-agents/mlagents/trainers/stats.py

vincentpierre · 2021-04-17T02:13:57Z

ml-agents/mlagents/plugins/stats_writer.py

@@ -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"],


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

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?

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

ml-agents/mlagents/trainers/stats.py

-format imports and comments

* collecting latest step as a stat * adding a list of hidden_keys to TB summarywriter to hide unnecessary stats from user * fixing precommit * fixing precommit * formating * defined the property types * moving custom defaults to get_default_stats_writers * new test for TensorboardWriter.hidden_keys * improved testing * explicit None evaluation Co-authored-by: Ervin T. <[email protected]> * make hidden_keys optional Co-authored-by: Ervin T. <[email protected]> * adding optional argument * lowering the training threshold to 0.8 on test_var_len_obs_and_goal_poca * Update pytest.yml * Do not merge! droping pytest 3.9 job * -add back pytest -format imports and comments * back to default threshold for test_var_len_obs_and_goal_poca Co-authored-by: mahon94 <[email protected]> Co-authored-by: Ervin T. <[email protected]>

* collecting latest step as a stat * adding a list of hidden_keys to TB summarywriter to hide unnecessary stats from user * fixing precommit * formating * defined the property types * moving custom defaults to get_default_stats_writers * new test for TensorboardWriter.hidden_keys * improved testing * explicit None evaluation Co-authored-by: Ervin T. <[email protected]> * make hidden_keys optional Co-authored-by: Ervin T. <[email protected]> * adding optional argument * lowering the training threshold to 0.8 on test_var_len_obs_and_goal_poca * Update pytest.yml * Do not merge! droping pytest 3.9 job * -add back pytest -format imports and comments * back to default threshold for test_var_len_obs_and_goal_poca Co-authored-by: mahon94 <[email protected]> Co-authored-by: Ervin T. <[email protected]> Co-authored-by: mahon94 <[email protected]> Co-authored-by: Ervin T. <[email protected]>

collecting latest step as a stat

0c95181

maryamhonari requested a review from ervteng April 15, 2021 01:04

maryamhonari commented Apr 15, 2021

View reviewed changes

adding a list of hidden_keys to TB summarywriter to hide unnecessary …

e9e6fea

…stats from user

maryamhonari requested a review from sini April 15, 2021 21:57

maryamhonari added 5 commits April 15, 2021 15:10

fixing precommit

1bef8cd

fixing precommit

f2fe073

fixing precommit2

2079450

formating

c93b64a

defined the property types

7828e6e

sini reviewed Apr 16, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Outdated Show resolved Hide resolved

maryamhonari added 3 commits April 15, 2021 20:59

moving custom defaults to get_default_stats_writers

e39ea76

new test for TensorboardWriter.hidden_keys

ec9885b

improved testing

e36a990

ervteng reviewed Apr 16, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Outdated Show resolved Hide resolved

ervteng reviewed Apr 16, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Outdated Show resolved Hide resolved

ervteng approved these changes Apr 16, 2021

View reviewed changes

maryamhonari and others added 7 commits April 16, 2021 10:31

explicit None evaluation

a29f49b

Co-authored-by: Ervin T. <[email protected]>

make hidden_keys optional

a231ba5

Co-authored-by: Ervin T. <[email protected]>

adding optional argument

71b545f

Merge branch 'main' of github.com:Unity-Technologies/ml-agents into d…

6f3bcbe

…evelop-report-step-in-trainer

lowering the training threshold to 0.8 on test_var_len_obs_and_goal_poca

ba862d5

Update pytest.yml

5657b05

Do not merge! droping pytest 3.9 job

daedc45

sini reviewed Apr 17, 2021

View reviewed changes

ml-agents/mlagents/trainers/stats.py Outdated Show resolved Hide resolved

vincentpierre reviewed Apr 17, 2021

View reviewed changes

maryamhonari added 2 commits April 19, 2021 09:03

-add back pytest

a03642f

-format imports and comments

back to default threshold for test_var_len_obs_and_goal_poca

38077f1

sini approved these changes Apr 19, 2021

View reviewed changes

maryamhonari merged commit e695fc1 into main Apr 19, 2021

delete-merged-branch bot deleted the develop-report-step-in-trainer branch April 19, 2021 18:21

maryamhonari mentioned this pull request Apr 19, 2021

[Cherry-pick for release] collecting latest step as a stat (#5264) #5295

Merged

10 tasks

github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collecting latest step as a stat #5264

collecting latest step as a stat #5264

maryamhonari commented Apr 15, 2021 •

edited

Loading

CLAassistant commented Apr 15, 2021

maryamhonari Apr 15, 2021

ervteng Apr 15, 2021

ervteng Apr 15, 2021

ervteng left a comment

vincentpierre Apr 17, 2021

sini Apr 17, 2021 •

edited

Loading

vincentpierre Apr 19, 2021

collecting latest step as a stat #5264

collecting latest step as a stat #5264

Conversation

maryamhonari commented Apr 15, 2021 • edited Loading

Proposed change(s)

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

Types of change(s)

Checklist

Other comments

CLAassistant commented Apr 15, 2021

maryamhonari Apr 15, 2021

Choose a reason for hiding this comment

ervteng Apr 15, 2021

Choose a reason for hiding this comment

ervteng Apr 15, 2021

Choose a reason for hiding this comment

ervteng left a comment

Choose a reason for hiding this comment

vincentpierre Apr 17, 2021

Choose a reason for hiding this comment

sini Apr 17, 2021 • edited Loading

Choose a reason for hiding this comment

vincentpierre Apr 19, 2021

Choose a reason for hiding this comment

maryamhonari commented Apr 15, 2021 •

edited

Loading

sini Apr 17, 2021 •

edited

Loading