Fix bugs in code that detects missing ROOT dictionaries #15198
Conversation
The changes here started when fixing the bug reported in issue 14242 where "std::type_info*" was incorrectly being reported as the type for missing dictionaries. While working on this, found several other problems in this part of the code and made significant revisions to it. One feature that is new is now dictionaries are checked for all consumed types. In addition, the pre-existing checking for dictionaries of produced types, present types, and types selected for output is done in a more consistent way with better and more consistent error messages. These changes rely on the ROOT function TClass::GetMissingDictionaries more than the previous version. A design choice was to use this instead of developing and maintaining CMS specific code to look for dictionaries for all the constituents of a class. This ROOT function currently does not work as we expect and as part of this effort we submitted bug report JIRA-8208 to the ROOT bug tracking system. When this bug is fixed in ROOT the missing dictionary checking will find more dictionaries to be missing. This also includes addition of dictionaries necessary for runTheMatrix to run successfully with the more thorough dictionary checking.
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for CMSSW_8_1_X. It involves the following packages: DataFormats/CTPPSReco @smuzaffar, @civanch, @Dr15Jones, @cvuosalo, @mdhildreth, @cmsbuild, @rekovic, @slava77, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
|
please test |
|
The tests are being triggered in jenkins. |
|
Comparison not run due to Build errors (RelVals and Igprof tests were also skipped) |
|
-1 Tested at: f5a4728 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: You can see the results of the tests here: I found follow errors while testing this PR Failed tests: Build
I found an error when building: ...now 80-column dumb tty Cannot remove alias '80-column dumb tty' Leaving library rule at src/DQMOffline/PFTau/plugins >> Building edm plugin tmp/slc6_amd64_gcc530/src/DQM/L1TMonitorClient/src/DQML1TMonitorClient/libDQML1TMonitorClient.so /cvmfs/cms-ib.cern.ch/2016-29/slc6_amd64_gcc530/external/gcc/5.3.0/bin/../lib/gcc/x86_64-pc-linux-gnu/5.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: cannot find -lDataFormatsLuminosity collect2: error: ld returned 1 exit status gmake: **\* [tmp/slc6_amd64_gcc530/src/DPGAnalysis/SiStripTools/src/DPGAnalysisSiStripTools/libDPGAnalysisSiStripTools.so] Error 1 Leaving library rule at DPGAnalysis/SiStripTools Copying tmp/slc6_amd64_gcc530/src/DQMOffline/RecoB/src/DQMOfflineRecoB/libDQMOfflineRecoB.so to productstore area: >> Leaving Package DQM/SiPixelPhase1TrackClusters >> Package DQM/SiPixelPhase1TrackClusters built The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
please test |
|
The tests are being triggered in jenkins. |
cmsbuild
added
tests-pending
tests-started
and removed
tests-rejected
tests-pending
labels
|
-1 Tested at: f5a4728 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests RelVals
I found errors in the following unit tests: ---> test testRecoMETMETProducers had ERRORS
When I ran the RelVals I found an error in the following worklfows: runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5/step3_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5.log |
|
Unit test failures look unrelated to this PR, all are file open errors. RelVal failure is related. Somehow I missed this one when I ran runTheMatrix.py. I will fix this one very soon and update the PR. |
|
please test |
cmsbuild
added
tests-pending
comparison-pending
and removed
comparison-available
tests-rejected
labels
|
The tests are being triggered in jenkins. |
|
-1 Tested at: 2ea2906 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: UnitTests
I found errors in the following unit tests: ---> test runtestUtilAlgos had ERRORS |
|
Everything except 3 unit tests passed. Again file open errors that look to me to be unrelated to my changes. Two are the same exceptions as before. This time the one in runtestUtilAlgos did something different, it actually seg faulted after the following error: Error in TNetXNGFile::Open: [ERROR] Server responded with an error: [3011] Unable to open file /eos/cms/store/relval/CMSSW_7_2_0_pre5/RelValProdTTbar/GEN-SIM-RECO/START72_V1-v1/00000/022350A9-AC30-E411-B225-0025905A6076.root; No such file or directory Be nice if this could get approved anyway ... The changes outside the core are relatively trivial. The thing that is causing so much difficulty is that the changes are in code that causes almost all CMSSW to build because of dependencies, so almost all the unit tests get run. If any one thinks these problems are related to my code changes I will dig into this some more, but it looks unrelated and these file open problems are something I do not know much about. |
|
+1 Bug fix for ROOT dictionaries. There should be no change in monitored RECO quantities. The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_1_X_2016-07-17-2300 show no significant differences, as expected. The Jenkins tests had minor unit test errors unrelated to this PR. |
|
I hope this can get approved soon. The changes outside the core are trivial. There is nothing other than adding lines to classes_def.xml files to cause dictionaries to be generated that were missing. All tests pass except for 3 unit tests that are failing already in the IB even without these changes. There are no failures related to these changes. |
|
Hi all - A number of groups have not signed this PR nor made comments/questions on it. Please make any further comments on this PR today. |
|
+1 |
|
+1 |
The changes here started when fixing the bug reported
in issue 14242 where "std::type_info*" was incorrectly
being reported as the type for missing dictionaries.
While working on this, found several other problems in
this part of the code and made significant revisions to
it.
One feature that is new is now dictionaries are checked
for all consumed types. In addition, the pre-existing
checking for dictionaries of produced types, present types,
and types selected for output is done in a more consistent
way with better and more consistent error messages.
These changes rely on the ROOT function TClass::GetMissingDictionaries
more than the previous version. A design choice was to
use this instead of developing and maintaining CMS specific
code to look for dictionaries for all the constituents
of a class. This ROOT function currently does not work as we
expect and as part of this effort we submitted bug report
JIRA-8208 to the ROOT bug tracking system. When this bug
is fixed in ROOT the missing dictionary checking will find
more dictionaries to be missing.
This also includes addition of dictionaries necessary for
runTheMatrix to run successfully with the more thorough
dictionary checking.