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

Update Run3 offline data GT and mcRun3 GTs with final DD4HEP #36326

Merged
merged 1 commit into from Dec 2, 2021

Conversation

francescobrivio
Copy link
Contributor

@francescobrivio francescobrivio commented Dec 1, 2021

PR description:

This PR includes several updates in the conditions:

GT diffs:

Run 3 data (offline)
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_dataRun3_v1/122X_dataRun3_v2

2021 design
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_mcRun3_2021_design_v2/122X_mcRun3_2021_design_v3

2021 realistic
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_mcRun3_2021_realistic_v2/122X_mcRun3_2021_realistic_v3

2021 cosmics
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_mcRun3_2021cosmics_realistic_deco_v2/122X_mcRun3_2021cosmics_realistic_deco_v3

2021 heavy ion
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_mcRun3_2021_realistic_HI_v2/122X_mcRun3_2021_realistic_HI_v3

2023 realistic
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_mcRun3_2023_realistic_v2/122X_mcRun3_2023_realistic_v3

2024 realistic
https://cms-conddb.cern.ch/cmsDbBrowser/diff/Prod/gts/122X_mcRun3_2024_realistic_v2/122X_mcRun3_2024_realistic_v3

PR validation:

Validated with several wfs:

  • runTheMatrix.py -l 12034.0,11634.0,7.23,159.0,12434.0,12834.0 for mcRun3 GTs
  • runTheMatrix.py -l 11634.911,11634.914 for DDD/DD4HEP changes
  • runTheMatrix.py -l 11725.0,11925.0 for PPS changes
  • runTheMatrix.py -l 136.897,136.899,138.3,139.001,139.002,139.003,139.004 for the offline run3 GT

Backport

Not a backport, but a similar PR will be opened soon for 12_1_X

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

test parameters:

  • workflows = 12034.0,11634.0,7.23,159.0,12434.0,12834.0,11634.911,11634.914,11725.0,11925.0,136.897,136.899,138.3,139.001,139.002,139.003,139.004

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

urgent

  • we need this in the pre-release to be cut today/tomorrow

@cmsbuild cmsbuild added the urgent label Dec 1, 2021
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36326/27066

  • This PR adds an extra 52KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

A new Pull Request was created by @francescobrivio for master.

It involves the following packages:

  • Configuration/AlCa (alca)
  • Configuration/PyReleaseValidation (pdmv, upgrade)

@malbouis, @yuanchao, @jordan-martins, @bbilin, @wajidalikhan, @cmsbuild, @AdrianoDee, @srimanob, @kskovpen, @tvami, @francescobrivio can you please review it and eventually sign? Thanks.
@makortel, @kpedro88, @Martin-Grunewald, @missirol, @tocheng, @mmusich, @fabiocos, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

@cmsbuild , please test

@tvami
Copy link
Contributor

tvami commented Dec 1, 2021

@francescobrivio maybe you could include what we changed for MC in the title of the PR? Maybe just saying DD4HEP? I'm just saying looking back this PR title doesn't help too much identifying what we did here

@francescobrivio francescobrivio changed the title Update Run3 offline data GT and mcRun3 GTs Update Run3 offline data GT and mcRun3 GTs with final DD4HEP Dec 1, 2021
@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 1, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c913fb/20915/summary.html
COMMIT: 64d7263
CMSSW: CMSSW_12_2_X_2021-12-01-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36326/20915/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c913fb/20915/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c913fb/20915/git-merge-result

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/11725.0_GluGluTo2Jets_14TeV+2021+GluGluTo2Jets_M_300_2000_14TeV_Exhume_GenSim+Digi+RecoNano+HARVESTNano+ALCA
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/11925.0_GluGluTo2Jets_14TeV+2021PU+GluGluTo2Jets_M_300_2000_14TeV_Exhume_GenSim+DigiPU+RecoNanoPU+HARVESTNanoPU
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/12034.0_TTbar_14TeV+2021Design+TTbar_14TeV_TuneCP5_GenSim+Digi+RecoNano+HARVESTNano
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/136.897_RunCosmics2021CRUZET+RunCosmics2021CRUZET+RECOCOSDRUN3+ALCACOSDRUN3+HARVESTDCR3
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/136.899_RunCosmics2021CRAFT+RunCosmics2021CRAFT+RECOCOSDRUN3+ALCACOSDRUN3+HARVESTDCR3
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/138.3_Splashes+RunMinimumBias2021Splash+COPYPASTER3+RECODR3Splash+HARVESTDR3
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/139.001_RunMinimumBias2021+RunMinimumBias2021+HLTDR3_2021+RECODR3_MinBiasOffline+HARVESTD2021MB
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/139.002_RunZeroBias2021+RunZeroBias2021+HLTDR3_2021+RECODR3_ZBOffline+HARVESTD2021ZB
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/139.003_RunHLTPhy2021+RunHLTPhy2021+HLTDR3_2021+RECODR3_HLTPhysics_Offline+HARVESTD2021HLTPhy
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/139.004_RunNoBPTX2021+RunNoBPTX2021+HLTDR3_2021+RECODR3_AlCaTkCosmics_Offline+HARVESTDR3
  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-c913fb/7.23_Cosmics_UP21+Cosmics_UP21+DIGICOS_UP21+RECOCOS_UP21+ALCACOS_UP21+HARVESTCOS_UP21

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 13789 differences found in the comparisons
  • DQMHistoTests: Total files compared: 41
  • DQMHistoTests: Total histograms compared: 3041955
  • DQMHistoTests: Total failures: 58198
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2983735
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 40 files compared)
  • Checked 175 log files, 37 edm output root files, 41 DQM output files
  • TriggerResults: found differences in 2 / 40 workflows

