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

[TF 2.0 API Docs] Docstring for tf.train.experimental.enable_mixed_precision_graph_rewrite #29249

Merged
merged 1 commit into from
Aug 13, 2019

Conversation

tlkh
Copy link

@tlkh tlkh commented Jun 1, 2019

In response to #29241

Improved the docstring for tf.train.experimental.enable_mixed_precision_graph_rewrite:

  • Added example for using function
  • Added colab notebook to demonstrate speed-up without performance penalty
  • Added original graphic for loss scaling (source)
  • Added more information about graph rewrite operation
  • Added performance guide
  • Added exception information
  • Added more clarification to loss_scale argument

A gist with the rendered docstring is here for ease of review.

Thank you, any feedback or criticism is welcome.

@tensorflow-bot tensorflow-bot bot added the size:M CL Change Size: Medium label Jun 1, 2019
@perfinion perfinion changed the title Docstring for tf.train.experimental.enable_mixed_precision_graph_rewrite [TF 2.0 API Docs] Docstring for tf.train.experimental.enable_mixed_precision_graph_rewrite Jun 1, 2019
@perfinion perfinion added the type:docs-bug Document issues label Jun 1, 2019
Copy link
Member

@perfinion perfinion left a comment

Choose a reason for hiding this comment

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

Great work! Just a few small things to tweak :)

@perfinion
Copy link
Member

Can you change the colours in the image as well? On my monitor the green and blue look fairly similar its hard to distinguish.

@tlkh tlkh force-pushed the master branch 2 times, most recently from 1129f0a to 85b7da7 Compare June 1, 2019 14:25
@tlkh
Copy link
Author

tlkh commented Jun 1, 2019

I have made all the requested changes!

perfinion
perfinion previously approved these changes Jun 1, 2019
@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 1, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 1, 2019
@perfinion
Copy link
Member

@rthadur Can you make sure the links are updated when this is merged?

@tlkh
Copy link
Author

tlkh commented Jun 1, 2019

Fixed the pylint 80 char line limit errors.

@rthadur rthadur requested a review from perfinion June 1, 2019 17:06
@tlkh
Copy link
Author

tlkh commented Jun 2, 2019

Pushed the amended commit with the requested changes.

@perfinion perfinion added the kokoro:force-run Tests on submitted change label Jun 2, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 2, 2019
@rthadur rthadur requested a review from martinwicke June 3, 2019 17:48
@recrusader
Copy link

@tlkh, when enabling "enable_mixed_precision_graph_rewrite", how about the data type of model network when defining the model?
still use FP32? if it is, how to output the FP16 trained model? Thanks!

@tlkh
Copy link
Author

tlkh commented Jul 2, 2019

Thanks a lot, tlkh. However, if FP16 trained network cannot be saved, it is difficult to use the network outside tensorflow after finishing the training.
In addition, it seems that ops conversion from FP32 to FP16 can happen on new GPU, like Volta. TF cannot do this conversion for old one.

If you wish to use the network outside of TensorFlow for inference in FP16, typically just converting all the datatypes to FP16 will work. Only for training does there have to be special considerations. There are other toolkits to help with inference optimization outside of TensorFlow, such as TensorRT.

Only Volta and Turing GPUs have the FP16 units and Tensor Cores that can benefit from mixed precision. Older GPUs will not see a benefit, hence the feature is not enabled for them.

@recrusader let's move this discussion to a new GitHub issue if you wish to raise any concerns or suggestions. Keep this thread for discussion specifically about this PR.

@recrusader
Copy link

I asked this question in official model. However, no one can give me an answer. Thank you very much! I think that I have got the answer.

@tlkh
Copy link
Author

tlkh commented Aug 10, 2019

@perfinion @martinwicke requesting review once again.

Changes made:

  • image and example notebook is now hosted by NVIDIA
  • docstring wording has been improved, and updated/merged with recent changes from master
  • docstring added to both enable_mixed_precision_graph_rewrite and enable_mixed_precision_graph_rewrite_v1, with the correct distinction made with which kinds of Optimizer are accepted

Thank you!

@tlkh tlkh force-pushed the master branch 3 times, most recently from 02d2eea to 75f7b2b Compare August 10, 2019 05:54
Copy link
Member

@martinwicke martinwicke left a comment

Choose a reason for hiding this comment

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

@MarkDaoust will this render correctly? I see there's no indent on the args and returns sections, I'm wondering whether the indent is required.

@tlkh
Copy link
Author

tlkh commented Aug 12, 2019

@martinwicke thanks for pointing that out. Let me fix that anyway.
EDIT: fixed the indent!

Copy link
Member

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

Hi @tlkh,

It looks like you miss understood @martinwicke's comment.

All the Args/Returns/Raises blocks need to be indented, or they will not be recognized by our linters, and will render incorrectly on tensorflow.org (They will be interpreted as markdown which will just flatten them into a single paragraph).

Thanks.

@tlkh
Copy link
Author

tlkh commented Aug 13, 2019

Sorry about that! I fixed it.

@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Aug 13, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 13, 2019
@tlkh
Copy link
Author

tlkh commented Aug 13, 2019

Hi tlkh,

It looks like you miss understood martinwicke's comment.

All the Args/Returns/Raises blocks need to be indented, or they will not be recognized by our linters, and will render incorrectly on tensorflow.org (They will be interpreted as markdown which will just flatten them into a single paragraph).

Thanks.

MarkDaoust Hmm I've addressed the changes but I'm not sure why GitHub is still complaining about "1 change requested". Do you need to approve it as well?

Edit: never mind

@tensorflow-copybara tensorflow-copybara merged commit ca9e667 into tensorflow:master Aug 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium type:docs-bug Document issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants