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

Fix for #12537 #15173

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Fix for #12537 #15173

merged 1 commit into from
Dec 14, 2017

Conversation

jzju
Copy link
Contributor

@jzju jzju commented Dec 7, 2017

I have built the fix on x86 linux no gpu and it's working.

There are some android specific code in logging that I ignored.
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/platform/default/logging.cc#L35-L75

@tensorflow-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@jzju
Copy link
Contributor Author

jzju commented Dec 7, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 7, 2017
@caisq caisq requested a review from vrv December 7, 2017 15:41
@caisq caisq self-assigned this Dec 7, 2017
@caisq caisq added the awaiting review Pull request awaiting review label Dec 7, 2017
@vrv vrv added the API review API Review label Dec 7, 2017
@vrv
Copy link

vrv commented Dec 7, 2017

I am going to defer to API review first, and possibly someone else who works on TF core team should be a real reviewer.

@vrv vrv requested review from asimshankar and removed request for vrv December 7, 2017 17:51
@vrv
Copy link

vrv commented Dec 7, 2017

The concerns I have are; API bloat, and internally our logging infrastructure under the LOG macros do a lot of extra helpful things relevant to Google-infrastructure that std:cerr would lose. I don't know how to rectify these things easily, even though I like the spirit of your change.

Perhaps this is a candidate for a custom op; it's not clear that we can make "Print" work generically for all environments in the world, and trying will just make Print more complicated than it needs to be.

@jzju
Copy link
Contributor Author

jzju commented Dec 7, 2017

Another solution would be to have an environment variable like TF_CPP_MIN_LOG_LEVEL that modifies the underlying fprintf

// TODO(jeff,sanjay): Replace this with something that logs through the env.
fprintf(stderr, "%s.%06d: %c %s:%d] %s\n", time_buffer, micros_remainder,
"IWEF"[severity_], fname_, line_, str().c_str());
}

Also is it a bug or feature that TF_CPP_MIN_LOG_LEVEL can disable tf.Print?

@vrv
Copy link

vrv commented Dec 7, 2017

@asimshankar and I chatted offline, there may be a simpler solution to this, but he'll have to pass it by the API review folks first.

It's a feature to allow users to completely disable all logging via LOG(INFO), and perhaps a bug that tf.Print uses LOG(INFO) in the OSS build.

@asimshankar
Copy link
Contributor

@Johanju : Thanks for the contribution. We feel that instead of adding a new API, it might be okay to just change the implementation of the Print kernel to always use std::cerr instead of LOG(INFO).

Thoughts?

@asimshankar asimshankar added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Dec 12, 2017
@jzju
Copy link
Contributor Author

jzju commented Dec 12, 2017

@asimshankar This is my first PR so I will just follow your lead, It won't be any problem the Google-infrastructure angel that @vrv talked about? What do you think is better between std::cerr and std::cout in this case?

@asimshankar
Copy link
Contributor

@Johanju : I think it is fine, @vrv 's concerns are valid, but I believe we have a workaround to integrate better with our internal logging system.

For now I'd stick with std::cerr, since the current behavior with LOG(INFO) is to write to stderr as well (IIUC).

Thanks very much!

Copy link
Contributor

@asimshankar asimshankar left a comment

Choose a reason for hiding this comment

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

Thanks!

@asimshankar asimshankar added awaiting testing (then merge) kokoro:force-run Tests on submitted change and removed API review API Review stat:awaiting response Status - Awaiting response from author labels Dec 13, 2017
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants