uploader: request ServerInfo from frontend

[wchargin]

Summary:
This commit integrates the new ServerInfo RPC with the uploader. It’s
not currently enabled by default: the current behavior is the same as
the existing behavior, except that experiment URLs now properly have a
trailing slash. We’ll soon remove the hard-coded API backend endpoint
behavior to enable this by default.

Test Plan:
Running a test frontend and a test backend, we observe the following
behavior with different arguments:

--origin	--api_endpoint	→	URL origin	Backend
empty	empty		prod	prod
empty	prod		prod	prod
empty	test		prod	test
test	empty		test	test
test	test		test	test
test	prod		test	prod

Here, “test” in the --origin column is like http://localhost:8080,
and “test” in the --api_endpoint column is like localhost:10000.
Note that the no-argument case is equivalent to the explicitly-empty
argument case because both arguments have empty default values.

Explicitly specifying --origin https://tensorboard.dev, with any value
of --api_endpoint, fails with “Corrupt response from backend” because
server-side support has not yet been rolled out. This is expected.

Specifying --origin http://localhost:0 or any other unreachable host
fails with ECONNREFUSED and a nice message.

My test frontend is configured to reject clients below version 2.0.0 and
warn on clients below version 2.0.1. Changing the local version.py to
2.0.0a0 or 2.0.1a0 exercises these cases.

Finally, double-checked that building the Pip package, installing it,
and running tensorboard dev list properly uses the production backend
and prints URLs that resolve to the production frontend.

wchargin-branch: uploader-serverinfo-request

[nfelt]

See comment on the other PR, but if passing --origin in addition to --api_endpoint it makes sense to me to still do the server handshake.

[wchargin]

Will change.

[wchargin]

Done.

[nfelt]

Optional, but maybe "Server warning" just to make it a bit clearer in debugging where this is coming from?

Same, also optional, for "Error".

[wchargin]

Hmm; I like that idea, but “Server error” makes it sound like the server
has an error. We expect these messages usually to be “your client is out
of date”, which is more like a client error than a server error.

What do you think of using “Warning [from server]:” instead?

Warning [from server]: Experiencing transient errors; be patient.

Error [from server]: Please upgrade to TensorBoard 3.0.0 or later.

[nfelt]

sys.stderr.flush() too?

Though now that I actually go look at the docs, apparently both it and stdout are supposed to be line-buffered, so maybe the fact that I usually flush() after doing a raw write() is just cargo culting?

[wchargin]

Fascinating. I had always assumed that stderr was unbuffered, like it is
in every other environment, because that’s one of the main points of
stderr. And so it was in Python 2, but indeed this appears to have
changed in Python 3. I’ll keep this in mind from now on; thanks.

(It’s not clear to me whether we should flush here: superfluous in the
interactive case, but not when outputting to a file. I guess the perf
isn’t really a problem since this isn’t in a loop, so I’ll go ahead and
add it.)

[wchargin]

Re-review not yet required.

[wchargin]

Changes addressed, mostly straightforwardly, and tested with a deployed
dev server, which correctly connects and prints working links.

[nfelt]

It looks like submitting this as-is would mean that tb-nightly's uploader would see a regression in behavior against current tensorboard.dev (until a new deploy with the server-side change) because it won't compute the correct URL pattern and would instead erroneously use localhost.

I'd prefer to avoid that so we don't have to worry as much about sequencing the changes. Can we just do that by replacing the http://localhost:8080 passed to create_server_info() with https://tensorboard.dev?

Thinking about it more I'm kind of inclined to avoid http://localhost:8080 as a default value anyway, even after there's no backwards compatibility concern, since we really have no idea what might be running on http://localhost:8080. It seems better to not even attempt to emit a URL if we have no clue whether it will resolve, and instead just emit the raw EID in that case or something, since if a user is developing with no frontend the URL is meaningless anyway, and if they do have a frontend, it would be better to explicitly pass it into --origin.

[nfelt]

this should just be origin here right?

[wchargin]

Yes; thanks.

[wchargin]

Ugh, you’re right. It connects correctly, but emits URLs with the wrong
origin. This certainly isn’t intentional; it was meant to be both
forward and backward compatible.

So I think the right table is

	Origin given	No origin
API given	Handshake, then override endpoint	Construct fake info without handshake
No API	Handshake	Set origin to default, then proceed as ↖

where currently we’re in the top-right cell by default—as it’s the only
cell that doesn’t end with a handshake—and use https://tensorboard.dev
as the fake origin. After the server-side change rolls out, we move to
the bottom-right cell by default, and can optionally change the behavior
of the top-right to emit no URL instead of either tensorboard.dev or
localhost. But we need that behavior for now for compatibility.

Thanks for the catch; will fix.

[wchargin]

Yes; thanks.

[nfelt]

Yep, that matches my understanding.

[wchargin]

(updated test plan)

[nfelt]

I think we actually should also remove the argparse-facing default value for --api_endpoint too, because otherwise passing --origin=https://staging.tensorboard.dev will not behave as desired, since the flags.api_endpoint override behavior will still kick in and default to the prod API. We can instead just set the hardcoded API endpoint as a fallback behavior used for the create_server_info() path only, which gets us closer to our desired final state.

Then we'd have the following code for now:

_DEFAULT_ORIGIN = 'https://tensorboard.dev'
_HARDCODED_API_ENDPOINT = 'api.tensorboard.dev:443'

def _get_server_info(flags):
  origin = flags.origin or _DEFAULT_ORIGIN
  if not flags.origin:
    # Temporary fallback to hardcoded API endpoint in the unspecified case.
    api_endpoint = flags.api_endpoint or _HARDCODED_API_ENDPOINT
    return server_info_lib.create_server_info(origin, api_endpoint)
  # same from here down

And after the server-side rollout, we'd remove _HARDCODED_API_ENDPOINT and update it to:

if not flags.origin and flags.api_endpoint:
  return server_info_lib.create_server_info(origin, flags.api_endpoint)

[wchargin]

I think we actually should also remove the argparse-facing default
value for --api_endpoint too, because otherwise passing
--origin=https://staging.tensorboard.dev will not behave as desired,
since the flags.api_endpoint override behavior will still kick in
and default to the prod API

Correct (this is why the test plan passes --api_endpoint "" explicitly
in each case). This was fine with me, as it’s only temporary and only
for devs, so I preferred to keep the minor inconvenience so that the
later change could just be a simple flag flip rather than a flag flip
plus removing some logic.

Does this make sense? I’m happy to make the change if you still prefer
it that way.

[nfelt]

Ok, sorry, should have also read the updated test plan.

That said, even though it's only dev-facing, I don't really love that it's possible to run --origin=https://staging.tensorboard.dev and encounter pretty misleading behavior (data uploaded to the wrong place, printed URL doesn't resolve). It seems pretty easy for one of us to accidentally hit that during testing. I'd be more comfortable making that work correctly now; it seems fine to me that the later change removes fallback logic since I think that's actually more reflective of what we're doing than just flipping a flag.

[wchargin]

No problem; that makes sense. Done. I think that you’re probably right,
as this simplifies the test plan, which is a good sign. :-)

[nfelt]

LGTM and actually read the test plan this time too :)

[wchargin]

Thanks for your patience and helpful reviews!