Temporarily disable usage of scalars_multirun endpoint by psybuzz · Pull Request #4233 · tensorflow/tensorboard · GitHub
Skip to content

Temporarily disable usage of scalars_multirun endpoint#4233

Merged
psybuzz merged 3 commits into
tensorflow:masterfrom
psybuzz:disable-multirun
Oct 14, 2020
Merged

Temporarily disable usage of scalars_multirun endpoint#4233
psybuzz merged 3 commits into
tensorflow:masterfrom
psybuzz:disable-multirun

Conversation

@psybuzz

@psybuzz psybuzz commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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
Copy Markdown
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.

@nfelt nfelt left a comment

Copy link
Copy Markdown
Contributor

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