Make sure compat PyRecordReader_New reads new data

[orionr]

This fixes pytorch/pytorch#20477

It looks like our original compat implementation of PyRecordReader_New didn't continue to try and read content from a file after it had done the original read. This updates the class to continue to request data from the file and read in new events. Note that this would only be seen in the non-TensorFlow case. Here's a test case:

conda create --name notf36 python=3.6
conda activate notf36
pip install --upgrade pip
pip install scipy
conda install -y protobuf
conda install -y absl-py
conda install -y numpy
conda install pytorch-nightly -c pytorch  # For the train.py below
pip install tb-nightly  # For the train.py below
bazel build tensorboard --verbose_failures
./bazel-bin/tensorboard/tensorboard --logdir ../realtime_runs/runs/ --verbosity 2
bazel test //tensorboard/summary/writer:record_writer_test --test_output=errors

While the above is running, execute the following train.py script located in ../realtime_runs/:

import numpy as np
import time

import torch
from torch.utils.tensorboard import SummaryWriter

writer = SummaryWriter()

for i in range(1000000):
    writer.add_scalar('data/s1', i % 100, i)
    # writer.add_scalar('data/s2', i % 200, i)
    # writer.add_scalars('data/scalar_group', {'xsinx': i * np.sin(i), 'xcosx': i * np.cos(i)})
    writer.flush()

    time.sleep(1)

with python train.py

With the old code TensorBoard won't update. With the updated code it will as expected.

cc @nfelt, @natalialunova, @wchargin, @lanpa

[orionr]

@nfelt, hopefully we can land #2260 first and then this one.

[nataliakliushkina]

Looks good!

[nfelt]

Can you add a minimal test for the record reader that at least covers this case and ideally a few other basic ones? I think the fact that this bug persisted is evidence that the implementation is different enough from the original that this really should have tests.

[orionr]

Good point - changing test. On Jun 17, 2019, at 3:40 PM, Nick Felt <[email protected]<mailto:[email protected]>> wrote: @nfelt commented on this pull request. Can you add a minimal test for the record reader that at least covers this case and ideally a few other basic ones? I think the fact that this bug persisted is evidence that the implementation is different enough from the original that this really should have tests.

________________________________ In tensorboard/compat/tensorflow_stub/pywrap_tensorflow.py<#2342 (comment)>:

-

- if self.index >= len(self.event_strs) - 1: + if self.file_handle is None: + if self.filename is None: + raise errors.NotFoundError( + None, None, 'No filename provided, cannot read Events') + if not gfile.exists(self.filename): + raise errors.NotFoundError( + None, None, + '{} does not point to valid Events file'.format(self.filename)) + if self.compression_type: + # TODO: Handle gzip and zlib compressed files + raise errors.UnimplementedError( + None, None, 'compression not supported by compat reader') + self.file_handle = gfile.GFile(self.filename, 'rb') + self.file_handle.seek(self.start_offset) TensorBoard's usage of PyRecordReader_New never passes in the start_offset argument, so I'd rather leave out that implementation and avoid expanding the GFile API surface we support. We can just raise an UnimplementedError instead if start_offset is provided.

________________________________ In tensorboard/compat/tensorflow_stub/pywrap_tensorflow.py<#2342 (comment)>:

def GetNext(self): - if not self.done_reading: - self.read() - - if self.index >= len(self.event_strs) - 1: + if self.file_handle is None: + if self.filename is None: + raise errors.NotFoundError( + None, None, 'No filename provided, cannot read Events') + if not gfile.exists(self.filename): + raise errors.NotFoundError( + None, None, + '{} does not point to valid Events file'.format(self.filename)) + if self.compression_type: Let's handle this in the constructor instead. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#2342?email_source=notifications&email_token=AAATQ6R23HF6KIDNPE4YTZLP3AHEBA5CNFSM4HXEHDQKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3ZJ4KY#pullrequestreview-250781227>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAATQ6WE4ND2UTGEDY2SGILP3AHEBANCNFSM4HXEHDQA>.

[orionr]

@nfelt if tests pass this should be ready for another review. Much appreciated.

[cswwp]

Hi, @nfelt, i have install tb_nightly (version 2.1.0a20191021), and restart tensorboard --log_dir target dir, and it still shows that "TensorFlow installation not found - running with reduced feature set." I'm confused