Reincarnate TensorBoardWSGIApp as new DataProvider-friendly entry point #2576

nfelt · 2019-08-19T20:39:40Z

This reincarnates TensorBoardWSGIApp(), which is removed in #2575, as a new API for constructing the TB WSGI app using a specific DataProvider implementation (with a deprecated argument for passing in a multiplexer instead, the current behavior that we intend to migrate away from).

To do this, we rearrange a lot of the logic in standard_tensorboard_wsgi() so that all three possible paths (regular logdir loading, db import, and db read-only) emit a single multiplexer instance. To that end, I've changed DbImportMultiplexer to implement the read surface (by returning empty data) and introduced a shim _DbModeMultiplexer for the db read-only mode just as a conduit for parameters.

This addresses core tasks 2 and 3 in #2573, with the first four commits corresponding to task 2 and the next 2 to task 3.

cc @bileschi

…xplicit Reload()

…board_wsgi()

…rectly

wchargin

Overall:

Understanding this flow takes a bit of mental gymnastics, but
perhaps it’s still less convoluted than before. I’ll have to let it
sink in a bit.
I like the setup of passing a null object around internally (e.g.,
to start_reloading_multiplexer) to reduce the number of data flow
paths. I like a bit less the setup of using this null object also as
a bag for TBContext attributes, but I can live with it. But
leaking this null object abstraction onto the plugin surface via
context.multiplexer seems worth avoiding, if we can swing it.

wchargin · 2019-08-19T21:05:07Z

tensorboard/backend/application.py

+  if isinstance(plugin, base_plugin.TBLoader):
+    return plugin
+  if issubclass(plugin, base_plugin.TBLoader):
+    return plugin()


This is a nice QOL change; I know that it’s bitten us in the past. Want
to update tensorboard/default.py to use this form for internal
consistency?

Sure, but I'll also need to update program.py to support this form since that's actually where we do the expansion in the current code, and I'd rather not do that in this PR.

I can either leave this functionality in this path only, or I can back out the commit adding it and do the change to both places and default.py in a separate PR. Any preference?

Down the road of course there should be just one path, but I was going to do more refactoring to pull out the contents of standard_tensorboard_wsgi() so that it can be invoked from program.py and then passed into TensorBoardWSGIApp() from there in a clean way.

Ah, didn’t realize that this was an additional path. Up to you.

I've reverted this change for this PR and will send a followup for this separately.

wchargin · 2019-08-19T21:06:09Z

tensorboard/backend/application.py

+    return plugin()
+  if issubclass(plugin, base_plugin.TBPlugin):
+    return base_plugin.BasicLoader(plugin)
+  raise ValueError("Not a TBLoader or TBPlugin subclass: %s" % plugin)


Should this be TypeError? Docs for ValueError say, “Inappropriate
argument value (of correct type).”

Done in the followup.

wchargin · 2019-08-19T21:07:02Z

tensorboard/backend/application.py

+    return plugin()
+  if issubclass(plugin, base_plugin.TBPlugin):
+    return base_plugin.BasicLoader(plugin)
+  raise ValueError("Not a TBLoader or TBPlugin subclass: %s" % plugin)


Consider "…" % (plugin,) to prevent explosions if plugin is a tuple.

Done in the followup.

wchargin · 2019-08-19T21:13:35Z

tensorboard/backend/application.py

+      flags, plugin_loaders, data_provider, assets_zip_provider, multiplexer)
+
+
+def TensorBoardWSGIApp(


Should we take this opportunity to give this function a better name? Its
casing suggests that it’s a type, but it’s a function; and it’s not
clear from the name how this differs from class TensorBoardWSGI.

Perhaps something like build_tensorboard_wsgi connotes its purpose?

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.

wchargin · 2019-08-19T21:14:58Z

tensorboard/backend/application.py

+    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 instance of EventMultiplexer to use


Worth noting that this is a plugin_event_multiplexer.EventMultiplexer?

wchargin · 2019-08-19T21:21:58Z

tensorboard/backend/application.py

+    """
+    logger.info('_DbModeMultiplexer initializing for %s', db_uri)
+    super(_DbModeMultiplexer, self).__init__()
+    self.db_uri = db_uri


Why does this serve as a conduit for DB-related parameters? IIUC nothing
uses this curently—the URI and connection provider are always pulled off
the context, not the multiplexer.

Oof, I see: there’s a getattr, which is why my greps turned up
nothing. What do you think of either explicitly mentioning that these
are read in TensorBoardWSGIApp, or changing that logic above to be
something like

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
the context at all?

See above response about the non-functional multiplexer.

I can change the getattr to two different isinstance checks (one per db multiplexer type) if you'd prefer that. I was not trying to be terribly principled since this code is all going away soon, but I'm happy to make that change.

Replaced getattrs with isinstance.

wchargin · 2019-08-19T21:41:11Z

tensorboard/backend/application.py

    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)


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 a
DB 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.)

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.multiplexer is 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.

Got it; that this is all slated to be removed soon changes the equation.
This is fine with me, then.

nfelt

Thanks for the review! Will update PR to address the remaining comments.

nfelt · 2019-08-20T00:00:10Z

tensorboard/backend/application.py

    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)


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.multiplexer is 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.

nfelt · 2019-08-20T00:01:58Z

tensorboard/backend/application.py

+      flags, plugin_loaders, data_provider, assets_zip_provider, multiplexer)
+
+
+def TensorBoardWSGIApp(


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.

nfelt · 2019-08-20T00:03:31Z

tensorboard/backend/application.py

+    """
+    logger.info('_DbModeMultiplexer initializing for %s', db_uri)
+    super(_DbModeMultiplexer, self).__init__()
+    self.db_uri = db_uri


See above response about the non-functional multiplexer.

I can change the getattr to two different isinstance checks (one per db multiplexer type) if you'd prefer that. I was not trying to be terribly principled since this code is all going away soon, but I'm happy to make that change.

wchargin · 2019-08-20T00:36:57Z

tensorboard/backend/application.py

    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)


Got it; that this is all slated to be removed soon changes the equation.
This is fine with me, then.

wchargin · 2019-08-20T00:37:11Z

tensorboard/backend/application.py

+      flags, plugin_loaders, data_provider, assets_zip_provider, multiplexer)
+
+
+def TensorBoardWSGIApp(


…asses directly" This reverts commit 8e2f334.

nfelt added 9 commits August 19, 2019 13:22

replace TensorBoardWSGIApp in plugin tests with TensorBoardWSGI and e…

d736dee

…xplicit Reload()

replace TensorBoardWSGIApp in application_test with TensorBoardWSGI

f0dcd0b

remove TensorBoardWSGIApp and move reloading logic to standard_tensor…

15a6368

…board_wsgi()

make DbImportMultiplexer expose db_uri and db_connection_provider

399061e

make DbImportMultiplexer implement the read APIs with empty results

6b696b1

add _DbModeMultiplexer to hold db_uri and db_connection_provider

032fda8

regroup standard_tb_wsgi() logic to factor out multiplexer creation

934ed98

reincarnate TensorBoardWSGIApp as new DataProvider-friendly entry point

cc8c7d3

make TensorBoardWSGIApp allow passing TBPlugin or TBLoader classes di…

8e2f334

…rectly

nfelt added the core:backend label Aug 19, 2019

nfelt requested a review from wchargin August 19, 2019 20:39

googlebot added the cla: yes label Aug 19, 2019

wchargin reviewed Aug 19, 2019

View reviewed changes

nfelt commented Aug 20, 2019

View reviewed changes

wchargin approved these changes Aug 20, 2019

View reviewed changes

nfelt changed the base branch from wsgi-app-refactor-1 to master August 20, 2019 04:46

nfelt changed the base branch from master to wsgi-app-refactor-1 August 20, 2019 04:48

nfelt added 3 commits August 20, 2019 09:34

Revert "make TensorBoardWSGIApp allow passing TBPlugin or TBLoader cl…

4b93e80

…asses directly" This reverts commit 8e2f334.

CR: deprecated_multiplexer doc fix

a50d21f

CR: replace hacky getattrs with somewhat more principled isinstance()

ddaf663

nfelt changed the base branch from wsgi-app-refactor-1 to master August 20, 2019 16:43

Merge branch 'master' into wsgi-app-refactor-2

6bc5e43

nfelt merged commit f145659 into master Aug 20, 2019

nfelt deleted the wsgi-app-refactor-2 branch August 20, 2019 18:54

nfelt mentioned this pull request Aug 20, 2019

Allow passing TBPlugin/TBLoader subclasses to TensorBoardWSGIApp #2582

Merged

nfelt mentioned this pull request May 20, 2020

Restructure the WSGI app for easier direct use #2573

Open

13 tasks

		flags, plugin_loaders, data_provider, assets_zip_provider, multiplexer)


		def TensorBoardWSGIApp(

Reincarnate TensorBoardWSGIApp as new DataProvider-friendly entry point #2576

Reincarnate TensorBoardWSGIApp as new DataProvider-friendly entry point #2576

Uh oh!

Conversation

nfelt commented Aug 19, 2019

Uh oh!

wchargin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfelt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants