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

Removed redundant lines from get_started.ipynb #3452

Closed
wants to merge 1 commit into from

Conversation

ManishAradwad
Copy link
Contributor

@ManishAradwad ManishAradwad commented Mar 31, 2020

Fixes : #3451
@lamberta Can you plz review this PR.

@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 wchargin self-requested a review March 31, 2020 15:44
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 for the pull request!

There are a lot of extraneous changes in the output here due to
re-running the whole notebook. Only 5–10 lines actually need to be
changed, but this diff touches 526 lines, which makes it hard to see
what the patch actually does. Could you please revert those extraneous
changes? The GitHub diff viewer should be able to comfortably show the
change rather than complaining that the diff is too large.

Also, it looks like there are more uses of %tensorflow_version 2.x in
the rest of the docs:

$ git grep '%tensorflow_version 2'
docs/graphs.ipynb:        "  %tensorflow_version 2.x\n",
docs/hyperparameter_tuning_with_hparams.ipynb:        "  %tensorflow_version 2.x\n",
docs/image_summaries.ipynb:        "  %tensorflow_version 2.x\n",
docs/migrate.ipynb:        "  %tensorflow_version 2.x\n",
docs/migrate.ipynb:        "  %tensorflow_version 2.x\n",
docs/scalars_and_keras.ipynb:        "  %tensorflow_version 2.x\n",
docs/tbdev_getting_started.ipynb:        "  %tensorflow_version 2.x\n",
docs/tensorboard_in_notebooks.ipynb:        "  %tensorflow_version 2.x\n",

It’d be best to fix all of them in the same PR. Can you update this
patch to include changes to those files, too?

@lamberta
Copy link
Member

Hi @ManishAradwad , to make the diffs easier to read, you can try running the notebook through nbfmt.py or downloading from Colab.

@wchargin
Copy link
Contributor

@lamberta: Thanks for the pointer to nbfmt.py (and thanks, apparently,
for writing it!). That looks like it would make things a bunch easier
for us. I’ll send a PR to reformat our docs and add that to CI.

@lamberta
Copy link
Member

@wchargin Cool, let me know how it goes. Now looking at ways to integrate it into the GitHub CI to reduce friction for tensorflow/docs PRs

@wchargin
Copy link
Contributor

Opened #3455, which does integrate into GitHub Actions. It’s pretty
painless (totally sufficient for me), though it would be even painlesser
if you made a GitHub Action that we could just depend on instead of
having to wget the file and pip install absl-py ourselves. :-)

@wchargin
Copy link
Contributor

@ManishAradwad: I’ve merged #3455; please feel free to update this PR
(after rebasing on master) at your convenience.

@ManishAradwad
Copy link
Contributor Author

@wchargin Thanks for the review. I'm unable to resolve the conflicts of this branch with the master branch(apparently the nb is not opening in vscode). I think to avoid any complexity, it would be better if I close this PR and open another one. I'll send one soon.
Thanks!

@ManishAradwad ManishAradwad deleted the issue#3451 branch April 1, 2020 06:00
@ManishAradwad
Copy link
Contributor Author

@wchargin I've created another PR at #3459
Plz review it

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.

No need to manually specify version in notebooks
4 participants