@tvami
Copy link
Contributor

tvami commented Dec 2, 2021

@cmsbuild , please test

  • there is a lot of changes here that are due to other PRs, let's do this now with the 23:00 IB

@mmusich
Copy link
Contributor

mmusich commented Dec 2, 2021

I was having a look to the outputs of the previous round of tests from #36326 (comment) for the 2021 workflows and I see a lot of:

%MSG-w clusterizeDetUnit():  SiPixelClusterProducer:siPixelClusters  01-Dec-2021 21:33:39 CET Run: 345755 Event: 176466
No digis to clusterize
%MSG

in both the CRAFT workflow 136.899 (not so surprisingly) but also in the MinimumBias one 139.001
(somewhat more surprisingly, excepted if the chosen LS hasn't pixel data?).

@cms-sw/trk-dpg-l2 @OzAmram @ferencek please take note.

Seemingly this is caused by:

if (begin == end)
edm::LogWarning("clusterizeDetUnit()") << "No digis to clusterize";

introduced at PR #35441 (https://github.com/cms-sw/cmssw/pull/35441/files#diff-7aa95db5a7413c63dc3c64cd194dff15ed9d03473750c6a61b9cd32be929fad5R134).
@emiglior FYI

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

New categories assigned: trk-dpg

@connorpa,@mmusich,@tsusa you have been requested to review this Pull request/Issue and eventually sign? Thanks

@mmusich
Copy link
Contributor

mmusich commented Dec 2, 2021

actually also 139.001 has same warnings in the IB: link

my take is that the modifier has no impact on that, though I would suggest to silence the warning which seems too loud.

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2021

my take is that the modifier has no impact on that, though I would suggest to silence the warning which seems too loud.

Ok, indeed I missed those two warnings in the IB for 136.899. But here their number is increased from 2 toe 71, thous the frequency of that message do depend on the modifier, I'd say. My question is: is it expected, or should we try to understand it better (and from my point of you delaying the merging of this PR).

Then, if all what above is expected: yes, I agree that the message should be better silenced somehow. But it can be done in another PR.

@mmusich
Copy link
Contributor

mmusich commented Dec 2, 2021

Ok, indeed I missed those two warnings in the IB for 136.899.

fine

But here their number is increased from 2 toe 71, thous the frequency of that message do depend on the modifier, I'd say. My question is: is it expected, or should we try to understand it better (and from my point of you delaying the merging of this PR).

the modifier is creating more "dead" ROCs, since it's killing at the pixel raw2digi level all digis coming from noisy ROCs, so I think one would expect to have more detUnits without any digi to clusterize.
I think it would be good if @tsusa cross-checks that - though.

yes, I agree that the message should be better silenced somehow. But it can be done in another PR.

I think the message was (re-)introduced recently in PR #35441 as pointed out in #36326 (comment).
My message was mostly to nudge the DPG to assess if that re-introduction is causing more trouble than benefits.

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2021

Ok, thank you Marco!
Let wait for @tsusa confirmation, then. It would be nice to get it as soon as possible, so that we can merge this and start building pre3.

@tsusa
Copy link
Contributor

tsusa commented Dec 2, 2021

@perrotta looking into it

@tsusa
Copy link
Contributor

tsusa commented Dec 2, 2021

@perrotta @mmusich For MiniBias wf (139.001), the number of warnings is the same with and without procModifier (86 warnings in 100 events).

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2021

@perrotta @mmusich For MiniBias wf (139.001), the number of warnings is the same with and without procModifier (86 warnings in 100 events).

Thank you @tsusa.
In wf 136.899 the warnings increase from 2 to 71 with this PR.
Anyhow, the real question is: is this PR good enough for being merged (and the nuisance of the flood of messages can be solved afterwards), or do those messages witness any problem that tracker-dpg wants to fix before merging?

@tsusa
Copy link
Contributor

tsusa commented Dec 2, 2021

I have just run 136.899 with and without modifier and see there 69 warnings in 100 events in both cases.

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2021

I have just run 136.899 with and without modifier and see there 69 warnings in 100 events in both cases.

Uhm, puzzzling...
In CMSSW_12_2_X_2021-12-01-2300 there are only 2 warnings in the log of that workflow, as also noticed by @mmusich here above in #36326 (comment)

@tsusa
Copy link
Contributor

tsusa commented Dec 2, 2021

but there are more of them, see for example lines 216, 219, 223, 373, ...

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2021

but there are more of them, see for example lines 216, 219, 223, 373, ...

Yes, you are right. For a strange reason, in my browser the search in the page goes only in the visibile part of the log when I open in my browser the IB logs, while it searches through the whole file when I open the test logs.

Ok, thank you Tatjana, and sorry for the noise. I think this PR can get merged as such (and the trk dpg will independently think about mitigating those messages, if deemed necessary)

@perrotta
Copy link
Contributor

perrotta commented Dec 2, 2021

+1

@mmusich
Copy link
Contributor

mmusich commented Dec 2, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 2, 2021

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

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

8 participants