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

Allow Jupyter+TensorBoard to work w/ hostname other than localhost #2407

Merged

Conversation

cliffwoolley
Copy link
Contributor

@cliffwoolley cliffwoolley commented Jul 3, 2019

  • Motivation for features / changes
    Fixes %tensorboard magic: host other than localhost #1956 to allow the combination of Jupyter+TensorBoard to work when the browser is running on a system other than TensorBoard's localhost .

  • Technical description of changes
    Rather than localhost being hardcoded, we use a short JavaScript snippet in the IFrame source to have the client determine for itself the hostname that should be contacted. This allows the solution also to work from inside a container, where we might not know the host's outward-facing IP address.

Test Plan:
Install Docker and nvidia-docker, then run

tmpdir="$(mktemp -d)" \
&& bazel run //tensorboard/pip_package:extract_pip_package "${tmpdir}" \
&& nvidia-docker run -it --rm -v "${tmpdir}":/tensorboard \
    -p 8888:8888 -p 6006:6006 nvcr.io/nvidia/tensorflow:19.03-py3

to get a Docker shell. Inside the container, run

pip install -qU /tensorboard/*py3* \
&& jupyter notebook --allow-root --ip 0.0.0.0

and navigate to the provided notebook URL via a hostname other than
localhost (e.g., your machine’s hostname). Create a new IPython
notebook and execute the cell

%load_ext tensorboard
%tensorboard --logdir /tmp/whatever

and verify that the output contains a working TensorBoard iframe whose
src attribute is under the main frame’s origin (e.g., your machine’s
hostname), not localhost.

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.

Hi @cliffwoolley—thanks for this PR! The high-level plan of using the
enclosing hostname seems reasonable to me. Requested some changes to the
implementation; see inline.

tensorboard/notebook.py Outdated Show resolved Hide resolved
@cliffwoolley cliffwoolley force-pushed the 1956-jupyter-tb-non-localhost branch from ccc6776 to 462bd9f Compare July 4, 2019 00:55
@cliffwoolley
Copy link
Contributor Author

Hi @wchargin ; anything further you'd like me to change for this one?
Thanks!

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.

My apologies for the delay; it wasn’t possible for me to test this until
this week (for entirely unrelated reasons). I’ll test it and approve and
merge if everything looks good. In the meantime, just a few nits. :-)

thank you for your patience!

height=height,
width="100%",
)
import cgi
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pull these imports to the top of the file. (We late-import
IPython so that this module can be imported in environments that don’t
have IPython, but these three modules are part of the standard library.)


frame_id = "tensorboard-frame-{:08x}".format(random.randrange(0, 1 << 64))
shell = """
<iframe id="%HTML_ID%" width="100%" height="%HEIGHT%" frameborder="0"></iframe>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Please break a line before </iframe> to stay within 80 columns.

</script>
"""
replacements = [
("%HTML_ID%", cgi.escape(frame_id, quote=True)),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: +2 spaces of indentation for continuations, please.

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.

I’ve taken the liberty of adding a test plan to your pull request, and
executed it on the merge commit (merging 192ab46 into 462bd9f;
resulting tree is 7c8efcd81299).

LGTM once above nits are addressed.

import json
import random

frame_id = "tensorboard-frame-{:08x}".format(random.randrange(0, 1 << 64))
Copy link
Contributor

@wchargin wchargin Jul 22, 2019

Choose a reason for hiding this comment

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

nit: random.randrange(0, 1 << 64) can be random.getrandbits(64),
which is a bit clearer (in particular about the boundary condition).

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@wchargin
Copy link
Contributor

wchargin commented Aug 2, 2019

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 2, 2019
@wchargin
Copy link
Contributor

wchargin commented Aug 2, 2019

I’ve gone ahead and fixed the nits for you, since they were all trivial.
Will merge once CI passes. (Re-tested manually; everything seems fine.)

@wchargin wchargin merged commit d6a9bcf into tensorflow:master Aug 2, 2019
@wchargin
Copy link
Contributor

wchargin commented Aug 2, 2019

@cliffwoolley: Thanks again for the contribution!

@cliffwoolley
Copy link
Contributor Author

Thanks for taking care of those last nits!

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.

%tensorboard magic: host other than localhost
3 participants