[Core] Split Eras.py into multiple files

[kpedro88]

We constantly have to deal with collisions in https://github.com/cms-sw/cmssw/blob/CMSSW_8_1_X/Configuration/StandardSequences/python/Eras.py, because all the different development efforts (Run2, Phase1, Phase2) have to modify this file often.

Ideally, we could at least split the Era definitions into a few separate files to reduce pointless collisions (and reduce the volume packages selected by checkdeps when recompiling). However, this requires some redesign, since right now Eras is just a class with a bunch of members.

@davidlange6, @slava77, @Dr15Jones

[cmsbuild]

A new Issue was created by @kpedro88 Kevin Pedro.

@davidlange6, @smuzaffar, @degano, @davidlt, @Dr15Jones can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[kpedro88]

Currently, a single edit to Eras.py causes checkdeps to suggest 108 dependent packages.

Could this issue at least be assigned to someone?

@davidlange6, @smuzaffar, @degano, @davidlt, @Dr15Jones

[davidlange6]

@smuzaffar

was thinking about solutions.

One can move the ModifiedChain declarations each into a separate file.

self.Run2_25ns = cms.ModifierChain( self.run2_common, self.run2_25ns_specific, self.stage1L1Trigger )

becomes

from Configuration.Eras.EraRun2_25ns import Run2_25ns self.Run2_25ns = Run2_25ns

where EraRun2_25ns.py is just

import FWCore.ParameterSet.Config as cms from EraRun2_common import run2_common from EraRun2_25ns_specific import run2_25ns_specific from EraStage1L1Trigger import stage1L1Trigger

Run2_25ns = cms.ModifierChain( run2_common, run2_25ns_specific, stage1L1Trigger )

this is easy to implement. The question is then if I change EraRun2_25ns.py does that trigger a massive checkdeps. Shahzad can you comment?

I don’t see how to avoid that the list of all eras ends up in one spot.

On Aug 5, 2016, at 10:56 PM, Kevin Pedro notifications@github.com<mailto:notifications@github.com> wrote:

Currently, a single edit to Eras.py causes checkdeps to suggest 108 dependent packages.

Could this issue at least be assigned to someone?

@davidlange6https://github.com/davidlange6, @smuzaffarhttps://github.com/smuzaffar, @deganohttps://github.com/degano, @davidlthttps://github.com/davidlt, @Dr15Joneshttps://github.com/Dr15Jones

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/14895#issuecomment-237964098, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw2rp8FymkmJ1KucYezdymVF9wwtAks5qc6OCgaJpZM4I2k9w.

[smuzaffar]

@davidlange6, yes checkdeps is still going to checkout all these 108 package (changing EraRun2_25ns.py means Eras.py needs to be re-checked which means every thing depending on Eras.py need to be re-checked)

[davidlange6]

ok, thanks - so we need kind of the reverse - a global eras object that then gets used in many places to define the eras - let me try

[davidlange6]

thinking more I see two ways - any preferences?

1) Turn Eras.py into a purely string driven constructor - eg, we make a list of Modifiers and ModifierChains via arrays/dictionaries and then a trivial loop creates the Modifiers and ModifierChains

2) make a nested hierarchy which has a similar array/dicitionary driven structure at the top but then something more pythonic below

validEras={} d='Configuration.Eras.' validEras['run1']=d+'EraRun1' validEras['run2_25ns']=d+'EraRun2_25ns'

class Eras (object): """ Dummy container for all the cms.Modifier instances that config fragments can use to selectively configure depending on what scenario is active. """ def init(self):

    for e in validEras:
        self.e=getattr(__import__(validEras[e],globals(),locals(),[e],0),e)

eras=Eras()

followed by:

import FWCore.ParameterSet.Config as cms

from EraRun2_common import run2_common from EraRun2_25ns_specific import run2_25ns_specific from EraStage1L1Trigger import stage1L1Trigger

run2_25ns = cms.ModifierChain( run2_common, run2_25ns_specific, stage1L1Trigger )

etc etc

On Aug 8, 2016, at 1:17 PM, Malik Shahzad Muzaffar notifications@github.com<mailto:notifications@github.com> wrote:

@davidlange6https://github.com/davidlange6, yes checkdeps is still going to checkout all these 108 package (changing EraRun2_25ns.py means Eras.py needs to be re-checked which means every thing depending on Eras.py need to be re-checked)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/14895#issuecomment-238207798, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw0rrNjs8_MZAwcdYVlmcvqr5Tm-Xks5qdxBggaJpZM4I2k9w.

[Dr15Jones]

Wouldn't an easier fix be to not have the different _cff.py files import the top level Eras module and instead only import a module containing the ProcessModifier they actually use? Then the top level Eras is only used when constructing the Process and by cmsDriver.py to get the available list.

[davidlange6]

On Aug 8, 2016, at 3:59 PM, Chris Jones notifications@github.com<mailto:notifications@github.com> wrote:

Wouldn't an easier fix be to not have the different _cff.py files import the top level Eras module and instead only import a module containing the ProcessModifier they actually use? Then the top level Eras is only used when constructing the Process and by cmsDriver.py to get the available list.

its even optional to have that top level Eras file for cmsDriver - it can import the “right” one too (or not?) - maybe its not that painful to script the replacement of eras. by an import of a specific one (which still needs to be in N files or otherwise hidden from the python dependencies check)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/14895#issuecomment-238246502, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw7ZiICr3BeIV_wTdB-j00d9eg2cWks5qdzYsgaJpZM4I2k9w.

[davidlange6]

some work in this direction

https://github.com/cms-sw/cmssw/pull/15469

(and my suggested next steps)

On Aug 8, 2016, at 4:06 PM, David Lange David.Lange@cern.ch<mailto:David.Lange@cern.ch> wrote:

On Aug 8, 2016, at 3:59 PM, Chris Jones notifications@github.com<mailto:notifications@github.com> wrote:

Wouldn't an easier fix be to not have the different _cff.py files import the top level Eras module and instead only import a module containing the ProcessModifier they actually use? Then the top level Eras is only used when constructing the Process and by cmsDriver.py to get the available list.

its even optional to have that top level Eras file for cmsDriver - it can import the “right” one too (or not?) - maybe its not that painful to script the replacement of eras. by an import of a specific one (which still needs to be in N files or otherwise hidden from the python dependencies check)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/14895#issuecomment-238246502, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw7ZiICr3BeIV_wTdB-j00d9eg2cWks5qdzYsgaJpZM4I2k9w.

[kpedro88]

@davidlange6 I think #15480 is sufficient for this issue. Making all the imports more specific is a good followup, but we probably don't need to leave this open until that is done.

[kpedro88]

After #16001 and #16086, most importing of Configuration.StandardSequences.Eras is gone. cms-checkdeps in CMSSW_8_1_X_2016-10-25-1100 lists the following 9 packages:

Configuration/Applications
Configuration/DataProcessing
DQM/Integration
DQM/TrackingMonitorSource
HLTrigger/Configuration
L1Trigger/L1TNtuples
L1Trigger/TrackTrigger
RecoTracker/IterativeTracking
Validation/RecoTau

Further investigation with grep:

grep -nrF "StandardSequences.Eras" .
./Configuration/Applications/python/ConfigBuilder.py:2070:            self.pythonCfgCode += "from Configuration.StandardSequences.Eras import eras\n\n"
./Configuration/Applications/python/cmsDriverOptions.py:243:        from Configuration.StandardSequences.Eras import eras
./Configuration/Applications/scripts/cmsDriver.py:23:            from Configuration.StandardSequences.Eras import eras
./Configuration/DataProcessing/python/Impl/HeavyIonsEra_Run2_HI.py:14:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/cosmicsEra_Run2_2016.py:12:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/cosmicsEra_Run2_25ns.py:12:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/cosmicsEra_Run2_50ns.py:12:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/hcalnzsEra_Run2_2016.py:13:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/hcalnzsEra_Run2_25ns.py:13:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/ppEra_Run2_2016.py:14:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/ppEra_Run2_2016_trackingLowPU.py:14:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/ppEra_Run2_25ns.py:14:import Configuration.StandardSequences.Eras as eras
./Configuration/DataProcessing/python/Impl/ppEra_Run2_50ns.py:14:import Configuration.StandardSequences.Eras as eras
./DQM/Integration/python/clients/l1tstage2_dqm_sourceclient-live_cfg.py:3:from Configuration.StandardSequences.Eras import eras
./DQM/Integration/python/clients/l1tstage2emulator_dqm_sourceclient-live_cfg.py:3:from Configuration.StandardSequences.Eras import eras
./DQM/TrackingMonitorSource/python/TrackingSourceConfig_Tier0_cff.py:2:from Configuration.StandardSequences.Eras import eras
./HLTrigger/Configuration/python/CustomConfigs.py:132:    from Configuration.StandardSequences.Eras import eras
./L1Trigger/L1TNtuples/python/L1NtupleRAW_cff.py:24:from Configuration.StandardSequences.Eras import eras
./L1Trigger/L1TNtuples/python/l1UpgradeTfMuonTree_cfi.py:13:from Configuration.StandardSequences.Eras import eras
./L1Trigger/L1TNtuples/python/l1UpgradeTree_cfi.py:13:from Configuration.StandardSequences.Eras import eras
./L1Trigger/TrackTrigger/python/TkOnlyDigi_cff.py:2:from Configuration.StandardSequences.Eras import eras
./RecoTracker/IterativeTracking/python/PixelPairStep_cff.py:2:from Configuration.StandardSequences.Eras import eras
./RecoTracker/IterativeTracking/python/iterativeTk_cff.py:2:from Configuration.StandardSequences.Eras import eras
./Validation/RecoTau/python/RecoTauValidation_cfi.py:128:from Configuration.StandardSequences.Eras import eras as _eras

I've addressed the few leftover tracking, L1, and validation configs in #16357. The DQM and HLT imports appear to be for the purposes of initializing a process, so they should be left alone. Configuration/Application is used for cmsDriver and should also be left alone.

Configuration/DataProcessing needs expert attention.

[slava77]

I updated Configuration/DataProcessing/python/Impl files in https://github.com/cms-sw/cmssw/pull/16405

On 10/25/16 2:10 PM, Kevin Pedro wrote:

After #16001 https://github.com/cms-sw/cmssw/pull/16001 and #16086 https://github.com/cms-sw/cmssw/pull/16086, most importing of |Configuration.StandardSequences.Eras| is gone. cms-checkdeps in |CMSSW_8_1_X_2016-10-25-1100| lists the following 9 packages:

|Configuration/Applications Configuration/DataProcessing DQM/Integration DQM/TrackingMonitorSource HLTrigger/Configuration L1Trigger/L1TNtuples L1Trigger/TrackTrigger RecoTracker/IterativeTracking Validation/RecoTau |

Further investigation with grep:

|grep -nrF "StandardSequences.Eras" . ./Configuration/Applications/python/ConfigBuilder.py:2070: self.pythonCfgCode += "from Configuration.StandardSequences.Eras import eras\n\n" ./Configuration/Applications/python/cmsDriverOptions.py:243: from Configuration.StandardSequences.Eras import eras ./Configuration/Applications/scripts/cmsDriver.py:23: from Configuration.StandardSequences.Eras import eras ./Configuration/DataProcessing/python/Impl/HeavyIonsEra_Run2_HI.py:14:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/cosmicsEra_Run2_2016.py:12:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/cosmicsEra_Run2_25ns.py:12:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/cosmicsEra_Run2_50ns.py:12:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/hcalnzsEra_Run2_2016.py:13:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/hcalnzsEra_Run2_25ns.py:13:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/ppEra_Run2_2016.py:14:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/ppEra_Run2_2016_trackingLowPU.py:14:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/ppEra_Run2_25ns.py:14:import Configuration.StandardSequences.Eras as eras ./Configuration/DataProcessing/python/Impl/ppEra_Run2_50ns.py:14:import Configuration.StandardSequences.Eras as eras ./DQM/Integration/python/clients/l1tstage2_dqm_sourceclient-live_cfg.py:3:from Configuration.StandardSequences.Eras import eras ./DQM/Integration/python/clients/l1tstage2emulator_dqm_sourceclient-live_cfg.py:3:from Configuration.StandardSequences.Eras import eras ./DQM/TrackingMonitorSource/python/TrackingSourceConfig_Tier0_cff.py:2:from Configuration.StandardSequences.Eras import eras ./HLTrigger/Configuration/python/CustomConfigs.py:132: from Configuration.StandardSequences.Eras import eras ./L1Trigger/L1TNtuples/python/L1NtupleRAW_cff.py:24:from Configuration.StandardSequences.Eras import eras ./L1Trigger/L1TNtuples/python/l1UpgradeTfMuonTree_cfi.py:13:from Configuration.StandardSequences.Eras import eras ./L1Trigger/L1TNtuples/python/l1UpgradeTree_cfi.py:13:from Configuration.StandardSequences.Eras import eras ./L1Trigger/TrackTrigger/python/TkOnlyDigi_cff.py:2:from Configuration.StandardSequences.Eras import eras ./RecoTracker/IterativeTracking/python/PixelPairStep_cff.py:2:from Configuration.StandardSequences.Eras import eras ./RecoTracker/IterativeTracking/python/iterativeTk_cff.py:2:from Configuration.StandardSequences.Eras import eras ./Validation/RecoTau/python/RecoTauValidation_cfi.py:128:from Configuration.StandardSequences.Eras import eras as _eras |

I've addressed the few leftover tracking, L1, and validation configs in

16357 https://github.com/cms-sw/cmssw/pull/16357. The DQM and HLT

imports appear to be for the purposes of initializing a process, so they should be left alone. |Configuration/Application| is used for |cmsDriver| and should also be left alone.

|Configuration/DataProcessing| needs expert attention.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cms-sw/cmssw/issues/14895#issuecomment-256176870, or mute the thread https://github.com/notifications/unsubscribe-auth/AEdcbuGDGANCMCsym7LQQTtDlnzR9BbTks5q3nA6gaJpZM4I2k9w.

[kpedro88]

In CMSSW_8_1_X_2016-11-03-1100, modifying Configuration.StandardSequences.Eras affects only 3 packages:

Configuration/Applications (python)
DQM/Integration (python)
HLTrigger/Configuration (python)

@smuzaffar, we should keep an eye on this wrt any fixes based on the discussion in #16385. Otherwise, unless @davidlange6 has further plans for handling Eras python files, we can plan to close this issue soon.

[kpedro88]

Checked up on this again and noticed a few more had crept in; fixed in #19732.

@davidlange6 @smuzaffar is there some way we could create a unit test to enforce this rule? (i.e. dependence on Configuration.StandardSequences.Eras only allowed in the python folders of the three packages listed in https://github.com/cms-sw/cmssw/issues/14895#issuecomment-258293494, with the possibility to add other exceptions in the future)

[davidlange6]

even these dependencies should get removed...that would be more productive use of time that making a unit test imo...

On Jul 13, 2017, at 3:38 PM, Kevin Pedro notifications@github.com wrote:

Checked up on this again and noticed a few more had crept in; fixed in #19732.

@davidlange6 @smuzaffar is there some way we could create a unit test to enforce this rule? (i.e. dependence on Configuration.StandardSequences.Eras only allowed in the python folders of the three packages listed in #14895 (comment), with the possibility to add other exceptions in the future)

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

[kpedro88]

I think the dependence in Configuration/Applications is unavoidable (cmsDriver internals). The others are due to full cmsRun configs being tracked in python folders. Do you think we should change cmsDriver so it doesn't create configs that import Configuration.StandardSequences.Eras?

Even then, we still need to prevent future dependencies from being introduced...

[davidlange6]

On Jul 13, 2017, at 3:53 PM, Kevin Pedro notifications@github.com wrote:

I think the dependence in Configuration/Applications is unavoidable (cmsDriver internals). The others are due to full cmsRun configs being tracked in python folders. Do you think we should change cmsDriver so it doesn't create configs that import Configuration.StandardSequences.Eras?

nah - you know what era to call when cmsDriver is started..no need to do anything more than import the one you want. But, right its not implemented.

Even then, we still need to prevent future dependencies from being introduced...

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

[kpedro88]

@davidlange6 what do you think of this? https://github.com/cms-sw/cmssw/compare/master...kpedro88:RemoveErasImport

StandardSequences contains ErasList (frequently modified) and Eras ("manager", depends on ErasList). Applications contains an instance of the manager that can be shared among the different python scripts (to avoid multiple constructions of it). When developers modify ErasList, it won't cause Applications to be checked out as a dependency (unless I've misunderstood how the Python dependency checking works). Since there's no longer an instance of the object in Configuration.StandardSequences.Eras, the imports are prevented forever.

If this seems acceptable, I'll first make a PR that goes through all tracked cmsRun configs that import Configuration.StandardSequences.Eras and switch it to the specific import from Configuration.Eras.

[davidlange6]

hi @kpedro88 - i think getting rid of the Eras_cff altogether in cmsDriver just means something like this. (I didn't look at the other users)

https://github.com/cms-sw/cmssw/compare/master...davidlange6:noEras

[kpedro88]

@davidlange6 I don't think that's general enough. In some cases, the user may want to specify an "internal use modifier", not just a top-level Era. So we need to know how to import any modifier/era.

[davidlange6]

@kpedro88 - its an 'era' option not a 'modifier' option - do we have a use case?

[kpedro88]

A couple of examples (remember, so far this has been allowed by cmsDriver):

1. run2_GEM_2017_MCTest (recently introduced): a temporary thing, but it needs to be chained with Run2_2017 to be used.
2. hcalHardcodeConditions and hcalSkipPacker that I introduced in #19712 - these can be chained with existing Eras to enable hardcode conditions for certain tests. And in general, if a user/developer wants to test the effect of combining certain modifiers with certain Eras, I think it's easier to allow that in cmsDriver vs. making them redefine the Era itself.

[kpedro88]

@davidlange6 further thoughts on this?

[kpedro88]

@davidlange6 ping

[kpedro88]

@davidlange6 if this is going to go in 930, I need feedback ASAP

[smuzaffar]

@kpedro88 , can we close this?

[kpedro88]

@smuzaffar I kept meaning to come back to this. I just submitted #26590, which is another step along the path outlined in this issue.

[makortel]

Can we close this now? Or have a list of steps that would need to be done before it could be closed?

[kpedro88]

A trivial modification to Eras.py still results in the following list from checkdeps:

Configuration/Applications (python)
DPGAnalysis/HcalTools (python)
DQM/Integration (python)
Geometry/HGCalGeometry (python)
Geometry/HcalTowerAlgo (python)
HLTrigger/Configuration (python)
L1Trigger/L1CaloTrigger (python)

Given that we now also have modifiers in Configuration/ProcessModifiers that are just imported directly rather than from a central location like Eras.py (compare https://github.com/cms-sw/cmssw/blob/5cc0e67f56fdf5cf3b0126da9b378733083ae17f/Configuration/Applications/python/ConfigBuilder.py#L2128-L2134 and https://github.com/cms-sw/cmssw/blob/5cc0e67f56fdf5cf3b0126da9b378733083ae17f/Configuration/Applications/python/ConfigBuilder.py#L2137-L2145), I would propose eliminating Eras.py entirely. This would imply the following:

1. Since modifiers in Configuration/Eras have several different filename conventions (Era_, Modifier_, ModifierChain_), a helper function should be provided to simplify the importing process (e.g. just looping over the 3 possibilities and doing the import in a try/except; open to cleverer suggestions).
2. Use the helper function in both ConfigBuilder.py and the various affected packages listed above. (There are also O(70) config files in test directories (based on a quick git grep) that import Eras.py; as always, I do not view these as centrally maintained, so I am personally not concerned about breaking them. Updating them could probably be automated to some extent, if someone were interested.)