ROOT 6.22 compatibility (CMSSW 11_2)

[nsmith-]

Mostly narrowing definitions (ROOT is becoming more precise) or header includes that are not for free anymore
One bug got fixed!
Should be backwards-compatible with 10_2 hence why I put it on the same branch

[adewit]

Just to note, for full root 622 compatibility I think we need a bit more (e.g. compare with: https://github.com/adewit/HiggsAnalysis-CombinedLimit/commits/standalone-root622) It's possible some types of cards / certain combine methods still work without these additional changes, but certainly not everything

[nsmith-]

Thanks @adewit looks like your branch made many of the same changes. I merged it in here. Though I cannot speak to the backwards-compatibility of the other changes you made, it does appear to still compile in 10_2.

[nsmith-]

Now with conda support (no more LCG dependency) thanks to @andrzejnovak

[nsmith-]

What do we need to do to get this merged?

[andrzejnovak]

By the way a conda setup would allow to run CI/tests directly on GitHub via GitHub Actions.

[adewit]

Sorry for not spotting it earlier, but this commit is a hack that I don't think we want to enter any official combine release : dd923fc - so that should be reverted.

We're planning to release a new combine release (ie 102x-compatible) very soon - for CMSSW112X/Root622 compatibility we would like to validate more things before making it the default (from our side we've only looked at a narrow range of cases so far). This is mainly with regards to the standalone compilation, since the C++ changes are compatible with 102x. So ideally we would branch off the development for 112X and make this PR the first thing to go in there. I think we have one bug fix to come for 102x, after which we can branch off. At that point I think we can merge this into the new branch, if that works for you? Alternatively the C++ changes could go into the 102x branch, keeping the standalone compilation setup for the new 112x branch.

(btw, I think the merge conflict is just because we had to add something to the standalone compilation env/makefile related to the additional use of the eigen package; the version shipped with CMSSW_11_2_0_pre10 is d812f411c3f9-ghbfee as far as I can see)

[nsmith-]

I'm happy to wait until the 112x branch starts up. I reverted the HZZ patch and resolved the eigen conflict as you suggested. Indeed 11_2_x eigen is as you say:

$ scram tool info eigen
Tool info as configured in location /uscms_data/d3/ncsmith/tmp/CMSSW_11_2_0
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

Name : eigen
Version : d812f411c3f9-ghbfee
++++++++++++++++++++
SCRAM_PROJECT=no
EIGEN_BASE=/cvmfs/cms.cern.ch/slc7_amd64_gcc900/external/eigen/d812f411c3f9-ghbfee
CPPDEFINES+=EIGEN_DONT_PARALLELIZE
INCLUDE=/cvmfs/cms.cern.ch/slc7_amd64_gcc900/external/eigen/d812f411c3f9-ghbfee/include/eigen3
ROOT_INCLUDE_PATH=/cvmfs/cms.cern.ch/slc7_amd64_gcc900/external/eigen/d812f411c3f9-ghbfee/include/eigen3

[adewit]

We now have a 112x branch - I thought there was a way to change the PR target but I don't see it atm... in any case if you're able to edit the target please go ahead with that (otherwise we can make a new PR from your branch into 112x)

[adewit]

Yes, the 102x compatibility can be removed!

[maxgalli]

Hi, having the conda support is really nice!
I just tried it out following the instructions here and I got this:

src/RooSplineND.cc:3:10: fatal error: Eigen/Dense: No such file or directory
 #include <Eigen/Dense>
          ^~~~~~~~~~~~~
compilation terminated.
make: *** [obj/RooSplineND.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** wait: No child processes.  Stop.

I managed to finish building after installing eigen3 in the conda environment created and changing the above mentioned line to

#include <eigen3/Eigen/Dense>

(this I could have probably avoided by simply adding -I$(CONDA)/include/eigen3 to the CCFLAGS in Makefile_conda).

Is it possible that there is a field missing for eigen in conda_env.yml or did I do something wrong?

[andrzejnovak]

@maxgalli thanks for reporting that, I can replicate it, but didn't see it before. I wonder if it was pulled as a dependency for something previously since the versions are mostly not pinned in the yml file.

EDIT: I see now the spline code was merged into the PR after, the conda env.

I'll make the fix and add a frozen env file to be able to check this in the future.

[maxgalli]

@maxgalli thanks for reporting that, I can replicate it, but didn't see it before. I wonder if it was pulled as a dependency for something previously since the versions are mostly not pinned in the yml file.

EDIT: I see now the spline code was merged into the PR after, the conda env.

I'll make the fix and add a frozen env file to be able to check this in the future.

Thank you for going through the effort of adding conda support, personally I was really looking forward to it :)

[nsmith-]

