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.

Sign up for GitHub

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

Jump to bottom

Replace_Geometry_cff with GeometryDB_cff in RecoVertex #38788

Merged
merged 2 commits into from Jul 29, 2022
Merged

Replace_Geometry_cff with GeometryDB_cff in RecoVertex #38788

merged 2 commits into from Jul 29, 2022

Conversation

jeongeun
Copy link
Contributor

PR description:
Review on the Reco part of #31113

process.load("Configuration.StandardSequences.Geometry_cff")
was outdated #8810
It should be replaced with
process.load("Configuration.StandardSequences.GeometryDB_cff")

In this PR, RecoVertex configuration files (3 files) are fixed.

modified:   RecoVertex/AdaptiveVertexFinder/test/sequenceTest.py
modified:   RecoVertex/BeamSpotProducer/test/alcareco.py
modified:   RecoVertex/KalmanVertexFit/test/analyzeKVF_cfg.py

PR validation:

Tested in CMSSW_12_5_X, the basic test all passed in the CMSSW PR instructions

@cmsbuild cmsbuild added this to the CMSSW_12_5_X milestone Jul 19, 2022
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38788/31141

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @jeongeun (JeongEun Lee) for master.

It involves the following packages:

  • RecoVertex/AdaptiveVertexFinder (reconstruction)
  • RecoVertex/BeamSpotProducer (reconstruction, alca)
  • RecoVertex/KalmanVertexFit (reconstruction)

@malbouis, @yuanchao, @clacaputo, @cmsbuild, @jpata, @tvami, @ChrisMisan, @francescobrivio can you please review it and eventually sign? Thanks.
@AlexDeMoor, @emilbols, @andrzejnovak, @GiacomoSguazzoni, @JanFSchulte, @tocheng, @VinInn, @JyothsnaKomaragiri, @rovere, @mmusich, @demuller, @mtosi, @dgulhan, @francescobrivio this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

tvami
tvami reviewed Jul 19, 2022
View reviewed changes
RecoVertex/BeamSpotProducer/test/alcareco.py Outdated
Comment on lines 67 to 68
process.GlobalTag.connect = "sqlite_file:/afs/cern.ch/user/f/futyand/public/globaltag/10PB_21X.db"
process.GlobalTag.globaltag = '10PB_V1::All'
#process.GlobalTag.globaltag = '10PB_V1::All'
Copy link
Contributor

@tvami tvami Jul 19, 2022 •
edited

Choose a reason for hiding this comment

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

Please remove these lines 67-68

Copy link
Contributor

Choose a reason for hiding this comment

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

please do not remove the lines, just comment them out!

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry @francescobrivio where do you see the loading the GT as a local .db file as useful comment? That shouldnt really be used, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, as discussed in #38788 (comment) we can remove alltogether this RecoVertex/BeamSpotProducer/test/alcareco.py file.

tvami
tvami reviewed Jul 19, 2022
View reviewed changes
RecoVertex/BeamSpotProducer/test/alcareco.py Outdated
@@ -65,7 +65,8 @@
#process.GlobalTag.globaltag = 'IDEAL_V9::All' # take your favourite

process.GlobalTag.connect = "sqlite_file:/afs/cern.ch/user/f/futyand/public/globaltag/10PB_21X.db"
process.GlobalTag.globaltag = '10PB_V1::All'
#process.GlobalTag.globaltag = '10PB_V1::All'
process.GlobalTag.globaltag = cms.string( autoCond[ 'phase1_2022_realistic' ] )
Copy link
Contributor

Choose a reason for hiding this comment

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

@francescobrivio is this config for MC or data, do you maybe know?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have never used it and as you can see it was last touched in 2013 😅 , but from a quick look it seems to me it can actually be used for both data or MC. So let's put a data GT (the Prompt one I'd suggest) and add a comment just afte: # pick your favourite

Copy link
Contributor

Choose a reason for hiding this comment

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

but @francescobrivio if it doesnt matter we should just go with what the PR author picked, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

All of the files should either be made unit tests or deleted..

Copy link
Contributor

Choose a reason for hiding this comment

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

ok I agree with Marco, the file RecoVertex/BeamSpotProducer/test/alcareco.py can actually be removed alltogether from the release. @jeongeun please remove it :)

@clacaputo
Copy link
Contributor

Hi @jeongeun could you please remove RecoVertex/BeamSpotProducer/test/alcareco.py as suggested here?

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38788/31287

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor

Pull request #38788 was updated. @malbouis, @yuanchao, @clacaputo, @cmsbuild, @jpata, @tvami, @ChrisMisan, @francescobrivio can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Jul 27, 2022

@cmsbuild , please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8101a2/26482/summary.html
COMMIT: 0ae735e
CMSSW: CMSSW_12_5_X_2022-07-27-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38788/26482/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: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3667670
  • DQMHistoTests: Total failures: 13
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3667634
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 210 log files, 47 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Jul 27, 2022

+alca

@clacaputo
Copy link
Contributor

+reconstruction

@cmsbuild
Copy link
Contributor

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

@qliphy
Copy link
Contributor

qliphy commented Jul 29, 2022

+1

@cmsbuild cmsbuild merged commit 230caf5 into cms-sw:master Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tvami tvami tvami left review comments

@mmusich mmusich mmusich left review comments

@francescobrivio francescobrivio francescobrivio left review comments

Assignees
No one assigned
Labels
alca-approved code-checks-approved fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_12_5_X
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jeongeun @cmsbuild @clacaputo @tvami @qliphy @mmusich @francescobrivio