-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
PPS diamond mapping fix for 2023 run (backport) [13_0_X] #41188
PPS diamond mapping fix for 2023 run (backport) [13_0_X] #41188
Conversation
A new Pull Request was created by @grzanka (Leszek Grzanka) for CMSSW_13_0_X. It involves the following packages:
@pmandrik, @emanueleusai, @tvami, @cmsbuild, @saumyaphor4252, @syuvivida, @rvenditti, @micsucmed, @francescobrivio can you please review it and eventually sign? Thanks. cms-bot commands are listed here
|
type ctpps |
backport of #41187 |
test parameters:
|
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-102eff/31598/summary.html Comparison SummarySummary:
|
@grzanka this seems to be touching online DQM code. Can you confirm you need this deployed in DQM machines at P5? |
@emanueleusai Yes , the bug in DQM is preventing DQM -for strips to work; also the new xml for timing detector will be important online at least we can debug the detectors. So the deployment in P5 before the first stable beam (in fact before the alignment run) would be mandatory. Thanks |
1 similar comment
@emanueleusai Yes , the bug in DQM is preventing DQM -for strips to work; also the new xml for timing detector will be important online at least we can debug the detectors. So the deployment in P5 before the first stable beam (in fact before the alignment run) would be mandatory. Thanks |
+alca |
@cmsbuild ping |
type bug-fix |
urgent
|
This needs to be tested at P5. So it cannot be approved right away. |
@emanueleusai 1) for the xml part you can test it with any recent global run done with CTPPS_TOT partition included |
Hi @emanueleusai, can the tests be done in parallel with the merge? We need to cut 13_0_2 ASAP (tomorrow would be ideal). |
alright |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_13_0_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_13_1_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1
|
... and with a fillDescriptions method in the TotemRPDQMSource plugin the bug would have been detected immediately, without even the need to fix it afterwards: please think about implementing it @grzanka (or someone else from @cms-sw/ctpps-dpg-l2) |
Hello, I was about to test this PR in online playback DQM machines at P5, but there was a compilation error when adding the PR to 13_0_0 + PR's: 40920+41002+41049. I tried to compile without this PR and there is no issue, so could you take a look into this? Here is an example of the errors we are getting:
|
@micsucmed this PR has evidently nothing to do with the issue you are encountering in the online DQM runs |
Hello, just to note that our deployment by default is using To avoid human-prone errors, we would like to propose NOT to deploy this PR, but wait until CMSSW_13_0_3 is deployed (which automatically merge this PR and other PRs we already included). |
I see the point. Fine to me. Provided that 13_0_3 comes out in a few days as discussed in last (emergency) ORP meeting. |
Dear CTPPS experts (@grzanka @fabferro @vavati ), |
PR description:
Backport of #41187