Removal of hardcoded testing L1T tag and move it to GT #36408

tvami · 2021-12-08T02:44:09Z

PR description:

Instead of #35554

Also resolves #27393

Original description
"This PR removes part of the configuration of L1T Calo which retrieves conditions from a specific location (instead of via GT) and which is deprecated for a while (non-used since we moved away from hacking conditions)."

Extra in this PR is the addition of this tag to the GT.

For the future:

This tag is for testing purposes, thus it should be fed to the code via a test python cfg file and not hardcoded in the cpp code nor really read from the GT

Here is the list of diffs wrt to the queue:

PR validation:

Previously failing addOnTest pass

addOnTests.py -j 8

if this PR is a backport please specify the original PR and why you need to backport that PR:

N/A

tvami · 2021-12-08T02:45:39Z

assign alca

tvami · 2021-12-08T02:46:12Z

@cmsbuild , please test

tvami · 2021-12-08T03:04:56Z

code-checks

cmsbuild · 2021-12-08T08:11:31Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36408/27221

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File L1Trigger/L1TCalorimeter/python/simDigis_cff.py modified in PR(s): PR121X L1T Calo - Remove retrieving conditions via config #35554

cmsbuild · 2021-12-08T08:11:48Z

New categories assigned: alca

@yuanchao,@francescobrivio,@malbouis,@tvami you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild · 2021-12-08T08:11:50Z

A new Pull Request was created by @tvami (Tamas Vami) for master.

It involves the following packages:

L1Trigger/L1TCalorimeter (l1)

@malbouis, @yuanchao, @rekovic, @francescobrivio, @cecilecaillol, @tvami can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol 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 · 2021-12-08T13:24:53Z

@smuzaffar tests are taking more than 11 hours, is there something wrong with the bot? Thanks!

tvami · 2021-12-08T13:26:39Z

@smuzaffar tests are taking more than 11 hours, is there something wrong with the bot? Thanks!

Same thing is happening in #36409

smuzaffar · 2021-12-08T13:48:05Z

all resources to test are busy, I am afraid you have to wait.

cmsbuild · 2021-12-08T14:58:48Z

-1

Failed Tests: RelVals RelVals-INPUT AddOn
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d73f32/21085/summary.html
COMMIT: f0bb5f6
CMSSW: CMSSW_12_3_X_2021-12-07-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/36408/21085/install.sh to create a dev area with all the needed externals and cmssw changes.

RelVals

136.731136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2_skimSinglePh/step2_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2_skimSinglePh.log

RelVals-INPUT

136.722136.722_RunDoubleEG2016B+RunDoubleEG2016B+HLTDR2_2016+RECODR2_2016reHLT_skimDoubleEG_HIPM+HARVESTDR2_skimDoubleEG/step2_RunDoubleEG2016B+RunDoubleEG2016B+HLTDR2_2016+RECODR2_2016reHLT_skimDoubleEG_HIPM+HARVESTDR2_skimDoubleEG.log
136.721136.721_RunHLTPhy2016B+RunHLTPhy2016B+HLTDR2_2016+RECODR2_2016reHLT_HIPM+HARVESTDR2/step2_RunHLTPhy2016B+RunHLTPhy2016B+HLTDR2_2016+RECODR2_2016reHLT_HIPM+HARVESTDR2.log
136.723136.723_RunDoubleMuon2016B+RunDoubleMuon2016B+HLTDR2_2016+RECODR2_2016reHLT_HIPM+HARVESTDR2/step2_RunDoubleMuon2016B+RunDoubleMuon2016B+HLTDR2_2016+RECODR2_2016reHLT_HIPM+HARVESTDR2.log

Expand to see more relval errors ...

AddOn Tests

hlt_data_Fake2cmsDriver.py RelVal -s L1REPACK:Full --data --scenario=pp -n 10 --conditions auto:run2_hlt_Fake2 --relval 9000,50 --datatier "RAW" --eventcontent RAW --customise=HLTrigger/Configuration/CustomConfigs.L1T --era Run2_2016 --fileout file:RelVal_Raw_Fake2_DATA.root --filein /store/data/Run2016B/JetHT/RAW/v1/000/272/762/00000/C666CDE2-E013-E611-B15A-02163E011DBE.root : FAILED - time: date Wed Dec 8 15:00:38 2021-date Wed Dec 8 14:56:02 2021 s - exit: 35584
hlt_mc_Fake2cmsRun /cvmfs/cms-ib.cern.ch/week0/slc7_amd64_gcc900/cms/cmssw/CMSSW_12_3_X_2021-12-07-2300/src/HLTrigger/Configuration/test/OnLine_HLT_Fake2.py realData=True globalTag=@ inputFiles=@ : FAILED - time: date Wed Dec 8 15:00:38 2021-date Wed Dec 8 14:56:02 2021 s - exit: 21504
hlt_data_Fake2

