pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.45k stars 2.09k forks source link

alias template error with Intel 2016.0.3 compilers #1121

Closed loriab closed 6 years ago

loriab commented 6 years ago

Issue description

In trying to update some projects from 2.0.0 to 2.2.1, I've found that they consistently won't compile with the icpc (ICC) 16.0.3 2016041 line of compilers. Fine with GCC 5.2 and Intel icpc (ICC) 17.0.4 20170411. An error is below:

In file included from /home/psilocaluser/gits/cmake_example/pybind11/include/pybind11/attr.h(13),
                 from /home/psilocaluser/gits/cmake_example/pybind11/include/pybind11/pybind11.h(43),
                 from /home/psilocaluser/gits/cmake_example/src/main.cpp(1):
/home/psilocaluser/gits/cmake_example/pybind11/include/pybind11/cast.h(837): error: alias template "pybind11::detail::type_caster_base<type>::cast_op_type" may not be used in the type-id of the alias-declaration
      template <typename T> using cast_op_type = cast_op_type<T>;
                                                 ^

In file included from /home/psilocaluser/gits/cmake_example/pybind11/include/pybind11/attr.h(13),
                 from /home/psilocaluser/gits/cmake_example/pybind11/include/pybind11/pybind11.h(43),
                 from /home/psilocaluser/gits/cmake_example/src/main.cpp(1):
/home/psilocaluser/gits/cmake_example/pybind11/include/pybind11/cast.h(1814): warning #3373: nonstandard use of "auto" to both deduce the type from an initializer and to announce a trailing return type
      static constexpr auto args_pos = constexpr_first<argument_is_args, Args...>() - (int) sizeof...(Args),
                       ^

compilation aborted for /home/psilocaluser/gits/cmake_example/src/main.cpp (code 2)
gmake[2]: *** [CMakeFiles/cmake_example.dir/src/main.cpp.o] Error 2

Reproducible example code

The error can be seen with @dean0x7d's sample cmake+pybind11 repo, https://github.com/pybind/cmake_example (admittedly pinned at 2.2.0). When forced to switch compiler families as below, it gives the error above.

>>> icpc --version
icpc (ICC) 16.0.3 20160415
Copyright (C) 1985-2016 Intel Corporation.  All rights reserved.
>>> mkdir build/temp.linux-x86_64-3.5
>>> cd build/temp.linux-x86_64-3.5 &&  cmake /path/to/cmake_example -DCMAKE_LIBRARY_OUTPUT_DIRECTORY=/path/to/cmake_example/build/lib.linux-x86_64-3.5 -DPYTHON_EXECUTABLE=/miniconda/envs/idp35p4/bin/python -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=icc -DCMAKE_CXX_COMPILER=icpc
>>> cmake --build . --config Release -- -j2

I'm sorry to not further narrow the error source. I'm glad to try out any changes since this particular compiler flavor may not be widespread.

klaw9 commented 6 years ago

This issue also affects nvcc (cuda 8). For now I made it work by adding a namespace qualifier to the type-id of the alias statement.

template <typename T> using cast_op_type = detail::cast_op_type<T>;

loriab commented 6 years ago

Thanks, @klaw9, I confirm that fixes the stated problem.

Next problem is below, if anyone's curious.

/home/psilocaluser/gits/hrw-matt/objdir_aggh2016/stage/usr/local/psi4/include/pybind11/detail/descr.h(59): error: expression must have a constant value
      static constexpr auto digits = descr<sizeof...(Digits)>(('0' + Digits)...);
henryiii commented 6 years ago

This error is a regression and makes pybind11 master and 2.x unusable with CUDA. Adding the word detail:: fixes the issue

jagerman commented 6 years ago

The original issue is fixed by 32ef69a which I just pushed. I suspect @loriab's issue is going to be more annoying to track down and fix (I think it was probably caused initially by #934).

henryiii commented 6 years ago

Did you push it to the 2.2 branch as well?

jagerman commented 6 years ago

No, but I'll set the milestone to remind us to include it in the 2.2.2 release.

jagerman commented 6 years ago

@loriab - is that issue under master, or the 2.2 release? I don't think the #934 changes made it into 2.2.

jagerman commented 6 years ago

(I only have ICC 17 available to me, where both of these compile okay).

henryiii commented 6 years ago

Okay, I'd like to have it in soon, because I've transitioned to 2.2.1 using a normal compiler, but it's now broken under CUDA, so switching the checkout to the 2.2 branch would be a temporary fix. I closed my PR since I assume it's best to cherry-pick the same commit.

