Add TensorBoard writer without TensorFlow #2065

lanpa · 2019-03-27T18:25:31Z

This PR addresses discussions in #1801.

move writer files to tensorboard/writer
correct way to handle plugin protos (use softlink)
unittests
rewrite the writer in case of license issue.

cc @orionr

orionr · 2019-03-27T22:21:46Z

Can you change the title for this PR to "Add TensorBoard writer without TensorFlow"? Thanks.

orionr

Looking good! Have some change requests on here.

tensorboard/compat/BUILD

tensorboard/compat/proto/layout.proto

tensorboard/writer/BUILD

tensorboard/writer/__init__.py

tensorboard/writer/event_file_writer.py

tensorboard/writer/record_writer.py

orionr

And one more comment. :)

tensorboard/compat/proto/BUILD

tensorboard/writer/BUILD

tensorboard/writer/crc32c.py

tensorboard/writer/event_file_writer.py

tensorboard/writer/event_file_writer_test.py

tensorboard/writer/record_writer.py

tensorboard/writer/event_file_writer_test.py

orionr

This is looking great. @nfelt I think we should be ready for your review very soon. Any updates you'd request here?

tensorboard/writer/record_writer.py

orionr

Looks great! @nfelt should be ready for your review.

tensorboard/writer/crc32c.py

tensorboard/writer/record_writer.py

orionr

Apologies - didn't realize you still had masked_crc32c and u32 here. Let's just use the shared version of those instead.

tensorboard/writer/record_writer.py

remove `directory_check` and `reopen`

- dummy tests

orionr

Looking good!

tensorboard/writer/event_file_writer_test.py

tensorboard/writer/BUILD

orionr · 2019-04-12T23:18:59Z

@nfelt, confirmed with @lanpa that this is ready for review! The optimization changes can hopefully be done as a followup, but let us know.

orionr · 2019-04-17T15:07:22Z

@nfelt and @wchargin, a gentle ping for review on this. Thanks.

nfelt

Thanks for the restructuring and adding tests, it's looking a lot better! I realize this is a lot of comments - many are just small naming/comment issues. The rest are primarily about the tests, with a few comments on the multithreading logic and correctness of flushing behavior.

tensorboard/summary/writer/event_file_writer.py

tensorboard/summary/writer/event_file_writer_test.py

nfelt · 2019-04-18T02:34:31Z

tensorboard/summary/writer/event_file_writer_test.py

+  # In my experiment, the tolerance can be set as high to roughly to 0.95.
+  # I set 0.9 here in case the CI is too slow.
+
+  def test_async_writer_auto_flushing(self):


We shouldn't have tests that use sleep(). It's better to instead access the w._queue property; accessing private members for test purposes is acceptable if there isn't a cleaner way to do it.

If you revise the flushing logic to call self._queue.task_done() at the very end (the way my suggestion has it) then it's possible to do w._queue.join() to ensure that all tasks have been finished (including any auto-flushing), which allows this to be tested deterministically.

The way that would work is that you could 'mock.patch' time.time() to return a fake timestamp, and change the writer to be (rather than a raw file) a fake object where write() and flush() populate a list of lists (each outer list representing a set of flushed data, and each inner list representing the series of written data flushed at that point). Then you could have a test like this:

set timestamp to 0

w.write(b"1")

w._queue.join()

assert that fake writer contains [[b"1"]] (automatic auto-flush on first write)

set timestamp to 0 + flush_secs - 1

w.write(b"2")

w._queue.join()

assert that fake writer contains no new data (no auto-flush yet)

set timestamp to 0 + flush_secs + 1

w.write(b"3")

w._queue.join()

assert that fake writer contains [[b"1"], [b"2", b"3"]] (automatic auto-flush after "3")

w.write(b"4")

w._queue.join()

assert that fake writer contains no new data

set timestamp to 0 + flush_secs + 1 + flush_secs - 1

w.write(b"5")

w._queue.join()

assert that fake writer contains no new data

set timestamp to 0 + flush_secs + 1 + flush_secs + 1

w.write(b"6")

w._queue.join()

assert that fake writer contains [[b"1"], [b"2", b"3"], [b"4", b"5", b"6"]] (automatic auto-flush after "6")

Yes, we should made this a follow up PR.

Okay, so can we remove the more complex flushing tests from this PR and add them back in the follow-up?

That would be test_async_writer_auto_flushing (along with the comment/diagram) and test_async_writer_flush_before_flush_secs now that the latter has gotten more complicated.

tensorboard/summary/writer/event_file_writer.py

nfelt

@lanpa - yes, I realize now what dummy_delay is used for, but I'm still not crazy about that usage - see some of my other comments on the tests. Using sleep() in tests can make them flaky which could impact our continuous testing stability.

Since we're on a tight timeframe, maybe what makes sense for now is to omit dummy_delay parameter and the auto_flushing test from this PR, and we can revisit how to make the tests work without sleep() calls in a follow up?

lanpa · 2019-04-20T17:57:04Z

