Skip to content
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

Create a backend for the pr_curves plugin #387

Merged
merged 19 commits into from
Aug 25, 2017
Merged

Conversation

chihuahua
Copy link
Member

@chihuahua chihuahua commented Aug 18, 2017

Made PrCurvesPlugin, which serves data for the PR Curves dashboard. Made a metadata.py file that centralizes information such as the plugin name and the indices used to obtain precision and recall from tensor data.

This PR is a part of dividing #334 (which contains resourceful input from @wchargin) into smaller PRs.

@chihuahua chihuahua requested review from wchargin and jart and removed request for wchargin August 18, 2017 07:58
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Looks good overall. Most important requested change is to the data format (use proto wire format instead of json). Then the docs and nondeterminism. Everything else is either minor or a suggestion.

"""
pr_curve_plugin_data = plugin_data_pb2.PrCurvePluginData(
version=PROTO_VERSION, num_thresholds=num_thresholds)
content = json_format.MessageToJson(pr_curve_plugin_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be content = pr_curve_plugin_data.SerializeToString(), for consistency with our stated best-practice and consistency with all other plugins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

response = http_util.Respond(
request, self.pr_curves_impl(runs, tag), 'application/json')
except ValueError as e:
return http_util.Respond(request, '%s' % e, 'text/plain', 400)
Copy link
Contributor

Choose a reason for hiding this comment

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

'%s' % e is more easily written as str(e).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

The JSON object for the tags route response.
"""
all_runs = self._multiplexer.PluginRunToTagToContent(
PrCurvesPlugin.plugin_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be metadata.PLUGIN_NAME. I know that they have the same value, but the argument to PluginRunToTagToContent corresponds to the string stored in the plugin_data of the summaries, not the route used to fetch data. If you changed PrCurvesPlugin.plugin_name to be something else, the value of this argument should still be the original metadata.PLUGIN_NAME.

Likewise elsewhere in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

# within the same run. If the latter occurs, TensorBoard will show the
# actual step of each tag atop the card for the tag.
tensor_events = self._multiplexer.Tensors(
run, list(tag_to_content.keys())[0])
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 nondeterministic, which rubs me the wrong way. Consider: min(six.iterkeys(tag_to_content)).

(General note: If you were to not apply this comment, prefer next(six.iterkeys(tag_to_content)) to what's written here. No need to serialize to list, duplicate the list (in Python 2), and then grab the first element.)

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I actually like next(six.iterkeys(tag_to_content)) because it does not involve looping through all tags like min. Indeed, both alternatives nicely avoid list serialization and duplication.

def setUp(self):
super(PrCurvesPluginTest, self).setUp()
logdir = os.path.join(self.get_temp_dir(), 'logdir')
tf.reset_default_graph()
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 not needed; it's done in the superclass setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

"""
self.assertEqual(expected_step, pr_curve_entry['step'])
# We use an absolute error instead of a relative one because the expected
# values are small. The default relative error (trol) of 1e-7 yields many
Copy link
Contributor

Choose a reason for hiding this comment

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

s/trol/rtol?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,198 @@
# Copyright 2017 Google Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an http_api.md? (Copy-paste and modify one of the other plugins'.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

The handler should raise a ValueError when no PR curve data can be found
for a certain run-tag combination.
"""
with self.assertRaises(ValueError):
Copy link
Contributor

Choose a reason for hiding this comment

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

You might prefer with six.assertRaisesRegex(self, ValueError, r'No PR curves could be fetched') to ensure that the error is the one that you're looking for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Done.

@@ -58,7 +57,7 @@ def op(
predictions: A float32 `Tensor` whose values are in the range `[0, 1]`.
Dimensions must match those of `labels`.
num_thresholds: Number of thresholds, evenly distributed in `[0, 1]`, to
compute PR metrics for. Should be `>= 2`. This value should be a
compute PR metrics for. Should be `>= 2`. This value should be a
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked the idea of handling these in #389. Perhaps you have a rogue commit in this PR?

Ideally, the "add backend" commit should make no changes to summary.py, and the "add frontend" commit (if separate) should make no changes to pr_curves_plugin.py/etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I am going to let #389 handle these style changes.

Copy link
Member Author

@chihuahua chihuahua left a comment

Choose a reason for hiding this comment

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

I manually started a backend and tried routes. They seem to work.

@@ -0,0 +1,198 @@
# Copyright 2017 Google Inc. All Rights Reserved.
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

response = http_util.Respond(
request, self.pr_curves_impl(runs, tag), 'application/json')
except ValueError as e:
return http_util.Respond(request, '%s' % e, 'text/plain', 400)
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

The JSON object for the tags route response.
"""
all_runs = self._multiplexer.PluginRunToTagToContent(
PrCurvesPlugin.plugin_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

# within the same run. If the latter occurs, TensorBoard will show the
# actual step of each tag atop the card for the tag.
tensor_events = self._multiplexer.Tensors(
run, list(tag_to_content.keys())[0])
Copy link
Member Author

Choose a reason for hiding this comment

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

True. I actually like next(six.iterkeys(tag_to_content)) because it does not involve looping through all tags like min. Indeed, both alternatives nicely avoid list serialization and duplication.

def setUp(self):
super(PrCurvesPluginTest, self).setUp()
logdir = os.path.join(self.get_temp_dir(), 'logdir')
tf.reset_default_graph()
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

"""
self.assertEqual(expected_step, pr_curve_entry['step'])
# We use an absolute error instead of a relative one because the expected
# values are small. The default relative error (trol) of 1e-7 yields many
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

The handler should raise a ValueError when no PR curve data can be found
for a certain run-tag combination.
"""
with self.assertRaises(ValueError):
Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! Done.

# within the same run. If the latter occurs, TensorBoard will show the
# actual step of each tag atop the card for the tag.
tensor_events = self._multiplexer.Tensors(
run, next(six.iterkeys(tag_to_content)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Like you, I appreciate that next(six.iterkeys(tag_to_content)) avoids looking at all the data, but it is just this that means that the result is still nondeterministic. If you can find a simple deterministic way to not look at all the data, then go for it, but short of changing the storage type used by the event accumulator to a key-ordered dict I think min(six.iterkeys(tag_to_content)) is what you want.

Note also that looking at all the data here is in no way expensive.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed generally inexpensive. Used min(six.iterkeys(tag_to_content)) for easier debugging if issues arise.

@@ -0,0 +1,97 @@
# Precision—Recall Curve plugin HTTP API

The scalar plugin name is `pr_curves`, so all its routes are under
Copy link
Contributor

Choose a reason for hiding this comment

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

s/scalar/PR curve/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


Each PR data entry contains the following properties.

* **wall_time**: The wall time (number) in seconds since the epoch at which data
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind removing the hanging indent here? It should look like:

Each PR data entry contains the following properties.

  * **wall_time**: The wall time (number) in seconds since the epoch at which data
    for the PR curve was allocated.

It makes diffs, editing, etc. more difficult, and renders just the same.

(Arguably you want a <dl> here, but Markdown doesn't have those, and you shouldn't drop down into HTML for it, so.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. A <dl> would seem apt if possible.

}
```

## `/data/plugin/scalars/pr_curves`
Copy link
Contributor

Choose a reason for hiding this comment

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

s/scalars/pr_curves

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


Used by the PR Curves dashboard to render plots.

## `/data/plugin/scalars/tags`
Copy link
Contributor

Choose a reason for hiding this comment

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

s/scalars/pr_curves

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Returns:
The JSON object for the tags route response.
"""
all_runs = self._multiplexer.PluginRunToTagToContent(metadata.PLUGIN_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

From #334 (comment):

You'll need to update this to return a run-to-tag-to-tag-info map, as in the other plugins. Then, you can pass this to tf-card-heading on the frontend. (The frontend part can be a TODO for now because of resolution, but we should get the backend right on the first go. Take a look at the runToTagInfo adaptations in the scalar dashboard, for instance.)

Best to resolve in this PR: there's no need for this backend to ever do the old thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thank you for noting that! I also updated http.md.

Copy link
Member Author

@chihuahua chihuahua left a comment

Choose a reason for hiding this comment

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

Manually verified that backend responses seem reasonable.

@@ -0,0 +1,97 @@
# Precision—Recall Curve plugin HTTP API

The scalar plugin name is `pr_curves`, so all its routes are under
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

}
```

## `/data/plugin/scalars/pr_curves`
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


Used by the PR Curves dashboard to render plots.

## `/data/plugin/scalars/tags`
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

# within the same run. If the latter occurs, TensorBoard will show the
# actual step of each tag atop the card for the tag.
tensor_events = self._multiplexer.Tensors(
run, next(six.iterkeys(tag_to_content)))
Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeed generally inexpensive. Used min(six.iterkeys(tag_to_content)) for easier debugging if issues arise.

Returns:
The JSON object for the tags route response.
"""
all_runs = self._multiplexer.PluginRunToTagToContent(metadata.PLUGIN_NAME)
Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thank you for noting that! I also updated http.md.

are objects with 2 keys: `displayName` and `description` (associated with the
run-tag combination).

The `displayName` is shown atop individual plots in TensorBoard. The description
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that the description contains sanitized HTML to be injected into the DOM, while the display name is simply an arbitrary string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

# within the same run. If the latter occurs, TensorBoard will show the
# actual step of each tag atop the card for the tag.
tensor_events = self._multiplexer.Tensors(
run, list(tag_to_content.keys())[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

This was reverted here: 382b13d#diff-9b643340c1c8537af733696bb8c0c641R166

I'm pausing review on the suspicion that this is a rogue commit (at a quick scan, its contents don't seem to match its title). Let me know when the diff is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for noticing! I undid the rogue changes in a recent commit. What happened was that I had copied some code from the PR with the demo, which did not include some of the recent changes in this particular PR. I need to be more cognizant of avoiding rogue changes and commits.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Okay; just one API question.

return {
'step': tensor_event.step,
'wall_time': tensor_event.wall_time,
'relative': tensor_event.wall_time - initial_wall_time,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why compute the relative times on the server? No other plugin does this, as far as I know; we use ChartHelpers.relativeAccessor to do this on the frontend:

export let relativeAccessor =
// tslint:disable-next-line:no-any be quiet tsc
(d: any, index: number, dataset: Plottable.Dataset) => {
// We may be rendering the final-point datum for scatterplot.
// If so, we will have already provided the 'relative' property
if (d.relative != null) {
return d.relative;
}
let data = dataset.data();
// I can't imagine how this function would be called when the data is
// empty (after all, it iterates over the data), but lets guard just
// to be safe.
let first = data.length > 0 ? +data[0].wall_time : 0;
return (+d.wall_time - first) / (60 * 60 * 1000); // ms to hours
};

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true. The recent commit does away with computing relative on the backend.

I think on the frontend, we could have pr-curve-card logic compute relative values. We could also generalize the logic within vz-chart-helpers, although the relative time in the PR curve UI changes the display in the sliders and doesn't modify the UI of the vz line chart.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

All right!

@chihuahua chihuahua force-pushed the chizeng-pr-curves-backend branch from 9ae9ca2 to f8e6721 Compare August 25, 2017 18:05
@chihuahua chihuahua merged commit 1c7b8ce into master Aug 25, 2017
@chihuahua chihuahua deleted the chizeng-pr-curves-backend branch August 25, 2017 18:38
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