Accumulating loggers #135
Accumulating loggers #135
Conversation
Codecov Report
@@ Coverage Diff @@
## master #135 +/- ##
==========================================
+ Coverage 86.83% 87.41% +0.57%
==========================================
Files 63 62 -1
Lines 4468 4465 -3
==========================================
+ Hits 3880 3903 +23
+ Misses 588 562 -26
Continue to review full report at Codecov.
|
…tion into accumulating_log
shwang
force-pushed
the
accumulating_log
branch
from
0930cf9 to
5418c9f
Compare
| def logkv(self, key, val): | ||
| if self.current_logger is not None: | ||
| assert self._subdir is not None | ||
| raw_key = os.path.join("raw", self._subdir, key) |
There was a problem hiding this comment.
Maybe this is bike-shedding a little, but is it really necessary to have the raw/{subdir}/ prefix on all of the entries in the self.current_logger log? As I understand it we'll end up with a bunch of log files in which all log entries start with raw/subdir/, which would be redundant given that you can infer what kind of entries they are from the directory hierarchy (i.e. the raw log file is already in a raw/subdir folder).
(overall I still like the design!)
There was a problem hiding this comment.
Adam thought this was weird too, we had a comment thread about this here.
Reasons for redundant-looking prefix:
(1) So that in Tensorboard plots, mean plots are automatically separated from raw plots (which have a different x axis). (TB plots are grouped by key name).
(2) To distinguish between raw and mean keys in stdout logging (otherwise they look identical).
(1) is my primary motivation. Adam was convinced by (2). Though now that I think about it, it might be better to just turn off raw stdout logs and show mean stdout logs only.
src/imitation/util/logger.py
Outdated
| else: | ||
| return self.default_logger | ||
|
|
||
| def logkv_mean(self, key, val): |
There was a problem hiding this comment.
Will this nest correctly? e.g. say I was in a discriminator context ("disc") and I did .logkv_mean("foo", <value>) J times, did a .dumpkvs(), then did .logkv_mean() J more times, and repeated that process K times. If I looked at the raw logs, I'd expect them to contain K entries of the form raw/disc/foo: <mean of J values>, while in the logs for the parent, I'd expect one entry of the form mean/disc/foo: <mean of J*K values>. As I understand it, the current implementation will write straight through to the "raw" child logger without putting anything in the parent, and without writing any key prefixes. Perhaps it's safer to remove this implementation & raise NotImplementedError() instead.
There was a problem hiding this comment.
Oh, on reflection the correct thing to do is probably raise NotImplementedError when self.current_logger is not None, else pass through to self.default_logger.
There was a problem hiding this comment.
I haven't thought too carefully about the inside-context logkv_mean case.
I'm a bit hesitant about raising NotImplementedError while inside the generator context. What if gen_policy.learn() (ie some arbitrary Stable Baselines algorithm) wants to call logkv_mean for some reason?.
There was a problem hiding this comment.
As I understand it, the current implementation will write straight through to the "raw" child logger without putting anything in the parent, and without writing any key prefixes.
This understanding seems correct, thanks for catching this
There was a problem hiding this comment.
@qxcv
How does this solution sound? Because we don't want to error out if some third-party package calls log or logkv_mean inside the context, and HierachicalLogger only claims to modify logkv behavior, we can just
(1) redirect log, logkv_mean to the default logger, unchanged and without prefixes and
(2) note this prominently in the documentation?
src/imitation/util/logger.py
Outdated
| self._logger.dumpkvs() | ||
|
|
||
| def log(self, *args, **kwargs): | ||
| self._logger.log(*args, **kwargs) |
There was a problem hiding this comment.
Same comment as above; this won't do the expected name prefixing. Perhaps just prohibit calls to this with a NotImplementedError?
There was a problem hiding this comment.
Response in #135 (comment)
|
Thanks for the review. I've pushed my proposed fixes here |
Add mean-accumulating log context for tracking discrim and generator stats together even though they are trained serially and with different numbers of kv logs.
Probably should forward_AccumulatingLogger.logkv_meanas well.