-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Reincarnate TensorBoardWSGIApp as new DataProvider-friendly entry point #2576
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
Changes from all commits
d736dee
f0dcd0b
15a6368
399061e
6b696b1
032fda8
934ed98
cc8c7d3
8e2f334
4b93e80
a50d21f
ddaf663
6bc5e43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,7 @@ | |
| # ============================================================================== | ||
| """TensorBoard WSGI Application Logic. | ||
|
|
||
| TensorBoardApplication constructs TensorBoard as a WSGI application. | ||
| It handles serving static assets, and implements TensorBoard data APIs. | ||
| Provides TensorBoardWSGIApp for building a TensorBoard WSGI app. | ||
| """ | ||
|
|
||
| from __future__ import absolute_import | ||
|
|
@@ -107,64 +106,98 @@ def standard_tensorboard_wsgi(flags, plugin_loaders, assets_zip_provider): | |
| :type plugin_loaders: list[base_plugin.TBLoader] | ||
| :rtype: TensorBoardWSGI | ||
| """ | ||
| event_file_active_filter = _get_event_file_active_filter(flags) | ||
| multiplexer = event_multiplexer.EventMultiplexer( | ||
| size_guidance=DEFAULT_SIZE_GUIDANCE, | ||
| tensor_size_guidance=tensor_size_guidance_from_flags(flags), | ||
| purge_orphaned_data=flags.purge_orphaned_data, | ||
| max_reload_threads=flags.max_reload_threads, | ||
| event_file_active_filter=event_file_active_filter) | ||
| if flags.generic_data == 'false': | ||
| data_provider = None | ||
| else: | ||
| data_provider = event_data_provider.MultiplexerDataProvider(multiplexer) | ||
| loading_multiplexer = multiplexer | ||
| data_provider = None | ||
| multiplexer = None | ||
| reload_interval = flags.reload_interval | ||
| db_uri = flags.db | ||
| db_connection_provider = None | ||
| # For DB import mode, create a DB file if we weren't given one. | ||
| if flags.db_import and not flags.db: | ||
| tmpdir = tempfile.mkdtemp(prefix='tbimport') | ||
| atexit.register(shutil.rmtree, tmpdir) | ||
| db_uri = 'sqlite:%s/tmp.sqlite' % tmpdir | ||
| if flags.db_import: | ||
| # DB import mode. | ||
| logger.info('Importing logdir into DB at %s', db_uri) | ||
| db_uri = flags.db | ||
| # Create a temporary DB file if we weren't given one. | ||
| if not db_uri: | ||
| tmpdir = tempfile.mkdtemp(prefix='tbimport') | ||
| atexit.register(shutil.rmtree, tmpdir) | ||
| db_uri = 'sqlite:%s/tmp.sqlite' % tmpdir | ||
| db_connection_provider = create_sqlite_connection_provider(db_uri) | ||
| loading_multiplexer = db_import_multiplexer.DbImportMultiplexer( | ||
| logger.info('Importing logdir into DB at %s', db_uri) | ||
| multiplexer = db_import_multiplexer.DbImportMultiplexer( | ||
| db_uri=db_uri, | ||
| db_connection_provider=db_connection_provider, | ||
| purge_orphaned_data=flags.purge_orphaned_data, | ||
| max_reload_threads=flags.max_reload_threads) | ||
| elif flags.db: | ||
| # DB read-only mode, never load event logs. | ||
| reload_interval = -1 | ||
| db_connection_provider = create_sqlite_connection_provider(db_uri) | ||
| db_connection_provider = create_sqlite_connection_provider(flags.db) | ||
| multiplexer = _DbModeMultiplexer(flags.db, db_connection_provider) | ||
| else: | ||
| # Regular logdir loading mode. | ||
| multiplexer = event_multiplexer.EventMultiplexer( | ||
| size_guidance=DEFAULT_SIZE_GUIDANCE, | ||
| tensor_size_guidance=tensor_size_guidance_from_flags(flags), | ||
| purge_orphaned_data=flags.purge_orphaned_data, | ||
| max_reload_threads=flags.max_reload_threads, | ||
| event_file_active_filter=_get_event_file_active_filter(flags)) | ||
| if flags.generic_data != 'false': | ||
| data_provider = event_data_provider.MultiplexerDataProvider(multiplexer) | ||
|
|
||
| if reload_interval >= 0: | ||
| # We either reload the multiplexer once when TensorBoard starts up, or we | ||
| # continuously reload the multiplexer. | ||
| path_to_run = parse_event_files_spec(flags.logdir) | ||
| start_reloading_multiplexer( | ||
| multiplexer, path_to_run, reload_interval, flags.reload_task) | ||
| return TensorBoardWSGIApp( | ||
| flags, plugin_loaders, data_provider, assets_zip_provider, multiplexer) | ||
|
|
||
|
|
||
| def TensorBoardWSGIApp( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we take this opportunity to give this function a better name? Its Perhaps something like
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My plan (in #2573, one of the WSGI cleanup items) was to convert this into the actual plugin class and remove TensorBoardWSGI. This is blocked on updating the plugin tests so they don't need it as a shim, which I was going to do as a cleanup task to avoid blocking this PR. So that's why I left it named in class style.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM. |
||
| flags, | ||
| plugins, | ||
| data_provider=None, | ||
| assets_zip_provider=None, | ||
| deprecated_multiplexer=None): | ||
| """Constructs a TensorBoard WSGI app from plugins and data providers. | ||
|
|
||
| Args: | ||
| flags: An argparse.Namespace containing TensorBoard CLI flags. | ||
| plugins: A list of TBLoader subclasses for the plugins to load. | ||
| assets_zip_provider: See TBContext documentation for more information. | ||
| data_provider: Instance of `tensorboard.data.provider.DataProvider`. May | ||
| be `None` if `flags.generic_data` is set to `"false"` in which case | ||
| `deprecated_multiplexer` must be passed instead. | ||
| deprecated_multiplexer: Optional `plugin_event_multiplexer.EventMultiplexer` | ||
| to use for any plugins not yet enabled for the DataProvider API. | ||
| Required if the data_provider argument is not passed. | ||
|
|
||
| Returns: | ||
| A WSGI application that implements the TensorBoard backend. | ||
| """ | ||
| db_uri = None | ||
| db_connection_provider = None | ||
| if isinstance( | ||
| deprecated_multiplexer, | ||
| (db_import_multiplexer.DbImportMultiplexer, _DbModeMultiplexer)): | ||
| db_uri = deprecated_multiplexer.db_uri | ||
| db_connection_provider = deprecated_multiplexer.db_connection_provider | ||
| plugin_name_to_instance = {} | ||
| context = base_plugin.TBContext( | ||
| data_provider=data_provider, | ||
| db_connection_provider=db_connection_provider, | ||
| db_uri=db_uri, | ||
| flags=flags, | ||
| logdir=flags.logdir, | ||
| multiplexer=multiplexer, | ||
| multiplexer=deprecated_multiplexer, | ||
| assets_zip_provider=assets_zip_provider, | ||
| plugin_name_to_instance=plugin_name_to_instance, | ||
| window_title=flags.window_title) | ||
| plugins = [] | ||
| for loader in plugin_loaders: | ||
| tbplugins = [] | ||
| for loader in plugins: | ||
| plugin = loader.load(context) | ||
| if plugin is None: | ||
| continue | ||
| plugins.append(plugin) | ||
| tbplugins.append(plugin) | ||
| plugin_name_to_instance[plugin.plugin_name] = plugin | ||
|
|
||
| if reload_interval >= 0: | ||
| # We either reload the multiplexer once when TensorBoard starts up, or we | ||
| # continuously reload the multiplexer. | ||
| path_to_run = parse_event_files_spec(flags.logdir) | ||
| start_reloading_multiplexer( | ||
| loading_multiplexer, path_to_run, reload_interval, flags.reload_task) | ||
| return TensorBoardWSGI(plugins, flags.path_prefix) | ||
| return TensorBoardWSGI(tbplugins, flags.path_prefix) | ||
|
|
||
|
|
||
| class TensorBoardWSGI(object): | ||
|
|
@@ -531,3 +564,40 @@ def _get_event_file_active_filter(flags): | |
| if inactive_secs < 0: | ||
| return lambda timestamp: True | ||
| return lambda timestamp: timestamp + inactive_secs >= time.time() | ||
|
|
||
|
|
||
| class _DbModeMultiplexer(event_multiplexer.EventMultiplexer): | ||
| """Shim EventMultiplexer to use when in read-only DB mode. | ||
|
|
||
| In read-only DB mode, the EventMultiplexer is nonfunctional - there is no | ||
| logdir to reload, and the data is all exposed via SQL. This class represents | ||
| the do-nothing EventMultiplexer for that purpose, which serves only as a | ||
| conduit for DB-related parameters. | ||
|
|
||
| The load APIs raise exceptions if called, and the read APIs always | ||
| return empty results. | ||
| """ | ||
| def __init__(self, db_uri, db_connection_provider): | ||
| """Constructor for `_DbModeMultiplexer`. | ||
|
|
||
| Args: | ||
| db_uri: A URI to the database file in use. | ||
| db_connection_provider: Provider function for creating a DB connection. | ||
| """ | ||
| logger.info('_DbModeMultiplexer initializing for %s', db_uri) | ||
| super(_DbModeMultiplexer, self).__init__() | ||
| self.db_uri = db_uri | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oof, I see: there’s a db_uri = None
db_connection_provider = None
if isinstance(deprecated_multiplexer, _DbModeMultiplexer):
db_uri = deprecated_multiplexer.db_uri
db_connection_provider = deprecated_multiplexer.db_connection_provider
deprecated_multiplexer = None # don't pass no-op muxer into `TBContext`to also resolve my gripe about including a non-functional multiplexer in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above response about the non-functional multiplexer. I can change the getattr to two different
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced getattrs with isinstance. |
||
| self.db_connection_provider = db_connection_provider | ||
| logger.info('_DbModeMultiplexer done initializing') | ||
|
|
||
| def AddRun(self, path, name=None): | ||
| """Unsupported.""" | ||
| raise NotImplementedError() | ||
|
|
||
| def AddRunsFromDirectory(self, path, name=None): | ||
| """Unsupported.""" | ||
| raise NotImplementedError() | ||
|
|
||
| def Reload(self): | ||
| """Unsupported.""" | ||
| raise NotImplementedError() | ||
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.
It’s a little funny for a non-functional multiplexer to exist in the
TBContext: currently plugins case on DB mode by saying, “if I have aDB connection provider, use DB mode, else use multiplexer mode”, but
they could just as well say, “if I have a multiplexer, use multiplexer
mode, else use DB mode”—but that will no longer work. Concern?
(See also comment on
_DbModeMultiplexer.__init__below.)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.
The problem is basically that I don't trust plugins to all defensively check whether multiplexer is not None. Some of them do this, but inconsistently. For example, the projector plugin checks that
self.multiplexeris not None in only 2 out of 3 uses. The profile plugin doesn't check at all.So rather than trying to go through all the existing plugins to ensure they can handle a non-existent multiplexer, I figured I'd ensure that there is always a dummy placeholder multiplexer that just returns empty data instead.
I realize this isn't that clean, but our near-term goal is to refactor away all the multiplexer-calling code anyway, so I was reluctant to spend much time improving it now.
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.
Got it; that this is all slated to be removed soon changes the equation.
This is fine with me, then.