Feature/eval intervals #323
Conversation
…re/eval-intervals
… mypy and numpy versions.
KaleabTessera
requested review from
arnupretorius and
DriesSmit
as code owners
There was a problem hiding this comment.
Thanks @KaleabTessera! Going to be super useful for comparisons. 🔥
Just left a few minor suggestions.
mava/environment_loop.py
Outdated
| counts = self._counter.get_counts() | ||
| return counts | ||
|
|
||
| def record_counts(self, episode_steps: int) -> Any: |
There was a problem hiding this comment.
I think we should make the function return type int.
There was a problem hiding this comment.
So I updated this. It returns a counting.Counter object.
mava/environment_loop.py
Outdated
| @@ -338,6 +348,36 @@ def _compute_episode_statistics( | |||
| ) -> None: | |||
| pass | |||
|
|
|||
| def get_counts(self) -> Any: | |||
There was a problem hiding this comment.
Same as comment below.
There was a problem hiding this comment.
Updated to counting.Counter.
| @@ -338,6 +348,36 @@ def _compute_episode_statistics( | |||
| ) -> None: | |||
| pass | |||
|
|
|||
| def get_counts(self) -> Any: | |||
| if hasattr(self._executor, "_counts"): | |||
There was a problem hiding this comment.
I didn't see this being used anywhere? I.e. executor examples that have this attribute. What is the use case for this?
There was a problem hiding this comment.
So this is for systems that haven't been moved to mava scaling yet. These systems use _counts .
|
|
||
| # We need to get the latest counts if we are using eval intervals. | ||
| if environment_loop_schedule: | ||
| self._executor.update() |
There was a problem hiding this comment.
Is this update not already performed in the run_episode call? Why are we doing it again here? If we want to force an update (for evironment_loop_schedule == True) we need to change the variable client update rate for the evaluator, right?
There was a problem hiding this comment.
So this update is important to get the latest counts.
Since these loops run on different processes, while the evaluator is waiting, it doesn't run the loop and so it vars counts don't get updated (this part doesn't run).
So while the evaluator is waiting, it still need to call self._executor.update() to get latest counts from the executor runs.
Not sure if that makes sense?
There was a problem hiding this comment.
I see. That makes sense yes. Should there not maybe be a time.sleep somewhere in here? This is so that the evaluator does not constantly speak to the variable_server while waiting. And also self._executor.update() only updates the executor every 1000 steps right? So we maybe need to change that setting to 1 in the case of using an evaluator that has waiting intervals?
There was a problem hiding this comment.
Thanks @KaleabTessera the changes looks great 🚀 Just see my few comments.
|
From my side this can be merged in. The issues with the github actions/ package versions are resolved in this PR - #310. |
| from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple, Union | ||
|
|
||
| import sonnet as snt | ||
| import tensorflow as tf | ||
| import trfl | ||
|
|
||
|
|
||
| def non_blocking_sleep(time_in_seconds: int) -> None: |
Co-authored-by: Dries <[email protected]>
Co-authored-by: Dries <[email protected]>
Co-authored-by: Dries <[email protected]>
Co-authored-by: Dries <[email protected]>
Co-authored-by: Dries <[email protected]>
What?
Why?
How?
Extra
mava-scalingand not intodevelop.