New Logger by pelillian · Pull Request #464 · rlworkgroup/garage

pelillian · 2019-01-18T01:19:39Z

Created new logger API & updated all references to use the new system.

Many commits were required to be squashed into 96afc22 for rebasing

pelillian · 2019-01-18T01:22:56Z

codecov · 2019-01-18T01:46:30Z

Codecov Report

Merging #464 into master will decrease coverage by 0.45%.
The diff coverage is 57.43%.

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
- Coverage    62.9%   62.44%   -0.46%     
==========================================
  Files         182      185       +3     
  Lines       12169    11927     -242     
==========================================
- Hits         7655     7448     -207     
+ Misses       4514     4479      -35

Impacted Files	Coverage Δ
garage/config.py	`80.43% <ø> (-0.42%)`	⬇️
garage/misc/autoargs.py	`32% <0%> (ø)`	⬆️
garage/logger/utils.py	`0% <0%> (ø)`
garage/logger/__init__.py	`100% <100%> (ø)`
garage/sampler/parallel_sampler.py	`65.67% <100%> (-0.51%)`	⬇️
garage/tf/algos/batch_polopt.py	`93.54% <100%> (-0.07%)`	⬇️
garage/algos/cem.py	`94.44% <100%> (-0.11%)`	⬇️
garage/envs/mujoco/hill/hill_env.py	`84.93% <100%> (ø)`	⬆️
garage/tf/regressors/gaussian_mlp_regressor.py	`79.5% <100%> (-0.34%)`	⬇️
garage/tf/algos/npo.py	`99.01% <100%> (-0.04%)`	⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33a03e...4975623. Read the comment docs.

coveralls · 2019-01-18T01:48:36Z

Coverage decreased (-0.2%) to 63.144% when pulling 3625e64 on newer-logger into 369d5b1 on master.

pelillian · 2019-01-18T01:58:39Z

@ryanjulian
For some reason Travis is giving me the following errors:
E0401: 8,0: Unable to import 'theano.tensor'
E0401: 16,0: Unable to import 'baselines.bench'

Any ideas what's going on?

naeioi · 2019-01-18T02:07:16Z

Could you give a brief on the design of new logger and also an example script to illustrate the basic usage?

CatherineSue · 2019-01-18T03:42:31Z

@ryanjulian
For some reason Travis is giving me the following errors:
E0401: 8,0: Unable to import 'theano.tensor'
E0401: 16,0: Unable to import 'baselines.bench'

Any ideas what's going on?

I have met the similar error here. But I met this error in local pre-commit. I updated the garage env then solved the problem.

naeioi · 2019-01-18T04:06:41Z

@ryanjulian
For some reason Travis is giving me the following errors:
E0401: 8,0: Unable to import 'theano.tensor'
E0401: 16,0: Unable to import 'baselines.bench'
Any ideas what's going on?

I have met the similar error here. But I met this error in local pre-commit. I updated the garage env then solved the problem.

Can you elaborate on how to update garage env? Do you mean reinstall deps in environment.yml?

CatherineSue · 2019-01-18T05:00:55Z

@ryanjulian
For some reason Travis is giving me the following errors:
E0401: 8,0: Unable to import 'theano.tensor'
E0401: 16,0: Unable to import 'baselines.bench'
Any ideas what's going on?

I have met the similar error here. But I met this error in local pre-commit. I updated the garage env then solved the problem.

Can you elaborate on how to update garage env? Do you mean reinstall deps in environment.yml?

conda env update -n garage

krzentner

This is a pretty large change, but mostly not scary.
You added a bunch of noqa, which kinda defeats the point of using flake8. I was going to tell you to fix them, but then I found that they mostly don't appear to be necessary.

I have good tooling to fix this, so I just added a commit which fixes them. Please pull it before making more changes. I didn't fix the test/ directory, since that didn't really seem as important.

The rest of the comments are hopefully pretty minor? Let me know if you have questions, as always.

jonashen

Just a general question, is there a reason why we have logger under misc instead of it being standalone? i.e. garage.misc.logger instead of garage.logger?

pelillian · 2019-01-21T04:42:21Z

Just a general question, is there a reason why we have logger under misc instead of it being standalone? i.e. garage.misc.logger instead of garage.logger?

@krzentner @ryanjulian What do you think about this?

pelillian · 2019-01-21T04:45:27Z

This is a pretty large change, but mostly not scary.
You added a bunch of noqa, which kinda defeats the point of using flake8. I was going to tell you to fix them, but then I found that they mostly don't appear to be necessary.

I have good tooling to fix this, so I just added a commit which fixes them. Please pull it before making more changes. I didn't fix the test/ directory, since that didn't really seem as important.

The rest of the comments are hopefully pretty minor? Let me know if you have questions, as always.

@krzentner
Thanks a million! What tooling do you use, if you don't mind my asking? I had tried autopep8 and a couple other tools, yet they seem to differ somewhat from what garage wants.

ryanjulian · 2019-01-21T19:20:23Z

