Skip to content

Add the tensorboard.summary public entry point #562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 26, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions tensorboard/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,32 @@ py_test(
],
)

py_library(
name = "summary",
srcs = ["summary.py"],
srcs_version = "PY2AND3",
visibility = ["//visibility:public"],
deps = [
"//tensorboard/plugins/audio:summary",
"//tensorboard/plugins/histogram:summary",
"//tensorboard/plugins/image:summary",
"//tensorboard/plugins/pr_curve:summary",
"//tensorboard/plugins/scalar:summary",
"//tensorboard/plugins/text:summary",
],
)

py_test(
name = "summary_test",
size = "small",
srcs = ["summary_test.py"],
srcs_version = "PY2AND3",
deps = [
":summary",
"//tensorboard:expect_tensorflow_installed",
],
)

py_library(
name = "test_util",
testonly = 1,
Expand Down
1 change: 1 addition & 0 deletions tensorboard/pip_package/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@ sh_binary(
"setup.py",
"//tensorboard",
"//tensorboard:version",
"//tensorboard:summary",
],
)
48 changes: 48 additions & 0 deletions tensorboard/summary.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Copyright 2017 The TensorFlow Authors. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# ==============================================================================
"""Central API entry point for summary operations.

This module simply offers a shorter way to access the members of modules
like `tensorboard.plugins.scalar.summary`.
"""
from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

from tensorboard.plugins.audio import summary as _audio_summary
from tensorboard.plugins.histogram import summary as _histogram_summary
from tensorboard.plugins.image import summary as _image_summary
from tensorboard.plugins.pr_curve import summary as _pr_curve_summary
from tensorboard.plugins.scalar import summary as _scalar_summary
from tensorboard.plugins.text import summary as _text_summary


audio = _audio_summary.op
audio_pb = _audio_summary.pb

histogram = _histogram_summary.op
histogram_pb = _histogram_summary.pb

image = _image_summary.op
image_pb = _image_summary.pb

pr_curve = _pr_curve_summary.op
pr_curve_raw_data = _pr_curve_summary.raw_data_op

scalar = _scalar_summary.op
scalar_pb = _scalar_summary.pb

text = _text_summary.op
text_pb = _text_summary.pb
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fantastic design. I like it a lot. You also followed good best practices for delegate imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool; thanks. I know that you had some reservations about doing this carefully, so I'm glad that you approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of curiosity…do you have a reference for these best practices for delegate imports? I just did what felt natural here, but I'd be interested to see what other people think.

81 changes: 81 additions & 0 deletions tensorboard/summary_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Copyright 2017 The TensorFlow Authors. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# ==============================================================================
"""API tests for the `tensorboard.summary` module.

These tests are especially important because this module is the standard
public entry point for end users, so we should be as careful as possible
to ensure that we export the right things.
"""
from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

import collections

import tensorflow as tf
from tensorboard import summary


STANDARD_PLUGINS = frozenset([
'audio',
'histogram',
'image',
'pr_curve',
'scalar',
'text',
])

# The subset of `STANDARD_PLUGINS` for which we do not currently have
# functions to generate a summary protobuf outside of a TensorFlow
# graph. This set should ideally be empty; any entries here should be
# considered temporary.
PLUGINS_WITHOUT_PB_FUNCTIONS = frozenset([
'pr_curve', # TODO(@chihuahua, #445): Fix this.
])


class SummaryExportsTest(tf.test.TestCase):

def test_each_plugin_has_an_export(self):
for plugin in STANDARD_PLUGINS:
self.assertIsInstance(getattr(summary, plugin), collections.Callable)

def test_plugins_export_pb_functions(self):
for plugin in STANDARD_PLUGINS:
Copy link
Contributor

Choose a reason for hiding this comment

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

Our unit test best practices guide actually recommends against using things like for loops in unit tests. One notable exception is the Go language. But generally speaking, folks at Google err on the side of just having a lot of boilerplate to test each thing, so when the test runner tells you which method failed, you know the plugin for sure. You also wouldn't need to have special case logic for the pr_curve plugin.

But this is fine. I don't feel too strongly about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never really understood that logic. I agree that the burden of being "clearly correct" is stronger for test code than main code, but this loop is clearly correct. (It looks like we have 130 for-loops in test code currently.) (Plus, why should the Go language be any different?)

I thought about setting a custom message, but the case in which this will fail will almost always be the getattr failing, not that the user exports something that is not a function, and in that case the name of the plugin that failed will appear in the error message. (It's very frustrating that a custom message replaces the default message instead of appending to it, or I'd use custom messages much more often.)

if plugin not in PLUGINS_WITHOUT_PB_FUNCTIONS:
self.assertIsInstance(
getattr(summary, '%s_pb' % plugin), collections.Callable)

def test_all_exports_correspond_to_plugins(self):
exports = [name for name in dir(summary) if not name.startswith('_')]
futures = frozenset(('absolute_import', 'division', 'print_function'))
bad_exports = [
name for name in exports
if name not in futures and not any(
name == plugin or name.startswith('%s_' % plugin)
for plugin in STANDARD_PLUGINS)
]
if bad_exports:
self.fail(
'The following exports do not correspond to known standard '
'plugins: %r. Please mark these as private by prepending an '
'underscore to their names, or, if they correspond to a new '
'plugin that you are certain should be part of the public API '
'forever, add that plugin to the STANDARD_PLUGINS set in this '
'module.' % bad_exports)


if __name__ == '__main__':
tf.test.main()