KEMBAR78
scalar: fix the timing issues by stephanwlee · Pull Request #2825 · tensorflow/tensorboard · GitHub
Skip to content

Conversation

@stephanwlee
Copy link
Contributor

@stephanwlee stephanwlee commented Oct 23, 2019

Repro case

  1. load scalars
  2. simulate slower network connection (on your favorite browser or
    proxy)
  3. toggle on a run
  4. toggle on another run
  5. (depending on the dataset) you should see nothing and line chart
    defaults to 0-1 scale.
  6. if you double click on the line-chart, the line is shown!

Explanation

Setting: dataToLoad was [a, b] and "changes" to [a, b].

Before:

  1. requests for [a, b] goes out
  2. before the requests resolve, second loadDataImpl gets invoked and
    cancels Promises for (1).
  3. requests from (1) resolve but because Promise was cancelled, it
    does not invoke the onLoadFinish and domain is not reset

After:
We now invoke onLoadFinish if we ever invoke loadDataCallback even
once.

  1. requests for [a, b] goes out
  2. before the requests resolve, second loadDataImpl gets invoked but
    it fetches no data and Promise.all resolves on next tick. Because no
    data is fetched, it does not call the onLoadFinish.
  3. requests from (1) resolve and it invokes onLoadFinish.

Remaining "issue"

Setting: dataToLoad was [a, b] and "changes" to [a, c]. Request to
fetch c ends before ones for a and b.
Problem: we set the domain based on c only but the fix will cause
major regression with regarding auto update where any data fetches will
cause the chart to rescale even if user overrode the domains.

Confirmed the fix by running the integration test 100 times on otherwise failing test.

Repro case
----------
1. load scalars
2. simulate slower network connection (on your favorite browser or
proxy)
3. toggle on a run
4. toggle on another run
5. (depending on the dataset) you should see nothing and line chart
defaults to 0-1 scale.
6. if you double click on the line-chart, the line is shown!

Explanation
-----------
Setting: `dataToLoad` was [a, b] and "changes" to [a, b].
Before:
  1. requests for [a, b] goes out
  2. before the requests resolve, second `loadDataImpl` gets invoked and
  cancels Promises for (1).
  3. requests from (1) resolve but because Promise was cancelled, it
  does not invoke the `onLoadFinish` and domain is not reset
After:
We now invoke `onLoadFinish` if we ever invoke `loadDataCallback` even
once.
  1. requests for [a, b] goes out
  2. before the requests resolve, second `loadDataImpl` gets invoked but
  it fetches no data and Promise.all resolves on next tick. Because no
  data is fetched, it does not call the `onLoadFinish`.
  3. requests from (1) resolve and it invokes `onLoadFinish`.

Remaining "issue"
-----------------
Setting: `dataToLoad` was [a, b] and "changes" to [a, c]. Request to
fetch `c` ends before ones for `a` and `b`.
Problem: we set the domain based on `c` only but the fix will cause
major regression with regarding auto update where any data fetches will
cause the chart to rescale even if user overrode the domains.
Copy link
Contributor

@psybuzz psybuzz left a comment

Choose a reason for hiding this comment

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

LGTM!

return Promise.all(promises).then(
this._canceller.cancellable((result) => {
// It was resetted. Do not notify of the data load.
if (!result.cancelled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

veeery small nit: I prefer early returns, to prevent deeper indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. Since our CI takes super long and we need this to unblock sync, I will merge as is and we can address this in future time if it truly bothers you.

@stephanwlee stephanwlee merged commit 3966966 into tensorflow:master Oct 24, 2019
@stephanwlee stephanwlee deleted the flaky branch October 24, 2019 14:46
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.

3 participants