Skip to content
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

Support jupyter-server-proxy (and other proxies) #3674

Merged
merged 7 commits into from
Jun 3, 2020

Conversation

zac-hopkinson
Copy link
Contributor

@zac-hopkinson zac-hopkinson commented May 27, 2020

Motivation for features / changes

Users will frequently want to run TensorBoard in environments where they are not allowed to open up or expose additional ports. For those cases jupyter-server-proxy allows forwarding web traffic to other services running on the system without opening new ports. (The Google Colab integration also works around this same issue of not being able to directly expose the port that TensorBoard runs on.)

This PR allows users to optionally pass the TensorBoard traffic through jupyter-server-proxy by setting the TENSORBOARD_PROXY_URL environment variable. When this variable is not set TensorBoard will behave as normal.

Technical description of changes

This adds support for a new optional environment variable TENSORBOARD_PROXY_URL which if set will be used as the URL's path for the IFRAME displayed in the notebook when running %tensorboard.

Screenshots of UI changes

image

Detailed steps to verify changes work correctly (as executed by you)

I've successfully run the Travis CI build on my fork of the project: https://travis-ci.com/github/zac-hopkinson/tensorboard

I've also built test environments of both JupyterLab and JupyterHub with the documentation that I used to both configure and test this feature. You can find this documentation here: https://github.com/zac-hopkinson/tensorboard-pr-test

You can skip reading the documentation and just export TENSORBOARD_PROXY_URL="/proxy/%PORT%/" to use this feature with JupyterLab or Jupyter Notebook

Alternate designs / implementations considered

My first attempt at this added a new context to go along with _CONTEXT_COLAB, _CONTEXT_IPYTHON, and _CONTEXT_NONE in notebook.py. After testing with different environments I decided that it wasn't a clean solution like I had hoped. I found that adding in the environment variable logic to _CONTEXT_IPYTHON and making sure it preserved backwards compatibility was a much cleaner solution. I'm open to ideas and feedback around this decision of course!

I also researched #3142. Initially I hadn't planned on supporting JupyterHub but after testing I found that this PR can handle both Jupyter Notebook/Lab and JupyterHub by just documenting how to configure each.

I also considered alternatives to the environment variable, such as a possible ~/.tensorboardrc file for example, but that seemed like overkill.

Copy link
Contributor

@bmd3k bmd3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Zac, thanks for your contribution!

Apologies in advance, I'm not familiar with Jupyter Server Proxy. But it looks like some users have already had success with it in the past:

#1270

Does that point to an approach for your use case?

@zac-hopkinson
Copy link
Contributor Author

zac-hopkinson commented May 27, 2020

Thanks Brian! Yeah, it's always been possible to get to TensorBoard manually when using jupyter-server-proxy. This PR is to get the IPython magic (%tensorboard) to be able to use it and for it to work seamlessly.

Part of the logic in notebook.py is to embed TensorBoard into Jupyter using an iframe. This PR allows that iframe to optionally support going through the proxy when it's not possible for the connection to go directly. I put together some examples showing how it's used here: https://github.com/zac-hopkinson/tensorboard-pr-test/#usingtesting

The example of running Jupyter inside of a Docker container and only exposing the Jupyter port gives a clear example of a common scenario where TensorBoard's web UI is not accessible without the use of jupyter-server-proxy or a similar solution.

@bmd3k
Copy link
Contributor

bmd3k commented May 29, 2020

Thanks for the explanation! The use case makes sense to me. I played around with your changes on a local Jupyter notebook install. I'll chat with some members on the team about the approach and get back to you.

@bmd3k bmd3k self-requested a review May 29, 2020 15:30
Copy link
Contributor

@bmd3k bmd3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience. I've left a couple comments and will also assign to a colleague who may leave further comments. Apologies in advance if this leads to any churn.

tensorboard/notebook.py Outdated Show resolved Hide resolved
tensorboard/notebook.py Outdated Show resolved Hide resolved
@bmd3k bmd3k requested a review from wchargin June 2, 2020 13:14
tensorboard/notebook.py Outdated Show resolved Hide resolved
tensorboard/notebook.py Outdated Show resolved Hide resolved
tensorboard/notebook.py Outdated Show resolved Hide resolved
tensorboard/notebook.py Outdated Show resolved Hide resolved
@zac-hopkinson
Copy link
Contributor Author

Thanks for the really good feedback! Suggested changes in the latest commit.

New generated JS and the iframe in the default config:
generated_standard

And with TENSORBOARD_PROXY_URL set:
generated_proxy

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks! One more suggested change, if you don’t mind
(apologies; I meant to call this out before but forgot).

tensorboard/notebook.py Show resolved Hide resolved
tensorboard/notebook.py Outdated Show resolved Hide resolved
@bmd3k
Copy link
Contributor

bmd3k commented Jun 3, 2020

The lint-build errors in your latest CI run is not the fault of your PR. Should be fixed now. You can push an empty commit to rerun.

@zac-hopkinson
Copy link
Contributor Author

Thank you both for the help and the great code review comments!

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! (All yours, @bmd3k.)

Copy link
Contributor

@bmd3k bmd3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@lsyarn
Copy link

lsyarn commented Dec 29, 2020

For any one who was confused with how to use this feature on version after 2.3 (included):

Due to #4421——tensorboard did follow --path_prefix, this feature does not working well on jupyterhub.
This feature released in version 2.3.0, and #4421 occurred in version 2.3.0 too.

@jameswex
Copy link
Contributor

@zac-hopkinson I'm looking to add JupyterHub support for another tool (https://github.com/PAIR-code/lit/, not TensorBoard). But our notebook-mode code is basically identical in function to what TensorBoard has done. I've gotten it working in colab and jupyter (as seen in PAIR-code/lit#163 and PAIR-code/lit#171), but the widget fails to render in JupyterHub environments (I've tried Google Cloud AI Notebooks and AWS Sagemaker notebooks specifically). If you have time, I'd like to pick your brain about this as you seem to be really knowledgeable about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants