[CMake] Add dataframe option #3957

stwunsch · 2019-06-13T09:29:33Z

Because RDataFrame is not available on 32bit, we have to disable the
experimental parts of TMVA which are dependent on it as well.

phsft-bot · 2019-06-13T09:30:03Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

amadio · 2019-06-13T11:30:02Z

It may be a good idea to make dataframe optional and disable (or require it to be off) on 32bits machines. The proliferation of these things not availble on 32bits will only get more and more complicated. We already have a few other places where this needs to be checked. It's better to have a dataframe option than keeping in sync all the 32bit checks everywhere (i.e. which tests to run, which tutorials to veto, which optional components that depend on dataframe and cannot be build as a consequence, etc).

@dpiparo @bluehood What are your thoughts?

stwunsch · 2019-06-13T11:34:59Z

@Axel-Naumann correct me if I'm wrong. We do not plan to make dataframe optional since we expect gcc/clang to figure out their discrepancies and resolve the 32bit issue for RDF?

However, from the technical side, I agree with @amadio having a dataframe flag would make stuff much easier and for sure more consistent on the long run.

eguiraud · 2019-06-13T11:38:08Z

@dpiparo I thought rdf was reenabled on 32 bits at some point, do I remember incorrectly?

@amadio I don't have anything against a dataframe option that is usually on and is turned off on 32 bit machines.

If I had time I'd check that we actually still need to disable rdf on 32 bits, but the failure was not easy to reproduce.

phsft-bot · 2019-06-13T12:15:27Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

stwunsch · 2019-06-13T12:16:51Z

@amadio I put a commit on top with a dataframe option. Better like this?

stwunsch · 2019-06-13T12:20:03Z

Oh, well, I forgot some places.

phsft-bot · 2019-06-13T12:22:19Z

Build failed on ROOT-performance-centos7-multicore/default.
See console output.

phsft-bot · 2019-06-13T12:27:09Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

amadio

Please rebase and put all changes in a single commit, to avoid having broken commits in between. Thanks.

cmake/modules/RootBuildOptions.cmake

tmva/tmva/CMakeLists.txt

phsft-bot · 2019-06-13T13:40:12Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

phsft-bot · 2019-06-13T13:46:57Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

phsft-bot · 2019-06-13T14:31:48Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

phsft-bot · 2019-06-14T07:36:21Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

stwunsch · 2019-06-14T07:48:52Z

@amadio @Axel-Naumann ok, that is getting out of hands 😅 we can have the very first commit to just fix the issue OR add the dataframe option to the build system, which is really a lot of changes.

Is just fixing the 32bit issue it worth?

Axel-Naumann · 2019-06-14T09:38:30Z

We should discuss that in person, given that this is easily possible :-) Can we do that on Monday?

amadio · 2019-06-14T10:37:47Z

As I mentioned on mattermost, the proliferation of bug prone if conditions is also getting out of hand and will be a nightmare to maintain. I think that we should reconsider this way of disabling RDataFrame and just re-enable it (and other deps) everywhere.

phsft-bot · 2019-06-14T12:19:21Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

phsft-bot · 2019-06-14T13:04:42Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

stwunsch · 2019-06-14T13:07:06Z

This is connected to the roottest PR here.

stwunsch · 2019-06-14T13:07:49Z

But hey, it works now :)

tmva/tmva/CMakeLists.txt

amadio · 2019-06-14T13:19:58Z

Much nicer! I think with a couple of tweaks this will be ready to merge.

phsft-bot · 2019-06-14T13:24:12Z

Build failed on ROOT-performance-centos7-multicore/default.
See console output.

Warnings:

CMake Warning at /data/sftnight/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:830 (message):

Failing tests:

projectroot.runtutorials.tutorial_fit_graph2dfit

The new cmake option `dataframe` enables building the ROOTDataFrame library and takes care of implication on the build system. The option is enabled by default unless a 32bit system is detected.

phsft-bot · 2019-06-14T14:25:15Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

stwunsch · 2019-06-14T15:45:36Z

@phsft-bot build

phsft-bot · 2019-06-14T15:45:43Z

Starting build on ROOT-performance-centos7-multicore/default, ROOT-fedora27/noimt, ROOT-fedora29/python3, ROOT-ubuntu16/rtcxxmod, mac1014/cxx17, windows10/default
How to customize builds

amadio

We might need "@dataframe@" == 1 in root-config, but otherwise, good to go, it seems.

amadio · 2019-06-17T13:13:15Z

config/root-config.in

          -lMultiProc"

-if [ "@UNIX@" != "1" ] || [ "@CMAKE_SIZEOF_VOID_P@" != "4" ]; then
+if [ "@dataframe@" == "ON" ]; then


We can use the suggestion below for now, but we should have a generic solution if more things are allowed to be OFF in the future:

Suggested change

if [ "@dataframe@" == "ON" ]; then

if echo "${features}" | grep -q "dataframe"; then

Meh, Stefan didn't enable pushing by repo owners. I'll merge (after amending) by hand.

Axel-Naumann · 2019-06-17T20:28:30Z

Merged as 31292b9 by hand, with @amadio 's suggested change amended.

stwunsch requested a review from amadio June 13, 2019 09:29

stwunsch self-assigned this Jun 13, 2019

This comment has been minimized.

Sign in to view

root-project deleted a comment from phsft-bot Jun 13, 2019

This comment has been minimized.

Sign in to view

amadio requested changes Jun 13, 2019

View reviewed changes

cmake/modules/RootBuildOptions.cmake Outdated Show resolved Hide resolved

tmva/tmva/CMakeLists.txt Show resolved Hide resolved

This comment has been minimized.

Sign in to view

stwunsch requested a review from Axel-Naumann as a code owner June 14, 2019 12:19

stwunsch force-pushed the tmva_32bit branch from 0fb0acf to c0db0c4 Compare June 14, 2019 13:04

stwunsch changed the title ~~[Experimental TMVA] Disable experimental TMVA on 32bit~~ [CMake] Add dataframe option Jun 14, 2019

stwunsch mentioned this pull request Jun 14, 2019

[RDF] Build test only if library has been built root-project/roottest#339

Merged

amadio reviewed Jun 14, 2019

View reviewed changes

tmva/tmva/CMakeLists.txt Outdated Show resolved Hide resolved

[CMake] Add dataframe option

b735895

The new cmake option `dataframe` enables building the ROOTDataFrame library and takes care of implication on the build system. The option is enabled by default unless a 32bit system is detected.

stwunsch force-pushed the tmva_32bit branch from c0db0c4 to b735895 Compare June 14, 2019 14:25

amadio approved these changes Jun 15, 2019

View reviewed changes

amadio reviewed Jun 17, 2019

View reviewed changes

Axel-Naumann closed this Jun 17, 2019

	if [ "@dataframe@" == "ON" ]; then
	if echo "${features}" \| grep -q "dataframe"; then

[CMake] Add dataframe option #3957

[CMake] Add dataframe option #3957

Uh oh!

Conversation

stwunsch commented Jun 13, 2019

Uh oh!

phsft-bot commented Jun 13, 2019

Uh oh!

amadio commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stwunsch commented Jun 13, 2019

Uh oh!

eguiraud commented Jun 13, 2019

Uh oh!

phsft-bot commented Jun 13, 2019

Uh oh!

This comment has been minimized.

stwunsch commented Jun 13, 2019

Uh oh!

stwunsch commented Jun 13, 2019

Uh oh!

phsft-bot commented Jun 13, 2019

Uh oh!

phsft-bot commented Jun 13, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

amadio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phsft-bot commented Jun 13, 2019

Uh oh!

phsft-bot commented Jun 13, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

phsft-bot commented Jun 13, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

phsft-bot commented Jun 14, 2019

Uh oh!

stwunsch commented Jun 14, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Axel-Naumann commented Jun 14, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

amadio commented Jun 14, 2019

Uh oh!

This comment has been minimized.

phsft-bot commented Jun 14, 2019

Uh oh!

phsft-bot commented Jun 14, 2019

Uh oh!

stwunsch commented Jun 14, 2019

Uh oh!

stwunsch commented Jun 14, 2019

Uh oh!

Uh oh!

amadio commented Jun 14, 2019

Uh oh!

phsft-bot commented Jun 14, 2019

Warnings:

Failing tests:

Uh oh!

phsft-bot commented Jun 14, 2019

Uh oh!

stwunsch commented Jun 14, 2019

amadio commented Jun 13, 2019 •

edited

Loading