google / yggdrasil-decision-forests

A library to train, evaluate, interpret, and productionize decision forest models such as Random Forest and Gradient Boosted Decision Trees.
https://ydf.readthedocs.io/
Apache License 2.0
497 stars 53 forks source link

Use of designated initializers requires at least '/std:c++20' #33

Closed JoseAF closed 11 months ago

JoseAF commented 1 year ago

Hi

I'm currently trying to build YDF master locally and am running into an unexpected issue:

"error C7555: use of designated initializers requires at least '/std:c++20'"

I say unexpected because, according to what I've read, designated initializers are a C++20 feature. However, YDF seems to allow only for C++14 or C++17 config build options (as found in .bazelrc, in test_bazel.bat, and in the YDF installation page). I can try and modify the code to make it compatible with C++17 if this is what's required. However, I was wondering whether adding to .bazelrc:

build:windows_cpp20 --cxxopt=/std:c++20 build:windows_cpp20 --host_cxxopt=/std:c++20 build:windows_cpp20 --config=windows

and trying to build using C++20 is an acceptable (as in supported) approach or I will run into other potentially more serious problems (I have briefly tried this and come across a constinit error, but if this approach is ok I might as well spend the time trying to fix this error instead of the one above!).

Thanks.

rstz commented 1 year ago

Hi Jose, can you tell us more about your build configuration (compiler, OS)? for the supported configurations, everything looks green from our side, but happy to take a closer look.

Also: If it's just this error, it might be easy to fix - can you find out in which file it occurs?

JoseAF commented 1 year ago

Hi Richard

Thanks for the reply.

OS: Windows 10 Enterprise 64-bit set BAZEL_VC=C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC (full version: 14.29.30133) Python 3.10.1 Bazel 5.1.1 (just because that's the one mentioned in the installation page, but I have also tried with 6.0.0)

Running test_bazel.bat as follows:

set BAZEL=bazel-5.1.1-windows-x86_64.exe set BAZEL_VC=C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC %BAZEL% version

set TF_SUPPORT=0

IF %TF_SUPPORT%==0 (

copy /Y WORKSPACE_NO_TF WORKSPACE

set FLAGS=--config=windows_cpp17 --config=windows_avx2 --jobs 1 %BAZEL% build %FLAGS% //yggdrasil_decision_forests/cli:all || goto :error [...]

causes the designated initializers error. The reason I added --jobs 1 is that with default 12 threads there were some timing issues. Previously I managed to build 0.2.3 using the above configuration and package (C++17) with only a few code changes. The error so far has only happened at:

yggdrasil_decision_forests/serving/decision_forest/quick_scorer_extended.cc(890)

I have VS 2022 so I can also use C++20. I have tried alternatively adding C++20 to .bazelrc as in my message above and using instead:

set BAZEL_VC=C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC set BAZEL_VC_FULL_VERSION=14.30.30705

and:

set FLAGS=--config=windows_cpp20 --config=windows_avx2 --jobs 1 %BAZEL% build %FLAGS% //yggdrasil_decision_forests/cli:all || goto :error

But this causes the consinit error.

Let me know if you need anything else.

JoseAF commented 1 year ago

Unfortunately, this same error appears in several places. So far I have seen it also at:

yggdrasil_decision_forests/serving/decision_forest/decision_forest.cc(734) external/com_google_absl/absl/crc/internal/crc_memcpy_x86_64.cc(391, 403, 409, 413)

For the first one (together with the quick_scorer_extended one) I'm making this change:

I'm unsure about how to fix the absl one. I'll keep looking into it...

achoum commented 1 year ago

Hi Jose,

Sorry about the hurdle.

It seems some c++20 only features were added to our code base. I'll remove them by the end of the week.

In the meantime, you can enabled c++20 in visual studio by adding the following line to the .bazelrc file:

build:windows_cpp20   --cxxopt=/std:c++20

and replace the flag --config=windows_cpp17 with --config=windows_cpp20 in your build command line (only adding the flag will do nothing).

Note that only the most recent version of visual studio have support for c++.

Cheers, Math

JoseAF commented 1 year ago

Hi Mathieu

Thanks for the reply above.

Unfortunately, I have already tried your suggestion by changing the .bazelrc file and building using:

set BAZEL_VC=C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\VC set BAZEL_VC_FULL_VERSION=14.30.30705 set BAZEL=bazel-5.1.1-windows-x86_64.exe %BAZEL% version

set TF_SUPPORT=0

IF %TF_SUPPORT%==0 (

copy /Y WORKSPACE_NO_TF WORKSPACE

set FLAGS=--config=windows_cpp20 --config=windows_avx2 --jobs 1 %BAZEL% build %FLAGS% //yggdrasil_decision_forests/cli:all || goto :error [...]

But I get different errors. One of them:

yggdrasil_decision_forests/learner/gradient_boosted_trees/loss/loss_imp_mean_square_error.cc(74): error C2039: 'accumulate': is not a member of 'std'

is easily fixed by adding the correct include in that file. However, there is another one on the protobuf folder which I'm unsure about how to fix:

external/com_google_protobuf/src/google/protobuf/generated_message_util.cc(81): error C2127: 'fixed_address_empty_string_test': illegal initialization of 'constinit' entity with a non-constant expression

The error is probably related to the use of PROTOBUF_CONSTINIT in that file. Have you tried building with C++20 and do you see a similar error? Any ideas? I see people in github.com/protocolbuffers suggesting hacks that I can try...

JoseAF commented 1 year ago

With a few hacks I managed to build using C++20 - thanks for your replies!

rstz commented 1 year ago

Hi Jose, That's great to hear! Would you mind sharing the hacks you had to use? Others might be facing the same issues and we can use them to see if we can integrate a C++20 windows build in our list of supported configurations.

JoseAF commented 1 year ago

Of course...

  1. ERROR: yggdrasil_decision_forests/learner/gradient_boosted_trees/loss/loss_imp_mean_square_error.cc(74): error C2039: 'accumulate': is not a member of 'std' => #include numeric lib
  2. ERROR: external/com_google_protobuf/src/google/protobuf/generated_message_util.cc(81): error C2127: 'fixed_address_empty_string_test': illegal initialization of 'constinit' entity with a non-constant expression => at google\protobuf\port_def.inc, change #define PROTOBUF_CONSTINIT constinit to #define PROTOBUF_CONSTINIT (this definitely is a hack, as the compiler complaint may be correct and we are just hiding it)
  3. At one point some files were not being generated/found by the compiler - this turned out to be an issue with long paths on my machine. I made 2 changes here that make things work: => enable long path behavior set the registry key at HKLM\SYSTEM\CurrentControlSet\Control\FileSystem LongPathsEnabled (Type: REG_DWORD) to true (or 0x00000001) => set flag --output_user_root=D:/ in the bazel build call
  4. At one point there were files not generated on time for the compiler to find them - this turned out to be an issue with using 12 threads on my machine and lack of synchronization between tasks. To fix this: => add flag --jobs 1 to bazel build call (it's slower but it works)

After these changes building completed successfully.

Hope this helps.

rstz commented 1 year ago

Thank you very much!

achoum commented 1 year ago

Hi Jose,

Thanks for the pointers. We added them (except for jobs 1)to YDF. Thanks :).

JoseAF commented 1 year ago

No problem - glad I could help :)