@nfelt Thank you for the precious comments. Now I have more sense about how to write unittests. (tests should be deterministic) Please have a look at those unresolved conversations for my questions. ps. The CI fails on tf-nightly only starting from bcbb1ac, It should be fixable by Merge remote-tracking branch 'upstream/master' into no-tf-sep-writer again before PR landing. Btw, since many asyncThreadTest related tests will be follow PR, I mark conversation resolved.
cc @orionr

orionr

Replying to a few things here. Looking better!

tensorboard/summary/writer/event_file_writer.py

orionr · 2019-04-22T22:44:28Z

tensorboard/summary/writer/event_file_writer_test.py

+from tensorboard import test as tb_test
+
+class EventFileWriterTest(tb_test.TestCase):
+  def __init__(self, *args, **kwargs):


I think @nfelt means you can just remove this def __init__(self, *args, **kwargs): method completely. Give that a try and see how it goes.

orionr · 2019-04-22T22:45:58Z

tensorboard/summary/writer/event_file_writer_test.py

+      assert f.read() == random_bytes
+
+
+def get_copy_by_OS(oldfilename):


Given that there is only one use, can we just inline this code? We can then remove it later.

orionr · 2019-04-22T22:46:19Z

tensorboard/summary/writer/record_writer_test.py

+
+class RecordWriterTest(tb_test.TestCase):
+  def __init__(self, *args, **kwargs):
+    super(RecordWriterTest, self).__init__(*args, **kwargs)


Yup - please just remove this method.

nfelt

Looking good, thanks for all the test updates! Just a few more comments.

One general comment that applies to several tests: let's avoid using os.urandom and instead just use fixed dummy strings like b"hello world" or if you need a specific length, use something like b"x" * 64.

tensorboard/summary/writer/event_file_writer.py

tensorboard/summary/writer/record_writer.py

nfelt · 2019-04-22T23:02:03Z

tensorboard/summary/writer/event_file_writer_test.py

+  # In my experiment, the tolerance can be set as high to roughly to 0.95.
+  # I set 0.9 here in case the CI is too slow.
+
+  def test_async_writer_auto_flushing(self):


Okay, so can we remove the more complex flushing tests from this PR and add them back in the follow-up?

That would be test_async_writer_auto_flushing (along with the comment/diagram) and test_async_writer_flush_before_flush_secs now that the latter has gotten more complicated.

tensorboard/summary/writer/event_file_writer_test.py

orionr · 2019-04-23T15:11:40Z

@nfelt, should be ready for review. Thanks @lanpa!

orionr · 2019-04-23T18:35:16Z

Also heads up that this landing will be critical for our 1.1 release, which we'll patch ASAP!

orionr · 2019-04-24T04:13:15Z

@nfelt, please merge when ready. Thank you. :)

Summary: This PR adds TensorBoard logging support natively within PyTorch. It is based on the tensorboardX code developed by lanpa and relies on changes inside the tensorflow/tensorboard repo landing at tensorflow/tensorboard#2065. With these changes users can simply `pip install tensorboard; pip install torch` and then log PyTorch data directly to the TensorBoard protobuf format using ``` import torch from torch.utils.tensorboard import SummaryWriter writer = SummaryWriter() s1 = torch.rand(1) writer.add_scalar('data/scalar1', s1[0], 0) writer.close() ``` Design: - `EventFileWriter` and `RecordWriter` from tensorboardX now live in tensorflow/tensorboard - `SummaryWriter` and PyTorch-specific conversion from tensors, nn modules, etc. now live in pytorch/pytorch. We also support Caffe2 blobs and nets. Action items: - [x] `from torch.utils.tensorboard import SummaryWriter` - [x] rename functions - [x] unittests - [x] move actual writing function to tensorflow/tensorboard in tensorflow/tensorboard#2065 Review: - Please review for PyTorch standard formatting, code usage, etc. - Please verify unittest usage is correct and executing in CI Any significant changes made here will likely be synced back to github.com/lanpa/tensorboardX/ in the future. cc orionr, ezyang Pull Request resolved: #16196 Differential Revision: D15062901 Pulled By: orionr fbshipit-source-id: 3812eb6aa07a2811979c5c7b70810261f9ea169e

Summary: This PR adds TensorBoard logging support natively within PyTorch. It is based on the tensorboardX code developed by lanpa and relies on changes inside the tensorflow/tensorboard repo landing at tensorflow/tensorboard#2065. With these changes users can simply `pip install tensorboard; pip install torch` and then log PyTorch data directly to the TensorBoard protobuf format using ``` import torch from torch.utils.tensorboard import SummaryWriter writer = SummaryWriter() s1 = torch.rand(1) writer.add_scalar('data/scalar1', s1[0], 0) writer.close() ``` Design: - `EventFileWriter` and `RecordWriter` from tensorboardX now live in tensorflow/tensorboard - `SummaryWriter` and PyTorch-specific conversion from tensors, nn modules, etc. now live in pytorch/pytorch. We also support Caffe2 blobs and nets. Action items: - [x] `from torch.utils.tensorboard import SummaryWriter` - [x] rename functions - [x] unittests - [x] move actual writing function to tensorflow/tensorboard in tensorflow/tensorboard#2065 Review: - Please review for PyTorch standard formatting, code usage, etc. - Please verify unittest usage is correct and executing in CI Any significant changes made here will likely be synced back to github.com/lanpa/tensorboardX/ in the future. cc orionr, ezyang Pull Request resolved: pytorch#16196 Differential Revision: D15062901 Pulled By: orionr fbshipit-source-id: 3812eb6aa07a2811979c5c7b70810261f9ea169e

