FS#65953 - [tensorflow-opt-cuda] Not possible to use the library with C++17

Attached to Project: Community Packages
Opened by Alexander Lyashuk (crem) - Monday, 23 March 2020, 21:06 GMT
Last edited by Sven-Hendrik Haase (Svenstaro) - Wednesday, 16 June 2021, 06:08 GMT
Task Type Bug Report
Category Packages
Status Closed
Assigned To Sven-Hendrik Haase (Svenstaro)
Konstantin Gizdov (kgizdov)
Architecture All
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Description:
It's not possible to use libtensorflow_cc with C++17.
I'm not sure what is the best way out of it, but probably it worth considering just building it with C++17? (but that will break linkage with pre-C++17 C++ versions)
Another option would be patching absl/base/config.h as described below.

The reason for it that there are ABSL headers bundled with tensorflow, and tensorflow uses absl::string_view in their API.
absl::string_view:
- is a typedef to std::string_view if it detects C++17,
- has a custom implementation otherwise.

libtensorflow_cc.so is built with C++11 so it uses the custom implementation
However when those headers are read with C++17, functions start to pass std::string_view as parameter instead.

To work around it, it's possible to comment out the line:
#define ABSL_HAVE_STD_STRING_VIEW 1
in /usr/include/tensorflow/absl/base/config.h

Additional info:
* package version: 2.1.0-4 (it has always been like that though)

Steps to reproduce:

1. Type this code and save to test.cc:
=== cut ===
#include <tensorflow/cc/ops/standard_ops.h>

using namespace tensorflow;
using namespace tensorflow::ops;

int main() {
auto scope = Scope::NewRootScope();
auto input = Const(scope, Tensor(DataType::DT_INT32, {}));
auto filter = Const(scope, Tensor(DataType::DT_INT32, {}));
Conv2D(scope, input, filter, {1, 1, 1, 1}, "SAME");
}
=== cut ===
2. Compile it this way (c++11), it compiles fine:
$ g++ test.cc -I/usr/include/tensorflow /usr/lib/libtensorflow_cc.so /usr/lib/libtensorflow_framework.so

3. Compile it with C++17, get a linker error:
$ g++ test.cc -I/usr/include/tensorflow /usr/lib/libtensorflow_cc.so /usr/lib/libtensorflow_framework.so --std=c++17
/usr/bin/ld: /tmp/ccTk1GBe.o: in function `main':
test.cc:(.text+0x2b6): undefined reference to `tensorflow::ops::Conv2D::Conv2D(tensorflow::Scope const&, tensorflow::Input, tensorflow::Input, absl::Span<int const> const&, std::basic_string_view<char, std::char_traits<char> >)'
collect2: error: ld returned 1 exit status
This task depends upon

Closed by  Sven-Hendrik Haase (Svenstaro)
Wednesday, 16 June 2021, 06:08 GMT
Reason for closing:  Implemented
Comment by Sven-Hendrik Haase (Svenstaro) - Tuesday, 24 March 2020, 02:57 GMT
Sure, sounds good. Lemme compile it against c++17 and I'll throw it into [community-testing].
Comment by Sven-Hendrik Haase (Svenstaro) - Tuesday, 24 March 2020, 04:21 GMT
Actually we'll have to wait for the next rc or release because I can't find a patch which applies cleanly that fixes some absl stuff. Should only be a few days.
Comment by Konstantin Gizdov (kgizdov) - Tuesday, 24 March 2020, 11:22 GMT
It seems it's actually compiled against c++14 - https://github.com/tensorflow/tensorflow/blob/f270180a6caa8693f2b2888ac7e6b8e69c4feaa8/.bazelrc#L130-L132. Could we not just patch these lines and try and rebuild 2.1?
Comment by Alexander Lyashuk (crem) - Tuesday, 24 March 2020, 11:49 GMT
Note that building it against C++17 will make linking with it break for all people using C++14 and earlier for their project (i.e. it will break for all people for whom it now works, they will have to switch their projects to C++17).
It is in line of Archlinux'es philosophy of focusing on supporting mainly the latest versions of everything, but I'm sure there will be people unhappy with this change.

The hacky way of commenting out ABSL_HAVE_STD_STRING_VIEW may be more reasonable as then the library will continue to work for everyone. But if the project happens also to use ABSL itself, it will add up to the mess.. And the switch to C++17 will have to happen anyway one day, so why not now?..

