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

Change Geometry_cff to GeometryDB_cff in AlCaDB related config files #35278

Merged
merged 2 commits into from Sep 22, 2021

Conversation

tvami
Copy link
Contributor

@tvami tvami commented Sep 15, 2021

PR description:

Taking care of the AlCa and DB part of #31113
i.e. the fact that
process.load("Configuration.StandardSequences.Geometry_cff")
was removed a while ago: #8810
and thus should be replaced with
process.load("Configuration.StandardSequences.GeometryDB_cff")

PR validation:

scramv1 b runtests though I guess this doesn't test any of these files, otherwise it would have been fixed already

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

Not a backport and no backport is needed

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35278/25275

  • This PR adds an extra 56KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Alignment/LaserAlignmentSimulation (alca)
  • Alignment/MuonAlignment (alca)
  • Alignment/TrackerAlignment (alca)
  • CalibMuon/CSCCalibration (alca)
  • CalibTracker/SiPixelConnectivity (alca)
  • CalibTracker/SiPixelLorentzAngle (alca)
  • CalibTracker/SiStripChannelGain (alca)
  • Calibration/EcalAlCaRecoProducers (alca)
  • Calibration/HcalAlCaRecoProducers (alca)
  • Calibration/HcalCalibAlgos (alca)
  • Calibration/TkAlCaRecoProducers (alca)
  • CommonTools/RecoAlgos (reconstruction)
  • CondTools/SiPixel (db)
  • CondTools/SiStrip (db)

@malbouis, @yuanchao, @cmsbuild, @jpata, @slava77, @ggovi, @francescobrivio, @tvami can you please review it and eventually sign? Thanks.
@echabert, @rappoccio, @pieterdavid, @robervalwalsh, @argiro, @bsunanda, @OzAmram, @thomreis, @tlampen, @threus, @pakhotin, @ahinzmann, @jhgoh, @jdolen, @simonepigazzini, @trocino, @abbiendi, @tocheng, @VinInn, @ferencek, @gkasieczka, @hatakeyamak, @alesaggio, @mmusich, @ptcox, @clelange, @adewit, @rchatter, @gbenelli, @dkotlins, @tvami 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 Author

tvami commented Sep 15, 2021

@cmsbuild , code checks

@tvami
Copy link
Contributor Author

tvami commented Sep 15, 2021

@cmsbuild , please test

@tvami tvami changed the title Change Geometry_cff to GeometryDB_cff Change Geometry_cff to GeometryDB_cff in AlCaDB related config files Sep 15, 2021
@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-35278/25277

  • This PR adds an extra 52KB to repository

@cmsbuild
Copy link
Contributor

Pull request #35278 was updated. @ggovi, @malbouis, @tvami, @yuanchao, @francescobrivio can you please check and sign again.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c5efc0/18615/summary.html
COMMIT: e31dff6
CMSSW: CMSSW_12_1_X_2021-09-14-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/35278/18615/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 39
  • DQMHistoTests: Total histograms compared: 3000833
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3000805
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 38 files compared)
  • Checked 165 log files, 37 edm output root files, 39 DQM output files
  • TriggerResults: no differences found

@@ -4,7 +4,7 @@
process.source = cms.Source("EmptySource")
process.maxEvents = cms.untracked.PSet(input = cms.untracked.int32(1))

process.load("Configuration.StandardSequences.Geometry_cff")
process.load("Configuration.StandardSequences.GeometryDB_cff")
Copy link
Contributor

@mmusich mmusich Sep 15, 2021

Choose a reason for hiding this comment

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

If I am not mistaken a Global Tag (or at least an ESSource providing the geometry records in the ES) is needed in order for GeometryDB_cff to work.
As far as I can see, that is not included in this file (and in many others of this PR).

@malbouis
Copy link
Contributor

+alca

Even though there seems to be missing GTs in some of the files, as @mmusich pointed out, we think this can move on as is and AlCa will be contacting the subsystems for a more a detailed check on these files and possible removal in case they are not used anymore.

@malbouis
Copy link
Contributor

+db

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

@mmusich
Copy link
Contributor

mmusich commented Sep 16, 2021

we think this can move on as is

seems you are trading a broken configuration for a differently broken one, not sure to see the advantage, also considering that some of these might have been explicitly designed in order to take the geometry from XML.

and AlCa will be contacting the subsystems for a more a detailed check on these files and possible removal in case they are not used anymore.

that seems indeed the way to go.
My 2 cents.

@malbouis
Copy link
Contributor

we think this can move on as is

seems you are trading a broken configuration for a differently broken one, not sure to see the advantage, also considering that some of these might have been explicitly designed in order to take the geometry from XML.

I see what you mean. I think there will be still a slight improvement, since some of the files that were changed might work again. The other reason for the change is that the original cff config file does not exist anymore, so it was accessing neither the DB nor XML.

I agree though it is not optimal, but still see some improvement and a reason for going forward with this now.

and AlCa will be contacting the subsystems for a more a detailed check on these files and possible removal in case they are not used anymore.

that seems indeed the way to go.
My 2 cents.

Thanks! We will follow up with that!

@perrotta
Copy link
Contributor

Even though there seems to be missing GTs in some of the files, as @mmusich pointed out, we think this can move on as is and AlCa will be contacting the subsystems for a more a detailed check on these files and possible removal in case they are not used anymore.

Given what you write, I have the impression that all these scripts are not used by now.
Instead of investing time in trying to fix them, shouldn't be more convenient to just remove them? Once fixed by adjusting the name of Geometry_cff, it will be more difficult to identify them as outdated.

@tvami
Copy link
Contributor Author

tvami commented Sep 16, 2021

Even though there seems to be missing GTs in some of the files, as @mmusich pointed out, we think this can move on as is and AlCa will be contacting the subsystems for a more a detailed check on these files and possible removal in case they are not used anymore.

Given what you write, I have the impression that all these scripts are not used by now.
Instead of investing time in trying to fix them, shouldn't be more convenient to just remove them? Once fixed by adjusting the name of Geometry_cff, it will be more difficult to identify them as outdated.

@perrotta I think our idea was to email the subsystems to have a look at these files based on the file change list in this PR
I should also add that it should be the subsystem's decision to delete these files (which are indeed not used, since the original Geometry_cff doesn't exist anymore)

@perrotta
Copy link
Contributor

@perrotta I think our idea was to email the subsystems to have a look at these files based on the file change list in this PR
I should also add that it should be the subsystem's decision to delete these files (which are indeed not used, since the original Geometry_cff doesn't exist anymore)

Sure!
I suggest to email the subsystems before merging this PR, suggesting them that the very first fix they must apply in case they want to resurrect these codes is what implemented here. Alternatively, they could delete them.
By letting the wrong include at its place for now would allow identifying more easily in the future which files haven't been updated: one could then even decide to simply forcedly remove them.

@perrotta
Copy link
Contributor

+1

  • Given the message sent by @tvami to the developers of thes code, which asks for either remove the files, if they are not used, or modify them (by adding the appropriate GT) on top of the first part of the fix implemented here, let merge this PR as is
  • To be considered an intermediate step

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

5 participants