-
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
ECAL - Switch off online ECAL RecHit GPU vs. CPU monitoring - 12_4_X #39395
ECAL - Switch off online ECAL RecHit GPU vs. CPU monitoring - 12_4_X #39395
Conversation
A new Pull Request was created by @thomreis (Thomas Reis) for CMSSW_12_4_X. It involves the following packages:
@emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Backport of #39393 |
enable gpu |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a86f4e/27544/summary.html GPU Comparison SummarySummary:
Comparison SummarySummary:
|
Tested this PR at the playback of P5 using collision runs 357900, 357815, and cosmic run 356932. The ecalgpu client ran at fu-c2f11-15-01.cms without errors. Plots in the DQMGUI also look fine. |
Hi @rovere, yes we would need this for data taking when we unpack the ECAL auxiliary collections with the CPU unpacker (see https://its.cern.ch/jira/browse/CMSHLT-2454). No, PR #39373 is not needed before this PR. However, once #39373 is merged lines 91-95 of this PR could be removed as well (https://github.com/cms-sw/cmssw/pull/39395/files#diff-7e9b17ce9f9a19ae82d8a705a90d5fab2e021a4f0e18f0a84d7a9875621c4224R91-R95). |
@thomreis , while I don't know whether those lines need to be removed, or it is just a "nice to have", if we want to speed up the merging, could you please give for granted that #39373 will get merged in the next few hours and remove those lines since now if needed, in particular starting from the master version of this PR? |
…ng is switched off.
Pull request #39395 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please check and sign again. |
@perrotta removing the lines is a nice to have to clean up the configuration if the RecHit monitoring is switched off. This is done now in all three PRs with the latest commit. |
please test with #39373 |
-1 Failed Tests: RelVals-INPUT RelVals-INPUTThe relvals timed out after 4 hours. GPU Comparison SummarySummary:
Comparison SummarySummary:
|
please test with #39373 |
-1 Failed Tests: UnitTests Unit TestsI found errors in the following unit tests: ---> test test-das-selected-lumis had ERRORS GPU Comparison SummarySummary:
Comparison SummarySummary:
|
please test with #39373 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-a86f4e/27613/summary.html GPU Comparison SummarySummary:
Comparison SummarySummary:
|
Just a note, the test at playback of this PR was done with also PR 39373. Results were OK. (no errors from DQM^2 and DQM Histograms of ECAL are normal.) |
+1
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_12_4_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_6_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 |
PR description:
Switches off the online ECAL RecHit GPU vs. CPU comparisons since the RecHit module is identical in both options.
Until PR #39373 is merged the collections are still consumed so the same existing RecHit collection is used for CPU and GPU inputs.Requires PR #39373 to be merged since the RecHit collection tags have been removed from the configuration.
PR validation:
Configured and ran cmsRun with the configuration but no events were processed since the source is a stream that was not available. Should be tested on the playback system as well.
Backport of #39393