root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.63k stars 1.26k forks source link

-Dtmva-sofie=OFF does not switch off sofie. #13055

Closed ktf closed 4 months ago

ktf commented 1 year ago

Check duplicate issues.

Describe the bug

If I pass -Dtmva-sofie=OFF to cmake, ROOT still builds it:

→ ls -la lib/lib*Sofie*
-rwxr-xr-x  1 ktf  staff  411192 Jun 21 10:17 lib/libROOTTMVASofie.6.28.03.so
lrwxr-xr-x  1 ktf  staff      27 Jun 21 10:17 lib/libROOTTMVASofie.6.28.so -> libROOTTMVASofie.6.28.03.so
lrwxr-xr-x  1 ktf  staff      24 Jun 21 10:17 lib/libROOTTMVASofie.so -> libROOTTMVASofie.6.28.so
CMAKE_INVOKE:STRING=/opt/homebrew/Cellar/cmake/3.25.2/bin/cmake  -DARROW_HOME="/Users/ktf/src/sw/osx_arm64/arrow/v11.0.0-alice1-local2" -DCMAKE_BUILD_TYPE="RELWITHDEBINFO" -DCMAKE_CXX_COMPILER="clang++" -DCMAKE_CXX_STANDARD="17" -DCMAKE_C_COMPILER="clang" -DCMAKE_INSTALL_PREFIX="/Users/ktf/src/sw/osx_arm64/ROOT/alice-v6-28-00-local5" -DCMAKE_LINKER="clang" -DCMAKE_PREFIX_PATH=";/opt/homebrew/opt/openssl@1.1;/opt/homebrew/opt/gsl;/Users/ktf/src/sw/osx_arm64/AliEn-Runtime/v2-19-le-local3;;/Users/ktf/src/sw/osx_arm64/Python-modules/1.0-local5;/opt/homebrew/opt/libpng;/Users/ktf/src/sw/osx_arm64/lzma/v5.2.3-local1;/Users/ktf/src/sw/osx_arm64/protobuf/v21.9-local1" -DGSL_DIR="/opt/homebrew/opt/gsl" -DMONALISA_DIR="/Users/ktf/src/sw/osx_arm64/AliEn-Runtime/v2-19-le-local3" -DOPENSSL_INCLUDE_DIR="/opt/homebrew/opt/openssl@1.1/include" -DOPENSSL_ROOT="/opt/homebrew/opt/openssl@1.1" -DPNG_INCLUDE_DIRS="/opt/homebrew/opt/libpng/include" -DPNG_LIBRARY="/opt/homebrew/opt/libpng/lib/libpng.dylib" -DPYTHON_PREFER_VERSION="3" -DXROOTD_ROOT_DIR="/Users/ktf/src/sw/osx_arm64/XRootD/v5.5.3-local7" -Dalien="OFF" -Darrow="ON" -Dbuiltin_afterimage="ON" -Dbuiltin_davix="OFF" -Dbuiltin_freetype="OFF" -Dbuiltin_pcre="ON" -Dbuiltin_vdt="ON" -Dcocoa="ON" -Ddavix="OFF" -Dfortran="OFF" -Dfreetype="ON" -Dgviz="OFF" -Dhttp="OFF" -Dminuit2="ON" -Dmonalisa="ON" -Dmysql="OFF" -Dpcre="OFF" -Dpgsql="OFF" -Dpyroot="ON" -Dpythia6_nolink="ON" -Droofit="ON" -Droot7="OFF" -Dshadowpw="OFF" -Dsoversion="ON" -Dspectrum="OFF" -Dsqlite="OFF" -Dtmva-sofie="OFF" -Dvdt="OFF" -Dwebgui="OFF" -Dxrootd="ON" /Users/ktf/src/sw/SOURCES/ROOT/alice-v6-28-00/0
//Build TMVA with support for sofie - fast inference code generation
tmva-sofie:BOOL=OFF
tmva-sofie-CACHED:STRING=OFF

What is the expected behaviour?

No sofie related libraries if -Dtmva-sofie=OFF is provided.

How to reproduce?

Build with -Dtmva-sofie=OFF on macOS.

ROOT version

6.28.04

How did you install ROOT?

https://github.com/alisw/alidist/blob/master/root.sh

Which operating system are you using?

macOS

Additional context

No response

bellenot commented 1 year ago

So removing the sofie build breaks many dependencies. I'll check with @lmoneta how to fix that correctly once he's back.

ktf commented 1 year ago

Thank you for the update. Could you give an ETA for having this fixed?

bellenot commented 1 year ago

Thank you for the update. Could you give an ETA for having this fixed?

I'll let @lmoneta comment on this issue which is apparently not a real issue...

lmoneta commented 1 year ago

-Dtmva-sofie=Off switch off the sofie parser library that depends on protobuf. There is no reason to remove the sofie library, that is a small one, so this is enabled always by default. If you really want to switch off sofie, you can do this by doing -Dtmva=Off

ktf commented 1 year ago

The "small" library seems to add 70MB of RSS per process due to loading a bunch of pcms (thanks to the fact everything sits in the Experimental namespace, AFAICT). We do want TMVA (which does not get loaded premptively, apparently).

lmoneta commented 1 year ago

You should anyway load SOFIE only when you use it. If this is not the case, then this is maybe the issue. But weird you add 70MB, I see only an increase of 2 MB, when loading the library

ktf commented 1 year ago

See the discussion in https://github.com/root-project/root/issues/13000. I think the issue is the fact that it sits in the Experimental namespace.

lmoneta commented 1 year ago

If I have understood well, doing this (using an arbitrary ROOT file)

ROOTDEBUG=7 root.exe -l -b -q test.root 2>&1 | grep Preloading

you see libROOTTMVASofie pre-loaded. I actually I do not see this with the master. I see other libs preloaded (e.g. libGraf)

ktf commented 1 year ago

Could you try with the file I have on CVMFS?

ktf commented 1 year ago

Could you also try:

ROOTDEBUG=7 root.exe -l -b -q ~/public/AO2D.root 2>&1 | grep 'on demand' | cut -d' ' -f 2 | sort | uniq | wc -l

?

lmoneta commented 1 year ago

With your file I have

...............
Loading 'PyMVA' on demand for 'Experimental'
Loading 'ROOTTMVASofie' on demand for 'Experimental'
Loading 'ROOTTMVASofieParser' on demand for 'Experimental'

Is it because ROOT::Experimental namespace objects are stored in the file? I see also RooFit and TMVA are pre-loaded. These are much bigger libraries using a lot of memory.

We can add a command to switch off SOFIE, but I think the problem is in the module loading and should be fixed in https://github.com/root-project/root/issues/13000

ktf commented 1 year ago

No, there is nothing stored in the file which is not a TTree of ints / floats and related arrays.

dpiparo commented 7 months ago

I think the unwanted memory hoarding has been fixed so far (CMS, ATLAS and LHCb confirmed and we are in the process of verifying this for Alice, too https://github.com/root-project/root/issues/13000#issuecomment-1926377943). I still think that if SOFIE is disabled, nothing should be built relative to it - this is also a desirable feature given the kind of modularisation ahead of us required by pip install ROOT. @lmoneta @bellenot can SOFIE be fully disabled if the user sets -Dsofie=OFF, as one would expect?

lmoneta commented 7 months ago

We can add another option to disable completely SOFIE, but it is itself a small library and we could consider in general as part of tmva. The Onyx parser instead needs to be switch is off for cases where we don't want to have the protobuf dependency

dpiparo commented 4 months ago

Closing since we reached consensus (and fixed the extra parsing due to missing symbols already in December)