Skip to content

Conversation

@psybuzz
Copy link
Contributor

@psybuzz psybuzz commented Oct 14, 2020

The /scalars_multirun endpoint triggers errors when TensorBoard is
hosted with a Google Cloud Spanner backend. This change reverts
TensorBoard back to using _requestDataGet unconditionally.

Once [1] is synced into Google, we plan to re-enable the
_requestDataPost path for non-Colab environments.

Manually checked that the scalars dashboard makes GET requests
to /data/plugin/scalars/scalars with and without the
?tensorboardColab=true flag.

Googlers, see b/170675710.

[1] googleapis/python-spanner#144

@google-cla google-cla bot added the cla: yes label Oct 14, 2020
@psybuzz psybuzz requested a review from nfelt October 14, 2020 17:18
} else {
return this._requestDataPost(items, onLoad, onFinish);
}
// TODO(psybuzz): Ideally, we would always make use of `_requestDataPost`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get away with a smaller delta here and leave most of the code intact so it's easy to revert? E.g.

// TODO(b/170675710): Stop unconditionally forcing legacy endpoint when fixed.
const forceLegacyEnpoint = true;
if (inColab || forceLegacyEndpoint) {
  return this._requestDataGet(items, onLoad, onFinish);
} else {
  return this._requestDataPost(items, onLoad, onFinish);
}

That makes it obvious that we'll still need to the inColab check after resolving the TODO, which is better IMO than stating this fact in a comment. And I'm not sure we need to link to the spanner PR given that the gating factor isn't anything to do with the PR per se but rather with Google-internal release details which aren't particularly visible or meaningful outside Google anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's much more concise! Done.

Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

LGTM

@psybuzz psybuzz merged commit 5794224 into tensorflow:master Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants