From 4b58f8f12549d5df64ad5f26d96dd7b7bc0ce283 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Fri, 26 Jul 2019 11:44:58 -0700 Subject: [PATCH 1/2] notebook: improve cross-version compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: The `manager` and `notebook` modules no longer choke when there are running TensorBoard instances on older or newer versions. We also now silently ignore invalid files. There isn’t really any useful information to report out to the user. Fixes #2453. Fixes #2467. Test Plan: Launch TensorBoard version 1.14.0 (stable), and keep it running in the background. Then, create a new virtualenv with Jupyter and latest nightly TensorBoard. Launch Jupyter, and execute the following cell: ``` %load_ext tensorboard %tensorboard --logdir /tmp/whatever ``` Before this patch, this would successfully launch TensorBoard, but also spam a bunch of “incompatible version” log warnings. Those warnings are now gone. Then run ``` from tensorboard import notebook notebook.list() ``` and note that the list contains _both_ TensorBoards, even though they come from different versions. Prior to this patch, this would have logged a warning and shown only the TensorBoard running on the new code. wchargin-branch: notebook-cross-version --- tensorboard/manager.py | 31 +++++++++++++++---------------- tensorboard/manager_test.py | 30 +++++++++++------------------- 2 files changed, 26 insertions(+), 35 deletions(-) diff --git a/tensorboard/manager.py b/tensorboard/manager.py index 609c8d21ec..df58242b32 100644 --- a/tensorboard/manager.py +++ b/tensorboard/manager.py @@ -139,10 +139,8 @@ def _info_from_string(info_string): A `TensorBoardInfo` value. Raises: - ValueError: If the provided string is not valid JSON, or if it does - not represent a JSON object with a "version" field whose value is - `tensorboard.version.VERSION`, or if it has the wrong set of - fields, or if at least one field is of invalid type. + ValueError: If the provided string is not valid JSON, or if it is + missing any required fields, or if any field is of incorrect type. """ try: @@ -151,17 +149,18 @@ def _info_from_string(info_string): raise ValueError("invalid JSON: %r" % (info_string,)) if not isinstance(json_value, dict): raise ValueError("not a JSON object: %r" % (json_value,)) - if json_value.get("version") != version.VERSION: - raise ValueError("incompatible version: %r" % (json_value,)) expected_keys = frozenset(_TENSORBOARD_INFO_FIELDS) actual_keys = frozenset(json_value) - if expected_keys != actual_keys: + missing_keys = expected_keys - actual_keys + if missing_keys: raise ValueError( - "bad keys on TensorBoardInfo (missing: %s; extraneous: %s)" - % (expected_keys - actual_keys, actual_keys - expected_keys) + "TensorBoardInfo missing keys: %r" + % (sorted(missing_keys),) ) + # For forward compatibility, silently ignore unknown keys. # Validate and deserialize fields. + fields = {} for key in _TENSORBOARD_INFO_FIELDS: field_type = _TENSORBOARD_INFO_FIELDS[key] if not isinstance(json_value[key], field_type.serialized_type): @@ -169,9 +168,9 @@ def _info_from_string(info_string): "expected %r of type %s, but found: %r" % (key, field_type.serialized_type, json_value[key]) ) - json_value[key] = field_type.deserialize(json_value[key]) + fields[key] = field_type.deserialize(json_value[key]) - return TensorBoardInfo(**json_value) + return TensorBoardInfo(**fields) def cache_key(working_directory, arguments, configure_kwargs): @@ -295,6 +294,9 @@ def get_all(): contain extraneous entries if TensorBoard processes exited uncleanly (e.g., with SIGKILL or SIGQUIT). + Entries in the info directory that do not represent valid + `TensorBoardInfo` values will be silently ignored. + Returns: A fresh list of `TensorBoardInfo` objects. """ @@ -315,11 +317,8 @@ def get_all(): try: info = _info_from_string(contents) except ValueError: - tb_logging.get_logger().warning( - "invalid info file: %r", - filepath, - exc_info=True, - ) + # Ignore unrecognized files. + pass else: results.append(info) return results diff --git a/tensorboard/manager_test.py b/tensorboard/manager_test.py index ada20691c2..40c3aa9a2b 100644 --- a/tensorboard/manager_test.py +++ b/tensorboard/manager_test.py @@ -111,30 +111,24 @@ def test_deserialization_rejects_missing_version(self): with six.assertRaisesRegex( self, ValueError, - "incompatible version:"): + re.escape("missing keys: ['version']")): manager._info_from_string(bad_input) - def test_deserialization_rejects_bad_version(self): + def test_deserialization_accepts_future_version(self): info = _make_info() json_value = json.loads(manager._info_to_string(info)) - json_value["version"] = "not likely" - bad_input = json.dumps(json_value) - with six.assertRaisesRegex( - self, - ValueError, - "incompatible version:.*not likely"): - manager._info_from_string(bad_input) + json_value["version"] = "99.99.99a20991232" + input_ = json.dumps(json_value) + result = manager._info_from_string(input_) + self.assertEqual(result.version, "99.99.99a20991232") - def test_deserialization_rejects_extra_keys(self): + def test_deserialization_ignores_extra_keys(self): info = _make_info() json_value = json.loads(manager._info_to_string(info)) json_value["unlikely"] = "story" bad_input = json.dumps(json_value) - with six.assertRaisesRegex( - self, - ValueError, - "bad keys on TensorBoardInfo"): - manager._info_from_string(bad_input) + result = manager._info_from_string(bad_input) + self.assertIsInstance(result, manager.TensorBoardInfo) def test_deserialization_rejects_missing_keys(self): info = _make_info() @@ -144,7 +138,7 @@ def test_deserialization_rejects_missing_keys(self): with six.assertRaisesRegex( self, ValueError, - "bad keys on TensorBoardInfo"): + re.escape("missing keys: ['start_time']")): manager._info_from_string(bad_input) def test_deserialization_rejects_bad_types(self): @@ -401,9 +395,7 @@ def test_get_all_ignores_bad_files(self): with open(os.path.join(self.info_dir, "pid-9012.info"), "w") as outfile: outfile.write('if a tbinfo has st_mode==0, does it make a sound?\n') os.chmod(os.path.join(self.info_dir, "pid-9012.info"), 0o000) - with mock.patch.object(tb_logging.get_logger(), "warning") as fn: - self.assertEqual(manager.get_all(), []) - self.assertEqual(fn.call_count, 2) # 2 invalid, 1 unreadable (silent) + self.assertEqual(manager.get_all(), []) if __name__ == "__main__": From 12477fdcd9565ad95923873ef09b8dbf08c5f7c3 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Fri, 26 Jul 2019 13:48:37 -0700 Subject: [PATCH 2/2] [update patch] wchargin-source: 2dd9a9ef1b0ad2b1b8031dc06921c7b1e1141794 wchargin-branch: notebook-cross-version --- tensorboard/manager.py | 8 ++++++-- tensorboard/manager_test.py | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/tensorboard/manager.py b/tensorboard/manager.py index df58242b32..f6495bdc00 100644 --- a/tensorboard/manager.py +++ b/tensorboard/manager.py @@ -317,8 +317,12 @@ def get_all(): try: info = _info_from_string(contents) except ValueError: - # Ignore unrecognized files. - pass + # Ignore unrecognized files, logging at debug only. + tb_logging.get_logger().debug( + "invalid info file: %r", + filepath, + exc_info=True, + ) else: results.append(info) return results diff --git a/tensorboard/manager_test.py b/tensorboard/manager_test.py index 40c3aa9a2b..7cdf7f9f23 100644 --- a/tensorboard/manager_test.py +++ b/tensorboard/manager_test.py @@ -395,7 +395,9 @@ def test_get_all_ignores_bad_files(self): with open(os.path.join(self.info_dir, "pid-9012.info"), "w") as outfile: outfile.write('if a tbinfo has st_mode==0, does it make a sound?\n') os.chmod(os.path.join(self.info_dir, "pid-9012.info"), 0o000) - self.assertEqual(manager.get_all(), []) + with mock.patch.object(tb_logging.get_logger(), "debug") as fn: + self.assertEqual(manager.get_all(), []) + self.assertEqual(fn.call_count, 2) # 2 invalid, 1 unreadable (silent) if __name__ == "__main__":