Skip to content

Conversation

MalcolmSlaney
Copy link
Contributor

Be more clear that the train_step and test_step are also moved to the accelerator. This was not clear before. This makes it explicit, which should help people understanding TPU efficiency.

Be more clear that the train_step and test_step are also moved to the accelerator.  This was not clear before.  This makes it explicit, which should help people understanding TPU efficiency.
@google-cla google-cla bot added the cla: yes CLA has been signed label Nov 8, 2021
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

Preview

Preview and run these notebook edits with Google Colab: Rendered notebook diffs available on ReviewNB.com.

Format and style

Use the TensorFlow docs notebook tools to format for consistent source diffs and lint for style:
$ python3 -m pip install -U --user git+https://github.com/tensorflow/docs

$ python3 -m tensorflow_docs.tools.nbfmt notebook.ipynb
$ python3 -m tensorflow_docs.tools.nblint --arg=repo:tensorflow/docs notebook.ipynb
If commits are added to the pull request, synchronize your local branch: git pull origin patch-1

@8bitmp3
Copy link
Contributor

8bitmp3 commented Dec 1, 2021

@rchao Can you PTAL? Thanks!

@@ -508,7 +508,7 @@
"Here's what you need to change in your code:\n",
"\n",
"1. Create an instance of the appropriate `tf.distribute.Strategy`.\n",
"2. Move the creation of Keras model, optimizer and metrics inside `strategy.scope`.\n",
"2. Move the creation of Keras model, optimizer and metrics inside `strategy.scope`. Thus the code in the model's run(), train_step(), and test_step() will all be distributed and executed on the accelerator(s).\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what "model's run()" refers to. Also, please backtick the symbols. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Made the references explicit by describing them as methods.

(Let me know if I need to be even more explicit.)

Thanks for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! train_step() and test_step() make sense to me, but I'm not sure what run() refers to because there is not such run() method of a Keras model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I should have said call method (not run). Sorry for the tardy response... I had to look up the right terminology.

Made the references to the run, train_step and test_step methods more clear.

[Do I need to be more explicit that the Keras model is a (sub) class and it does the work with the run, train_step and test_step methods, all of which might be subclassed for a specific model implementation?]
@lamberta lamberta removed their request for review January 4, 2022 21:50
@MalcolmSlaney
Copy link
Contributor Author

Ping? Is the update (using the proper terminology) still pending? I think I have pushed everything correctly...

@MarkDaoust
Copy link
Member

Hi, thanks for the ping.

I'll see if I can get this merged.

@MarkDaoust MarkDaoust added the ready to pull Start merge process label May 4, 2022
@github-actions github-actions bot added the lgtm Community-added approval label May 4, 2022
@copybara-service copybara-service bot merged commit 20f4caa into tensorflow:master May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA has been signed lgtm Community-added approval ready to pull Start merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants