relval update to fix hlt in 2017 wfs #16881

hengne · 2016-12-06T10:30:17Z

relval update to fix hlt in 2017 wfs :
add customization function as the default, to customize the hlt modules using the 2017 pixel geometry
add one fullsim pu workflow ttbarlepton for hlt validation
update gen-sim input string for 2017 workflows

cmsbuild · 2016-12-06T10:30:35Z

A new Pull Request was created by @hengne (Hengne Li) for CMSSW_9_0_X.

It involves the following packages:

Configuration/PyReleaseValidation

@cmsbuild, @srimanob, @davidlange6, @hengne, @fabozzi can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @ghellwig, @Martin-Grunewald this is something you requested to watch as well.
@slava77, @smuzaffar you are the release manager for this.

cms-bot commands are listed here #13028

hengne · 2016-12-06T10:37:51Z

please test

cmsbuild · 2016-12-06T10:38:07Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/16800/console

cmsbuild · 2016-12-06T11:51:13Z

-1

Tested at: 9d0f562

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16881/16800/summary.html

I found follow errors while testing this PR

Failed tests: RelVals

RelVals:

When I ran the RelVals I found an error in the following worklfows:
10021.0 step1

runTheMatrix-results/10021.0_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step1_TenMuE_0_200+TenMuE_0_200_pythia8_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log

10024.0 step1

runTheMatrix-results/10024.0_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017/step1_TTbar_13+TTbar_13TeV_TuneCUETP8M1_2017_GenSimFull+DigiFull_2017+RecoFull_2017+ALCAFull_2017+HARVESTFull_2017.log

cmsbuild · 2016-12-06T11:51:15Z

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

hengne · 2016-12-06T12:13:40Z

please test with #16867

makortel · 2016-12-06T12:17:09Z

Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py

@@ -87,13 +87,15 @@
        'Geom' : 'DB:Extended',
        'GT' : 'auto:phase1_2017_realistic',
        'HLTmenu': '@relval2016',
+        'Custom': 'HLTrigger/Configuration/customizeHLTTrackingForPhaseI2017.customizeHLTPhaseIPixelGeom',


AFAICT the customize gets now called for each of the workflow steps (not just HLT), but in practice the customize should have no effect on the other steps (in RECO ClusterShapeHitFilterESProducer PixelShapeFile already points to the "new" shape file).

Thought to note anyway.

one way is to make it a list or dictionary for each step in 'ScenToRun', but this will make the upgrade framework more complicated..
another is just to make sure the customization function only modify hlt modules, no-touch of the reco, for ClusterShapeHitFilterESProducer it is actually also modified the reco modules, just that as you mentioned it was already changed to phase1 through eras modifiers in reco, thus nothing is really changed. should actually make a selection based on the "hlt" label to only touch the hlt modules as the way this function modifies the other parameter.

but i think the final - best solution is really to use eras modifiers in the hlt menu configuration file...

davidlange6 · 2016-12-06T12:28:59Z

On Dec 6, 2016, at 6:17 AM, Matti Kortelainen ***@***.***> wrote: @makortel commented on this pull request. In Configuration/PyReleaseValidation/python/upgradeWorkflowComponents.py: > @@ -87,13 +87,15 @@ 'Geom' : 'DB:Extended', 'GT' : 'auto:phase1_2017_realistic', 'HLTmenu': ***@***.***', + 'Custom': 'HLTrigger/Configuration/customizeHLTTrackingForPhaseI2017.customizeHLTPhaseIPixelGeom', AFAICT the customize gets now called for each of the workflow steps (not just HLT), but in practice the customize should have no effect on the other steps (in RECO ClusterShapeHitFilterESProducer PixelShapeFile already points to the "new" shape file). Thought to note anyway.

Right - but only because we insisted that the customize be implemented that way during the code review.. as for this request - the customize should just get called within the HLT (which already has such an infrastructure and not here. Apparently the goals have changed since the PR for the customization was made, as at that time it was first an alternative config meant to be cross validated and then made the default...

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

makortel · 2016-12-06T12:36:57Z

Adding @mtosi

hengne · 2016-12-06T13:01:27Z

it would be anyway best to implement this default in the hlt customization function, instead of being called here. seems to me it is possible to be implemented in the hlt GRun config file. no?

hengne · 2016-12-06T13:18:06Z

some thing like, at the end of
HLTrigger/Configuration/python/HLT_GRun_cff.py

add:

from Configuration.Eras.Modifier_phase1Pixel_cff import phase1Pixel

for esproducer in esproducers_by_type(fragment,"ClusterShapeHitFilterESProducer"):
    phase1Pixel.toModify(esproducer,  PixelShapeFile = 'RecoPixelVertexing/PixelLowPtUtilities/data/pixelShape_Phase1TkNewFPix.par'  )

for producer in producers_by_type(fragment,"SiPixelRawToDigi"):
      phase1Pixel.toModify(producer, UsePhase1=True)

mtosi · 2016-12-07T17:04:06Z

I would have done the same, indeed
but the bosses are @Martin-Grunewald @silviodonato ...

silviodonato · 2016-12-09T10:03:30Z

Are you suggesting to create a new HLT_GRunFor2017 that is basically HLT_GRun_cff + 2017 customizer ? In this way, you want to call directly the HLT_GRunFor2017 in 2017 relvals, is it correct?

hengne · 2016-12-09T12:42:00Z

please test

cmsbuild · 2016-12-09T16:09:01Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_9_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @smuzaffar

hengne · 2016-12-11T19:00:39Z

@davidlange6 if this hlt default implementation with eras in HLTrigger/Configuration still takes time, this PR need to be merged to temporarily make the workflows works (reasonable physics results) for pre2. although it should be applied in only the HLT/DIGI step, but the functionality to split steps for customization functions in the upgrade framework is still not yet implemented. considering this is only a temporary fix for the hlt default, and also considering the customization function will indeed not changing anything else than the hlt modules, seems to me this fix is an economic solution temporarily. I will remove it once the default is implemented with eras in HLTrigger packages.

hengne · 2016-12-11T19:01:13Z

the above comments also applies to the 81x PR #16882

davidlange6 · 2016-12-12T15:38:41Z

Hi - @hengne @silviodonato - what is the down side to having a separate menu as @silviodonato suggests? Seems like the robust solution..

Martin-Grunewald · 2016-12-12T15:41:38Z

It needs to be maintained!

davidlange6 · 2016-12-12T15:44:19Z

err - yes? we are planning to take data with this menu next year, right? I take it the HLT plan for 2016->17 differs from that of 2015->16. Is there a summary I can use as a guidance for whats to come?

…

On Dec 12, 2016, at 4:41 PM, Martin Grunewald ***@***.***> wrote: It needs to be maintained! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Martin-Grunewald · 2016-12-12T16:07:11Z

Well, David, obviously no menu we have right now is the one we use in 2017.
This whole saga is just to use a current menu with some specific 2017 approximation.
If customisations for cmsDriver are no longer allowed, then lets drop these wflows
or make the matrix more flexible. The solution can not be to duplicate a menu and
then make a small customisation on the copy.

For what concerns the plan, please see the 1st/2nd page of
https://docs.google.com/document/d/1nbaiz_SvnqBs4UTKBZHrTMNYv9aNxU-3rAYu_QaucKo/edit?usp=sharing

In the long run the customisation will no longer be needed as it will be part of the
(real 2017) menus.

I add @fwyzard for more details on the plan...

davidlange6 · 2016-12-12T16:15:14Z

ok, so yes, the plan is different than 2015-2016. Thanks. I would suggest putting your customize function into a new “menu” that simply wraps the 2016 one. that is by definition the same maintenance burden as a customize function.

…

On Dec 12, 2016, at 5:07 PM, Martin Grunewald ***@***.***> wrote: Well, David, obviously no menu we have right now is the one we use in 2017. This whole saga is just to use a current menu with some specific 2017 approximation. If customisations for cmsDriver are no longer allowed, then lets drop these wflows or make the matrix more flexible. The solution can not be to duplicate a menu and then make a small customisation on the copy. For what concerns the plan, please see the 1st/2nd page of https://docs.google.com/document/d/1nbaiz_SvnqBs4UTKBZHrTMNYv9aNxU-3rAYu_QaucKo/edit?usp=sharing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Martin-Grunewald · 2016-12-12T16:20:30Z

Sorry, I do not see the point to add a menu which is the same as some other menu and adds some custom. function.

davidlange6 · 2016-12-12T16:24:27Z

you presumably want two different configurations (with and without the proposed customize) to be runnable or no?

…

On Dec 12, 2016, at 5:20 PM, Martin Grunewald ***@***.***> wrote: Sorry, I do not see the point to add a menu which is the same as some other menu and adds some custom. function. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

Martin-Grunewald · 2016-12-12T16:28:51Z

Yes - 2016 wflows with 2016 menus, 2017 wflows with 2016 menus plus custom - for now.
In the future, 2016 wflows (if still needed) with "fake" menus, and 2017 wflows with 2017 menus.
This PR does that. What is the problem?

fwyzard · 2016-12-12T16:31:39Z

ok, github keeps dropping me out of the thread, so I'll have to go through it all...

in the meantime, are you suggesting to include a new file, e.g.
HLTrigger/Configuration/python/HLT_GRun_2017_cff.py
which includes something line

from HLTrigger.Configuration.HLT_GRun_cff import fragment
from HLTrigger.Configuration.customizeHLTTrackingForPhaseI2017 import customizeHLTPhaseIPixelGeom, customizeHLTForPFTrackingPhaseI2017

fragment = customizeHLTPhaseIPixelGeom(fragment)
fragment = customizeHLTForPFTrackingPhaseI2017(fragment)

?

The objection I have to this approach is that we try to maintain multiple triggers

one or to frozen menus
the GRun menu with the ongoing HLT development
the PbPb, p-Pb, and pp reference menus
etc
meaning we would need a similar file for each of them.

And, we should start putting together a menu for Phase 2, with again a similar set of customisations.
I would like to avoid keeping around 3 sets of 6 almost identical customisation files.

About Eras: I've been working on it last Friday and during the weekend, but the problem is that the current implementation of Eras is honestly quite crappy.
To me it would make much more sense if Eras were attached to the objects, not applied immediately.
More importantly, I need a consistent syntax for adding new objects that works with a full process and with a cff fragment.

davidlange6 · 2016-12-12T16:31:59Z

Right, so the point is you have two configs you want to use. there are two non-hacky approaches. A new menu (which can be a few lines btw..), or eras. Both can do what you need

…

On Dec 12, 2016, at 5:28 PM, Martin Grunewald ***@***.***> wrote: Yes - 2016 wflows with 2016 menus, 2017 wflows with 2016 menus plus custom - for now. In the future, 2016 wflows (if still needed) with "fake" menus, and 2017 wflows with 2017 menus. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

fwyzard · 2016-12-12T16:33:15Z

Except that Eras cannot.

davidlange6 · 2016-12-12T16:40:20Z

I guess I missed what they can not do as it would be use case not brought up in the three years since proposed - I’ll go back and find that thread - anyway, that leaves another option that lets the hlt control and maintain hlt configs in its area (which I think we can agree is the most appropriate solution…)

…

On Dec 12, 2016, at 5:33 PM, Andrea Bocci ***@***.***> wrote: Except that Eras cannot. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

fwyzard · 2016-12-12T17:11:38Z

if you can tell me how to add a module to a cff file within an era, I'll be happy to use it to era-ify the HLT customisation.

davidlange6 · 2016-12-12T17:13:30Z

I guess you mean to add a module to a sequence in an era? On Dec 12, 2016, at 6:11 PM, Andrea Bocci <notifications@github.com<mailto:notifications@github.com>> wrote: if you can tell me how to add a module to a cff file within an era, I'll be happy to use it to era-ify the HLT customisation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#16881 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AEzyw21NKZadVWDriBDovS_t-yJ1wqp2ks5rHYBKgaJpZM4LFP-Y>.

fwyzard · 2016-12-12T17:16:58Z

Actually, I meant how to add a completely new module to the Process.

I do also need to add it to a Sequence, but I think that I can implement that replacing the sequence with a modified copy of itself.

fwyzard · 2016-12-12T19:01:44Z

so, it looks like I found a way to do this with Eras, though I might be abusing the mechanism...

basically, you can do

eras.myEra.toModify(process, myCustomisationFunction)

to wrap the existing customisation functions inside myEra

In #16978 I've simply wrapped the two customise calls.
In #16977 I've tried to do it with a much finer granularity, but I don't know if that's actually better - it does look more confusing to me.

fwyzard · 2016-12-12T19:03:45Z

with both approaches

import FWCore.ParameterSet.Config as cms
from Configuration.StandardSequences.Eras import eras

process = cms.Process( "HLT", eras.Run2_2017 )
process.load( 'HLTrigger.Configuration.HLT_GRun_cff' )

gives the same python configuration as

process = cms.Process( "HLT" )
process.load( 'HLTrigger.Configuration.HLT_GRun_cff' )

from HLTrigger.Configuration.customizeHLTTrackingForPhaseI2017 import customizeHLTPhaseIPixelGeom
process = customizeHLTPhaseIPixelGeom(process)

from HLTrigger.Configuration.customizeHLTTrackingForPhaseI2017 import customizeHLTForPFTrackingPhaseI2017
process = customizeHLTForPFTrackingPhaseI2017(process)

Martin-Grunewald · 2016-12-12T19:32:08Z

Isn't that then always applied, regardless of which cmsDriver cmdline Era is chosen?

fwyzard · 2016-12-12T22:07:15Z

no, what I meant is that the customisation is applied only if you do

process = cms.Process( "HLT", eras.Run2_2017 )

but not if you do

process = cms.Process( "HLT" )

Martin-Grunewald · 2016-12-13T07:01:45Z

Hmm, but where is the check on 2017 era - as opposed to 2016 era?
Ie, wouldn't the custom be applied also if you do process = cms.Process( "HLT", eras.Run2_2016 )?
(2016 instead 2017)? IOW, where is the check or the connection to the 2017 era rather than to another era?

fwyzard · 2016-12-13T07:22:32Z

The customisations are attached to the trackingPhase1 era:

    # modify the HLT configuration for the Phase I pixel geometry
    from HLTrigger.Configuration.customizeHLTTrackingForPhaseI2017 import customizeHLTPhaseIPixelGeom
    eras.trackingPhase1.toModify(fragment, customizeHLTPhaseIPixelGeom)

etc.
trackingPhase1 is included in the Run2_2017 era (see Configuration/Eras/python/Era_Run2_2017_cff.py ), and not in the Run2_2016 era (see Configuration/Eras/python/Era_Run2_2016_cff.py )

davidlange6 · 2017-01-04T07:43:00Z

Hi @hengne -this is now obsolete I think, is that correct?

silviodonato · 2017-01-05T14:01:03Z

Let me add a reference to #17042 , ie. PR in 90X that implemented Eras at HLT.
@hengne could you update this PR to use Eras instead of customization function?

hengne added 3 commits December 6, 2016 10:33

add default customization function for 2017

9d03261

add ttbarlepton fullsim pu workflow in 2016 framework

2809818

gensim input string update

9d0f562

cmsbuild added this to the Next CMSSW_9_0_X milestone Dec 6, 2016

cmsbuild added comparison-pending orp-pending pdmv-pending pending-signatures tests-pending labels Dec 6, 2016

cmsbuild added tests-started and removed tests-pending labels Dec 6, 2016

cmsbuild added comparison-notrun tests-rejected and removed comparison-pending tests-started labels Dec 6, 2016

makortel reviewed Dec 6, 2016

View reviewed changes

cmsbuild removed the comparison-notrun label Dec 9, 2016

cmsbuild added the pdmv-approved label Dec 9, 2016

hengne mentioned this pull request Dec 11, 2016

From cmssw 9 0 0 pre1 relval fix job splitting #16969

Closed

hengne mentioned this pull request Dec 13, 2016

seperate custom for gensim, digi, reco, alca, harvest in upgrade framework #17003

Closed

davidlange6 closed this Jan 14, 2017

relval update to fix hlt in 2017 wfs #16881

relval update to fix hlt in 2017 wfs #16881

Conversation

hengne commented Dec 6, 2016

cmsbuild commented Dec 6, 2016

hengne commented Dec 6, 2016

cmsbuild commented Dec 6, 2016 • edited

cmsbuild commented Dec 6, 2016

cmsbuild commented Dec 6, 2016

hengne commented Dec 6, 2016

makortel Dec 6, 2016

Choose a reason for hiding this comment

hengne Dec 6, 2016 • edited

Choose a reason for hiding this comment

davidlange6 commented Dec 6, 2016 via email

makortel commented Dec 6, 2016

hengne commented Dec 6, 2016

hengne commented Dec 6, 2016

mtosi commented Dec 7, 2016

silviodonato commented Dec 9, 2016

hengne commented Dec 9, 2016

cmsbuild commented Dec 9, 2016

hengne commented Dec 11, 2016

hengne commented Dec 11, 2016

davidlange6 commented Dec 12, 2016

Martin-Grunewald commented Dec 12, 2016

davidlange6 commented Dec 12, 2016 via email

Martin-Grunewald commented Dec 12, 2016 • edited

davidlange6 commented Dec 12, 2016 via email

Martin-Grunewald commented Dec 12, 2016

davidlange6 commented Dec 12, 2016 via email

Martin-Grunewald commented Dec 12, 2016 • edited

fwyzard commented Dec 12, 2016

davidlange6 commented Dec 12, 2016 via email

fwyzard commented Dec 12, 2016

davidlange6 commented Dec 12, 2016 via email

fwyzard commented Dec 12, 2016

davidlange6 commented Dec 12, 2016 via email

fwyzard commented Dec 12, 2016 • edited

fwyzard commented Dec 12, 2016

fwyzard commented Dec 12, 2016

Martin-Grunewald commented Dec 12, 2016

fwyzard commented Dec 12, 2016

Martin-Grunewald commented Dec 13, 2016

fwyzard commented Dec 13, 2016

davidlange6 commented Jan 4, 2017

silviodonato commented Jan 5, 2017

cmsbuild commented Dec 6, 2016 •

edited

hengne Dec 6, 2016 •

edited

Martin-Grunewald commented Dec 12, 2016 •

edited

Martin-Grunewald commented Dec 12, 2016 •

edited

fwyzard commented Dec 12, 2016 •

edited