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

Add plugin to redirect users to new profile plugin #3216

Merged
merged 6 commits into from
Feb 5, 2020

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Feb 5, 2020

Summary:
We add a new profile_redirect plugin, called “Profile (moved)” in the
UI, which teaches users how to install the dynamic plugin:

Screenshot of the new plugin

The instructions currently pull in Test PyPI sources; we’ll swap these
out for real PyPI once the package is published.

Test Plan:
Create a new virtualenv, install tf-nightly, uninstall tb-nightly,
and run //tensorboard/pip_package:extract_pip_package and install the
resulting wheel. Launch TensorBoard and note that the “Profile (moved)”
plugin appears in the inactive plugins list. Click the “Copy to
clipboard” button and execute the copied pip install command from the
test virtualenv. Relaunch TensorBoard, and note that the dynamic profile
plugin now appears and the “Profile (moved)” plugin is gone entirely.

Unit tests included for the conditional loading behavior. Changing the
attempted import (in ProfileRedirectPluginLoader.load) to a module
that always can be imported (like sys) causes one of the tests to
fail; changing it to one that can never be imported (like zzz) causes
the other to fail.

wchargin-branch: profile-redirect

Summary:
We add a new `profile_redirect` plugin, called “Profile (moved)” in the
UI, which teaches users how to install the dynamic plugin:

![Screenshot of the new plugin][ss]

The instructions currently pull in Test PyPI sources; we’ll swap these
out for real PyPI once the package is published.

[ss]: https://user-images.githubusercontent.com/4317806/73811360-229f1300-478e-11ea-9341-f38d59170e3b.png

Test Plan:
Create a new virtualenv, install `tf-nightly`, uninstall `tb-nightly`,
and run `//tensorboard/pip_package:extract_pip_package` and install the
resulting wheel. Launch TensorBoard and note that the “Profile (moved)”
plugin appears in the inactive plugins list. Click the “Copy to
clipboard” button and execute the copied `pip install` command from the
test virtualenv. Relaunch TensorBoard, and note that the dynamic profile
plugin now appears and the “Profile (moved)” plugin is gone entirely.

Unit tests included for the conditional loading behavior. Changing the
attempted import (in `ProfileRedirectPluginLoader.load`) to a module
that always can be imported (like `sys`) causes one of the tests to
fail; changing it to one that can never be imported (like `zzz`) causes
the other to fail.

wchargin-branch: profile-redirect
@wchargin
Copy link
Contributor Author

wchargin commented Feb 5, 2020

Reviewers: @qiuminxu for anything profile-specific (wording, etc.);
@stephanwlee for general frontend.

wchargin-branch: profile-redirect
wchargin-source: e627adb6fbe65899b25d7d79e6166593d2268107
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

Besides the rel="import" on the dashboard-style, other comments are inquiries or nits.

</div>
</div>

<style include="dashboard-style"></style>
Copy link
Contributor

Choose a reason for hiding this comment

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

Omit this unless you are making use of it. Else, I believe you need to import https://github.com/tensorflow/tensorboard/blob/master/tensorboard/components/tf_dashboard_common/dashboard-style.html in rel="import".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks! (I’d used this originally but decided that I didn’t need
it; removed the build dep and import, but forgot to remove this include.
Done.)

}
}
})();
tryCopy
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: since we are only "officially supporting" evergreen browsers, you may use async/await and make this code read better (wait... you are already using async/await :)).

async _copyInstallCommand() {
    const tryCopy = async () => {
      const textarea = this.$.commandTextarea;
      textarea.select();
      try {
        await navigator.clipboard.writeText(textarea.textContent);
      } catch (error) {
        // Fallback approach.
        if (!document.execCommand('copy')) {
          return Promise.reject();
        }
      }
    };

    try {
      await tryCopy();
      this.$.copiedMessage.innerText = 'Copied.';
    }  catch (e) {
      this.$.copiedMessage.innerText = 'Failed to copy to clipboard..';
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that second try structure is definitely nicer. Done; thanks!

wchargin-branch: profile-redirect
wchargin-source: e3a25f6f2e337aa81576524367fdb9f619e7a04c
wchargin-branch: profile-redirect
wchargin-source: e3a25f6f2e337aa81576524367fdb9f619e7a04c
Copy link
Contributor Author

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Thanks for the careful review!

</div>
</div>

<style include="dashboard-style"></style>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks! (I’d used this originally but decided that I didn’t need
it; removed the build dep and import, but forgot to remove this include.
Done.)

}
}
})();
tryCopy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that second try structure is definitely nicer. Done; thanks!

wchargin-branch: profile-redirect
wchargin-source: c832dba267a4820a123e80f7d699d7eeeb4fdf6c
properties: {
_installCommand: {
type: String,
readonly: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

readonly -> readOnly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch; thanks! Done.

wchargin-branch: profile-redirect
wchargin-source: 29a94e1df6ff0579dbe49cce3bceb76308709bde
@qiuminxu
Copy link
Contributor

qiuminxu commented Feb 5, 2020

Tested works great, thanks William for the friendlier solution!

@wchargin
Copy link
Contributor Author

wchargin commented Feb 5, 2020

Thanks for the review, @qiuminxu, and thanks for testing!

@wchargin wchargin merged commit d349c6f into master Feb 5, 2020
@wchargin wchargin deleted the wchargin-profile-redirect branch February 5, 2020 19:57
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.

4 participants