Skip to content

Conversation

@anirudh161
Copy link
Contributor

  • Motivation for features / changes:
  1. Saved the notebooks with outputs enabled (and private outputs disabled) as a workaround to prevent broken iframes rendering on tensorflow.org
  2. Made some minor text changes and added a link to the Dev Summit 2020 talk

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@wchargin
Copy link
Contributor

wchargin commented Apr 1, 2020

For context, this supersedes #3441.

Copy link
Contributor

@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.

As listed in the check failure, please run this through nbfmt.py to
fix formatting errors and make the patch easier to review. The patch as
written is 240 lines, but is only 30 lines after formatting.

Copy link
Contributor

@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! The prose changes look good to me. Was the broken-iframe issue
resolved separately? I don’t see any outputs added in this change, as
suggested by the PR description.

@anirudh161
Copy link
Contributor Author

That's weird. The outputs were there before I ran nbfmt.py on the notebook. Let me look into it and submit another commit

@wchargin
Copy link
Contributor

wchargin commented Apr 2, 2020

As I mentioned on #3441, make sure that private_outputs: false is
set in the notebook to preserve the contents. If you have a notebook
with output cells, you can set that easily by running nbfmt.py on the
notebook and passing --preserve_outputs. This will write the option
into the notebook so that future nbfmt.py runs know not to remove
them, even when the flag is not passed.

It doesn’t look like private_outputs: false was set at any point in
this change, but it also doesn’t look like the outputs were saved. The
“Formatted notebook with nbfmt” commit has a delta of 111 insertions and
110 deletions (i.e., just sorted keys and fixed trailing newline), and
you can see from the diff that it didn’t actually remove any outputs
(they were all outputs: [] already).

@anirudh161
Copy link
Contributor Author

I was so sure I saved the outputs then! Guess I didn't after all :) I added another commit with outputs saved

@anirudh161 anirudh161 requested a review from wchargin April 3, 2020 07:09
Copy link
Contributor

@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.

The TensorBoard output cells in this notebook don’t have cache entries
for the profile dashboard. When I open this notebook in Colab and
navigate to the TensorBoard cells without running them, I can use the
scalars dashboard, but the profile dashboard just shows “Not found”.

You can fix this by making sure to load the profile dashboard and select
the correct trace after running all cells in the notebook. The way that
this works is that any requests made during execution are cached when
you save the notebook, so you have to actually load the dashboard and
its data for the requests to persist.

This seems kind of important to me given that it takes multiple minutes
just to run the notebook to the point where it can be interactive, but
I’ll leave it up to you. Shall I merge this as is, or would you like to
fix the cache?

@anirudh161
Copy link
Contributor Author

That's a good point. I fixed the cache entries and added a new commit

@anirudh161 anirudh161 requested a review from wchargin April 6, 2020 17:37
@wchargin
Copy link
Contributor

wchargin commented Apr 7, 2020

Awesome; thanks! Tested; this works for me both before and after running
the notebook.

I’ll go ahead and merge this now.

@wchargin wchargin changed the title Fix bad images and other minor text changes docs: update profiler tutorial Apr 7, 2020
@wchargin wchargin merged commit 646debc into tensorflow:master Apr 7, 2020
bileschi pushed a commit to bileschi/tensorboard that referenced this pull request Apr 15, 2020
Changes:
  
 1. Saved the notebooks with outputs enabled (and private outputs
    disabled) as a workaround to prevent broken iframes rendering on
    tensorflow.org
 2. Made some minor text changes and added a link to the Dev Summit 2020
    talk
bileschi pushed a commit that referenced this pull request Apr 15, 2020
Changes:
  
 1. Saved the notebooks with outputs enabled (and private outputs
    disabled) as a workaround to prevent broken iframes rendering on
    tensorflow.org
 2. Made some minor text changes and added a link to the Dev Summit 2020
    talk
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