lanpa added 2 commits March 28, 2019 01:40

initial commit

5c33058

dummy unit test

fdbc2d1

orionr suggested changes Mar 27, 2019

View reviewed changes

orionr reviewed Mar 27, 2019

View reviewed changes

tensorboard/compat/proto/BUILD Outdated Show resolved Hide resolved

lanpa changed the title ~~[WIP] tensorboard-notf for pytorch (again)~~ [WIP] Add TensorBoard writer without TensorFlow Mar 28, 2019

lanpa added 6 commits March 28, 2019 09:03

addresses code review comment (the trivial ones)

7a4b528

need license even if no code.

fe80915

fix dummy test

10fbb48

remove S3, relative import

72b7f12

remove remaining S3 code

f60d55d

use correct dependency

c29823e

lanpa mentioned this pull request Mar 29, 2019

TensorBoard support within PyTorch pytorch/pytorch#16196

Closed

4 tasks

lanpa added 3 commits March 29, 2019 08:26

remove irrelavant proto for writer

58c32e7

fix docstring

0cab12e

remove deps

a6b3ffb

orionr reviewed Mar 29, 2019

View reviewed changes

tensorboard/writer/event_file_writer_test.py Outdated Show resolved Hide resolved

lanpa added 2 commits April 1, 2019 01:38

add unit test

944752f

fix for remaining reviews

aec879a

orionr reviewed Mar 31, 2019

View reviewed changes

tensorboard/writer/record_writer.py Outdated Show resolved Hide resolved

rewrite record_writer

7bf21f5

orionr approved these changes Apr 1, 2019

View reviewed changes

orionr suggested changes Apr 1, 2019

View reviewed changes

tensorboard/writer/crc32c.py Outdated Show resolved Hide resolved

tensorboard/writer/record_writer.py Outdated Show resolved Hide resolved

lanpa changed the title ~~[WIP] Add TensorBoard writer without TensorFlow~~ Add TensorBoard writer without TensorFlow Apr 2, 2019

use crc in tensorflow_stub

d90406a

orionr approved these changes Apr 2, 2019

View reviewed changes

orionr suggested changes Apr 3, 2019

View reviewed changes

tensorboard/writer/record_writer.py Outdated Show resolved Hide resolved

tensorboard/writer/record_writer.py Outdated Show resolved Hide resolved

lanpa added 2 commits April 4, 2019 21:08

reduce duplicated code

f4129ca

remove useless import

4a1940d

lanpa added 4 commits April 7, 2019 02:26

more simple fix

c9c1529

fix time format

c3e5d40

remove `directory_check` and `reopen`

- unique filename

15ea771

- dummy tests

add many tests and simplifies async

69bd4b5

orionr reviewed Apr 12, 2019

View reviewed changes

lanpa added 4 commits April 13, 2019 00:39

remove tf dependency

fa4d7a7

move to summary/writer

bafa04d

fix test, prepare for tb_test

2551cbc

enable tb_test (expect failure on CI)

72b6d7d

nfelt reviewed Apr 18, 2019

View reviewed changes

nfelt reviewed Apr 19, 2019

View reviewed changes

lanpa added 5 commits April 20, 2019 01:50

Merge remote-tracking branch 'upstream/master' into no-tf-sep-writer

e0e70c4

fix (1 of 3)

149535a

fix (2 of 3)

6b3d377

fix (3 of 3)

bcbb1ac

remove dummy_delay

0c732fc

orionr reviewed Apr 22, 2019

View reviewed changes

nfelt reviewed Apr 22, 2019

View reviewed changes

addressing comments on apr23

6fde811

nfelt approved these changes Apr 23, 2019

View reviewed changes

nfelt merged commit 740be35 into tensorflow:master Apr 24, 2019

nfelt mentioned this pull request Jan 29, 2021

Provide a TensorBoard-native summary writer API #4581

Open

26 tasks

		assert f.read() == random_bytes


		def get_copy_by_OS(oldfilename):

Add TensorBoard writer without TensorFlow #2065

Add TensorBoard writer without TensorFlow #2065

Uh oh!

Conversation

lanpa commented Mar 27, 2019

Uh oh!

orionr commented Mar 27, 2019

Uh oh!

orionr 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!

orionr 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!

orionr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orionr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orionr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

orionr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orionr commented Apr 12, 2019

Uh oh!

orionr commented Apr 17, 2019

Uh oh!

nfelt 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!

nfelt Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

lanpa Apr 20, 2019

Choose a reason for hiding this comment

Uh oh!

nfelt Apr 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nfelt left a comment

Choose a reason for hiding this comment