cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.08k stars 4.29k forks source link

Drop CXXMODULES IBs #41810

Open iarspider opened 1 year ago

iarspider commented 1 year ago

The CXXMODULES IB are broken since November 2022 (https://github.com/cms-sw/cmsdist/issues/8197). We propose to stop building this IB flavor: it takes too much effort to maintain them, compared to improvements these IBs were supposed to provide. Pinging @vgvassilev and @davidlange6 for feedback.

cmsbuild commented 1 year ago

A new Issue was created by @iarspider .

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

makortel commented 1 year ago

assign core

cmsbuild commented 1 year ago

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

vgvassilev commented 1 year ago

I'd have cycles to spend on this soon. How urgent is the decision for dropping?

There are several good things for cmssw in general there. One example is the use of rootcling instead of genreflex.

smuzaffar commented 1 year ago

@vgvassilev , this IB has been broken for over 6 months now. We are not getting any thing out of it and really a overhead for us to maintain it. I would like to drop it as soon as possible to.

rootcling instead of genreflex to going to be integrated in default CMSSW IBs any way.

vgvassilev commented 1 year ago

@vgvassilev , this IB has been broken for over 6 months now.

I can fix the immediate issue we are seeing in a few days.

We are not getting any thing out of it and really a overhead for us to maintain it. I would like to drop it as soon as possible to.

This build has been a very good tool for finding layering violations and header-wise dependency problems. We have been able to fix these things early on when the IB was working.

By overhead do you mean computational resources or manpower?

rootcling instead of genreflex to going to be integrated in default CMSSW IBs any way.

That's great to hear!

smuzaffar commented 1 year ago

By overhead do you mean computational resources or manpower?

both; porting root master changes to cxxcompule branch some time needs manual work. Also whenever we update externals (clhep tinyxml2 boost fftw3 cuda python3 hepmc tbb gcc py3-pybind11 fmt xerces-c dd4hep hls ) then it also require manual fixes.

vgvassilev commented 1 year ago

By overhead do you mean computational resources or manpower?

both; porting root master changes to cxxcompule branch some time needs manual work. Also whenever we update externals (clhep tinyxml2 boost fftw3 cuda python3 hepmc tbb gcc py3-pybind11 fmt xerces-c dd4hep hls ) then it also require manual fixes.

In that case can we freeze that and do it upon need/request?

makortel commented 1 year ago

Just to add that from the framework point of view I'd like to eventually switch to CXXMODULES by default in order to reduce auto ROOT header auto parsing (among others)

vgvassilev commented 1 year ago

@makortel that was my original intent. Currently we have two outstanding issues which need to be addressed:

  1. Implement a feature in ROOT where we support dictionaries which introduces synonyms not real declarations (think of typedefs) -- In ROOT we seem to have an issue with modules for the instantiations where the dictionary does not define the declaration. For example, map<string,string> where the instantiation is in Core.pcm and we cannot get the relevant dictionary. The same issue holds for CMSSW. Even though in CMSSW we have noticed that pattern is mostly a bug (most of these were fixed) where the dictionary seemed to have incorrectly put at the wrong library. However, there are around 10-20 cases in which this is used as an idiom and there is not good suitable place to put it (as others have said, it's been a long time so I don't remember these anymore). We can solve this by implementing a more robust dictionary to entity discovery in ROOT by using the more powerful dynamic linker infrastructure where we will be looking for the symbol instead of guessing how codegen mapped the symbol to load the relevant library. That will significantly reduce the bug reports of missing dictionary.
  2. Improve the performance of pre-loading of C++ modules in Clang. The C++ modules implementation in Clang does almost everything on-demand except for allocation of source locations when a module file is pre-loaded. Currently a single line of code is hindering the efficient adoption of this feature in CMSSW (for those who are curious that's here). Upon registering a module Clang would do nothing except for pre-allocation of entries in a vector which represents all source location IDs for all source files. That does not scale in rss at all for the sparsely used modules which is what many CMSSW workflows have. Re-modelling the way we handle source locations in Clang is tricky but not out of reach for us.

Unfortunately, that has not been part of my work plan the last years and this also was not a CMS priority for ROOT. Hence, the best I can do is to work on this on a best effort basis. Removing the IB will probably make this practically impossible for me.

smuzaffar commented 1 year ago

By overhead do you mean computational resources or manpower?

both; porting root master changes to cxxcompule branch some time needs manual work. Also whenever we update externals (clhep tinyxml2 boost fftw3 cuda python3 hepmc tbb gcc py3-pybind11 fmt xerces-c dd4hep hls ) then it also require manual fixes.

In that case can we freeze that and do it upon need/request?

that will create more manual work ... more man power to support multuiple versions of externals and more tests for cmssw changes for an extra set of externals

davidlange6 commented 1 year ago

Remind me of what git command is used to forward prs to the various branches in cmsdist. It seems like we should be able to convince it to work in my post all cases if the cmsdist differences are contained

smuzaffar commented 1 year ago

@davidlange6 , https://github.com/cms-sw/cms-bot/blob/master/merge-git-branch (it runs [a] commands) is the script which does the automatic forward ports from rootmaster to rootmodule branch. It mostly works out of the box. Problem is with external version changes for which we are generating pcm module. IIRC, last time CXXModules IB externals failed to build was when we updated pybind11 to version 2.10. IBs remained broken for months as pcm for pybind11 were failing. https://github.com/cms-sw/cmsdist/pull/8509 was needed to atleast get the externals build

[a]

git clean -fdx
git checkout -q --force IB/CMSSW_13_2_X/rootmodule
git reset --hard origin/IB/CMSSW_13_2_X/rootmodule
HEAD is now at 3e4cce8 Merge IB/CMSSW_13_2_X/rootmaster into IB/CMSSW_13_2_X/rootmodule.
git merge -m 'Merge IB/CMSSW_13_2_X/rootmaster into IB/CMSSW_13_2_X/rootmodule.' --no-ff origin/IB/CMSSW_13_2_X/rootmaster
davidlange6 commented 1 year ago

@smuzaffar and I talked a bit and I understood that the work is coming not from failed git merges but from fixing things like the pybind11 pcm - the proposal from my end is to just let these fail until someone has cycles to work on modules and fix them. That should mean minimal human effort from the core side and meanwhile the modules branch remains relatively up to date with respect to the external changes.