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

Remove C++ content from tensorflow/c/c_api_macros.h #52313

Merged
merged 1 commit into from
Oct 22, 2021

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Oct 9, 2021

This PR tries to remove C++ conent from tensorflow/c/c_api_macros.h
so that it is possible to include this file from an external plugin
without depending on tensorflow's C++ headers (tensorflow/core/platform/status.h).

The reason for this move is that for external plugins (e.g., file systems plugins)
that utilize C API, it cannot include status.h as otherwise the whole tensorflow's
C++ library will be a dependency. This defeat the purpose of C APi modular plugin.

This PR place C++ content to tensorflow/core/platform/status.h instead so that
everything will still be combined both for C external plugins and C++ plugins.

(Note the C++ inclusion was added in PR #51647.)

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Oct 9, 2021
@google-cla google-cla bot added the cla: yes label Oct 9, 2021
@yongtang yongtang marked this pull request as ready for review October 10, 2021 01:09
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Oct 10, 2021
@yongtang
Copy link
Member Author

@mihaimaruseac At the moment upgrade of tensorflow-io's dependency to TF 2.7.0rc0 is not working due to the C++ inclusion. For that it might make sense to include this PR to 2.7 if there is still enough time.

@yongtang yongtang added the kokoro:force-run Tests on submitted change label Oct 10, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 10, 2021
@yongtang yongtang added the kokoro:force-run Tests on submitted change label Oct 11, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 11, 2021
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Oct 11, 2021
@mihaimaruseac
Copy link
Collaborator

Hmm, can you post what the error message is?

These lines got added recently via #51647 (manual import + fix) as Intel needed this for the 2.7 release.

CC @penpornk

@penpornk
Copy link
Member

@yongtang The C++ part is wrapped with #ifdef __cplusplus so I think it should be fine with C compilers. Is the error message something like this?

cc: @jbaiocchi

@yongtang
Copy link
Member Author

yongtang commented Oct 13, 2021

Thanks @mihaimaruseac @penpornk.

The issue is because of the following:

  1. While C++ part is wrapped within #ifdef __cplusplus, c_api_macros.h itself is a header file and can be included by a C++ source file or C source file.
  2. However, if a C++ source file include this header, it will invoke the inclusion of "status.h", which, will include almost lots of C++ header files under tensorflow (not just for C headers under tensorflow/c/:

So the issue is not really about C++ itself, but the include of status.h that expands the inclusion of too many header files (and defeat the purpose of a small limited set of C header files):

Below are the included headers within "status.h":

#include "absl/types/optional.h"
#include "tensorflow/core/platform/logging.h"
#include "tensorflow/core/platform/macros.h"
#include "tensorflow/core/platform/stack_frame.h"
#include "tensorflow/core/platform/stringpiece.h"
#include "tensorflow/core/platform/types.h"
#include "tensorflow/core/protobuf/error_codes.pb.h"

Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

@yongtang Thank you for the explanation! I agree with refactoring them out. However, I don't think platform/status.h is the right place.

Would you mind creating a new file //tensorflow/c/c_api_macros_internal.h? (This will also require updating the dependencies to c_api_macros.h for //tensorflow/c/experimental/stream_executor and //tensorflow/c/experimental/pluggable_profiler.) -- Or I can also do it. Let me know.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Oct 15, 2021
This PR tries to remove C++ conent from tensorflow/c/c_api_macros.h
so that it is possible to include this file from an external plugin
without depending on tensorflow's C++ headers (tensorflow/core/platform/status.h).

The reason for this move is that for external plugins (e.g., file systems plugins)
that utilize C API, it cannot include status.h as otherwise the whole tensorflow's
C++ library will be a dependency. This defeat the purpose of C APi modular plugin.

This PR place C++ content to tensorflow/core/platform/status.h instead so that
everything will still be combined both for C external plugins and C++ plugins.

(Note the C++ inclusion was added in PR 51647.)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Update to create c_api_macros_internal.h based on review feedback

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang yongtang force-pushed the c_api_macros-fix branch 2 times, most recently from 5bb3c7b to 3cd9b31 Compare October 15, 2021 14:35
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Oct 15, 2021
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Oct 15, 2021
@penpornk penpornk removed the awaiting review Pull request awaiting review label Oct 15, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 15, 2021
@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Oct 18, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 18, 2021
@yongtang
Copy link
Member Author

@mihaimaruseac @penpornk wondering if there is any update on this PR?

It would be great if this PR can be part of the 2.7 release as external plugin will be impacted.

@yongtang yongtang added the kokoro:force-run Tests on submitted change label Oct 22, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Oct 22, 2021
@copybara-service copybara-service bot merged commit 4f996b4 into tensorflow:master Oct 22, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Oct 22, 2021
@yongtang yongtang deleted the c_api_macros-fix branch October 22, 2021 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes size:M CL Change Size: Medium
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

5 participants