-
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
Including more general DQM compare modules for pixel tracks objects #45206
Conversation
cms-bot internal usage |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45206/40555
|
A new Pull Request was created by @borzari for master. It involves the following packages:
@makortel, @tjavaid, @Martin-Grunewald, @mmusich, @syuvivida, @cmsbuild, @fwyzard, @antoniovagnerini, @nothingface0, @jfernan2, @mandrenguyen, @rvenditti can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@@ -260,6 +260,90 @@ def checkHLTfor43774(process): | |||
print('# TSG WARNING: check value of parameter "useAbs" in',filt,'(expect True but is False)!') | |||
|
|||
return process | |||
|
|||
def customizeHLTforPR(process): |
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.
def customizeHLTforPR(process): | |
def customizeHLTfor45206(process): |
@@ -326,5 +410,6 @@ def customizeHLTforCMSSW(process, menuType="GRun"): | |||
process = checkHLTfor43774(process) | |||
process = customizeHLTfor44576(process) | |||
process = customizeHLTfor45063(process) | |||
process = customizeHLTforPR(process) |
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.
process = customizeHLTforPR(process) | |
process = customizeHLTfor45206(process) |
@@ -1,244 +0,0 @@ | |||
#include "DQMServices/Core/interface/MonitorElement.h" |
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.
@borzari, please do NOT delete the old plugins.
If you do that, when we we'll run (from the TSG side) the confDB parsing in next pre-release, the modules currently in the HLT menu will disappear, leading to have to redo the sequence by hand with the new plugins.
In the past what we did was:
- add the new plugins in the master (and backport down to the cycle used by HLT - currently CMSSW_14_0_X)
- JIRA ticket to change the sequence in ConfDB
- PR to
cmssw
to remove the old plugins in the folllowing pre-release (that could be done by TSG).
See e.g. #40646 for some past instance.
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.
@mmusich I already reverted to the previous commit when the files weren't deleted and made the changes requested in your comments above, regarding changing the name of function customizeHLTforPR
to customizeHLTfor45206
. I am also going to clarify in the description of the PR that this will be backported to 14_0_X. Thank you for the suggestions
<class name="pixelTrack::TracksHostPhase1" ClassVersion="3"> | ||
<version ClassVersion="3" checksum="794224446"/> | ||
</class> | ||
<class name="pixelTrack::TracksHostPhase1"/> |
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.
out of curiosity (even if this lies beyond the HLT proper review) why these changes to the class version are needed?
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 followed this comment #43964 (comment) from the previous PR. Maybe @makortel can provide more specific information about why this is the case now
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.
Quoting @makortel:
Given that
TrackingRecHitHostPhase1
itself is an instantiation of a class template, its version should not be tracked this way (but withCMS_CLASS_VERSION()
in the class template definition). But given that schema evolution ofPortableHostCollection
is unexplored anyway (and therefore nothing is promised), I'd suggest to remove the class version for now (until the schema evolution is properly understood)
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45206/40577
|
Pull request #45206 was updated. @mandrenguyen, @Martin-Grunewald, @fwyzard, @cmsbuild, @rvenditti, @tjavaid, @syuvivida, @mmusich, @jfernan2, @nothingface0, @makortel, @antoniovagnerini can you please check and sign again. |
test parameters:
|
@cmsbuild, please test |
the first three changes are due to new additions to the relvals (these plots were not filled before, thanks for adding them!) while the fourth seems likely to come to some tiny residual discrepancy in the gpu vs cpu tracking code. |
+1 |
+heterogeneous |
Once the PR(s) are integrated, I assume an update of https://its.cern.ch/jira/browse/CMSHLT-3147 is warranted, to get the updated path configuration. |
It will have to be a different ticket because since the persistence issue (see #44700) is not solved we still can't kick these plugins out of the menu. They will be simply replaced by this (better) version. |
+1 |
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. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR is the replacement of #43964 with essentially the same changes, except for the Alpaka vs CUDA compare modules that are not needed anymore. It still includes generalized DQM compare modules for pixel rechits, tracks and vertices, which are templated if needed for other SoA types, an HLT customize function to include the module changes and pixel digi errors monitoring (that is also included in the DQM step), and the changes requested in #43964 (comment).
PR validation:
Executed the code with wf
12834.403
to check DQM plots and everything works as expected.If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
This is not a backport. These changes will be backported to 14_0_X in a following PR.
Pinging @mmusich @makortel @fwyzard @AdrianoDee