----- Begin Fatal Exception 08-Dec-2021 15:00:37 CET-----------------------
An exception of category 'FileOpenError' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing input source of type PoolSource
   [2] Calling RootInputFileSequence::initTheFile()
   Additional Info:
      [a] Input file file:RelVal_Raw_Fake2_DATA.root could not be opened.
      [b] Fatal Root Error: @SUB=TStorageFactoryFile::ReadBuffer
read from Storage::xread returned 262. Asked to read n bytes: 300 from offset: 0 with file size: 262

----- End Fatal Exception -------------------------------------------------

tvami · 2021-12-08T15:10:16Z

Input file file:RelVal_Raw_Fake2_DATA.root could not be opened.

I guess we should just test again? @smuzaffar

mmusich · 2021-12-08T15:19:05Z

I guess we should just test again? @smuzaffar

There's a tonne of segfaults in L1TStage2Layer2Producer:simCaloStage2Digis (see e.g. log). I doubt that will go away re-testing.
FWIW, I still think the right thing to do is to add instead the payloads to GlobalTag: #35554 (comment)

tvami · 2021-12-08T15:25:24Z

There's a tonne of segfaults in L1TStage2Layer2Producer:simCaloStage2Digis (see e.g. log). I doubt that will go away re-testing.

But this means that the HLT menu has m_useStaticConfig on True, right? It should not be, see "use static config for fw testing"

mmusich · 2021-12-08T15:31:22Z

But this means that the HLT menu has m_useStaticConfig on True, right?

I don't see what the HLT menu has anything to do with it. That's in the configuration of L1TStage2Layer2Producer which should be run in the L1REPACK step.

cmsbuild · 2021-12-19T02:48:41Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36408/27482

This PR adds an extra 12KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File L1Trigger/L1TCalorimeter/python/simDigis_cff.py modified in PR(s): PR121X L1T Calo - Remove retrieving conditions via config #35554

cmsbuild · 2021-12-19T02:49:06Z

Pull request #36408 was updated. @malbouis, @yuanchao, @epalencia, @rekovic, @francescobrivio, @cecilecaillol, @tvami can you please check and sign again.

cmsbuild · 2021-12-19T06:30:23Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d73f32/21391/summary.html
COMMIT: 5120c82
CMSSW: CMSSW_12_3_X_2021-12-18-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36408/21391/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:

@fabiocos MTD reconstruction: Implementation of MTDTrayBarrelLayer::groupedCompatibleDets() #36525

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

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 43
DQMHistoTests: Total histograms compared: 3461688
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3461666
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 42 files compared)
Checked 181 log files, 42 edm output root files, 43 DQM output files
TriggerResults: no differences found

tvami · 2021-12-19T06:34:28Z

+alca

finally we see what's expected: no changes at all

cecilecaillol · 2021-12-20T08:52:06Z

+l1

cmsbuild · 2021-12-20T08:52:33Z

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 now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

tvami · 2021-12-20T15:47:14Z

Hi @perrotta @qliphy this doesnt actually require the external PR, please feel to merge it any time. Thanks!

perrotta · 2021-12-20T16:28:26Z

@tvami which are the unit tests that were failing? And which is the unit test to look at in order to get assurance that the new tags are correctly picked up?

tvami · 2021-12-20T16:33:56Z

Hi @perrotta the unit test failure comes from outside of this PR, that is due to some geometry changes.

The orignal PR #35554 has failed both the addOnTests and several RelVal workflows. By adding the condition to the GT all of that is resolved. You can be assured that it's correct by seeing that there are no diffs in the RelVal comparisons (in any of them)

Speaking of the original PR, when merging this, can you please close #35554 ?

perrotta · 2021-12-20T16:55:39Z

@tvami I read in the PR description "This tag is for testing purposes, thus it should be fed to the code via a test python cfg file and not hardcoded in the cpp code nor really read from the GT".
I wonder where is that "python config" which can test the newly inserted tags? Or do you plan to let them unattended, and have them tested at the first occasion anyone needs them?

tvami · 2021-12-20T17:20:15Z

@perrotta the issue is that this "testing purposes tag" has been in CMSSW from forever... (specifically from 9_2_X) and it was read directly from the DB instead of reading it from the GT. This is not allowed, and this is what #27393 is about. Now I first tried to get rid of this fully, and deleted all the code that reference the flag that switches this test tag usage on... Having very little knowledge of the L1T code, this lead to a segfault. So I went to the alternative no-op solution to feed the tag through the GT.

As for the longer term, the L1T experts should rewrite the code so there is a test python config that ESPrefers this tag if it's really needed, or just get rid of it all if it's not needed. Then we can remove it from the GT too. So what you see in this PR is a solution to a problem that can be done from the AlCa point of view. Everything else would need the L1T experts knowledge. Since #35554 has not converged in the last 2 month, I did this PR.

perrotta · 2021-12-21T13:01:53Z

+1

Take conditions from the GT instead of hardcoding the tag in L1Trigger/L1TCalorimeter/python/simDigis_cff.py
@cms-sw/alca-l2 and @cms-sw/l1-l2 will take care of adapting the code in order to get rid of the tag if not needed

cmsbuild added this to the CMSSW_12_3_X milestone Dec 8, 2021

cmsbuild added code-checks-pending l1-pending orp-pending pending-signatures tests-pending labels Dec 8, 2021

tvami mentioned this pull request Dec 8, 2021

PR121X L1T Calo - Remove retrieving conditions via config #35554

Closed

cmsbuild added the alca-pending label Dec 8, 2021

cmsbuild added tests-started and removed tests-pending labels Dec 8, 2021

tvami mentioned this pull request Dec 8, 2021

Developers overriding production location for retrieving conditions in configurations #27393

Open

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 8, 2021

cmsbuild added tests-rejected and removed tests-started labels Dec 8, 2021

missirol mentioned this pull request Dec 8, 2021

increase max length of addOnTests log-file names #36423

Closed

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 19, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 19, 2021

cmsbuild added tests-approved and removed tests-started labels Dec 19, 2021

cmsbuild added alca-approved and removed alca-pending labels Dec 19, 2021

cmsbuild added fully-signed l1-approved and removed l1-pending pending-signatures labels Dec 20, 2021

cmsbuild added orp-approved and removed orp-pending labels Dec 21, 2021

cmsbuild merged commit 6d8172f into cms-sw:master Dec 21, 2021

cmsbuild mentioned this pull request Dec 21, 2021

Removed unused member BaseHolder::fillRefIfMyTypeMatches #36562

Merged

epalencia mentioned this pull request Feb 17, 2022

Removal of hardcoded testing L1T tag and move it to GT (bacport from master) cms-l1t-offline/cmssw#985

Merged

missirol mentioned this pull request Oct 9, 2022

Add Service type in an exception message from ServiceRegistry #39636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removal of hardcoded testing L1T tag and move it to GT #36408

Removal of hardcoded testing L1T tag and move it to GT #36408

tvami commented Dec 8, 2021 •

edited

tvami commented Dec 8, 2021

tvami commented Dec 8, 2021

tvami commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

tvami commented Dec 8, 2021

tvami commented Dec 8, 2021

smuzaffar commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

tvami commented Dec 8, 2021

mmusich commented Dec 8, 2021

tvami commented Dec 8, 2021

mmusich commented Dec 8, 2021

cmsbuild commented Dec 19, 2021

cmsbuild commented Dec 19, 2021

cmsbuild commented Dec 19, 2021

tvami commented Dec 19, 2021

cecilecaillol commented Dec 20, 2021

cmsbuild commented Dec 20, 2021

tvami commented Dec 20, 2021

perrotta commented Dec 20, 2021

tvami commented Dec 20, 2021

perrotta commented Dec 20, 2021

tvami commented Dec 20, 2021

perrotta commented Dec 21, 2021

Removal of hardcoded testing L1T tag and move it to GT #36408

Removal of hardcoded testing L1T tag and move it to GT #36408

Conversation

tvami commented Dec 8, 2021 • edited

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

tvami commented Dec 8, 2021

tvami commented Dec 8, 2021

tvami commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

tvami commented Dec 8, 2021

tvami commented Dec 8, 2021

smuzaffar commented Dec 8, 2021

cmsbuild commented Dec 8, 2021

RelVals

RelVals-INPUT

AddOn Tests

tvami commented Dec 8, 2021

mmusich commented Dec 8, 2021

tvami commented Dec 8, 2021

mmusich commented Dec 8, 2021

cmsbuild commented Dec 19, 2021

cmsbuild commented Dec 19, 2021

cmsbuild commented Dec 19, 2021

Comparison Summary

tvami commented Dec 19, 2021

cecilecaillol commented Dec 20, 2021

cmsbuild commented Dec 20, 2021

tvami commented Dec 20, 2021

perrotta commented Dec 20, 2021

tvami commented Dec 20, 2021

perrotta commented Dec 20, 2021

tvami commented Dec 20, 2021

perrotta commented Dec 21, 2021

tvami commented Dec 8, 2021 •

edited