refactor: slice the two-headed application.py monster in half #3643
Conversation
nfelt
force-pushed
the
refactor-application
branch
from
748bd41
to
850786e
Compare
nfelt
changed the base branch from
nickfelt-diffbase-refactor-application
to
master
tensorboard/backend/application.py
Outdated
| @@ -48,39 +38,14 @@ | |||
| from tensorboard.backend import experiment_id | |||
| from tensorboard.backend import experimental_plugin | |||
| from tensorboard.backend import http_util | |||
| from tensorboard.backend import http_util | |||
There was a problem hiding this comment.
Duplicate import; remove?
There was a problem hiding this comment.
Done, thanks. Must have gotten this mixed up when munging all of these from the internal CL I started with.
tensorboard/backend/BUILD
Outdated
| @@ -66,13 +66,7 @@ py_library( | |||
| "//tensorboard:errors", | |||
| "//tensorboard:plugin_util", | |||
| "//tensorboard/backend/event_processing:data_provider", | |||
There was a problem hiding this comment.
Need to remove this dep to actually prune the build path, no?
$ git status
HEAD detached at nf/refactor-application
nothing to commit, working tree clean
$ git rev-parse @
799f17f214d66e5d526374f8d1c79b50b3cc3a0f
$ bazel query 'somepath(//tensorboard/backend:application, //tensorboard/compat:tensorflow)'
//tensorboard/backend:application
//tensorboard/backend/event_processing:data_provider
//tensorboard/backend/event_processing:event_accumulator
//tensorboard/compat:tensorflow
Unfortunately, doing so breaks some tests:
//tensorboard/plugins/image:images_plugin_test
//tensorboard/plugins/image:images_plugin_notf_test
//tensorboard/plugins/histogram:histograms_plugin_test
//tensorboard/plugins/histogram:histograms_plugin_notf_test
…because apparently I have a blind spot for missing this dep; sorry.
(Would send a PR to fix, but I think it might be logistically easier for
you to do so?)
There was a problem hiding this comment.
Ugh, yes, thanks. Not sure where I lost this change since obviously that's the point of all this...
Re: the deps - sure, sent you #3654 to fix. You know, I thought when fixing some of the other plugins in #3641 that I should check all of the plugins just in case, and I spot-checked to see that they had the event multiplexer dep, but didn't consider that they were missing the data provider dep :/
This addresses the remaining tasks from the "CORE" category of #2573 by extricating all of the multiplexer-loading-thread related logic from application.py into a new dedicated
LocalDataIngesterclass. The class is intended to encapsulate the messy business of configuring the multiplexer and the reloading thread so that it still makes sense in a world where we get rid ofEventMultiplexerentirely. I've prefixed it withLocalto leave conceptual room for a possible future in which we might want to have several specializations ofDataIngester(akin toDataProvider) but for now this is entirely an implementation detail ofprogram.TensorBoard, so we shouldn't feel constrained to continue that pattern.We leave behind in
application.pyonly the logic that is actually necessary for constructing the WSGI application (including cases where the app does not use a multiplexer at all, but instead uses a genericDataProvider), so essentially justTensorBoardWSGIAppas the entry point (withTensorBoardWSGIdue to be cleaned up as further work in #2573). This means crucially thatapplicationno longer has any BUILD dependency path on TensorFlow.As a result of this carnage, the old
standard_tensorboard_wsgisymbol is eliminated entirely.Tested by running
tensorboardlocally and verifying that it still launches as expected.