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

TFTRT: LOG(ERROR) --> VLOG(1) when use_calibration=True with fp32/fp16 #24302

Conversation

pooyadavoodi
Copy link

This is just to keep the default LOG cleaner.

The API mentions that use_calibration is ignored in case of precisions fp32/fp16.

My preference would be to change the default to use_calibration = False but that's a change in API behavior, and @aaroey asked to postpone that to TF2.0.

@pooyadavoodi pooyadavoodi force-pushed the tftrt_change_log_use_calibration branch from 8165173 to ecb1d04 Compare December 11, 2018 23:15
Copy link
Contributor

@smit-hinsu smit-hinsu left a comment

Choose a reason for hiding this comment

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

Thanks, Pooya!

@aaroey
Copy link
Member

aaroey commented Dec 11, 2018

Can we use LOG(WARNING)?

@smit-hinsu smit-hinsu requested review from aaroey and removed request for aaroey and azaks2 December 11, 2018 23:20
@pooyadavoodi
Copy link
Author

Can we use LOG(WARNING)?

@aaroey WARNING is definitely better than ERROR. But I still wouldn't print a warning just because it would confuse users when they see the message. My guess is that your concern is about users who actually want to do calibration with fp32/fp16, but I don't think anyone would be interested in that functionality. Please let me know if you still want to change it to WARNING.

@ymodak ymodak self-assigned this Dec 11, 2018
@ymodak ymodak added the kokoro:force-run Tests on submitted change label Dec 11, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 11, 2018
@aaroey
Copy link
Member

aaroey commented Dec 12, 2018

@pooyadavoodi fair enough, thanks!

@aaroey aaroey added the ready to pull PR ready for merge process label Dec 12, 2018
@tensorflow-copybara tensorflow-copybara merged commit ecb1d04 into tensorflow:master Dec 13, 2018
tensorflow-copybara pushed a commit that referenced this pull request Dec 13, 2018
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants