Skip to content

Conversation

@wchargin
Copy link
Contributor

@wchargin wchargin commented Feb 4, 2019

Summary:
This type describes information about a running TensorBoard instance. In
a future commit, TensorBoard’s main function will be outfitted to write
an “info file” containing this structure to a well-known location, which
other processes can inspect to find all running TensorBoard instances,
the ports on which they’re listening, their arguments and context, etc.

The wire format is JSON as opposed to pbtxt for two reasons: it’s more
open-source friendly (tools like jq have no publicly available
protobuf equivalents), and the Python protobuf APIs are so frustrating
that it doesn’t feel right to force callers to use them, even if this
means that we have to implement our own validation and serialization.

Supersedes part of #1795.

Test Plan:
Unit tests included; run bazel test //tensorboard:manager_test.

wchargin-branch: tensorboardinfo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will admit I see no style rule prohibiting this, but it is inconsistent with the rest of TensorBoard to separate out the closing paren like this when it's opening an indented block. My eye reads the with and looks for the closing out-indent and finds this parent and gets very confused.

So if you're not particularly opposed, maybe let's keep the closeparen and colon on the end of the preceding line? Unlike for e.g. a tuple definition where it's nice to be able to end each element with a comma for consistency when adding/removing them, in this case there will always be 3 arguments to this function so that seems less relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k

@wchargin wchargin force-pushed the wchargin-tensorboardinfo branch from ed4f8b0 to 9f13d23 Compare February 5, 2019 00:52
Summary:
This type describes information about a running TensorBoard instance. In
a future commit, TensorBoard’s main function will be outfitted to write
an “info file” containing this structure to a well-known location, which
other processes can inspect to find all running TensorBoard instances,
the ports on which they’re listening, their arguments and context, etc.

The wire format is JSON as opposed to pbtxt for two reasons: it’s more
open-source friendly (tools like `jq` have no publicly available
protobuf equivalents), and the Python protobuf APIs are so frustrating
that it doesn’t feel right to force callers to use them, even if this
means that we have to implement our own validation and serialization.

Supersedes part of #1795.

Test Plan:
Unit tests included; run `bazel test //tensorboard:manager_test`.

wchargin-branch: tensorboardinfo
@wchargin wchargin force-pushed the wchargin-tensorboardinfo branch from 9f13d23 to faa16b0 Compare February 5, 2019 00:55
@wchargin wchargin merged commit 642a0a5 into master Feb 5, 2019
@wchargin wchargin deleted the wchargin-tensorboardinfo branch February 5, 2019 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants