-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor: slice the two-headed application.py monster in half #3643
Conversation
748bd41
to
850786e
Compare
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.
Lovely; thank you! Approach sounds good to me.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate import; remove?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
LocalDataIngester
class. 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 ofEventMultiplexer
entirely. I've prefixed it withLocal
to 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.py
only 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 justTensorBoardWSGIApp
as the entry point (withTensorBoardWSGI
due to be cleaned up as further work in #2573). This means crucially thatapplication
no longer has any BUILD dependency path on TensorFlow.As a result of this carnage, the old
standard_tensorboard_wsgi
symbol is eliminated entirely.Tested by running
tensorboard
locally and verifying that it still launches as expected.