Add the tensorboard.summary public entry point

[wchargin]

Summary:
Resolves #561.

Test Plan:
Ran the build_pip_package.sh script to build the Python 2 package,
then created a new Python 2 virtualenv in which I installed only the
latest nightly TensorFlow release (2017-09-22) and the generated wheel.
Then wrote

import tensorflow as tf
from tensorboard import summary
result = tf.Session().run(summary.scalar('shelby', tf.constant(3)))
s = tf.Summary()
s.ParseFromString(result)
print(s)

and verified that the result was a proper scalar summary.

I then repeated the process using pip install tensorflow==1.3.0
instead of the nightly version to ensure that the module works for
normal TensorFlow users. (Note: the audio summary will not work because
TensorFlow 1.3 lacks 22730fd4c633a74e59c03ff76dc92e6ae2d5d020. This is
fine; users can continue to use the old API for that one.)

wchargin-branch: summary-module

[chihuahua]

Nice. Could we note in plugin docs to update tensorboard/summary.py when a summary's added?

[wchargin]

"note in plugin docs"—what plugin docs? We don't have a readme for tensorboard/plugins, nor do we need one; this is not public-facing, and it's not something that plugin developers need to know about.

It should be a rare thing for a summary to be added to TensorBoard core, and it may well be the case that we want to offer a plugin in tensorboard/plugins before with publish it in tensorboard.summary.

[teamdandelion]

Looks nice. Could the test verify that the _pb method is present too? Maybe with an exclusions set for those summaries that don't have a _pb method (although that should tend towards the empty set).

[wchargin]

Could the test verify that the _pb method is present too? Maybe with an exclusions set for those summaries that don't have a _pb method

Sure.

@chihuahua what's the status of writing pr_curve_pb?

[jart]

LGTM. Please wait for Dandelion to approve.

[jart]

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.

[wchargin]

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.)

[jart]

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

[wchargin]

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

[wchargin]

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.

[wchargin]

Will await Dandelion's approval. Thanks for reviewing!

[wchargin]

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.)

[wchargin]

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

[jart]

Here's how the best practices guide justifies it:

Tests should be straightforward enough to be 'obviously correct' upon inspection. Control structures (like loops and conditionals) can increase the chance for errors. When a test fails, having complex logic in the test makes it harder to diagnose the root cause of the failure.

It's usually impossible for production code to exhaustively list every output for each possible input, so we have to use the full power of our programming languages there. But test code doesn't need to cover every input - only a small set of representative values that mark the interesting cases in the code. Increasing the complexity of a single test by using conditionals or loops to cover multiple input types can quickly lead to logic errors in the tests themselves. In other words, tests should strive to be "above suspicion" when it comes to faulty behavior, lest they require tests of their own!

Tests written without control structures are clearer since the reader doesn't have to do any mental computations to understand them, and more likely to be correct since it's harder to have bugs in code without these constructs.

When tests do need their own logic, such logic should often be moved out of the test bodies and into utilities and helper functions. Since such helpers can get quite complex, it's usually a good idea for any nontrivial test utility to have its own tests.

Exception: It it okay to loop through an enumeration of values (e.g. an enum or a list of error codes) to perform a common set of assertions on them, often called data-driven testing. However, if you find yourself introducing conditionals to further optimize your data- driven tests (e.g. to apply different asserts to different inputs) you are better off creating separate test cases instead.

So it seems like you were correct in the beginning, until the pr_curve plugin broke your assumption and extra logic had to be added.

[jart]

Go gets to be special because it's Go. Without exceptions or macros, it's probably not possible to write unit tests the normal way. So they'll usually just have some complicated function that loops through an array of test vector objects.

[teamdandelion]

Looks good, modulo one small nit :) Thanks William!

[teamdandelion]

Nit: Instead of making this a raw TODO, can we link it to a GitHub issue?

[wchargin]

Sure.

[wchargin]

Added reference to #445, which already exists and is assigned to @chihuahua . Will merge once Travis finishes.