Skip to content

Implemented TFLogSinks#41733

Merged
copybara-service[bot] merged 18 commits intotensorflow:masterfrom
avitebskiy:feature/log-sinks
Nov 19, 2020
Merged

Implemented TFLogSinks#41733
copybara-service[bot] merged 18 commits intotensorflow:masterfrom
avitebskiy:feature/log-sinks

Conversation

@avitebskiy
Copy link
Contributor

@avitebskiy avitebskiy commented Jul 25, 2020

- added a `no_default_logger` config setting
- changed `TFLogEntry::log_severity()` to `TFLogEntry::Severity()`
- changed `TFLogEntry::ToString()` to `TFLogEntry::Message()`
- added `TFLogEntry::FName()` and `TFLogEntry::Line()`
- added TFDefaultLogSink
- TFAddLogSink and TFRemoveLogSink now work
@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label Jul 25, 2020
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@avitebskiy
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gbaned gbaned self-assigned this Jul 26, 2020
@gbaned gbaned requested a review from jaingaurav July 26, 2020 05:28
@gbaned gbaned added the awaiting review Pull request awaiting review label Jul 29, 2020
@gbaned gbaned requested a review from allenlavoie August 6, 2020 17:21
@allenlavoie
Copy link
Member

Rohan, adding you because it looks like you expressed interest in reviewing in the attached issue. But please comment if that's changed.

Rohan is currently out, so I wouldn't expect an immediate response.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Aug 8, 2020
@gbaned gbaned added the awaiting review Pull request awaiting review label Aug 18, 2020
@gbaned
Copy link
Contributor

gbaned commented Sep 2, 2020

@rohan100jain Any update on this PR? Please. Thanks!

1 similar comment
@gbaned
Copy link
Contributor

gbaned commented Sep 23, 2020

@rohan100jain Any update on this PR? Please. Thanks!

@jaingaurav jaingaurav removed their request for review September 23, 2020 22:56
@gbaned
Copy link
Contributor

gbaned commented Oct 6, 2020

@rohan100jain Any update on this PR? Please. Thanks!

@avitebskiy
Copy link
Contributor Author

Come on y'all. This has been pending since July. Any updates?

@avitebskiy
Copy link
Contributor Author

avitebskiy commented Oct 16, 2020

Look, I know this is not the most sophisticated logging scheme: this will work just the same for what it already did and allow users to actually redirect TF logging. It is not an asynchronous logging system, so adding multiple sinks will degrade performance. However, this implements the interface that existed in TF for years and lied about it being implemented the whole time.

If you have any questions about whether the code is valid, I'll certainly answer them, but I don't understand why this is taking so long to approve and why so many people want to look at it before merging. Sure, it adds critical functionality to TF, but this is textbook stuff, not exactly rocket science. At the end of the day, it doesn't change the existing behavior one bit, and it's a starting point for someone to make this a better logging system in the future if you ever decide to dedicate resources to this. This is one of many patches I already have to apply to our TensorFlow build, please make this one less.

P.S. We actually back this by a proper, high performance logger, as I think most users would. A logging hook like this is absolutely critical for any big library.

@kkimdev kkimdev added the kokoro:force-run Tests on submitted change label Oct 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 16, 2020
@evu
Copy link

evu commented Oct 16, 2020

I too am interested in seeing this make it into tensorflow. This has been needed for a while.

@avitebskiy
Copy link
Contributor Author

Alright, I see the CI now!

Sorry about the broken build: I was merging this from a patch to 1.15.4, must have made a mistake with that. Will fix today.

@kkimdev kkimdev added the kokoro:force-run Tests on submitted change label Oct 16, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 16, 2020
@kkimdev kkimdev added the kokoro:force-run Tests on submitted change label Nov 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 2, 2020
@avitebskiy
Copy link
Contributor Author

@kkimdev, sorry, I didn't notice your other comment, so I made two checkins instead of one. I think you'll need to re-kick the CI.

@avitebskiy
Copy link
Contributor Author

avitebskiy commented Nov 4, 2020

@kkimdev , all your comments have been addressed. It should all be there now...

@gbaned gbaned requested a review from kkimdev November 5, 2020 10:16
@gbaned gbaned added the awaiting review Pull request awaiting review label Nov 11, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 12, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 12, 2020
@gbaned gbaned removed the awaiting review Pull request awaiting review label Nov 12, 2020
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Nov 12, 2020
@gbaned gbaned removed the ready to pull PR ready for merge process label Nov 13, 2020
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Nov 13, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 13, 2020
@kkimdev kkimdev added the kokoro:force-run Tests on submitted change label Nov 18, 2020
@kkimdev
Copy link
Contributor

kkimdev commented Nov 18, 2020

@gbaned I don't see the corresponding CL internally, is this because it didn't pass all the Kokoro tests?

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Nov 18, 2020
@kkimdev
Copy link
Contributor

kkimdev commented Nov 19, 2020

Update: getting internal reviews now, we should be able to land after getting one more lgtm internally! :)

@copybara-service copybara-service bot merged commit 5f9f2d2 into tensorflow:master Nov 19, 2020
@kkimdev
Copy link
Contributor

kkimdev commented Nov 19, 2020

@avitebskiy I had to update quite a bit to be consistent with internal logger behavior and just removed linux tests that didn't work internally for some reason, but the functionality should be merged and available now. In case if you have a follow-up changes, please keep it coming. Thanks!

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:L CL Change Size: Large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants