uploader: add --json flag to the list command
#3480
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| # Copyright 2019 The TensorFlow Authors. All Rights Reserved. | ||
|
There was a problem hiding this comment. nit: 2019 → 2020 (and test) There was a problem hiding this comment. Done here and in |
||
| # | ||
| # 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. | ||
| # ============================================================================== | ||
| """Helpers that format experiment metadata as strings.""" | ||
|
|
||
| from __future__ import absolute_import | ||
| from __future__ import division | ||
| from __future__ import print_function | ||
|
|
||
| import collections | ||
| import json | ||
|
|
||
| EXPERIMENT_METADATA_URL_JSON_KEY = "url" | ||
| ExperimentMetadataField = collections.namedtuple( | ||
| "ExperimentMetadataField", | ||
| ("json_key", "readable_name", "value", "formatter"), | ||
|
There was a problem hiding this comment. I was confused by the name collision of "formatter". Can this one be "field_formatter" or "to_readable" or similar? Also, it still feels a bit intertwined to pass a lambda that is specifically used only in the ReadableFormatter case. I think you can remove the field entirely and automate this. In The typecheck there is a little iffy (because, as written, There was a problem hiding this comment. Adding a formatter interface sounds reasonable to me, but this doesn’t Why introduce a new type with a big list of vague “fields” when we class ExperimentFormatter(metaclass=abc.ABCMeta):
def format(self, experiment):
"""Format an experiment.
Args:
experiment: An `experiment_pb2.Experiment` value.
Returns:
A string.
"""
passThe Then, you could remove the giant block of field definitions in There was a problem hiding this comment. Done effecting the refactoring that you suggested. This revision has pushed a lot of the variables related to the formatting into Also note that I changed the signature of the |
||
| ) | ||
|
|
||
|
|
||
| class BaseExperimentMetadataFormatter(object): | ||
| """Abstract base class for formatting experiment metadata as a string.""" | ||
|
|
||
| def format_experiment(self, experiment_metadata): | ||
| """Format a list of `ExperimentMetadataField`s as a representing string. | ||
|
|
||
| Args: | ||
| experiment_metadata: A list of `ExperimentMetadataField`s that | ||
| describes an experiment. | ||
|
|
||
| Returns: | ||
| A string that represents the `experiment_metadata`. | ||
| """ | ||
| raise NotImplementedError() | ||
|
There was a problem hiding this comment. In Python it’s generally preferable to If you want to ensure that all concrete subclasses actually implement |
||
|
|
||
|
|
||
| class ReadableFormatter(BaseExperimentMetadataFormatter): | ||
| """A formatter implementation that outputs human-readable text.""" | ||
|
|
||
| def __init__(self, name_column_width): | ||
| """Constructor of ReadableFormatter. | ||
|
|
||
| Args: | ||
| name_column_width: The width of the column that contains human-readable | ||
| field names (i.e., `readable_name` in `ExperimentMetadataField`). | ||
| Must be greater than the longest human-readable field name. | ||
| """ | ||
| super(ReadableFormatter, self).__init__() | ||
| self._name_column_width = name_column_width | ||
|
|
||
| def format_experiment(self, experiment_metadata): | ||
| output = [] | ||
| for metadata_field in experiment_metadata: | ||
| if metadata_field.json_key == EXPERIMENT_METADATA_URL_JSON_KEY: | ||
| output.append(metadata_field.value) | ||
| else: | ||
| output.append( | ||
| "\t%s %s" | ||
| % ( | ||
| metadata_field.readable_name.ljust( | ||
| self._name_column_width | ||
| ), | ||
| metadata_field.formatter(metadata_field.value), | ||
| ) | ||
| ) | ||
| return "\n".join(output) | ||
|
|
||
|
|
||
| class JsonFormatter(object): | ||
| """A formatter implementation: outputs metadata of an experiment as JSON.""" | ||
|
|
||
| def __init__(self, indent): | ||
| """Constructor of JsonFormatter. | ||
|
|
||
| Args: | ||
| indent: Size of indentation (in number of spaces) used for JSON | ||
| formatting. | ||
| """ | ||
| super(JsonFormatter, self).__init__() | ||
| self._indent = indent | ||
|
|
||
| def format_experiment(self, experiment_metadata): | ||
| return json.dumps( | ||
| collections.OrderedDict( | ||
| (metadata_field.json_key, metadata_field.value) | ||
| for metadata_field in experiment_metadata | ||
| ), | ||
| indent=self._indent, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| # Copyright 2019 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. | ||
| # ============================================================================== | ||
| # Lint as: python3 | ||
| """Tests for tensorboard.uploader.formatters.""" | ||
|
|
||
| from __future__ import absolute_import | ||
| from __future__ import division | ||
| from __future__ import print_function | ||
|
|
||
| from tensorboard import test as tb_test | ||
| from tensorboard.uploader import formatters | ||
|
|
||
|
|
||
| class TensorBoardExporterTest(tb_test.TestCase): | ||
| def testReadableFormatterWithNonemptyNameAndDescription(self): | ||
| data = [ | ||
| formatters.ExperimentMetadataField( | ||
| formatters.EXPERIMENT_METADATA_URL_JSON_KEY, | ||
| "URL", | ||
| "http://tensorboard.dev/deadbeef", | ||
| str, | ||
| ), | ||
| formatters.ExperimentMetadataField( | ||
| "name", | ||
| "Name", | ||
| "A name for the experiment", | ||
| lambda x: x or "[No Name]", | ||
| ), | ||
| formatters.ExperimentMetadataField( | ||
| "description", | ||
| "Description", | ||
| "A description for the experiment", | ||
| lambda x: x or "[No Description]", | ||
| ), | ||
| ] | ||
| formatter = formatters.ReadableFormatter(12) | ||
| output = formatter.format_experiment(data) | ||
| lines = output.split("\n") | ||
| self.assertLen(lines, 3) | ||
| self.assertEqual(lines[0], "http://tensorboard.dev/deadbeef") | ||
| self.assertEqual(lines[1], "\tName A name for the experiment") | ||
| self.assertEqual( | ||
| lines[2], "\tDescription A description for the experiment" | ||
| ) | ||
|
|
||
| def testReadableFormatterWithEmptyNameAndDescription(self): | ||
| data = [ | ||
| formatters.ExperimentMetadataField( | ||
| formatters.EXPERIMENT_METADATA_URL_JSON_KEY, | ||
| "URL", | ||
| "http://tensorboard.dev/deadbeef", | ||
| str, | ||
| ), | ||
| formatters.ExperimentMetadataField( | ||
| "name", "Name", "", lambda x: x or "[No Name]", | ||
| ), | ||
| formatters.ExperimentMetadataField( | ||
| "description", | ||
| "Description", | ||
| "", | ||
| lambda x: x or "[No Description]", | ||
| ), | ||
| ] | ||
| formatter = formatters.ReadableFormatter(12) | ||
| output = formatter.format_experiment(data) | ||
| lines = output.split("\n") | ||
| self.assertLen(lines, 3) | ||
| self.assertEqual(lines[0], "http://tensorboard.dev/deadbeef") | ||
| self.assertEqual(lines[1], "\tName [No Name]") | ||
| self.assertEqual(lines[2], "\tDescription [No Description]") | ||
|
|
||
| def testJsonFormatterWithEmptyNameAndDescription(self): | ||
| data = [ | ||
| formatters.ExperimentMetadataField( | ||
| formatters.EXPERIMENT_METADATA_URL_JSON_KEY, | ||
| "URL", | ||
| "http://tensorboard.dev/deadbeef", | ||
| str, | ||
| ), | ||
| formatters.ExperimentMetadataField( | ||
| "name", "Name", "", lambda x: x or "[No Name]", | ||
| ), | ||
| formatters.ExperimentMetadataField( | ||
| "description", | ||
| "Description", | ||
| "", | ||
| lambda x: x or "[No Description]", | ||
| ), | ||
| formatters.ExperimentMetadataField("runs", "Runs", 8, str,), | ||
| formatters.ExperimentMetadataField("tags", "Tags", 12, str,), | ||
| formatters.ExperimentMetadataField( | ||
| "binary_object_bytes", "Binary object bytes", 2000, str, | ||
| ), | ||
| ] | ||
| formatter = formatters.JsonFormatter(2) | ||
| output = formatter.format_experiment(data) | ||
| lines = output.split("\n") | ||
| self.assertLen(lines, 8) | ||
| self.assertEqual(lines[0], "{") | ||
| self.assertEqual( | ||
| lines[1], ' "url": "http://tensorboard.dev/deadbeef",' | ||
| ) | ||
| self.assertEqual(lines[2], ' "name": "",') | ||
| self.assertEqual(lines[3], ' "description": "",') | ||
| self.assertEqual(lines[4], ' "runs": 8,') | ||
| self.assertEqual(lines[5], ' "tags": 12,') | ||
| self.assertEqual(lines[6], ' "binary_object_bytes": 2000') | ||
| self.assertEqual(lines[7], "}") | ||
|
There was a problem hiding this comment. Can we just expected_lines = [
"{",
' "url": "http://tensorboard.dev/deadbeef",',
# ...
"}"
]
self.assertEqual(lines, expected_lines)rather than asserting on each line individually? It’s easier to read, Alternative, you could just |
||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| tb_test.main() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bileschi As pointed out by @wchargin, "prior art" varies. So I opted to err on the side of conciseness. Also see my earlier comments regarding this.