-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
rename 2023 -> 2026, fix bug in D44 #27488
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27488/10822
|
A new Pull Request was created by @kpedro88 (Kevin Pedro) for master. It involves the following packages: CalibCalorimetry/HcalPlugins @cmsbuild, @benkrikler, @Dr15Jones, @alja, @cvuosalo, @civanch, @christopheralanwest, @ianna, @mdhildreth, @pgunnell, @rekovic, @franzoni, @tocheng, @zhenhu, @tlampen, @pohsun, @prebello, @fabiocos, @kpedro88, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpedro88 - Please, update CondTools/Geometry
package to make sure we still are capable of producing geometry payloads for both 2021 and 2023 scenarios.
'Geometry/CMSCommonData/data/cmsCalo.xml', | ||
'Geometry/CMSCommonData/data/muonBase/2023/v2/muonBase.xml', | ||
'Geometry/CMSCommonData/data/muonBase/2026/v2/muonBase.xml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpedro88 - shouldn't it be 2026/v1
? When we move forward we do not need to keep the older versions. The same applies for other files with v > 1.
For example, 2026/v1/brm.xml
is not used and should be replaced by 2026/v2/brm.xml
and the latter should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ianna this is a simple renaming of 2023 -> 2026. We are not moving forward or re-versioning anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If during renaming we can drop a file or two it would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really prefer not to mix renaming and cleanup in the same PR. That is why I separated this change from the previous Phase 2 cleanup PR. If this PR has identified some further cleanup issues in geometry, a new issue can be opened to track them.
@@ -9,11 +9,11 @@ | |||
'Geometry/CMSCommonData/data/extend/cmsextent.xml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpedro88 - I think, we should either remove or rename this scenario. It's confusing when 2015 scenario is using the 2026 description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are outdated geometry scenarios, those should be addressed in a separate issue/PR. It is beyond the scope of this PR.
@@ -16,8 +16,8 @@ | |||
'Geometry/CMSCommonData/data/muonBase/2017/v1/muonBase.xml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpedro88 - I thought we have moved 2019 scenarios to 2021?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it was copied instead of moved? In any case, see above re: scope of this PR.
@@ -16,8 +16,8 @@ | |||
'Geometry/CMSCommonData/data/muonBase/2018/v1/muonBase.xml', | |||
'Geometry/CMSCommonData/data/cmsMuon.xml', | |||
'Geometry/CMSCommonData/data/mgnt.xml', | |||
'Geometry/CMSCommonData/data/beampipe/2023/v1/beampipe.xml', | |||
'Geometry/CMSCommonData/data/cmsBeam/2023/v1/cmsBeam.xml', | |||
'Geometry/CMSCommonData/data/beampipe/2026/v1/beampipe.xml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kpedro88 - 2021 should not use future definitions. The definition of the beam pipe should be moved to 2021
. There might be more changes to the beam pipe description later this year.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, separate from the purpose of this PR, which only has to do with naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, however, this file should be renamed to
CMSCommonData/data/beampipe/2021/v1/beampipe.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a separate issue than the run3/run4 naming conflict being addressed in this PR.
-1 Update of a |
The code-checks are being triggered in jenkins. |
@ianna I propagated this change to the CondTools/Geometry files. That package also contains some files named "run3" that reference 2023. I am not sure if these files are used anymore; please advise. |
+operations the update of StandardSequences is coherent with the rest of the PR and its purpose |
+1 |
+1 |
+1 |
+1 |
+1 |
@benkrikler @rekovic please sign |
+1 |
merge |
PR description:
To avoid conflicts with new Run3 workflows, as the LHC timeline has shifted, the Phase 2 workflows and associated files are renamed from 2023 -> 2026.
The various unmaintained configs in test directories are not included in this renaming. Most of these refer to Phase 2 detector configurations that no longer exist, so it is not worth the effort to propagate the renaming. A few exceptions are made here for configs that may be used in unit tests.
A mistake from #27449 in the D44 workflow (wrong version of the forward region) is also fixed.
PR validation:
Matrix workflow 20034.0 runs successfully.
Unit tests also run successfully.