Skip to content
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

[Core] Split Eras.py into multiple files #14895

Open
kpedro88 opened this issue Jun 15, 2016 · 29 comments
Open

[Core] Split Eras.py into multiple files #14895

kpedro88 opened this issue Jun 15, 2016 · 29 comments

Comments

@kpedro88
Copy link
Contributor

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
Copy link
Contributor

cmsbuild commented Jun 15, 2016

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
Copy link
Contributor Author

kpedro88 commented Aug 5, 2016

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
Copy link
Contributor

@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.commailto: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//issues/14895#issuecomment-237964098, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw2rp8FymkmJ1KucYezdymVF9wwtAks5qc6OCgaJpZM4I2k9w.

@smuzaffar
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

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.commailto: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//issues/14895#issuecomment-238207798, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw0rrNjs8_MZAwcdYVlmcvqr5Tm-Xks5qdxBggaJpZM4I2k9w.

@Dr15Jones
Copy link
Contributor

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
Copy link
Contributor

On Aug 8, 2016, at 3:59 PM, Chris Jones <notifications@github.commailto: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//issues/14895#issuecomment-238246502, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw7ZiICr3BeIV_wTdB-j00d9eg2cWks5qdzYsgaJpZM4I2k9w.

@davidlange6
Copy link
Contributor

some work in this direction

#15469

(and my suggested next steps)

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

On Aug 8, 2016, at 3:59 PM, Chris Jones <notifications@github.commailto: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//issues/14895#issuecomment-238246502, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw7ZiICr3BeIV_wTdB-j00d9eg2cWks5qdzYsgaJpZM4I2k9w.

@kpedro88
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

slava77 commented Nov 2, 2016

I updated Configuration/DataProcessing/python/Impl files in
#16405

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

After #16001 #16001 and #16086
#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 #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
#14895 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEdcbuGDGANCMCsym7LQQTtDlnzR9BbTks5q3nA6gaJpZM4I2k9w.

@kpedro88
Copy link
Contributor Author

kpedro88 commented Nov 3, 2016

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
Copy link
Contributor Author

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)

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 13, 2017 via email

@kpedro88
Copy link
Contributor Author

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
Copy link
Contributor

davidlange6 commented Jul 13, 2017 via email

@kpedro88
Copy link
Contributor Author

@davidlange6 what do you think of this?
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
Copy link
Contributor

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)

master...davidlange6:noEras

@kpedro88
Copy link
Contributor Author

@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
Copy link
Contributor

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

@kpedro88
Copy link
Contributor Author

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 Eras for HCAL hardcode conditions and packer skipping #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
Copy link
Contributor Author

@davidlange6 further thoughts on this?

@kpedro88
Copy link
Contributor Author

kpedro88 commented Aug 1, 2017

@davidlange6 ping

@kpedro88
Copy link
Contributor Author

kpedro88 commented Aug 7, 2017

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

@smuzaffar
Copy link
Contributor

@kpedro88 , can we close this?

@kpedro88
Copy link
Contributor Author

kpedro88 commented May 1, 2019

@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
Copy link
Contributor

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

@kpedro88
Copy link
Contributor Author

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

if hasattr(self._options,"era") and self._options.era :
# Multiple eras can be specified in a comma seperated list
from Configuration.StandardSequences.Eras import eras
for requestedEra in self._options.era.split(",") :
modifierStrings.append(requestedEra)
modifierImports.append(eras.pythonCfgLines[requestedEra])
modifiers.append(getattr(eras,requestedEra))
and
if hasattr(self._options,"procModifiers") and self._options.procModifiers:
import importlib
thingsImported=[]
for c in self._options.procModifiers:
thingsImported.extend(c.split(","))
for pm in thingsImported:
modifierStrings.append(pm)
modifierImports.append('from Configuration.ProcessModifiers.'+pm+'_cff import '+pm)
modifiers.append(getattr(importlib.import_module('Configuration.ProcessModifiers.'+pm+'_cff'),pm))
), 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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants