CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

Glitchmin · 2024-02-13T16:04:08Z

this PR updates HLTPPSJetComparisonFilter to be able to use both old LHCInfo (for Run 2 & Direct Simulation calculations) and new LHCInfoPer* records (Run 3).
record types. The update is a follow-up to #42515 and #42890

cmsbuild · 2024-02-13T16:04:35Z

cms-bot internal usage

cmsbuild · 2024-02-13T16:10:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43951/38828

This PR adds an extra 16KB to repository

cmsbuild · 2024-02-13T16:10:44Z

A new Pull Request was created by @Glitchmin (Adam Kulczycki) for master.

It involves the following packages:

HLTrigger/special (hlt)

@Martin-Grunewald, @cmsbuild, @mmusich can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @silviodonato this is something you requested to watch as well.
@rappoccio, @sextonkennedy, @antoniovilela you are the release manager for this.

cms-bot commands are listed here

mmusich · 2024-02-13T16:10:46Z

HLTrigger/special/python/hltppsJetComparisonFilter_cfi.py

+run3_common.toModify(HLTPPSJetComparisonFilter, useNewLHCInfo = True)
+
+from Configuration.Eras.Modifier_ctpps_directSim_cff import ctpps_directSim
+ctpps_directSim.toModify(HLTPPSJetComparisonFilter, useNewLHCInfo = False)


what's the purpose of this file?

To set useLHCInfo to true but only for Run3 not-DirectSimulation runs

sorry, where is this file referenced elsewhere in cmssw?

I don't know, but if hltppsJetComparisonFilter_cfi.py is used anywhere in the code the one I created should override it

mmusich · 2024-02-13T16:12:11Z

HLTrigger/special/plugins/HLTPPSJetComparisonFilter.cc

+  desc.add<std::string>("lhcInfoLabel", "")->setComment("label used for LHCInfo");
+  desc.add<std::string>("lhcInfoPerLSLabel", "")->setComment("label of the LHCInfoPerLS record");
+  desc.add<std::string>("lhcInfoPerFillLabel", "")->setComment("label of the LHCInfoPerFill record");
+  desc.add<bool>("useNewLHCInfo", false)->setComment("flag whether to use new LHCInfoPer* records or old LHCInfo");


as this module is not already in use, why not setting the default to true (as meant for production in 2024)?

is the DirectSimulation exercised anywhere? I think you should put there the false, via modifier.

This code will never be used with Run 2 data, right?

I don't see how it can be used for Run2 data, given that the HLT for past data has already run.
The only remote use case I can see is if we want to use this filter in Run2 HLT simulation, but it makes little sense to me since the trigger using it was not active during data-taking.

Martin-Grunewald · 2024-02-13T16:20:09Z

HLTrigger/special/plugins/HLTPPSJetComparisonFilter.cc

@@ -83,7 +86,7 @@ void HLTPPSJetComparisonFilter::fillDescriptions(edm::ConfigurationDescriptions
  desc.add<bool>("do_xi", true)->setComment("flag to require xi matching");
  desc.add<bool>("do_my", false)->setComment("flag to require m,y matching");

-  descriptions.addWithDefaultLabel(desc);
+  descriptions.add("hltppsJetComparisonFilterDefault", desc);


Revert, addWithDefaultLabel is preferred.

Actually, it's not the same. With .add I'm creating a file hltppsJetComparisonFilterDefault_cfi.py, and addWithDefaultLabel would create a file hltppsJetComparisonFilter_cfi.py. This is needed so that hltppsJetComparisonFilter_cfi.py I added in this could override and import params from hltppsJetComparisonFilterDefault_cfi.py.

Reverted cause overriding hltppsJetComparisonFilter_cfi.py turned out to be unnecessary

mmusich · 2024-02-13T16:28:47Z

HLTrigger/special/python/hltppsJetComparisonFilter_cfi.py

@@ -0,0 +1,8 @@
+from Validation.CTPPS.hltppsJetComparisonFilterDefault_cfi import HLTPPSJetComparisonFilterDefault as _HLTPPSJetComparisonFilterDefault


in any case from Validation.CTPPS.... doens't look right if you are planning to making it derive from the fillDescriptions.

You're right, fixing it rn

this file has no reason to be, please remove it.

Glitchmin · 2024-02-13T16:42:41Z

Ok, I didn't know this filter would use only Run 3 data. Therefore useNewLHCInfo can be set only to true and the _cfi file is not needed

cmsbuild · 2024-02-13T16:46:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43951/38831

This PR adds an extra 20KB to repository

cmsbuild · 2024-02-13T16:46:54Z

Pull request #43951 was updated. @cmsbuild, @mmusich, @Martin-Grunewald can you please check and sign again.

Glitchmin · 2024-02-13T16:51:48Z

Please look at the updates again. This time I updated the module so that it only uses new LHCInfoPer* records

mmusich · 2024-02-13T16:52:37Z

Please look at the updates again. This time I updated the module so that it only uses new LHCInfoPer* records

looks good. Will run tests here and also TSG integration tests.

mmusich · 2024-02-13T16:58:13Z

@cmsbuild, please test

Martin-Grunewald · 2024-02-13T17:24:35Z

Please prepare a backport PR for 14_0.

mmusich · 2024-02-13T18:09:03Z

type ctpps

mmusich · 2024-02-13T18:12:28Z

Please prepare a backport PR for 14_0.

@Glitchmin, to speed up integration (deadline tomorrow) I took the liberty of opening #43954

mmusich · 2024-02-13T18:17:08Z

This PR has been tested successfully with the trigger path proposal at https://its.cern.ch/jira/browse/CMSHLT-3030 with:

cmsrel CMSSW_14_0_0_pre3
cd CMSSW_14_0_0_pre3/src/
cmsenv
git cms-addpkg HLTrigger/special
git remote add CTPPS git@github.com:CTPPS/cmssw.git
git fetch CTPPS
git cherry-pick 6e4e6b85c48d7734d7b25788980243e38b6424b8
scram b -j 20
hltIntegrationTests /users/musich/tests/dev/CMSSW_14_0_0/CMSHLT-3030/HLT/V2 --setup /users/musich/tests/dev/CMSSW_14_0_0/CMSHLT-3030/GRun/V1 -n -1 --input /store/data/Run2023D/EphemeralHLTPhysics0/RAW/v1/000/370/293/00000/2ef73d2a-1fb7-4dac-9961-149525f9e887.root -x "--globaltag 140X_dataRun3_HLT_Candidate_2024_02_12_21_27_47" -x "--no-output" -x "--eras Run3 --l1-emu uGT --l1 L1Menu_Collisions2023_v1_3_0_xml" -d hltIntegTest4

without this PR the test failed.

Glitchmin · 2024-02-13T18:49:28Z

Please prepare a backport PR for 14_0.

@Glitchmin, to speed up integration (deadline tomorrow) I took the liberty of opening #43954

Ok, thank you very much. Is there something more that needs to be done before the deadline?

mmusich · 2024-02-13T19:24:00Z

Is there something more that needs to be done before the deadline?

I think we should be OK with this for the time being.

cmsbuild · 2024-02-13T23:36:33Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-74b7bd/37429/summary.html
COMMIT: 6e4e6b8
CMSSW: CMSSW_14_1_X_2024-02-13-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/43951/37429/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 13 lines to the logs
Reco comparison results: 60 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3248554
DQMHistoTests: Total failures: 1192
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3247340
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 200 log files, 161 edm output root files, 48 DQM output files
TriggerResults: no differences found

mmusich · 2024-02-14T06:36:50Z

+hlt

cmsbuild · 2024-02-14T06:37:11Z

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. @sextonkennedy, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

antoniovilela · 2024-02-14T11:46:15Z

+1

cmsbuild added this to the CMSSW_14_1_X milestone Feb 13, 2024

cmsbuild added hlt-pending pending-signatures tests-pending orp-pending code-checks-pending labels Feb 13, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 13, 2024

mmusich reviewed Feb 13, 2024

View reviewed changes

Martin-Grunewald reviewed Feb 13, 2024

View reviewed changes

mmusich reviewed Feb 13, 2024

View reviewed changes

Glitchmin force-pushed the ak_update_HLTPPSJetComparisonFilter branch from a85e180 to ede3e41 Compare February 13, 2024 16:37

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 13, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Feb 13, 2024

Glitchmin changed the title ~~CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombiend~~ CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined Feb 13, 2024

update HLTPPSJetComparisonFilter to use LHCInfoCombined

6e4e6b8

Glitchmin force-pushed the ak_update_HLTPPSJetComparisonFilter branch from ede3e41 to 6e4e6b8 Compare February 13, 2024 16:49

cmsbuild added code-checks-pending and removed code-checks-approved labels Feb 13, 2024

cmsbuild added tests-started and removed tests-pending labels Feb 13, 2024

cmsbuild added the ctpps label Feb 13, 2024

mmusich mentioned this pull request Feb 13, 2024

[14.0.X] update HLTPPSJetComparisonFilter to use LHCInfoCombined #43954

Merged

francescobrivio mentioned this pull request Feb 13, 2024

Remove "old" LHCInfo tag from Run3 GTs cms-AlCaDB/AlCaTools#87

Open

francescobrivio mentioned this pull request Feb 13, 2024

Update LHCInfoProducer to use LHCInfoCombined #43956

Merged

cmsbuild added tests-approved and removed tests-started labels Feb 13, 2024

cmsbuild added hlt-approved fully-signed and removed hlt-pending pending-signatures labels Feb 14, 2024

cmsbuild added orp-approved and removed orp-pending labels Feb 14, 2024

cmsbuild merged commit 6ab9e7d into cms-sw:master Feb 14, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

Glitchmin commented Feb 13, 2024

cmsbuild commented Feb 13, 2024 •

edited

Loading

cmsbuild commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

mmusich Feb 13, 2024

Glitchmin Feb 13, 2024

mmusich Feb 13, 2024

Glitchmin Feb 13, 2024

mmusich Feb 13, 2024

mmusich Feb 13, 2024 •

edited

Loading

Glitchmin Feb 13, 2024

mmusich Feb 13, 2024

Martin-Grunewald Feb 13, 2024

Glitchmin Feb 13, 2024

Glitchmin Feb 13, 2024

mmusich Feb 13, 2024

Glitchmin Feb 13, 2024

mmusich Feb 13, 2024

Glitchmin commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

Glitchmin commented Feb 13, 2024

mmusich commented Feb 13, 2024

mmusich commented Feb 13, 2024

Martin-Grunewald commented Feb 13, 2024

mmusich commented Feb 13, 2024

mmusich commented Feb 13, 2024

mmusich commented Feb 13, 2024 •

edited

Loading

Glitchmin commented Feb 13, 2024

mmusich commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

mmusich commented Feb 14, 2024

cmsbuild commented Feb 14, 2024

antoniovilela commented Feb 14, 2024

		@@ -0,0 +1,8 @@
		from Validation.CTPPS.hltppsJetComparisonFilterDefault_cfi import HLTPPSJetComparisonFilterDefault as _HLTPPSJetComparisonFilterDefault

CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

CTPPS update HLTPPSJetComparisonFilter to use LHCInfoCombined #43951

Conversation

Glitchmin commented Feb 13, 2024

cmsbuild commented Feb 13, 2024 • edited Loading

cmsbuild commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glitchmin commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

Glitchmin commented Feb 13, 2024

mmusich commented Feb 13, 2024

mmusich commented Feb 13, 2024

Martin-Grunewald commented Feb 13, 2024

mmusich commented Feb 13, 2024

mmusich commented Feb 13, 2024

mmusich commented Feb 13, 2024 • edited Loading

Glitchmin commented Feb 13, 2024

mmusich commented Feb 13, 2024

cmsbuild commented Feb 13, 2024

Comparison Summary

mmusich commented Feb 14, 2024

cmsbuild commented Feb 14, 2024

antoniovilela commented Feb 14, 2024

cmsbuild commented Feb 13, 2024 •

edited

Loading

mmusich Feb 13, 2024 •

edited

Loading

mmusich commented Feb 13, 2024 •

edited

Loading