Sorry for the delay, removed the 102x business. I also added some CI thanks to Andrzej's conda setup, but you might need to enable github actions in the repo to trigger it.

[andrzejnovak]

actions will trigger after at least one workflows yml is on master IIRC

[maxgalli]

Hi, I have another question: is the command source env_conda.sh supposed to be run every time after activating the environment (conda activate combine2) in a new session?

I'm asking because I just tried it and it wasn't found in $PATH; it became available only after running source env_conda.sh.
Here's what I did exactly:

(base) [gallim@t3ui01.psi.ch:/work/gallim/diffComb2017/HiggsAnalysis/CombinedLimit]
$ conda activate combine2
(combine2) [gallim@t3ui01.psi.ch:/work/gallim/diffComb2017/HiggsAnalysis/CombinedLimit]
$ which combine
/usr/bin/which: no combine in (/work/gallim/anaconda3/envs/combine2/bin:/work/gallim/anaconda3/condabin:/bin:/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/opt/puppetlabs/bin:/t3home/gallim/bin:/t3home/gallim/bin)
(combine2) [gallim@t3ui01.psi.ch:/work/gallim/diffComb2017/HiggsAnalysis/CombinedLimit]
$ source env_conda.sh 
(combine2) [gallim@t3ui01.psi.ch:/work/gallim/diffComb2017/HiggsAnalysis/CombinedLimit]
$ which combine
/work/gallim/diffComb2017/HiggsAnalysis/CombinedLimit/exe/combine
(combine2) [gallim@t3ui01.psi.ch:/work/gallim/diffComb2017/HiggsAnalysis/CombinedLimit]

Am I doing something wrong?
If not, wouldn't it be worth installing the executables in $(CONDA)/bin? Unless maybe this solution has drawbacks that I don't see at the moment.

Thanks again :)

[andrzejnovak]

You have a good point. I was trying to change as little as possible from the "usual" recipe, to not complicate the PR. Of course the easiest solution is to stick conda activate combine2 into env_conda.sh though it's a bit less elegant.

Looks like one could also add the paths in the conda config https://stackoverflow.com/questions/31598963/how-to-set-specific-environment-variables-when-activating-conda-environment

[andrzejnovak]

@adewit Can we help to get this merged in? Are there perhaps instructions to run the test suite?

[ajgilbert]

Hi all - I think it's more or less ok for me to merge. Assuming this will eventually become the default recommended branch for all of CMS, it would be helpful to understand if someone is planning to support & maintain the conda setup and CI integration in the long term? (FWIW for the CI we have a mirror repo in CERN gitlab where we can get the full environment we need to run larger scale tests - I think we should keep fit validation there).

[nsmith-]

Small update: Andrzej made some fixes to the conda build, and also it seems his PR to my PR managed to trigger github tests, you can see the result in: https://github.com/nsmith-/HiggsAnalysis-CombinedLimit/pull/4/checks

I figure the value of CI here is just to catch compilation errors and/or see regressions in the conda setup. If you want to incorporate large scale tests it seems possible to do it here (barring private data concerns) since the conda environment is as "full" as standalone w/CVMFS.

[andrzejnovak]

Always better to have a safety :) Can you point me to the CI setup you use? Maybe I can recreate it in GHA. If this is the primary development repo, it might be helpful to have faster feedback on PRs.

For what it's worth the maintenance should be minimal, as the conda Makefile is only changed w.r.t. the packages sourced from cvmfs, otherwise, it's the same as the default and the CI should tell us anytime it breaks.

[nsmith-]

Could we parameterize the Makefile to allow conda and cvmfs builds to share the same, just pointing to different locations for the dependent libraries?

[andrzejnovak]

I originally wanted to avoid interfering with the "normal" build process, but it can be done.

andrzejnovak@bec9d55

@ajgilbert would Makefile modified in this way be ok?

• make -j4 normal build
• make CONDA=1 -j4 conda build

Edit: only the library paths that change are under the conditional.

[adewit]

Always better to have a safety :) Can you point me to the CI setup you use? Maybe I can recreate it in GHA.

The mirror repo works for us, I'd propose to leave it as it is.

If this is the primary development repo, it might be helpful to have faster feedback on PRs.

With the ci we can't (at the moment) test everything that could break. So unfortunately feedback always requires some level of human intervention.

For what it's worth the maintenance should be minimal, as the conda Makefile is only changed w.r.t. the packages sourced from cvmfs, otherwise, it's the same as the default and the CI should tell us anytime it breaks.

I think Andrew's question was more if anyone is comitting to fixing it when it breaks :-)

[adewit]

Sorry for the delay, removed the 102x business. I also added some CI thanks to Andrzej's conda setup, but you might need to enable github actions in the repo to trigger it.

Sorry I completely missed that the 102x stuff had been removed. Will merge.