By the way, just for general information, you can grab NVIDIA's docker containers and build with CUDA there in normal Docker, just (obviously) can't run.

(CUDA seems to be happy with the Digits line)

loriab commented 6 years ago

Thanks for the help. I just rebuilt with pybind11 master. Confirm that the original cast_op_type fixed for me in master and the next issue Digits remains.

jagerman commented 6 years ago

Try playing around with that line to see if you can make ICC 16 happy (as I said, 17 is fine with it).

Does changing ('0' + Digits)... to ('0' + static_cast<char>(Digits))... help? (I'm basically just guessing right now).

jagerman commented 6 years ago

Another possible solution is replace ('0' + Digits)... with digit_to_str(Digits)... and add:

inline constexpr char digit_to_str(size_t digit) { return static_cast<char>('0' + digit); }

just before the templated struct int_to_str.

loriab commented 6 years ago

Regrettably, icpc hates all your kind suggestions, as well as those variations my limited c++ imagination can devise. I'm giving up for the moment, but I'm glad to try more.

And while we like 2016 for some software, I'll accept it if you want to give up on 2016 support.

loriab commented 6 years ago

@bennybp, who actually understands templating in C++, took a look at this with 2016 icpc and, though not looking up any particular known error, concluded that it couldn't be fixed through ready code modifications. I've forced cmake_example to use pybind11 master and get the same digits error.

So, I suggest pybind11 officially drop 2016 support, codify it, and close this issue.

I can't pinpoint the fix point any nearer than >2016.3, <=2017.4, but if you want a PR with blanket Intel 15 --> 17 requirements, I can probably do that. Otherwise, feel free to close.

jagerman commented 6 years ago

Personally I stick to all open source components for my projects as much as possible, which means I'm not at all familiar with the Intel compiler ecosystem: Is a 2017 minimum requirement here reasonable, or do ICC users tend to stick with older releases (e.g. Intel 15)?

It might be feasible to reintroduce the older implementation (perhaps stuffing it out into something like detail/icc.h) if upping the requirement would cause difficulties.

loriab commented 6 years ago

Upping the requirement is no hardship to my project (get Linux compilers from Intel's open-source developers program) and our users get pre-compiled or use gcc/clang. For academic labs that buy Intel compilers, I expect many do keep them around for a few years until compile failures prompt an upgrade. But the overlap of code dev, modern C++, and old compilers is probably low.

I'm not keen to download 4gb packages for the ~5 releases between 2016.3 and 2017.4, so I'm willing to call 2017 the cutoff.

loriab commented 6 years ago

Update, I know someone who'll try out 2017.0.2 tomorrow, so that'll be a better approximation to an outright 2017 cutoff.

jturney commented 6 years ago

I can confirm that it works with Intel 2017.0.2. No compile/link issues and test cases pass with our project.

wjakob commented 6 years ago

Can somebody comment on the status of this discussion? My understanding is as follows:

loriab commented 6 years ago

Done! See #1363.

proteneer commented 5 years ago

I'm still seeing template using cast_op_type = cast_op_type; on pybind 2.2.4 - did this go into master?

loriab commented 5 years ago

Yes, this went into release v2.2.3. Since then, there's at least #1649 where cast_op_type pops up.

proteneer commented 5 years ago

So why is that upon pip installing pybind (2.2.4) I'm still seeing the lack of detail::cast_op_type?

loriab commented 5 years ago

special header, perhaps?

proteneer commented 5 years ago

I'm confused. On master it's fixed:

https://github.com/pybind/pybind11/blob/master/include/pybind11/cast.h#L881

Clearly shows the detail:: prefix.

but none of the releases 2.2.3, 2.2.4 have that line included:

https://github.com/pybind/pybind11/blob/v2.2/include/pybind11/cast.h#L838

loriab commented 5 years ago

ah, the PR assoc. with this issue was only restricting Intel versions: https://github.com/pybind/pybind11/pull/1363/files . Some other PR (presumably unreleased from master) must have made the change you're looking at.

bstaletic commented 5 years ago

Perhaps it wasn't a PR at all: https://github.com/pybind/pybind11/commit/32ef69acde3b3345411fec40317f34cba423f075

proteneer commented 5 years ago

This isn't a big deal - since I've already refactored out the cuda specific out to a separate module. It was surprising to see the compiler failing despite it being in master. Any idea why this change wasn't cherry picked into 2.2?