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

MTD geometry: remove obsolete geometry scenario D73, superseded by D75 and D76 #32886

Merged
merged 2 commits into from Feb 14, 2021

Conversation

fabiocos
Copy link
Contributor

@fabiocos fabiocos commented Feb 12, 2021

PR description:

This PR is a follow-up of #32840 and addresses the issue #32882 . The scenario D73 has been effectively replaced by scenarios D75 and D76 (Identical in terms of MTD content), and it has no more reason to exist, as it was made just for ETL v5 integration. The update of this geometry causes D73 to fail, but there is no benefit in fixing this, as its removal was already scheduled in discussions with the Upgrade software coordinator.

PR validation:

All the instances of the D73 string referring to the scenario in LXR look to have been addressed, and

10:38 cmsdev25 5309> runTheMatrix.py -n | grep D73 | wc -l
0
10:38 cmsdev25 5310> runTheMatrix.py -n --what upgrade | grep D73 | wc -l
0

The list of existing wfs is preserved (with the second commit).

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32886/21117

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Configuration/Geometry
Configuration/PyReleaseValidation
Configuration/StandardSequences
Geometry/CMSCommonData

@civanch, @Dr15Jones, @jordan-martins, @chayanit, @cvuosalo, @wajidalikhan, @ianna, @mdhildreth, @cmsbuild, @makortel, @franzoni, @silviodonato, @kpedro88, @srimanob, @qliphy, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks.
@vargasa, @makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @Martin-Grunewald, @lecriste, @mtosi, @dgulhan, @slomeo this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@fabiocos
Copy link
Contributor Author

please test

@fabiocos
Copy link
Contributor Author

please abort

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32886/21119

@cmsbuild
Copy link
Contributor

@fabiocos
Copy link
Contributor Author

please test

@fabiocos
Copy link
Contributor Author

@davidlange6 which xmls are you referring to? The master configurations of the scenario are removed, what else am I missing?

@davidlange6
Copy link
Contributor

davidlange6 commented Feb 12, 2021 via email

@ianna
Copy link
Contributor

ianna commented Feb 12, 2021

as apparently this PR leaves behind some no longer useful xmls - what is the strategy for cleaning out xmls no longer used by any geometry?

AFAIK if the xml files that are no longer used by any scenario they are deleted. The burden is on a PR submitter and reviewer :-)

@fabiocos
Copy link
Contributor Author

@davidlange6 in the particular case of this PR for MTD there should be none, v5/etl.xml is used in the newer scenarios D75 and D76, and this was the cause of the issue reported by @makortel . Actually the removal of D73 might have happened at the same time, I did not immediately realise that fixing the geometry for the newer caloBase envelope would cause such a problem. As there was no official use of D73, which was made by me for test/development, it looked appropriate an update of the xml instead of a proliferation of temporary development versions.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4bc9a9/12854/summary.html
COMMIT: 64ecbf2
CMSSW: CMSSW_11_3_X_2021-02-11-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/32886/12854/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /data/cmsbld/jenkins/workspace/compare-root-files-short-matrix/data/PR-4bc9a9/11634.911_TTbar_14TeV+2021_DD4hep+TTbar_14TeV_TuneCP5_GenSim+Digi+Reco+HARVEST+ALCA

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2750846
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2750823
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@cvuosalo
Copy link
Contributor

+1

@srimanob
Copy link
Contributor

+Upgrade

@chayanit
Copy link

+1

@qliphy
Copy link
Contributor

qliphy commented Feb 14, 2021

+operations

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

@qliphy
Copy link
Contributor

qliphy commented Feb 14, 2021

+1

@cmsbuild cmsbuild merged commit b68775c into cms-sw:master Feb 14, 2021
@fabiocos fabiocos deleted the fc-fixETLv5 branch February 17, 2021 08:33
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

8 participants