Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

profile: fix loading progress behavior #1945

Merged
merged 1 commit into from
Mar 2, 2019

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Mar 2, 2019

Summary:
The profile dashboard had an existing bug that was revealed by #1914:
the loading progress bar never actually finishes. Prior to #1914, you
could actually see the loading bar if you made your window narrow
enough:
Screenshot of a zombie progress bar

This was because the prior version of the dashboard rendered the main
tool irrespective of whether the data had actually finished loading. As
of #1914, the “loading” and “active” states are mutually exclusive, so
instead of just having a zombie progress bar we actually block the view.

The cause of the bug is a promise that never resolves. (The executor
provided to the Promise constructor’s return value is ignored; only
the resolve and reject callbacks can fulfill a promise.)

This was not caught while testing #1914 because it cannot be reproduced
with the demo data present at that commit; with that more complete set
of data, there are other code paths that clobber the progress value to
100% (which is really a separate issue, but that’s not important right
now), such as _maybeUpdateData.

Test Plan:
Regenerate profile plugin demo data if you haven’t done so since #1942,
then launch TensorBoard:

$ rm -rf /tmp/profile_demo/
$ bazel run //tensorboard/plugins/profile:profile_demo
$ bazel run //tensorboard -- --logdir /tmp/profile_demo

The trace viewer should be displayed (prior to this commit, it wasn’t).

wchargin-branch: profile-broken-promises

Summary:
The profile dashboard had an existing bug that was revealed by #1914:
the loading progress bar never actually finishes. Prior to #1914, you
could actually see the loading bar if you made your window narrow
enough:
![Screenshot of a zombie progress bar](https://user-images.githubusercontent.com/4317806/53674700-95317580-3c44-11e9-924f-82f6dc8d5f18.png)

This was because the prior version of the dashboard rendered the main
tool irrespective of whether the data had actually finished loading. As
of #1914, the “loading” and “active” states are mutually exclusive, so
instead of just having a zombie progress bar we actually block the view.

The cause of the bug is a promise that never resolves. (The executor
provided to the `Promise` constructor’s return value is ignored; only
the `resolve` and `reject` callbacks can fulfill a promise.)

This was not caught while testing #1914 because it cannot be reproduced
with the demo data present at that commit; with that more complete set
of data, there are other code paths that clobber the progress value to
100% (which is really a separate issue, but that’s not important right
now), such as `_maybeUpdateData`.

Test Plan:
Regenerate profile plugin demo data if you haven’t done so since #1942,
then launch TensorBoard:

```
$ rm -rf /tmp/profile_demo/
$ bazel run //tensorboard/plugins/profile:profile_demo
$ bazel run //tensorboard -- --logdir /tmp/profile_demo
```

The trace viewer should be displayed (prior to this commit, it wasn’t).

wchargin-branch: profile-broken-promises
@qiuminxu
Copy link
Contributor

qiuminxu commented Mar 2, 2019

Thanks for the fix!

@wchargin wchargin merged commit b0d4a2c into master Mar 2, 2019
@wchargin wchargin deleted the wchargin-profile-broken-promises branch March 2, 2019 01:58
wchargin added a commit to wchargin/tensorboard that referenced this pull request Mar 5, 2019
Summary:
The profile dashboard had an existing bug that was revealed by tensorflow#1914:
the loading progress bar never actually finishes. Prior to tensorflow#1914, you
could actually see the loading bar if you made your window narrow
enough:
![Screenshot of a zombie progress bar](https://user-images.githubusercontent.com/4317806/53674700-95317580-3c44-11e9-924f-82f6dc8d5f18.png)

This was because the prior version of the dashboard rendered the main
tool irrespective of whether the data had actually finished loading. As
of tensorflow#1914, the “loading” and “active” states are mutually exclusive, so
instead of just having a zombie progress bar we actually block the view.

The cause of the bug is a promise that never resolves. (The executor
provided to the `Promise` constructor’s return value is ignored; only
the `resolve` and `reject` callbacks can fulfill a promise.)

This was not caught while testing tensorflow#1914 because it cannot be reproduced
with the demo data present at that commit; with that more complete set
of data, there are other code paths that clobber the progress value to
100% (which is really a separate issue, but that’s not important right
now), such as `_maybeUpdateData`.

Test Plan:
Regenerate profile plugin demo data if you haven’t done so since tensorflow#1942,
then launch TensorBoard:

```
$ rm -rf /tmp/profile_demo/
$ bazel run //tensorboard/plugins/profile:profile_demo
$ bazel run //tensorboard -- --logdir /tmp/profile_demo
```

The trace viewer should be displayed (prior to this commit, it wasn’t).

wchargin-branch: profile-broken-promises
@wchargin wchargin mentioned this pull request Mar 5, 2019
wchargin added a commit that referenced this pull request Mar 6, 2019
Summary:
The profile dashboard had an existing bug that was revealed by #1914:
the loading progress bar never actually finishes. Prior to #1914, you
could actually see the loading bar if you made your window narrow
enough:
![Screenshot of a zombie progress bar](https://user-images.githubusercontent.com/4317806/53674700-95317580-3c44-11e9-924f-82f6dc8d5f18.png)

This was because the prior version of the dashboard rendered the main
tool irrespective of whether the data had actually finished loading. As
of #1914, the “loading” and “active” states are mutually exclusive, so
instead of just having a zombie progress bar we actually block the view.

The cause of the bug is a promise that never resolves. (The executor
provided to the `Promise` constructor’s return value is ignored; only
the `resolve` and `reject` callbacks can fulfill a promise.)

This was not caught while testing #1914 because it cannot be reproduced
with the demo data present at that commit; with that more complete set
of data, there are other code paths that clobber the progress value to
100% (which is really a separate issue, but that’s not important right
now), such as `_maybeUpdateData`.

Test Plan:
Regenerate profile plugin demo data if you haven’t done so since #1942,
then launch TensorBoard:

```
$ rm -rf /tmp/profile_demo/
$ bazel run //tensorboard/plugins/profile:profile_demo
$ bazel run //tensorboard -- --logdir /tmp/profile_demo
```

The trace viewer should be displayed (prior to this commit, it wasn’t).

wchargin-branch: profile-broken-promises
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