From 06a24bc06fa9e4f90c2ed7a4dfe7372e380677e4 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Tue, 12 Mar 2019 17:20:15 -0700 Subject: [PATCH 1/6] init: fix top-level module reloading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Fixes #1989 (also described in a new comment in `__init__.py`). This will require a Google-internal sync change, which I’ll link in shortly. Test Plan: Regression test added. It fails before this change in Python 2 and 3, and passes after this change in both as well. wchargin-branch: reload-tensorboard --- tensorboard/BUILD | 12 +++++++++ tensorboard/__init__.py | 55 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/tensorboard/BUILD b/tensorboard/BUILD index e4a8773f0e..b47d4cb373 100644 --- a/tensorboard/BUILD +++ b/tensorboard/BUILD @@ -71,6 +71,18 @@ py_library( ], ) +py_test( + name = "lib_test", + srcs = ["lib_test.py"], + srcs_version = "PY2AND3", + visibility = ["//tensorboard:internal"], + deps = [ + ":lib", + "@org_pythonhosted_six", + ], +) + + py_library( name = "manager", srcs = ["manager.py"], diff --git a/tensorboard/__init__.py b/tensorboard/__init__.py index 6737c220d8..be4a2c263c 100644 --- a/tensorboard/__init__.py +++ b/tensorboard/__init__.py @@ -22,19 +22,62 @@ from tensorboard import lazy +# Please be careful when changing the structure of this file. +# +# The lazy imports in this file must use `importlib.import_module`, not +# `import tensorboard.foo` or `from tensorboard import foo`, or it will +# be impossible to reload the TensorBoard module without breaking these +# top-level public APIs. This has to do with the gory details of +# Python's module system. Take `tensorboard.notebook` as an example: +# +# - When the `tensorboard` module (that's us!) is initialized, its +# `notebook` attribute is initialized to a new LazyModule. The +# actual `tensorboard.notebook` submodule is not loaded. +# +# - When the `tensorboard.notebook` submodule is first loaded, Python +# _reassigns_ the `notebook` attribute on the `tensorboard` module +# object to point to the underlying `tensorboard.notebook` module +# object, rather than its former LazyModule value. This occurs +# whether the module is loaded via the lazy module or directly as an +# import: +# +# - import tensorboard; tensorboard.notebook.start(...) # one way +# - from tensorboard import notebook # other way; same effect +# +# - When the `tensorboard` module is reloaded, its `notebook` +# attribute is once again bound to a (new) LazyModule, while the +# `tensorboard.notebook` module object is unaffected and still +# exists in `sys.modules`. But then... +# +# - When the new LazyModule is forced, it must resolve to the existing +# `tensorboard.notebook` module object rather than itself (which +# just creates a stack overflow). If the LazyModule load function +# uses `import tensorboard.notebook; return tensorboard.notebook`, +# then the first statement will do _nothing_ because the +# `tensorboard.notebook` module is already loaded, and the second +# statement will return the LazyModule itself. The same goes for the +# `from tensorboard import notebook` form. We need to ensure that +# the submodule is loaded and then pull the actual module object out +# of `sys.modules`... which is exactly what `importlib` handles for +# us. +# +# See for +# additional discussion. + + @lazy.lazy_load('tensorboard.notebook') def notebook(): - import tensorboard.notebook as module # pylint: disable=g-import-not-at-top - return module + import importlib # pylint: disable=g-import-not-at-top + return importlib.import_module('tensorboard.notebook') @lazy.lazy_load('tensorboard.program') def program(): - import tensorboard.program as module # pylint: disable=g-import-not-at-top - return module + import importlib # pylint: disable=g-import-not-at-top + return importlib.import_module("tensorboard.program") @lazy.lazy_load('tensorboard.summary') def summary(): - import tensorboard.summary as module # pylint: disable=g-import-not-at-top - return module + import importlib # pylint: disable=g-import-not-at-top + return importlib.import_module("tensorboard.summary") From 09f5840979a4135d7cd0940c58968b94597a47e6 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Tue, 12 Mar 2019 17:52:11 -0700 Subject: [PATCH 2/6] fixup: actually add the test wchargin-branch: reload-tensorboard --- tensorboard/lib_test.py | 43 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tensorboard/lib_test.py diff --git a/tensorboard/lib_test.py b/tensorboard/lib_test.py new file mode 100644 index 0000000000..157322447e --- /dev/null +++ b/tensorboard/lib_test.py @@ -0,0 +1,43 @@ +# 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. + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +from six import moves +import sys +import unittest + + +class StopshipTest(unittest.TestCase): + + def test_functional_after_reload(self): + self.assertNotIn("tensorboard", sys.modules) + import tensorboard as tensorboard # it makes the Google sync happy + submodules = ["notebook", "program", "summary"] + dirs_before = { + module_name: dir(getattr(tensorboard, module_name)) + for module_name in submodules + } + tensorboard = moves.reload_module(tensorboard) + dirs_after = { + module_name: dir(getattr(tensorboard, module_name)) + for module_name in submodules + } + self.assertEqual(dirs_before, dirs_after) + + +if __name__ == '__main__': + unittest.main() From 7069c157ab5047127bb60ba89fa1b36d0e7bb283 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Tue, 12 Mar 2019 17:57:15 -0700 Subject: [PATCH 3/6] fixup: use single quotes consistently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (Just for consistency with the rest of the module—we’re not so far down the rabbit hole that this is a functional change.) wchargin-branch: reload-tensorboard --- tensorboard/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorboard/__init__.py b/tensorboard/__init__.py index be4a2c263c..0594707ec0 100644 --- a/tensorboard/__init__.py +++ b/tensorboard/__init__.py @@ -74,10 +74,10 @@ def notebook(): @lazy.lazy_load('tensorboard.program') def program(): import importlib # pylint: disable=g-import-not-at-top - return importlib.import_module("tensorboard.program") + return importlib.import_module('tensorboard.program') @lazy.lazy_load('tensorboard.summary') def summary(): import importlib # pylint: disable=g-import-not-at-top - return importlib.import_module("tensorboard.summary") + return importlib.import_module('tensorboard.summary') From 3f031d28cc08c6ef8a103d8c3bba1c1a5b118932 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Tue, 12 Mar 2019 18:08:54 -0700 Subject: [PATCH 4/6] fixup: remove spurious newline in BUILD file wchargin-branch: reload-tensorboard --- tensorboard/BUILD | 1 - 1 file changed, 1 deletion(-) diff --git a/tensorboard/BUILD b/tensorboard/BUILD index b47d4cb373..8ea9af4179 100644 --- a/tensorboard/BUILD +++ b/tensorboard/BUILD @@ -82,7 +82,6 @@ py_test( ], ) - py_library( name = "manager", srcs = ["manager.py"], From ec57670bc0fceb618285f305417261ca3328cb3a Mon Sep 17 00:00:00 2001 From: William Chargin Date: Tue, 12 Mar 2019 18:33:12 -0700 Subject: [PATCH 5/6] fixup: rename test class wchargin-branch: reload-tensorboard --- tensorboard/lib_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/lib_test.py b/tensorboard/lib_test.py index 157322447e..aab06b023e 100644 --- a/tensorboard/lib_test.py +++ b/tensorboard/lib_test.py @@ -21,7 +21,7 @@ import unittest -class StopshipTest(unittest.TestCase): +class ReloadTensorBoardTest(unittest.TestCase): def test_functional_after_reload(self): self.assertNotIn("tensorboard", sys.modules) From 93eac037a46daf86bef4ba7a7d607e14c343b017 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Tue, 12 Mar 2019 18:33:53 -0700 Subject: [PATCH 6/6] fixup: add size=small attribute wchargin-branch: reload-tensorboard --- tensorboard/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/tensorboard/BUILD b/tensorboard/BUILD index 8ea9af4179..239219ac28 100644 --- a/tensorboard/BUILD +++ b/tensorboard/BUILD @@ -73,6 +73,7 @@ py_library( py_test( name = "lib_test", + size = "small", srcs = ["lib_test.py"], srcs_version = "PY2AND3", visibility = ["//tensorboard:internal"],