Just a general question, is there a reason why we have logger under misc instead of it being standalone? i.e. garage.misc.logger instead of garage.logger?

@krzentner @ryanjulian What do you think about this?

Put it in garage.logger

ryanjulian · 2019-01-21T19:20:58Z

This is a pretty large change, but mostly not scary.
You added a bunch of noqa, which kinda defeats the point of using flake8. I was going to tell you to fix them, but then I found that they mostly don't appear to be necessary.
I have good tooling to fix this, so I just added a commit which fixes them. Please pull it before making more changes. I didn't fix the test/ directory, since that didn't really seem as important.
The rest of the comments are hopefully pretty minor? Let me know if you have questions, as always.

@krzentner
Thanks a million! What tooling do you use, if you don't mind my asking? I had tried autopep8 and a couple other tools, yet they seem to differ somewhat from what garage wants.

You have to make sure to use our configurations with automated tools. They live in setup.cfg

krzentner · 2019-01-21T21:34:08Z

@krzentner
Thanks a million! What tooling do you use, if you don't mind my asking? I had tried autopep8 and a couple other tools, yet they seem to differ somewhat from what garage wants.

I'm not sure if there's any fully automated tooling which works with our current linters. My workflow (which I've used across lots of environments / teams / companies) is basically:

Run the linter to find failures (e.g. flake8).
Filter them down to a single failure type (e.g. flake8 --select=F401).
Pipe them all into (n)vim (e.g. flake8 --format="+%(row)s %(path)s" --select=F401 | xargs -L 1 nvim).
Write a vim macro which fixes the error.
Spam the macro and :x or :bNext until all the files are fixed.

Obviously, a fully automated tooling solution is preferred, but this workflow has served me well for making formatting fixes and refactorings.

* Renames TensorBoard logging files and tests to follow convention * Changes Histogram* types in garage.logger.tensorboard_inputs to simpler names in garage.logger.distributions

* Move LogOutput base class to `garage.logger.logger`, so that the entire API is in one place * Rename `garage.logger.outputs` to `simple_outputs` to better reflect its purpose

* Moves tests for simple_outputs to test_simple_outputs * Makes proper use of temporary files

* Replace TensorBoardOutput with tensorboardX. * Add unittest for TensorBoardOutput. * Delete tensor log as it is too complex and not used anywhere right now. * Rebase with origin/HEAD.

* garage.experiment.run_experiment didn't add `StdOutput`. Fixed. * `TextOutput` doesn't support `TabularInput`. This would make `TextOutput` useless as it doesn't log algorithm information any more. Therefore I prefer we added `TabularInput` in `TextOutput`. * Sort tabular in alphabetical order. * Fix that `TabularInput.as_primitive_dict` doesn't log `np.float32`.

* snapshotter fails because `Snapshotter._snapshot_dir=None` and `snapshot_dir.setter` calls `mkdir_p` which doesn't support NoneType. Fixed this.

* Rewrites parts of TensorBoardLogger * Tests TensorBoardLogger with mocks * Makes proper use of the dump() API * Modifies LocalRunner to use dump() * Updates tests which no longer need the logger * Updates test fixtures to use NullOutput * Add TabularInput support to TextOutput * Detects and warns when TabularInput values are not logged * Move NullOutput to tests.fixtures

* Refactor Snapshotter unittests * Delete duplicate log line in tf/DDPG * Fix flake8 complaints * Fix CsvOutput bug. For some off policy algorithms that use replay buffer, there is usually a min buffer size. It means the algorithms won't start training until it has sampled min-buffer-size samples. But CsvOutput set `CsvOutput._fieldnames` from the beginning, which is empty in this case. Later when data is not empty, it will raise Inconsistent TabularInput keys warning. Fix this by only start to set `CsvOutput._writer` when `to_csv.keys()` is not None.

* Add more CsvOutput and Logger tests. The logger module now reaches 98% coverage. * Delete `with tf.Session()` in TensorboardOutput test as a session is created in setUp. * There is a bug in TfTestCase and TfGraphTestVase that leaves dangling tf.Session. tf.Session.close() releases all the resources associated within the session. However, the default session context manage still refers to the session. Thus, when setUp is called next time, it creates another session. As a result, the graph is nested. And when tests.garage.experiment.test_snapshit.reset_tf() is called, it only exits the most nestted session. Then it calls tf.reset_default_graph() and raises error because the graph is still nested. An easy fix would be adding `self.sess.__exit__()` to TfTestCase.tearDown(). However, it gets mixed up with LocalRunner. When we pass a `self.sess` to LocalTfRunner, LocalTfRunner will set the sess as the default session. And delete it when LocalTfRunner exits. Then we can't call `self.sess.__exit__()` anymore as it has already been deleted. To fix this, I assume that we always use LocalTfRunner along with the with statement, and I used an if check to decide whether call `self.sess.__exit__()` or not.

ryanjulian · 2019-03-22T23:40:36Z

+    def __init__(self, file_name, with_timestamp=True):
+        super().__init__(file_name, 'a')
+        self._with_timestamp = with_timestamp
+        self._delimiter = " | "


single quote

ryanjulian · 2019-03-22T23:40:48Z

+
+    def __init__(self, file_name, mode='w'):
+        mkdir_p(os.path.dirname(file_name))
+        self._log_file = open(file_name,


probably less awkward for the comment to go above this line

ryanjulian

Great work, everyone

pelillian requested a review from a team as a code owner January 18, 2019 01:19

pelillian requested review from a user, naeioi and ryanjulian January 18, 2019 01:20

pelillian added the logging label Jan 18, 2019

pelillian mentioned this pull request Jan 18, 2019

Rewrite the logger #33

Closed

CatherineSue reviewed Jan 18, 2019

View reviewed changes

Comment thread garage/envs/mujoco/randomization/variation.py Outdated

pelillian requested a review from krzentner January 18, 2019 02:13

krzentner requested changes Jan 18, 2019

View reviewed changes

jonashen reviewed Jan 19, 2019

View reviewed changes

ryanjulian reviewed Jan 21, 2019

View reviewed changes

Comment thread garage/misc/logger/__init__.py Outdated

ryanjulian reviewed Jan 21, 2019

View reviewed changes

Comment thread garage/misc/logger/logger_outputs.py Outdated

ryanjulian reviewed Jan 21, 2019

View reviewed changes

pelillian requested review from krzentner and ryanjulian January 22, 2019 00:16

pelillian self-assigned this Jan 22, 2019

ryanjulian reviewed Jan 22, 2019

View reviewed changes

pelillian and others added 14 commits March 21, 2019 19:50

Smaller api changes

2fc1a0f

Deleted old test

cdd5fa3

Add unit tests for TabularInput and CsvOutput

cd93e7c

Reorganize TensorBoard logging API and input types

c8acd6e

* Renames TensorBoard logging files and tests to follow convention * Changes Histogram* types in garage.logger.tensorboard_inputs to simpler names in garage.logger.distributions

Reorganize logger outputs

72b1b35

* Move LogOutput base class to `garage.logger.logger`, so that the entire API is in one place * Rename `garage.logger.outputs` to `simple_outputs` to better reflect its purpose

Move StdOutput and TextOutput tests

d27e4d6

* Moves tests for simple_outputs to test_simple_outputs * Makes proper use of temporary files

Replace double-quotes with single-quotes

2e3aa2e

Replace TensorBoardOutput with tensorboardX

be04e09

* Replace TensorBoardOutput with tensorboardX. * Add unittest for TensorBoardOutput. * Delete tensor log as it is too complex and not used anywhere right now. * Rebase with origin/HEAD.

Fix snapshotter failure

017a2ae

* snapshotter fails because `Snapshotter._snapshot_dir=None` and `snapshot_dir.setter` calls `mkdir_p` which doesn't support NoneType. Fixed this.

Refactor logger unittest

97f1db7

Fix pylint complaints

cf21a5c

CatherineSue force-pushed the newer-logger branch from 87dd21e to cf21a5c Compare March 22, 2019 03:01

CatherineSue added 4 commits March 21, 2019 20:28

Remove tensorboard from setup

d18cbf3

Fix tests/garage/experiment/test_snapshot

fa29a61

Use NullOutput in test_snapshot

b65b6af

Add NullOutput to test_local_tf_runner

7b9ff47

CatherineSue force-pushed the newer-logger branch 3 times, most recently from 6531e46 to 1006a68 Compare March 22, 2019 08:39

CatherineSue force-pushed the newer-logger branch from 1006a68 to 4df26b8 Compare March 22, 2019 11:26

Cleanup code

3bda4db

ryanjulian reviewed Mar 22, 2019

View reviewed changes

ryanjulian approved these changes Mar 22, 2019

View reviewed changes

pelillian merged commit 5254d33 into master Mar 23, 2019

CatherineSue deleted the newer-logger branch March 26, 2019 00:04

Conversation

pelillian commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pelillian commented Jan 18, 2019

Uh oh!

Uh oh!

codecov Bot commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coveralls commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pelillian commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naeioi commented Jan 18, 2019

Uh oh!

CatherineSue commented Jan 18, 2019

Uh oh!

naeioi commented Jan 18, 2019

Uh oh!

CatherineSue commented Jan 18, 2019

Uh oh!

krzentner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonashen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pelillian commented Jan 21, 2019

Uh oh!

pelillian commented Jan 21, 2019

Uh oh!

ryanjulian commented Jan 21, 2019

Uh oh!

ryanjulian commented Jan 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krzentner commented Jan 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanjulian Mar 22, 2019

Choose a reason for hiding this comment

pelillian commented Jan 18, 2019 •

edited

Loading

codecov Bot commented Jan 18, 2019 •

edited

Loading

coveralls commented Jan 18, 2019 •

edited

Loading

pelillian commented Jan 18, 2019 •

edited

Loading