Also note that while it's possible for C++17 users to work around the problem now by modifying /usr/include/tensorflow/absl/base/config.h, if the libtensorflow_cc will be compiled with C++17, projects using C++14 won't have the similar trick.

So I'm a bit in a doubt what would be the best way to proceed (building the library with C++17 vs patching the header file vs leaving it as it and waiting the change to happen upstream).
Comment by Konstantin Gizdov (kgizdov) - Tuesday, 24 March 2020, 12:50 GMT
@crem, TF 2.2.0 will ship with C++17 by default, so we're not doing anything out of the ordinary here - just skipping a few day, if even that...
Comment by Konstantin Gizdov (kgizdov) - Wednesday, 25 March 2020, 13:25 GMT
@crem, whoops, no, C++17 will become an officially supported option from 2.2, not the default. Anyway, we will build it with that option and hopefully it works.
Comment by Alexander Lyashuk (crem) - Sunday, 06 June 2021, 09:14 GMT
  • Field changed: Percent Complete (100% → 0%)
One year later (current version is 2.5.0-2), the libraries are still compiled with C++14, making them unusable if used from C++17 project, unless you comment out that ABSL_HAVE_STD_STRING_VIEW.

I don't know whether anything should be done, but I think it's still something to consider.
Comment by Sven-Hendrik Haase (Svenstaro) - Sunday, 06 June 2021, 10:43 GMT
I think we should continue to follow upstream here. From our research, upstream doesn't really target coexistence of these libs in C++14 and C++17 currently. I'm not really sure what can be done downstream in order to enable both use cases without drawbacks.
Comment by Alexander Lyashuk (crem) - Sunday, 06 June 2021, 10:46 GMT
Is having a patch that comments out `#define ABSL_HAVE_STD_STRING_VIEW 1` in /usr/include/tensorflow/absl/base/config.h an option?

Then it will both work in C++14 (where it's not defined anyway), and in C++17 (where it will stop trying to use std::string_view).
Comment by Sven-Hendrik Haase (Svenstaro) - Sunday, 06 June 2021, 21:14 GMT
Could you approach upstream about it and link the discussion here? I'd just like to see what they have to say about your suggestion or how they intend for this library to be used in this circumstance.
Comment by Alexander Lyashuk (crem) - Monday, 07 June 2021, 07:26 GMT
Upstream they don't have such problem because they don't distribute C++ libraries in binary form (they only do C, Python, Java and Go).
I also couldn't find any other repository that distributes tensorflow C++ library (and it's great that Arch does, as building it yourself is pretty painful).
If you mean https://github.com/FloopCZ/tensorflow_cc/ this repo as an upstream, will create an issue there and see what they say.

The problem occurs when the compiler setting during building of the library differs from the compiler settings when using it.

So:
1) (current state) Library built in C++14 mode and used from C++14 code -> works.
2) (current state) Library built in C++14 mode and used from C++17 code -> doesn't work.
3) Library built in C++17 mode and used from C++17 code -> works.
4) Library built in C++14 mode, has patch that disables "std::string_view" and used from C++17 code -> works, but ugly.
5) Library built in C++17 mode and used from C++14 code -> doesn't work, and not possible to have a patch.

So the two options of fixing it that I see is:
A) Providing two versions of the library, compiled with C++17 and C++14.
B) Applying the patch in .h file.

Also as it seems that I'm the only one who complains, it's not a widespread issue, so the current way of working around it on the user side is possible to:
C) (what I'm doing) edit that config.h manually after every package update.
D) Not use C++17 (not really possible in many projects, but I think that's what everyone is doing as they don't notice this problem)
Comment by Alexander Lyashuk (crem) - Monday, 07 June 2021, 07:42 GMT Comment by Sven-Hendrik Haase (Svenstaro) - Sunday, 13 June 2021, 11:47 GMT
I put a patched package into [community-testing]. Can you check whether that works for you?
Comment by Alexander Lyashuk (crem) - Monday, 14 June 2021, 06:30 GMT
Thanks for fixing it!
I've just checked it both in C++17 and C++11 and it works well.
Checked with tensorflow-opt-cuda and tensorflow packages (didn't check tensorflow-opt and tensorflow-cuda, but if they have similar fix, it should be good).

By the way, as a fix in https://github.com/FloopCZ/tensorflow_cc/issues/266, FloopCZ added a way to build tensorflow with C++17. However, if built for C++17, the library won't be usable from C++11 with no easy workaround.
So unless the packages are split into two separate C++17 and C++11 builds, I think the current fix is better.

Loading...