Add premixing workflows for phase2 #22375
Conversation
Not enabled for IBs yet, as there is no attempt to have them working yet.
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22375/3589 |
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: Configuration/PyReleaseValidation @GurpreetSinghChahal, @cmsbuild, @prebello, @kpedro88, @fabozzi can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready @slava77 comparisons for the following workflows were not done due to missing matrix map:
Comparison Summary:
|
| '--eventcontent': 'PREMIX', | ||
| '--procModifiers': 'premix_stage1', | ||
| }, | ||
| upgradeStepDict[stepName][k]]) |
There was a problem hiding this comment.
this needs to be upgradeStepDict[stepNamePmx][k]] or else you get heCollapse flags
There was a problem hiding this comment.
this needs to be upgradeStepDict[stepNamePmx][k]]
Umm, no. At this stage upgradeStepDict[stepNamePmx][k] is empty (and is actually set on the next line). The point here is to take the baseline PU workflow, and replace these parameters for premixing.
or else you get heCollapse flags
My understanding is that merge gives priority on the first elements of the argument list
cmssw/Configuration/PyReleaseValidation/python/MatrixUtil.py
Lines 192 to 193 in 45370be
so actually the
hecollapse would be replaced with premix_stage1.
But this raises a point that for some parameters (procModifiers, customise, maybe also era) sometimes one wants to concatenate the arguments rather than replace them. For customise there are already copy-pasted pattern of "check manually if it exists, then manually append, else set", but given that the need for appending starts to spread, maybe it would be good to think more "central" support for that in merge? I can also try to think of something.
There was a problem hiding this comment.
I think there are actually two problems:
- The variable
stepNameis not defined anywhere in this loop, so it picks up the last definition, which is in theheCollapsemodification. Indeed, this shouldn't bestepNamePmx(I looked too quickly), but needs to be something. - The
heCollapsemodifications use--procModifierinstead of--procModifiers; this is a typo that should be fixed.
The idea about concatenating in merge is a good one, but separate from the problem I observed here.
There was a problem hiding this comment.
- The variable stepName is not defined anywhere in this loop, so it picks up the last definition, which is in the heCollapse modification. Indeed, this shouldn't be stepNamePmx (I looked too quickly), but needs to be something.
Good catch (what was I thinking...). The stepName here should be step + 'PU' + upgradeSteps[stepType]['suffix'], but now I have to re-think why did I place this piece here instead of being below before the line 2497 (where the lines 2493-2494 set the stepName for this piece).
- The heCollapse modifications use --procModifier instead of --procModifiers; this is a typo that should be fixed.
I agree, see #22437.
There was a problem hiding this comment.
@kpedro88 For 1, I added the definition of stepName before its use and did some minor related cleanup. The generated configuration looks fine now.
There was a problem hiding this comment.
I created an issue #22492 to remind the concatenation option to merge().
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22375/3725 |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
cmsbuild
added
fully-signed
pdmv-approved
and removed
pdmv-pending
pending-signatures
labels
|
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
This PR adds premixing workflows for phase2 as variations of the PU workflows as discussed in Feb 6th simulation meeting
https://indico.cern.ch/event/702324/contributions/2885358/attachments/1596021/2527902/slides_mk_simulation_20180206.pdf
.98and it is added only for theNuGunfragment.99and it is, for now, added only for theTTbar_14TeVfragmentThe workflows are close copies from the run2 premixing workflows, so supposedly they do not run (a bit accidentally
.98does not crash if one gives it privately generatedMinBiasevents as input). The main benefit is to be able to generate the workflow configurations.Tested in CMSSW_10_1_X_2018-02-26-2300, no changes expected in standard workflows.
@mdhildreth @kpedro88