-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add popup dialog for plugins that require TensorFlow #2032
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
Add popup dialog for plugins that require TensorFlow #2032
Conversation
72cc11b to
678e6b9
Compare
|
@nfelt if tests pass this should be good for review. Thanks! |
|
And tests pass! We're good. |
nfelt
left a comment
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.
Looks good overall, just a few pretty minor code cleanup requests.
The debugger plugin currently doesn't have any indication about it not working with TF - can we change the recently introduced try-catch in the loader to raise an exception indicating that TF is required to use the debugger? This only happens when the --debugger_port has been passed so it doesn't impact the default no-TF use case.
tensorboard/tensorboard/plugins/debugger/debugger_plugin_loader.py
Lines 98 to 102 in 90a386d
| try: | |
| # pylint: disable=g-import-not-at-top,unused-import | |
| import tensorflow | |
| except ImportError: | |
| return None |
Ideally we would message that through the dialog, but the way the debugger is structured with its own separate initial dialog makes that difficult, so for now this would at least prevent if from silently not working.
tensorboard/plugins/beholder/tf_beholder_dashboard/tf-beholder-dashboard.html
Outdated
Show resolved
Hide resolved
tensorboard/plugins/beholder/tf_beholder_dashboard/tf-beholder-dashboard.html
Outdated
Show resolved
Hide resolved
tensorboard/plugins/beholder/tf_beholder_dashboard/tf-beholder-dashboard.html
Outdated
Show resolved
Hide resolved
tensorboard/plugins/hparams/tf_hparams_dashboard/tf-hparams-dashboard.html
Outdated
Show resolved
Hide resolved
...ractive_inference/tf_interactive_inference_dashboard/tf-interactive-inference-dashboard.html
Show resolved
Hide resolved
tensorboard/plugins/profile/tf_profile_dashboard/tf-profile-dashboard.html
Outdated
Show resolved
Hide resolved
|
@nfelt (and @wchargin) all comments should be addressed. I needed to use a function scope to add |
|
@nfelt any thoughts on the CI failures? Seems unrelated to my changes, but not 100% sure. |
nfelt
left a comment
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.
Thanks for the PLUGIN_NAME cleanup!
...ractive_inference/tf_interactive_inference_dashboard/tf-interactive-inference-dashboard.html
Show resolved
Hide resolved
...ractive_inference/tf_interactive_inference_dashboard/tf-interactive-inference-dashboard.html
Show resolved
Hide resolved
|
@nfelt ready for final review. Thanks. |
...ractive_inference/tf_interactive_inference_dashboard/tf-interactive-inference-dashboard.html
Show resolved
Hide resolved
|
One last else clause coming - stay tuned. |
|
@nfelt made that one last fix - thanks. |
|
@nfelt, please merge when ready. Thank you. :) |
Adding a popup dialog for when plugins are not created. Note that this is different from being "inactive", which can be due to data, etc.
Fixes #2027
Everything should be good to go and I've tested on my Mac with the following:
cc @lanpa, @nfelt