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

import additions for UL-HLT in 10_2_X #27446

Merged
merged 1 commit into from Jul 20, 2019

Conversation

srimanob
Copy link
Contributor

@srimanob srimanob commented Jul 5, 2019

PR description:

Preparation for the UL-HLT in 10_2_X. The content of this PR is based on #26399 (except no need of backport of CSCDigi) and #25171

PR validation:

This PR is tested by producing GEN-SIM-RAW using 10_6, then pass to 10_2 for the HLT step. The RAW is re-written from 10_2 and read again in 10_6 for RECO and DQM.

GEN-SIM; 10_6_1_patch1
cmsDriver.py ZEE_13TeV_TuneCUETP8M1_cfi --conditions auto:phase1_2018_realistic -n 10 --era Run2_2018 --eventcontent RAWSIM --relval 9000,50 -s GEN,SIM --datatier GEN-SIM --beamspot Realistic25ns13TeVEarly2018Collision --geometry DB:Extended --io ProdZEE_13UP18.io --python ProdZEE_13UP18.py --no_exec --fileout file:step1.root --nThreads 4

DIGI; 10_6_1_patch1
cmsDriver.py step2 --datamix PreMix --conditions auto:phase1_2018_realistic --pileup_input das:/RelValPREMIXUP18_PU25/CMSSW_10_6_0-PU25ns_106X_upgrade2018_realistic_v4-v1/PREMIX --era Run2_2018 --eventcontent FEVTDEBUGHLT --procModifiers premix_stage2 -s DIGI:pdigi_valid,DATAMIX,L1,DIGI2RAW --datatier GEN-SIM-DIGI-RAW-HLTDEBUG --io DIGIPRMXUP18_PU25.io --python DIGIPRMXUP18_PU25.py -n -1 --no_exec --filein file:step1.root --fileout file:step2.root --nThreads 4

HLT; CMSSW_10_2_15_patch2 with contents of this PR
cmsDriver.py HLT --conditions auto:phase1_2018_realistic --era Run2_2018 --eventcontent FEVTDEBUGHLT -s HLT:@relval2018 --datatier GEN-SIM-DIGI-RAW-HLTDEBUG --io step2a_HLT.io --python step2a_HLT.py -n -1 --no_exec --filein filelist:step2.root --fileout file:step2a.root --nThreads 4

RECO; 10_6_1_patch1
cmsDriver.py step3 --conditions auto:phase1_2018_realistic -n 10 --era Run2_2018 --eventcontent RECOSIM,MINIAODSIM,DQM --runUnscheduled --procModifiers premix_stage2 -s RAW2DIGI,L1Reco,RECO,RECOSIM,EI,PAT,VALIDATION:@standardValidation+@miniAODValidation,DQM:@standardDQM+@ExtraHLT+@miniAODDQM --datatier GEN-SIM-RECO,MINIAODSIM,DQMIO --io RECOPRMXUP18_PU25.io --python RECOPRMXUP18_PU25.py --no_exec --filein file:step2a.root --fileout file:step3.root --nThreads 4

if this PR is a backport please specify the original PR:

Before submitting your pull requests, make sure you followed this checklist:

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 5, 2019

A new Pull Request was created by @srimanob (Phat Srimanobhas) for CMSSW_10_2_X.

It involves the following packages:

Configuration/DataProcessing
DataFormats/Provenance
SimDataFormats/GeneratorProducts

@smuzaffar, @efeyazgan, @Dr15Jones, @cmsbuild, @franzoni, @agrohsje, @alberto-sanchez, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @makortel, @rovere, @wddgit this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

@wddgit do you foresee any problems including the meta data object in the back port without also backporting the code that uses the meta data?

@srimanob
Copy link
Contributor Author

srimanob commented Jul 5, 2019

Hi @Dr15Jones
What am I missing, as I use #25171 as the reference?
Thanks.

@wddgit
Copy link
Contributor

wddgit commented Jul 5, 2019

I think there would be no problems with anything that is not a mergeable run product. No crashes or obvious bad behavior. This metadata object itself is stored in the MetaData TTree. So if you read a file created with 10_3_X or later with 10_2_X as modified by this code, then this object would simply be dropped. There would be nothing to read or write the object or the branch that contains it. Note that this implies any information it contains would be lost.

Mergeable run products would not behave properly in some cases. For example, if you had split a run in one of a series of 10_3_X processes, then processed the file with 10_2_X the mergeable run products would be merged via the old merging algorithm in that 10_2_X process or any subsequent process, whether or not the subsequent process is 10_2_X or 10_3_X. The information about how the runs were split would be lost and I am pretty sure it would just use the old algorithm when merging those run products. This is what is already done when reading file created and only processed with 10_2_X which is later read by something from after 10_2_X. Now it is likely that no cares about this problem ... I don't know.

I'll also add in a warning that I did not test this and only thought about it for about 30 minutes so I could be missing something ...

@franzoni
Copy link

franzoni commented Jul 7, 2019

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/1351/console Started: 2019/07/07 08:39

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2019

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2019

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 7, 2019

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ece98b/1351/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 31
  • DQMHistoTests: Total histograms compared: 3007468
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3007276
  • DQMHistoTests: Total skipped: 190
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 30 files compared)
  • Checked 129 log files, 14 edm output root files, 31 DQM output files

@srimanob
Copy link
Contributor Author

srimanob commented Jul 8, 2019

Hi @Dr15Jones @wddgit @fabiocos
Regarding the concern, could you please explain a bit more if something we should think in 94 release also that we did the same backports, or this is a concern on 10_2 only. Do I miss anything?

Thanks very much in advance.

@fabiocos
Copy link
Contributor

@Dr15Jones @wddgit @srimanob the backport of the provenance code is part of both 80X and 94X UL versions, so if there is an issue it is a general one with the UL processing strategy I would say. As far as I can see in practice this would affect LHERunInfoProduct.h and GenLumiInfoProduct.h correct me in case. @srimanob @franzoni were these objects checked in the validation of the UL chain for 94X for instance?

@srimanob
Copy link
Contributor Author

srimanob commented Jul 11, 2019

@pgunnell @qliphy @efeyazgan
In case, you can answer @fabiocos question. Thanks.

I don't think we use these objects in the validation. But I don't know if GEN validation is using it or not, or what is missing will affect any analysis or not. Still try to understand what is used from the missing.

@Dr15Jones
Copy link
Contributor

@fabiocos you'd just get the regular 8_0_X and 9_4_X behavior for merging and not the improved algorithm used in 10_3_X. So from the framework side we do not think it will cause any (new) problems.

@@ -52,6 +55,7 @@ def mergeProcess(*inputFiles, **options):
process.add_(Service("DQMStore"))
else:
process.source = Source("PoolSource")
process.source.bypassVersionCheck = CfgTypes.untracked.bool(bypassVersionCheck)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srimanob this is not the final version we have in master and 10_6_X, although it is consistent with 9_4_X and 8_0_X. As far as I can see they are anyway equivalent as final functionality

Copy link
Contributor Author

@srimanob srimanob Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos Thanks for checking. This is following the discussion we had before that we turn on the bypassversioncheck by default for the release that will be used for UL-HLT.

If we follow the master, we need to handle at the request configuration level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srimanob yes, just let's keep in mind that while new 8_0_X and 9_4_X will now be used basically only for UL, 10_2_X is still being used for other purposes, and this change will affect not only UL but whatever other production updated to this version, there is no special UL branch

Copy link
Contributor Author

@srimanob srimanob Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiocos
This option can't config from McM side as McM don't touch the Merge cmsDriver. This can't be configured from wmcore too at the moment.

What we agreed on
dmwm/WMCore#9054
is to set the default value to true by default. The PR in the master was done in such a way that we can make it configurable in future. If you want to me to make it exactly the same as master, but change the default value to be true, I can do that.

Thx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@srimanob right, thank you for the reminder, the important things is that we are all aware of the implication

@fabiocos
Copy link
Contributor

@efeyazgan @qliphy any comment on this PR? What about the run products handling? Otherwise are you ready to sign it?

@Dr15Jones I understand that there is no issue in your view. Do you suggest any further check? Or can we merge it as it is as far as core is concerned?

@qliphy
Copy link
Contributor

qliphy commented Jul 17, 2019

Adding @agrohsje

We can check more, but it may take some time. And as we agree with @Dr15Jones that it will not cause new problems, so if it is urgent, we can sign it first.

@Dr15Jones
Copy link
Contributor

I understand that there is no issue in your view. Do you suggest any further check? Or can we merge it as it is as far as core is concerned?

It should be fine as far as core is concerned.

@fabiocos
Copy link
Contributor

@efeyazgan @qliphy @agrohsje the check above is not only for this PR, actually as I suggested at the PPD general meeting I would make the check for the 2017 chain using 94X first, this PR will make 10_2_X not better nor worse than that. It depends on the urgency to have the release for tests, I would say that we could move forward with the integration and have the tests in parallel

@qliphy
Copy link
Contributor

qliphy commented Jul 19, 2019

@fabiocos Thanks! GEN will then sign this PR and in the meantime we will try the checks you suggested.

@qliphy
Copy link
Contributor

qliphy commented Jul 19, 2019

+1

@fabiocos
Copy link
Contributor

+operations

@fabiocos
Copy link
Contributor

@Dr15Jones so is core ready to sign this PR? I understand PPD would like to have a new version to start testing the 2018 chain

@Dr15Jones
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_10_2_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_11_0_X is complete. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit b44ab7a into cms-sw:CMSSW_10_2_X Jul 20, 2019
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