Confirm that the connection to tensorboard works or change to localhost #2371

mm3509 · 2019-06-20T17:33:50Z

Motivation for features / changes

The TensorBoard log told me to go to http://laptop-name.corp.domain.tld:6006 and that connection failed, but http://localhost:6006 worked.

Technical description of changes

I tested the connection to the host with socket(), and if it is closed, the host becomes localhost.

Screenshots of UI changes

No UI changes.

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

I ran export PYTHONPATH="$PYTHONPATH:$HOME/code/tensorflow:$HOME/code:tensorboard"; python3 tensorboard/program.py, which threw this unrelated error (and both repos are in sync with upstream):

  File "/Users/mmorin/code/tensorboard/tensorboard/data_compat.py", line 24, in <module>
    from tensorboard.compat.proto import summary_pb2
ImportError: cannot import name 'summary_pb2'

Alternate designs / implementations considered

None.

nfelt

Thanks for taking this on :)

nfelt · 2019-06-27T22:55:39Z

tensorboard/program.py

+    # Confirm that the connection is open, otherwise change to `localhost`
+    socket.setdefaulttimeout(1)
+    try:
+        socket.socket().connect((display_host, self.server_port))


Let's use socket.create_connection() instead - it's a higher level API that should handle, for example, IPv4 and IPv6 (rather than picking just one, at which point if the hostname is only available on the other protocol it will incorrectly fall back to localhost).

Also, I think it only makes sense to do this fallback when the user didn't specify an explicit host via the --host flag, since if --host was passed we should use it without modification. That would mean moving this fallback code up into the first case of the if statement above, where we do socket.gethostname(). And actually it's probably better to replace socket.gethostname() with socket.getfqdn() so we try to resolve the hostname to a reachable domain name first if we can.

I agree and addressed this in 88e25, 6a5fc8, and 6c796 respectively.

nfelt · 2019-06-27T22:56:22Z

tensorboard/program.py

@@ -526,6 +526,14 @@ def get_url(self):
      host = self._flags.host
      display_host = (
          '[%s]' % host if ':' in host and not host.startswith('[') else host)
+
+    # Confirm that the connection is open, otherwise change to `localhost`
+    socket.setdefaulttimeout(1)


Rather than changing the default for the entire socket module, let's use .settimeout() on the individual socket object - or better yet, pass it as an argument into create_connection() per the comment below.

Well spotted, fixed in 1ae50.

nfelt · 2019-06-27T23:04:58Z

tensorboard/program.py

+    socket.setdefaulttimeout(1)
+    try:
+        socket.socket().connect((display_host, self.server_port))
+    except socket.timeout as e:


Let's catch socket.error generally so that if there is some other error attempting the connection (not a timeout), we fall back to localhost in that case too. For example, it looks like if the DNS name doesn't resolve at all, this line fails with socket.gaierror rather than socket.timeout:

>>> socket.socket().connect(("foobartensorboard.com", 6006)) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python2.7/socket.py", line 228, in meth return getattr(self._sock,name)(*args) socket.gaierror: [Errno -2] Name or service not known

Very well spotted, fixed in 71307.

nfelt · 2019-06-27T23:07:51Z

tensorboard/program.py

+
+    # Confirm that the connection is open, otherwise change to `localhost`
+    socket.setdefaulttimeout(1)
+    try:


Now that creating the URL involves an actual network request, let's avoid doing that on each call to get_url() and just cache the constructed URL in a property and return that if it's called another time.

Yes, I proposed a solution in bf05d. Another solution could be to move the code into __init__() of class WerkzeugServer and change get_url() to only the last two lines of the current function (return 'http:// ... .rstrip('/'))).. Also, why do we have the property _auto_wildcard if it's simply not host?

nfelt

Thanks for your patience, I was on vacation for a while.

FYI, we're planning on soon changing the default behavior of TensorBoard to only serve on localhost, in which case the URL printed out would use "localhost" always in that case. (Basically, it'll be like passing --host=localhost by default.) So after that change, the logic in this PR would only affect cases where users specifically opt-in to the wildcard behavior.

nfelt · 2019-07-18T21:37:40Z

tensorboard/program.py

-    else:
-      host = self._flags.host
-      display_host = (
+    if not self.display_host:


Let's make the variable self._url instead and just store the full URL.

It would probably read better if we factor out something like get_url_host() e.g.

def get_url_host(self): if not self._auto_wildcard: return self._flags.host host = socket.getfqdn() try: socket.create_connection((host, self.server_port), timeout=1) return host except socket.error as e: return 'localhost' def get_url(self): if not self._url: host = self.get_url_host() display_host = ( '[%s]' % host if ':' in host and not host.startswith('[') else host) self._url = 'http://%s:%d%s/' % (self.display_host, self.server_port, self._flags.path_prefix.rstrip('/')) return self._url

nfelt · 2019-07-18T21:43:10Z

tensorboard/program.py

+    if not self.display_host:
+
+      if self._auto_wildcard:
+        display_host = socket.getfqdn()


Just "host" is a better name here since this is still a real hostname; the name "display_host" was meant to be only for the host after adding brackets around an IPv6 address so it renders correctly when printed out in the URL. See also suggestion above for a get_url_host() helper.

mm3509 · 2019-12-07T17:21:41Z

@nfelt Thanks for your patience too, I was overwhelmed for a while. Given the move of tensorboard and the conflicts with the base branch, does this PR still make sense, or shall I open a new one from the new codebase?

nfelt · 2019-12-12T02:50:22Z

@miguelmorin Thanks for following up - I might be misunderstanding but I don't think anything actually moved? There was a behavior change in #2589 though that as mentioned in my last comment makes it so that now by default the URL would use localhost anyway in all cases (unless passing --bind_all or an explicit hostname).

This PR would still affect the default value shown when using --bind_all but it's more of an edge case now. If you're still interested in landing this and want to update the PR I think we're still open to it, but also fine if you'd prefer to drop it.

mm3509 · 2019-12-20T22:29:33Z

I misnamed the change of default behavior as a "move". Yes, I still want to update the PR for this edge case. What is simpler for you: that I start a new branch from the latest code or that I rebase master onto this branch?

Iin #2589 you mention "wildcard dual-binding behavior": is that when TensorBoard "defaults to serving on the entire local network"?

nfelt · 2020-01-02T23:59:51Z

What is simpler for you: that I start a new branch from the latest code or that I rebase master onto this branch?

@miguelmorin the easiest option is probably if you can rebase your local changes onto latest master and then force-push that branch to minguelmorin:localhost to update this PR. Or if you prefer you can close this and open a new PR, either way.

Iin #2589 you mention "wildcard dual-binding behavior": is that when TensorBoard "defaults to serving on the entire local network"?

Yes, this was the default behavior prior to 2.0.

…it in an object field

mm3509 · 2020-02-25T18:18:55Z

Hi @nfelt, I've finally made time to finish this. The rebasing was straightforward and I force-pushed the branch. Your requested changes are still marked as unsolved and those commits are now gone. How would you like to proceed?

nfelt

Thanks for resuming this! Mostly looks good, just a couple last comments.

nfelt · 2020-03-10T21:04:58Z

tensorboard/program.py

@@ -597,6 +597,7 @@ def __init__(self, wsgi_app, flags):
            host = "localhost"

        self._host = host
+        self.display_host = None  # Will be set by get_url() below


Let's just cache the entire URL for this, so self._url.

That way we can skip even the last formatting step in get_url() if we already have generated the URL.

Yes, I cached the entire URL in 3f99276.

nfelt · 2020-03-10T21:24:10Z

tensorboard/program.py

+        if not self.display_host:
+            if self._auto_wildcard:
+                self.display_host = socket.getfqdn()
+


nit: can we leave out the blank lines inside the function, since it's not that long?

If we want separation for clarity I'd recommend just factoring out a private helper function as described in the previous comment.

Do you mean just removing line 734, which I did in f3bbb87? If not, I don't see you mean by "as described in the previous comment". And what do you mean by "nit: "?

Well I had in mind both blank lines (so also 742), but it's fine.

The second part of the comment was an alternative suggestion, that for the purpose of clarifying the sub-parts of the function, instead of setting them off with blank lines, to just refactor it into smaller functions. By "previous comment" I meant from earlier in the review thread (#2371 (comment)).

Anyway, sorry that was a bit confusing, this is fine as is.

nfelt · 2020-03-11T14:46:40Z

tensorboard/program.py

+        if not self.display_host:
+            if self._auto_wildcard:
+                self.display_host = socket.getfqdn()
+


Well I had in mind both blank lines (so also 742), but it's fine.

The second part of the comment was an alternative suggestion, that for the purpose of clarifying the sub-parts of the function, instead of setting them off with blank lines, to just refactor it into smaller functions. By "previous comment" I meant from earlier in the review thread (#2371 (comment)).

Anyway, sorry that was a bit confusing, this is fine as is.

mm3509 · 2020-03-13T13:45:44Z

Sorry, I missed that second part of the 18 July comment. My changes already seem outdated, do you want me pull the master branch and do a manual rebase of my changes?

nfelt · 2020-03-13T21:44:53Z

I don't see any conflicts with the base branch? I can go ahead and merge now unless there were further changes you were going to make.

mm3509 · 2020-03-13T22:30:36Z

No, I don't see conflicts with the base branch. I see tensorboard/program.py Outdated above the code in your comment under commit 2b6067a. Is the notification of "outdated" itself outdated? If so, I'm fine with merging.

nfelt · 2020-03-13T22:38:17Z

Outdated just means the comment is on code that isn't at the most recent commit touching that file; it's fine in this case. Will merge.

nfelt suggested changes Jun 27, 2019

View reviewed changes

nfelt reviewed Jul 18, 2019

View reviewed changes

This was referenced Aug 7, 2019

Bind to localhost rather than all interfaces (0.0.0.0) #2513

Closed

Listen only localhost by default #2387

Closed

mm3509 added 2 commits February 25, 2020 17:43

used fully qualified domain name instead of just hostname

9ce0f8b

tested connection to server IP, defaulted to localhost, and included …

8bdd196

…it in an object field

mm3509 force-pushed the localhost branch from bf05d42 to 8bdd196 Compare February 25, 2020 18:03

googlebot added the cla: yes label Feb 25, 2020

ran Black reformatting

5defff3

moved display_host field to WerkzeugServer object

c01fe3b

nfelt reviewed Mar 10, 2020

View reviewed changes

mm3509 added 3 commits March 11, 2020 14:29

cached entire url, per nfelt's comment

3f99276

removed blank line

f3bbb87

removed trailing whitespace

2b6067a

nfelt approved these changes Mar 13, 2020

View reviewed changes

nfelt merged commit 9465b89 into tensorflow:master Mar 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Confirm that the connection to tensorboard works or change to localhost #2371

Confirm that the connection to tensorboard works or change to localhost #2371

mm3509 commented Jun 20, 2019 •

edited

Loading

nfelt left a comment

nfelt Jun 27, 2019

mm3509 Jul 5, 2019

nfelt Jun 27, 2019

mm3509 Jul 5, 2019

nfelt Jun 27, 2019

mm3509 Jul 5, 2019

nfelt Jun 27, 2019

mm3509 Jul 5, 2019 •

edited

Loading

nfelt left a comment

nfelt Jul 18, 2019

nfelt Jul 18, 2019

mm3509 commented Dec 7, 2019

nfelt commented Dec 12, 2019

mm3509 commented Dec 20, 2019

nfelt commented Jan 2, 2020

mm3509 commented Feb 25, 2020

nfelt left a comment

nfelt Mar 10, 2020

mm3509 Mar 11, 2020

nfelt Mar 10, 2020

mm3509 Mar 11, 2020 •

edited

Loading

nfelt Mar 11, 2020

nfelt Mar 11, 2020

mm3509 commented Mar 13, 2020

nfelt commented Mar 13, 2020

mm3509 commented Mar 13, 2020 •

edited

Loading

nfelt commented Mar 13, 2020

Confirm that the connection to tensorboard works or change to localhost #2371

Confirm that the connection to tensorboard works or change to localhost #2371

Conversation

mm3509 commented Jun 20, 2019 • edited Loading

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm3509 Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm3509 commented Dec 7, 2019

nfelt commented Dec 12, 2019

mm3509 commented Dec 20, 2019

nfelt commented Jan 2, 2020

mm3509 commented Feb 25, 2020

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm3509 Mar 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mm3509 commented Mar 13, 2020

nfelt commented Mar 13, 2020

mm3509 commented Mar 13, 2020 • edited Loading

nfelt commented Mar 13, 2020

mm3509 commented Jun 20, 2019 •

edited

Loading

mm3509 Jul 5, 2019 •

edited

Loading

mm3509 Mar 11, 2020 •

edited

Loading

mm3509 commented Mar 13, 2020 •

edited

Loading