Release 1.13.1 #1963
Release 1.13.1 #1963
Conversation
…ensorflow#1898) When loading just examples, without inferring through a model (such as loading data from a csv file), still need to call refreshDive_ to fully refresh the dataset display, including calculating statistics.
Summary: Fixes tensorflow#1895 by replacing the (apparently unportable) `%s` format specifier with an explicit subtraction from epoch. Test Plan: I don’t have easy access to a Windows machine with a Bazel/TensorBoard setup, but I verified that this expression can be evaluated by itself: ``` Python 3.6.8 (tags/v3.6.8:3c6b436a57, Dec 24 2018, 00:16:47) [MSC v.1916 64 bit (AMD64)] on win32 Type "help", "copyright", "credits" or "license()" for more information. >>> import datetime >>> dt = datetime.datetime.now() >>> dt.strftime("%s") Traceback (most recent call last): File "<pyshell#2>", line 1, in <module> dt.strftime("%s") ValueError: Invalid format string >>> int((dt - datetime.datetime.fromtimestamp(0)).total_seconds()) 1551203889 ``` Running `git grep strftime.*%s` returns no matches as of this commit. wchargin-branch: windows-strftime
Summary: The profile dashboard now shows a “disabled” message on the frontend when we detect that we’re running in a Colab notebook. See tensorflow#1913. This commit does _not_ cause the profile dashboard to be marked inactive by the main TensorBoard UI. We might want to do that, but it’s less straightforward: currently, the backend is responsible for telling the frontend which plugins are active, and the backend can’t know whether we’re running in Colab. As a quick hack, we could add special-case logic in `tf-tensorboard.html` to check for the profile plugin specifically. As a longer-term solution, we could let the `tf_tensorboard.Dashboard` interface specify an `isActive` callback, defaulting to `() => true`. ![Screenshot of the “Profiling isn’t yet supported in Colab” message][s] [s]: https://user-images.githubusercontent.com/4317806/53541886-b83b1880-3ad0-11e9-9501-299f5e671d7a.png The implementation includes a small refactor to the profile dashboard to make it easier to introduce a new state. Test Plan: Evaluate `!!(window.TENSORBOARD_ENV || {}).IN_COLAB` in both standalone TensorBoard and Colab `%tensorboard`, and note that each evaluates to the appropriate boolean. Run TensorBoard locally with the profile plugin’s demo data, and note that its behavior is unchanged. To trigger the loading state, it suffices to load the dashboard, then throttle your network to “Slow 3G”, then change the selected tool. To trigger the “no data” state, relaunch TensorBoard pointing at a different log directory. Then, test the Colab behavior by either pointing TensorBoard at a log directory that does not contain profile data, then executing ```js window.TENSORBOARD_ENV = window.TENSORBOARD_ENV || {}; window.TENSORBOARD_ENV["IN_COLAB"] = true; ``` in the main TensorBoard pane before switching to the inactive profile dashboard. Note that this is the same setup JavaScript as is injected into the Colab output frame by `notebook.py`. wchargin-branch: profile-disable-colab
Summary: Not only is opening in new tabs a better user experience, it’s necessary in notebook contexts so that we don’t try to load the GitHub docs in an iframe, which would fail because GitHub sets `frame-ancestors 'none'`. (When I tested tensorflow#1914 in Colab, I instinctively Ctrl-clicked the link, thus not hitting this issue.) Test Plan: Tested that in both Jupyter and Colab, clicking on a help link before this change yields a white frame with a “Refused to display…” console error, while after this change it opens the appropriate link in a new tab. Checked statically that each link has `rel="noopener" target="_blank"`: ```shell $ <./tensorboard/plugins/profile/tf_profile_dashboard/tf-profile-dashboard.html tee \ > >(grep 'href=' | grep -Fcv '<link') \ > >(grep -Fc 'rel="noopener"') \ > >(grep -Fc 'target="_blank"') \ > >/dev/null 6 6 6 ``` wchargin-branch: profile-help-in-new-tab
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:  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
|
Additional notes:
|
RELEASE.md
Outdated
| # Release 1.13.1 | ||
|
|
||
| ## Bug fixes | ||
| - Fix launch error on Windows (#1900) |
There was a problem hiding this comment.
If these fixed a bug that was actually filed, I list that first, and then put the PR fixing it as (PR #NNNN) after that. If no bug I list the PR first as being the documentation for the bug. See 1.13.0 for the usual style.
So in this case that would be #1895 fixed by #1900, and #1794 fixed by #1898. For the profile dashboard I guess we could use #1913 but probably makes sense to just reference #1914.
I think #1945 can be broken out as a separate fix with no attached bug, since it fixes an issue that's not inherently related to Colab and was observable before (by compressing the viewport enough).
Finally, "launch error" is pretty vague - perhaps "Fix strftime-related launch error on Windows"?
There was a problem hiding this comment.
If these fixed a bug that was actually filed, I list that first, and
then put the PR fixing it […] If no bug I list the PR first as being
the documentation for the bug
Ah, so the first token is either a PR or an issue depending on the
line item—okay, I understand now. Done. (Rest done as well.)
There was a problem hiding this comment.
Yes... I won't argue it's the most straightforward precedent ever, but that is what I've been doing. I'm open to alternative suggestions though as long as we commit to being consistent with some convention. Mostly the thing I'd like is to include the bug reference itself in the text when fixing a numbered bug so that it's clear from the relnotes without having to visit the PR first.
An alternative convention that's maybe clearer would just be to insert the bug number in the prose text somewhere, like:
- Fix #ISSUE strftime-related startup error on Windows (#PR)
- Disable profile dashboard in Colab where it doesn't work yet (#PR)
There was a problem hiding this comment.
Yep, no worries; consistency is good, and this is fine with me.
RELEASE.md
Outdated
| ## Bug fixes | ||
| - Fix launch error on Windows (#1900) | ||
| - Fix What-If Tool loading examples without inference (#1898) | ||
| - Disable the profile dashboard inside Colab, where it doesn’t work (#1914, #1922, #1945) |
There was a problem hiding this comment.
I've been wrapping to 80 chars except for long URLs per the internal markdown style.
RELEASE.md
Outdated
| # Release 1.13.1 | ||
|
|
||
| ## Bug fixes | ||
| - Fix launch error on Windows (#1900) |
There was a problem hiding this comment.
If these fixed a bug that was actually filed, I list that first, and
then put the PR fixing it […] If no bug I list the PR first as being
the documentation for the bug
Ah, so the first token is either a PR or an issue depending on the
line item—okay, I understand now. Done. (Rest done as well.)
RELEASE.md
Outdated
| ## Bug fixes | ||
| - Fix launch error on Windows (#1900) | ||
| - Fix What-If Tool loading examples without inference (#1898) | ||
| - Disable the profile dashboard inside Colab, where it doesn’t work (#1914, #1922, #1945) |
|
Force-pushed; tree unchanged: 5dc725c74fa75cbf5dbab42ca37812ece5e11a21. |
Merge this pull request ONLY via rebase, NOT via squash or merge-commit.
This PR cherry-picks #1898, #1900, #1914, #1922, and #1945 on top of the
1.13 tracking branch to form 1.13.1, per our plan in #1906.
Tested in Python 3 on Windows and in Python 2 on Linux: TensorBoard
starts, and the profile dashboard works (all tools) and is disabled if
you set
window.TENSORBOARD_ENV = {IN_COLAB: true}, in which case thehelp link opens in a new tab.