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-TRT] Cast Converter Re-Engineered #55491

Merged

Conversation

DEKHTIARJonathan
Copy link
Contributor

@DEKHTIARJonathan DEKHTIARJonathan commented Apr 5, 2022

This PR changes the way TF-TRT deal with Cast nodes. TensorRT engineers advised us to treat Cast as an Identity node and let TensorRT decides on which compute precision to use according to precision_mode=....

This behavior can be deactivated using TF_TRT_EXPERIMENTAL_FEATURES=reject_fp32_fp16_cast if needed to work around any unforeseen issue

This PR also adds converter.summary() to TF-TRT test files in order to ease test debugability

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Apr 5, 2022
@DEKHTIARJonathan
Copy link
Contributor Author

@bixia1 for review
CC: @tfeher

@gbaned gbaned added the comp:gpu:tensorrt Issues specific to TensorRT label Apr 5, 2022
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Apr 5, 2022
@gbaned gbaned requested a review from bixia1 April 5, 2022 12:41
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Apr 5, 2022
@bixia1
Copy link
Contributor

bixia1 commented Apr 5, 2022

I have two general comment on this PR:
(1) the existing support is cast from fp16 to fp32, the feature name added here is called reject_fp32_fp16_cast. Shouldn't it be reject_fp16_fp32_cast?
(2) the existing code only supports cast from fp16 to fp32, the change here make us support fp32 to fp16 as well. Is this an intensionally change? If yes, please add test and add this to the PR description.

@gbaned
Copy link
Contributor

gbaned commented Apr 7, 2022

Hi @DEKHTIARJonathan Can you please check @bixia1's comments and keep us posted ? Thank you!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Apr 7, 2022
@gbaned
Copy link
Contributor

gbaned commented Apr 19, 2022

@DEKHTIARJonathan Any update on this PR? Please. Thank you!

@DEKHTIARJonathan DEKHTIARJonathan force-pushed the tftrt/cast_rework branch 5 times, most recently from ad942a9 to 4b7d551 Compare May 5, 2022 09:44
@DEKHTIARJonathan
Copy link
Contributor Author

@bixia1

(1) the existing support is cast from fp16 to fp32, the feature name added here is called reject_fp32_fp16_cast. Shouldn't it be reject_fp16_fp32_cast?

What I mean by reject_fp32_fp16_cast is to reject all FP Cast (fp32 => fp16 and fp16 => fp32), I am renaming the feature to reject_all_fp_cast_ops, I think it's more self explanatory.

(2) the existing code only supports cast from fp16 to fp32, the change here make us support fp32 to fp16 as well. Is this an intensionally change? If yes, please add test and add this to the PR description.

Yes absolutely, it is intentional. This PR changes the way Cast Operation in handled by TRT to and from FP16/32 precision. I am updating the PR description

@DEKHTIARJonathan
Copy link
Contributor Author

@bixia1 please review. This PR is good to go ;)

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 5, 2022
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer May 5, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 5, 2022
@copybara-service copybara-service bot merged commit 9306d07 into tensorflow:master May 6, 2022
@DEKHTIARJonathan DEKHTIARJonathan deleted the tftrt/cast_rework branch May 6, 2022 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:gpu:tensorrt Issues specific to TensorRT ready to pull PR ready for merge process size:M CL Change Size: Medium stat:awaiting response Status - Awaiting response from author
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

4 participants