Skip to content
Open
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
118 changes: 79 additions & 39 deletions tensorboard/plugins/histogram/histograms_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@
from __future__ import print_function

import collections
import csv
import random

import numpy as np
import six
from six import StringIO
from werkzeug import wrappers

from tensorboard import plugin_util
Expand All @@ -36,6 +38,10 @@
from tensorboard.plugins.histogram import metadata
from tensorboard.util import tensor_util

class OutputFormat(object):
"""An enum used to list the valid output formats for API calls."""
JSON = 'json'
CSV = 'csv'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test for the CSV output format. See e.g. the equivalent test for the scalars plugin:

def _test_scalars_csv(self, run_name, tag_name, should_work=True):
self.set_up_with_runs([self._RUN_WITH_LEGACY_SCALARS,
self._RUN_WITH_SCALARS,
self._RUN_WITH_HISTOGRAM])
if should_work:
(data, mime_type) = self.plugin.scalars_impl(
tag_name, run_name, None, scalars_plugin.OutputFormat.CSV)
self.assertEqual('text/csv', mime_type)
s = StringIO(data)
reader = csv.reader(s)
self.assertEqual(['Wall time', 'Step', 'Value'], next(reader))
self.assertEqual(len(list(reader)), self._STEPS)
else:
with self.assertRaises(KeyError):
self.plugin.scalars_impl(self._SCALAR_TAG, run_name, None,
scalars_plugin.OutputFormat.CSV)

Copy link
Author

Choose a reason for hiding this comment

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

I added a test case. Is there a way to trigger the tests on my local machine? Didn't find anything in DEVELOPMENT.md oder CONTRIBUTING.md

Copy link
Contributor

@nfelt nfelt Feb 1, 2019

Choose a reason for hiding this comment

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

You can run bazel test --test_output=errors //tensorboard/... to run all tests, or bazel test --test_output=errors //tensorboard/plugins/histogram:histogram_plugin_test to run just the one you changed.


class HistogramsPlugin(base_plugin.TBPlugin):
"""Histograms Plugin for TensorBoard.
Expand Down Expand Up @@ -63,8 +69,8 @@ def __init__(self, context):

def get_plugin_apps(self):
return {
'/histograms': self.histograms_route,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert the whitespace changes to indentation - continuation lines should be indented +4 spaces

'/tags': self.tags_route,
'/histograms': self.histograms_route,
'/tags': self.tags_route,
}

def is_active(self):
Expand Down Expand Up @@ -124,7 +130,35 @@ def index_impl(self):

return result

def histograms_impl(self, tag, run, downsample_to=None):
def _get_csv_response(self, events):
"""

Args:
events: A list of (wall_time, step, tensor_as_list) event tuples.

Returns:
CSV string representation of the given events.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please document the representation. There’s no reasonable way for
someone to correctly guess the form of CSV returned by this handler
without reading its implementation. By reading this documentation (and
nothing more), I should be able to confidently write code that processes
the CSV returned by this function.

"""
string_io = StringIO()
writer = csv.writer(string_io)
writer.writerow(['Wall time', 'Step', 'BinStart', 'BinEnd', 'BinValue'])
# Convert the events in a way that we can sensibly export
# them as csv. Therefore, we split the start, end, value of
# each bin by semicolon.
for e in events:
writer.writerows(
[
e[0],
e[1],
';'.join(['{:.17f}'.format(el[0]) for el in e[2]]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why format with fixed precision as opposed to just str(el[0])?

';'.join(['{:.17f}'.format(el[1]) for el in e[2]]),
';'.join(['{}'.format(el[2]) for el in e[2]]),
]
)
return string_io.getvalue()

def histograms_impl(self, tag, run, downsample_to=None,
output_format=OutputFormat.JSON):
"""Result of the form `(body, mime_type)`, or `ValueError`.

At most `downsample_to` events will be returned. If this value is
Expand All @@ -136,17 +170,17 @@ def histograms_impl(self, tag, run, downsample_to=None):
cursor = db.cursor()
# Prefetch the tag ID matching this run and tag.
cursor.execute(
'''
SELECT
tag_id
FROM Tags
JOIN Runs USING (run_id)
WHERE
Runs.run_name = :run
AND Tags.tag_name = :tag
AND Tags.plugin_name = :plugin
''',
{'run': run, 'tag': tag, 'plugin': metadata.PLUGIN_NAME})
'''
SELECT
tag_id
FROM Tags
JOIN Runs USING (run_id)
WHERE
Runs.run_name = :run
AND Tags.tag_name = :tag
AND Tags.plugin_name = :plugin
''',
{'run': run, 'tag': tag, 'plugin': metadata.PLUGIN_NAME})
row = cursor.fetchone()
if not row:
raise ValueError('No histogram tag %r for run %r' % (tag, run))
Expand All @@ -160,32 +194,32 @@ def histograms_impl(self, tag, run, downsample_to=None):
# can be formally expressed as the following:
# [s_min + math.ceil(i / k * (s_max - s_min)) for i in range(0, k + 1)]
cursor.execute(
'''
'''
SELECT
MIN(step) AS step,
computed_time,
data,
dtype,
shape
FROM Tensors
INNER JOIN (
SELECT
MIN(step) AS step,
computed_time,
data,
dtype,
shape
MIN(step) AS min_step,
MAX(step) AS max_step
FROM Tensors
INNER JOIN (
SELECT
MIN(step) AS min_step,
MAX(step) AS max_step
FROM Tensors
/* Filter out NULL so we can use TensorSeriesStepIndex. */
WHERE series = :tag_id AND step IS NOT NULL
)
/* Ensure we omit reserved rows, which have NULL step values. */
/* Filter out NULL so we can use TensorSeriesStepIndex. */
WHERE series = :tag_id AND step IS NOT NULL
/* Bucket rows into sample_size linearly spaced buckets, or do
no sampling if sample_size is NULL. */
GROUP BY
IFNULL(:sample_size - 1, max_step - min_step)
* (step - min_step) / (max_step - min_step)
ORDER BY step
''',
{'tag_id': tag_id, 'sample_size': downsample_to})
)
/* Ensure we omit reserved rows, which have NULL step values. */
WHERE series = :tag_id AND step IS NOT NULL
/* Bucket rows into sample_size linearly spaced buckets, or do
no sampling if sample_size is NULL. */
GROUP BY
IFNULL(:sample_size - 1, max_step - min_step)
* (step - min_step) / (max_step - min_step)
ORDER BY step
''',
{'tag_id': tag_id, 'sample_size': downsample_to})
events = [(computed_time, step, self._get_values(data, dtype, shape))
for step, computed_time, data, dtype, shape in cursor]
else:
Expand All @@ -196,11 +230,15 @@ def histograms_impl(self, tag, run, downsample_to=None):
raise ValueError('No histogram tag %r for run %r' % (tag, run))
if downsample_to is not None and len(tensor_events) > downsample_to:
rand_indices = random.Random(0).sample(
six.moves.xrange(len(tensor_events)), downsample_to)
six.moves.xrange(len(tensor_events)), downsample_to)
indices = sorted(rand_indices)
tensor_events = [tensor_events[i] for i in indices]
events = [[e.wall_time, e.step, tensor_util.make_ndarray(e.tensor_proto).tolist()]
for e in tensor_events]

if output_format == OutputFormat.CSV:
return (self._get_csv_response(events), 'text/csv')

return (events, 'application/json')

def _get_values(self, data_blob, dtype_enum, shape_string):
Expand All @@ -225,9 +263,11 @@ def histograms_route(self, request):
"""Given a tag and single run, return array of histogram values."""
tag = request.args.get('tag')
run = request.args.get('run')
output_format = request.args.get('format')
try:
(body, mime_type) = self.histograms_impl(
tag, run, downsample_to=self.SAMPLE_SIZE)
tag, run, downsample_to=self.SAMPLE_SIZE,
output_format=output_format)
code = 200
except ValueError as e:
(body, mime_type) = (str(e), 'text/plain')
Expand Down
29 changes: 29 additions & 0 deletions tensorboard/plugins/histogram/histograms_plugin_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,11 @@
from __future__ import print_function

import collections
import csv
import os.path

import six
from six import StringIO
from six.moves import xrange # pylint: disable=redefined-builtin
import tensorflow as tf

Expand Down Expand Up @@ -173,6 +175,33 @@ def test_histograms_with_histogram(self):
self._test_histograms(self._RUN_WITH_HISTOGRAM,
'%s/histogram_summary' % self._HISTOGRAM_TAG)

def _test_histograms_csv(self, run_name, tag_name, should_work=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't actually executed as a test, it would need to start with test (no underscore). The code I referenced in the scalars plugin is a helper that's called by other test methods, sorry if that wasn't clear.

For this code, you would want to put this helper up next to the existing _test_histograms() renaming that one to _test_histograms_json() and then update the above test_histograms_with_* test methods to test_histograms_json_with_*, and then add three new copies that are test_histograms_csv_with_* that call this helper.

See the rest of the scalars test for what this should look like:

def test_scalars_json_with_legacy_scalars(self):
self._test_scalars_json(self._RUN_WITH_LEGACY_SCALARS,
self._LEGACY_SCALAR_TAG)
def test_scalars_json_with_scalars(self):
self._test_scalars_json(self._RUN_WITH_SCALARS,
'%s/scalar_summary' % self._SCALAR_TAG)
def test_scalars_json_with_histogram(self):
self._test_scalars_json(self._RUN_WITH_HISTOGRAM, self._HISTOGRAM_TAG,
should_work=False)
def test_scalars_csv_with_legacy_scalars(self):
self._test_scalars_csv(self._RUN_WITH_LEGACY_SCALARS,
self._LEGACY_SCALAR_TAG)
def test_scalars_csv_with_scalars(self):
self._test_scalars_csv(self._RUN_WITH_SCALARS,
'%s/scalar_summary' % self._SCALAR_TAG)
def test_scalars_csv_with_histogram(self):
self._test_scalars_csv(self._RUN_WITH_HISTOGRAM, self._HISTOGRAM_TAG,
should_work=False)

self.set_up_with_runs([self._RUN_WITH_LEGACY_HISTOGRAM,
self._RUN_WITH_HISTOGRAM])
if should_work:
(data, mime_type) = self.plugin.histograms_impl(
tag_name, run_name,
output_format=histograms_plugin.OutputFormat.CSV)
self.assertEqual('text/csv', mime_type)
s = StringIO(data)
reader = csv.reader(s)
self.assertEqual(['Wall time', 'Step', 'BinStart',
'BinEnd', 'BinValue'], next(reader))
self.assertEqual(len(list(reader)), self._STEPS)
else:
with self.assertRaises(KeyError):
self.plugin.histograms_impl(
self._HISTOGRAM_TAG, run_name,
output_format=histograms_plugin.OutputFormat.CSV)

def test_histograms_csv_with_legacy_histograms(self):
self._test_histograms_csv(self._RUN_WITH_LEGACY_HISTOGRAM,
self._LEGACY_HISTOGRAM_TAG)

def test_histograms_csv_with_histogram(self):
self._test_histograms_csv(self._RUN_WITH_HISTOGRAM, self._HISTOGRAM_TAG,
Copy link
Contributor

Choose a reason for hiding this comment

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

    self._test_histograms_csv(self._RUN_WITH_HISTOGRAM, self._HISTOGRAM_TAG,
                              should_work=False)

This indeed fails, but not for the right reason. A histogram run with a
histogram tag should work, but you’ve specified the wrong tag: it
should be "%s/histogram_summary" % self._HISTOGRAM_TAG, as listed in
the test_histograms_with_histogram test case above.

Please add a test case that actually should fail (e.g., requesting a
scalar tag on a histograms run).

should_work=False)

def test_active_with_legacy_histogram(self):
self.set_up_with_runs([self._RUN_WITH_LEGACY_HISTOGRAM])
self.assertTrue(self.plugin.is_active())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@
<template>
<tf-dashboard-layout>
<div class="sidebar">
<div class="sidebar-section">
<div class="line-item">
<paper-checkbox
id="show-download-links"
checked="{{_showDownloadLinks}}"
>Show data download links</paper-checkbox>
</div>
</div>
<div class="sidebar-section">
<tf-option-selector
id="histogramModeSelector"
Expand All @@ -66,7 +74,7 @@
<tf-runs-selector selected-runs="{{_selectedRuns}}">
</tf-runs-selector>
</div>
</div>
</div>
</div>
<div class="center">
<template is="dom-if" if="[[_dataNotFound]]">
Expand Down Expand Up @@ -107,6 +115,7 @@ <h3>No histogram data was found.</h3>
tag="[[item.tag]]"
tag-metadata="[[_tagMetadata(_runToTagInfo, item.run, item.tag)]]"
time-property="[[_timeProperty]]"
show-download-links="[[_showDownloadLinks]]"
histogram-mode="[[_histogramMode]]"
request-manager="[[_requestManager]]"
></tf-histogram-loader>
Expand Down Expand Up @@ -141,6 +150,13 @@ <h3>No histogram data was found.</h3>
type: String,
value: "step",
},
_showDownloadLinks: {
type: Boolean,
notify: true,
value: tf_storage.getBooleanInitializer('_showDownloadLinks',
{defaultValue: false, useLocalStorage: true}),
observer: '_showDownloadLinksObserver',
},

_selectedRuns: Array,
_runToTag: Object, // map<run: string, tags: string[]>
Expand Down Expand Up @@ -179,6 +195,9 @@ <h3>No histogram data was found.</h3>
'content-visibility-changed': '_redrawCategoryPane',
},

_showDownloadLinksObserver: tf_storage.getBooleanObserver(
'_showDownloadLinks', {defaultValue: false, useLocalStorage: true}),

_redrawCategoryPane(event, val) {
if (!val) return;
event.target.querySelectorAll('tf-histogram-loader')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,28 @@
icon="fullscreen"
on-tap="_toggleExpanded"
></paper-icon-button>
<template is="dom-if" if="[[showDownloadLinks]]">
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than reimplementing this, please use the tf-downloader element.
You can see an example of its use in tf-scalar-card as of #1846.

<div class="download-links" style="display: flex; flex-direction: row;">
<paper-dropdown-menu
no-label-float="true"
label="run to download"
selected-item-label="{{_runToDownload}}"
>
<paper-menu class="dropdown-content" slot="dropdown-content">
<template is="dom-repeat" items="[[dataToLoad]]">
<paper-item no-label-float=true>[[item.run]]</paper-item>
</template>
</paper-menu>
</paper-dropdown-menu>
<a
download="run_[[_runToDownload]]-tag-[[tag]].csv"
href="[[_csvUrl(tag, _runToDownload)]]"
>CSV</a> <a
download="run_[[_runToDownload]]-tag-[[tag]].json"
href="[[_jsonUrl(tag, _runToDownload)]]"
>JSON</a>
</div>
</template>
</div>
<style>
:host {
Expand Down Expand Up @@ -91,6 +113,36 @@
margin-bottom: 10px;
width: 90%;
}

.download-links {
display: flex;
height: 32px;
}

.download-links a {
align-self: center;
font-size: 10px;
margin: 2px;
}

.download-links paper-dropdown-menu {
width: 100px;
--paper-input-container-label: {
font-size: 10px;
};
--paper-input-container-input: {
font-size: 10px;
}
}

paper-menu-button {
padding: 0;
}
paper-item a {
color: inherit;
text-decoration: none;
white-space: nowrap;
}
</style>
</template>
<script>
Expand All @@ -114,8 +166,11 @@
value: () => ({tag, run}) => {
const router = tf_backend.getRouter();
return tf_backend.addParams(
router.pluginRoute('histograms', '/histograms'),
{tag, run});
router.pluginRoute('histograms', '/histograms'),
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this reformatting change.

tag,
run,
});
},
},
loadDataCallback: {
Expand All @@ -128,6 +183,7 @@
};
},
},

/** @type {{description: string, displayName: string}} */
tagMetadata: Object,
timeProperty: String,
Expand Down Expand Up @@ -155,6 +211,16 @@

behaviors: [tf_dashboard_common.DataLoaderBehavior],


_csvUrl(tag, run) {
return tf_backend.addParams(
this.getDataLoadUrl({tag, run}),
{format: 'csv'});
},
_jsonUrl(tag, run) {
return this.getDataLoadUrl({tag, run});
},

_computeDataToLoad(run, tag) {
return [{run, tag}];
},
Expand Down
1 change: 1 addition & 0 deletions tensorboard/plugins/scalar/scalars_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,5 @@ def scalars_route(self, request):
experiment = request.args.get('experiment')
output_format = request.args.get('format')
(body, mime_type) = self.scalars_impl(tag, run, experiment, output_format)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this newline.

return http_util.Respond(request, body, mime_type)