Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DD4hep] Update to latest commit v01-15x #6612

Merged
merged 3 commits into from Feb 22, 2021

Conversation

smuzaffar
Copy link
Contributor

@smuzaffar smuzaffar commented Feb 2, 2021

the dd4hep plugin generator is renamed to listcomponents_dd4hep now

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2021

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for branch IB/CMSSW_11_3_X/master.

@cmsbuild, @smuzaffar, @mrodozov can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2021

Pull request #6612 was updated.

@smuzaffar
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 2, 2021

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ba0ab/12640/summary.html
COMMIT: 4965ccb
CMSSW: CMSSW_11_3_X_2021-02-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6612/12640/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test GeometryDTGeometryBuilderTestDriver had ERRORS

RelVals

  • 11624.91111624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@smuzaffar
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 4, 2021

-1

Failed Tests: UnitTests RelVals
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ba0ab/12702/summary.html
COMMIT: 4965ccb
CMSSW: CMSSW_11_3_X_2021-02-03-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6612/12702/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test GeometryDTGeometryBuilderTestDriver had ERRORS

RelVals

  • 11624.91111624.911_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA/step1_TTbar_13+2021_DD4hep+TTbar_13TeV_TuneCUETP8M1_GenSim+Digi+Reco+HARVEST+ALCA.log

@cvuosalo
Copy link

cvuosalo commented Feb 4, 2021

#32809 should fix the failure of the 11624.911 workflow. I will submit a PR to remove the failing GeometryDTGeometryBuilderTestDriver test.

@cvuosalo
Copy link

cvuosalo commented Feb 5, 2021

#32823 is fixing more files that are impacted by the DD4hep change in this PR. Several more files will also need to be fixed.

@cvuosalo
Copy link

cvuosalo commented Feb 5, 2021

#32834 disables the failing GeometryDTGeometryBuilderTestDriver unit test.

@mrodozov
Copy link
Contributor

mrodozov commented Feb 6, 2021

please test with cms-sw/cmssw#32823, cms-sw/cmssw#32834

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ba0ab/12956/summary.html
COMMIT: e1b8967
CMSSW: CMSSW_11_3_X_2021-02-17-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6612/12956/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 8145430 lines to the logs
  • Reco comparison results: 2974 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2750983
  • DQMHistoTests: Total failures: 75032
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2675929
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@mrodozov
Copy link
Contributor

@cvuosalo this integrated with the latest changes from ROOT 6.22 but it has many comparison differences
@slava77 I'd like to ask if the diffs are explicable/reasonable

@slava77
Copy link
Contributor

slava77 commented Feb 17, 2021

@slava77 I'd like to ask if the diffs are explicable/reasonable

Reco comparisons are apparently significantly different only in the DD4HEP workflow 11634.911. A few spot-checks suggest that this has a different random number sequence either in SIM or later in DIGI; so, many distributions are somewhat different.

Based on

You potentially added 8145430 lines to the logs

This looks pretty bad.

A few files that I checked have a lot of

%MSG-e Root_Error:  AfterModBeginStream TThreadExecutor::ParallelFor()  17-Feb-2021 18:07:39 CET BeforeEvents
tbb::global_control is limiting the number of parallel workers. Proceeding with 1 threads this time

these are not present in the baseline.

@mrodozov
Copy link
Contributor

@slava77 I'm very sorry I did a mistake. sorry for the noise this is not tested with the proper root

@mrodozov
Copy link
Contributor

no no, it's fine actually. it's the proper ROOT.
but the ROOT update itself is showing only 12 diffs (see #6657)

@makortel
Copy link
Contributor

%MSG-e Root_Error:  AfterModBeginStream TThreadExecutor::ParallelFor()  17-Feb-2021 18:07:39 CET BeforeEvents
tbb::global_control is limiting the number of parallel workers. Proceeding with 1 threads this time

@Dr15Jones Seems that cms-sw/cmssw#32782 turned the tbb::global_control messages (from elsewhere than RTaskArenaWrapper) from exception to ERROR level messages instead of INFO that was the goal of that PR.

@slava77
Copy link
Contributor

slava77 commented Feb 17, 2021

no no, it's fine actually. it's the proper ROOT.
but the ROOT update itself is showing only 12 diffs (see #6657)

As I tried to summarize in #6612 (comment)
Relevant differences are only in the DD4HEP workflow 11634.911.

I don't see anything bad for reco, the differences in 11634.911 are apparently happening earlier, in the SIM or perhaps DIGI stage.
I'm leaving it to @cms-sw/geometry-l2 to sign-off/confirm if this update is expected to introduce some differences.

@Dr15Jones
Copy link

Seems that cms-sw/cmssw#32782 turned the tbb::global_control messages (from elsewhere than RTaskArenaWrapper) from exception to ERROR level messages instead of INFO that was the goal of that PR.

See cms-sw/cmssw#32947

@mrodozov
Copy link
Contributor

please test with #6657, cms-sw/cmssw#32947

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ba0ab/12973/summary.html
COMMIT: e1b8967
CMSSW: CMSSW_11_3_X_2021-02-18-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6612/12973/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2967 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2750983
  • DQMHistoTests: Total failures: 75028
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2675933
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@cvuosalo
Copy link

Unit tests and DD4hep workflow results look good. I think this PR can be merged. I will test it some more in parallel.
+geometry

@mrodozov
Copy link
Contributor

+externals
alright then :) we need #6657 , too

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_11_3_X/master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants